This is an archive of the discontinued Mercurial Phabricator instance.

commitctx: document a fast path in _filecommit
ClosedPublic

Authored by marmoute on Jul 8 2020, 4:36 AM.

Details

Summary

This block cut off a lot of logic, documenting the why and how seems useful to
future reader.

This is part of a larger refactoring/cleanup of the commitctx code to clarify
and augment the logic gathering metadata useful for copy tracing. The current
code is a tad too long and entangled to make such update easy. We start with
easy and small cleanup.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

marmoute created this revision.Jul 8 2020, 4:36 AM
pulkit added a subscriber: pulkit.Jul 9 2020, 4:32 AM
pulkit added inline comments.
mercurial/localrepo.py
2806

seems like is should not be there

2807

there are no chance?

marmoute added inline comments.Jul 10 2020, 3:49 PM
mercurial/localrepo.py
2806

It should not

2807

there are no chance

durin42 requested changes to this revision.Jul 17 2020, 4:45 PM
durin42 added a subscriber: durin42.
durin42 added inline comments.
mercurial/localrepo.py
2805

I tried to copyedit this comment, but it's still somewhat self-contradictory. I'm also completely lost on the first clause ("This block fast path most comparison") - could you take a fresh start on this comment (and maybe spend more words on it?) and/or ask someone else to help interactively?

(I also see that line 2182 looks like you marked the comment resolved but didn't actually update the code?)

This revision now requires changes to proceed.Jul 17 2020, 4:45 PM
marmoute updated this revision to Diff 21984.Jul 20 2020, 12:49 PM
pulkit added inline comments.Jul 21 2020, 10:38 AM
mercurial/localrepo.py
2805

This comment still needs some work.

marmoute added inline comments.Jul 21 2020, 11:36 AM
mercurial/localrepo.py
2805

What work do you want to see? (this comment exists because I tried to be nice after reading the code, If this is delaying this series further, we can drop it).

pulkit added inline comments.Jul 21 2020, 12:55 PM
mercurial/localrepo.py
2805

I understand your efforts here. I mean it's still confusing for a non-native like me.

This block fast path most comparisons which are usually done. It assumes that bare filectx is used and no merge happened, hence no need to create a new file revision in this case.

Shall I reword it to above in flight?

Also, it's not blocking the series since I already queued some of later ones.

marmoute added inline comments.Jul 21 2020, 1:00 PM
mercurial/localrepo.py
2805

Reads good to me. Go ahead with the rephrasing.

pulkit accepted this revision.Jul 22 2020, 4:08 AM
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.