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.

Download Raw Diff

Details

Reviewers

durin42
pulkit

Group Reviewers

hg-reviewers

Commits

rHG31393ec06cef: commitctx: document a fast path in _filecommit

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

Herald added a reviewer: hg-reviewers. · View Herald TranscriptJul 8 2020, 4:36 AM

Herald added a subscriber: mercurial-patches. · View Herald Transcript

marmoute added a child revision: D8701: commitctx: reorder some conditional for efficiency in _filecommit.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?

pulkit added a child revision: D8702: commitctx: return "touched" status from _filecommit.Jul 10 2020, 3:45 PM

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.

marmoute added a child revision: D8709: commitctx: extract the function in a dedicated module.Jul 21 2020, 6:09 PM

marmoute updated this revision to Diff 22029.

pulkit accepted this revision.Jul 22 2020, 4:08 AM

marmoute added a commit: rHG31393ec06cef: commitctx: document a fast path in _filecommit.Jul 22 2020, 8:16 AM

This revision was not accepted when it landed; it landed in state Needs Review.

Closed by commit rHG31393ec06cef: commitctx: document a fast path in _filecommit (authored by marmoute). · Explain Why

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			mercurial/localrepo.py (3 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	21797		Jul 8 2020, 4:36 AM	★	★
Diff 2	21984		Jul 20 2020, 12:49 PM	★	★
Diff 3	22029		Jul 21 2020, 6:09 PM	★	★
Diff 4	22041	rHG31393ec06cef7346015bafe8691095e0cc1b2862	Jul 6 2020, 1:13 PM	★	★

Status	Author	Revision
Closed	marmoute	D8708 commitctx: move a tiny else clause above the very long one
Closed	marmoute	D8707 commitctx: consider removed as touched
Closed	marmoute	D8706 commitctx: rename "changed" to touched
Closed	marmoute	D8705 commitctx: document the manifest writing function
Closed	marmoute	D8704 commitctx: more filesremoved assignment closer to removed computation
Closed	marmoute	D8703 commitctx: compute files added from _filecommit returns
Closed	marmoute	D8702 commitctx: return "touched" status from _filecommit
Closed	marmoute	D8710 commitctx: extract _filecommit too
Closed	marmoute	D8709 commitctx: extract the function in a dedicated module
Closed	marmoute	D8701 commitctx: reorder some conditional for efficiency in _filecommit
Closed	marmoute	D8700 commitctx: document a fast path in _filecommit
Closed	marmoute	D8699 commitctx: document _filecommit input and output

Diff 22041

mercurial/localrepo.py

	fname = fctx.path()			fname = fctx.path()
	fparent1 = manifest1.get(fname, nullid)			fparent1 = manifest1.get(fname, nullid)
	fparent2 = manifest2.get(fname, nullid)			fparent2 = manifest2.get(fname, nullid)
	touched = None			touched = None
	if fparent1 == fparent2 == nullid:			if fparent1 == fparent2 == nullid:
	touched = 'added'			touched = 'added'

	if isinstance(fctx, context.filectx):			if isinstance(fctx, context.filectx):
				# This block fast path most comparisons which are usually done. It
				durin42Unsubmitted Not Done 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?) durin42: I tried to copyedit this comment, but it's still somewhat self-contradictory. I'm also…
				pulkitUnsubmitted Not Done This comment still needs some work. pulkit: This comment still needs some work.
				marmouteAuthorUnsubmitted Done 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). marmoute: What work do you want to see? (this comment exists because I tried to be nice after reading the…
				pulkitUnsubmitted Not Done 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. pulkit: I understand your efforts here. I mean it's still confusing for a non-native like me. ``` This…
				marmouteAuthorUnsubmitted Done Reads good to me. Go ahead with the rephrasing. marmoute: Reads good to me. Go ahead with the rephrasing.
				# assumes that bare filectx is used and no merge happened, hence no
				pulkitUnsubmitted Not Done seems like `is` should not be there pulkit: seems like `is` should not be there
				marmouteAuthorUnsubmitted Done It should not marmoute: It should not
				# need to create a new file revision in this case.
				pulkitUnsubmitted Not Done there are no chance? pulkit: there are no chance?
				marmouteAuthorUnsubmitted Done there are no chance marmoute: there are no chance
	node = fctx.filenode()			node = fctx.filenode()
	if node in [fparent1, fparent2]:			if node in [fparent1, fparent2]:
	self.ui.debug(b'reusing %s filelog entry\n' % fname)			self.ui.debug(b'reusing %s filelog entry\n' % fname)
	if (			if (
	fparent1 != nullid			fparent1 != nullid
	and manifest1.flags(fname) != fctx.flags()			and manifest1.flags(fname) != fctx.flags()
	) or (			) or (
	fparent2 != nullid			fparent2 != nullid