This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9802

copies: explicitly filter out existing file in graftcopies
ClosedPublic

Authored by marmoute on Jan 15 2021, 8:23 PM.

Download Raw Diff

Details

Reviewers

pulkit

Group Reviewers

hg-reviewers

Commits

rHGf213b250fed0: copies: explicitly filter out existing file in graftcopies
rHG3f12a6064e1c: copies: explicitly filter out existing file in graftcopies

Summary

The graftcopies function does something very strange (maybe even wrong), it
calls _filter with a pair of changeset that does not match the one used to
compute the copies informations.

We are about to do some rework of _filter to make it closer to its documented
intent and fix a couple of bug. This means some of the logic that only make
sense for graft need to go somewhere else. We add the extra filtering with
proper documentation to graftcopies.

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.Jan 15 2021, 8:23 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptJan 15 2021, 8:23 PM

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

marmoute added a child revision: D9794: copies: clarify which case some conditional are handling.Jan 15 2021, 8:23 PM

✅ refresh by Heptapod after a successful CI run (🐙 💚)

pulkit accepted this revision.Jan 26 2021, 3:09 PM

This revision is now accepted and ready to land.Jan 26 2021, 3:09 PM

marmoute added a commit: rHG3f12a6064e1c: copies: explicitly filter out existing file in graftcopies.Jan 27 2021, 8:13 AM

Closed by commit rHG3f12a6064e1c: copies: explicitly filter out existing file in graftcopies (authored by marmoute). · Explain Why

This revision was automatically updated to reflect the committed changes.

marmoute added a commit: rHGf213b250fed0: copies: explicitly filter out existing file in graftcopies.Jan 27 2021, 6:21 PM

martinvonz added a subscriber: martinvonz.Jan 28 2021, 1:55 AM

martinvonz added inline comments.

mercurial/copies.py
1225–1226	That sounds pretty bad (creating a filelog entry with two parents for a non-merge commit), even though I'm not sure what user-visible effect it would have. Could you add a test case demonstrating the bug?

marmoute added inline comments.Jan 28 2021, 2:35 AM

mercurial/copies.py
1225–1226	The coverage stayed the same. I am adding new code explicitly catching this before removing older code that handled this at a less adequate location.

martinvonz added inline comments.Jan 28 2021, 2:37 AM

mercurial/copies.py
1225–1226	Okay. Is there an existing test for it? Which one?

marmoute added inline comments.Jan 28 2021, 2:56 AM

mercurial/copies.py
1225–1226	They are tests related to graft dedicated to this kind of copy tracing dropping that fails, if we take change the other logic without adding this one. I forgot which one exactly.

martinvonz added inline comments.Jan 28 2021, 3:05 AM

mercurial/copies.py
1225–1226	I'm guessing you meant `test-rebase-rename.t`. If that's correct, I don't think the comment here is correct. I'll send a patch to fix it.

martinvonz mentioned this in rHG892eb7c5edaa: copies: fix an incorrect comment in graftcopies() from recent D9802.Jan 28 2021, 4:16 PM

Revision Contents
Changeset List

			Path	Packages
M			mercurial/copies.py (8 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	24916		Jan 15 2021, 8:23 PM	★	★
Diff 2	25105		Jan 18 2021, 8:54 AM	★	★
Diff 3	25264		Jan 26 2021, 1:31 AM	★	★
Diff 4	25299	rHG3f12a6064e1c738549ae98ba3d9578421cf046b7	Jan 15 2021, 8:18 PM	★	★

Status	Author	Revision
Closed	marmoute	D9796 copies: add an devel option to trace all files
Closed	marmoute	D9795 copies: simplify the conditional for _filter's case 3
Closed	marmoute	D9794 copies: clarify which case some conditional are handling
Closed	marmoute	D9802 copies: explicitly filter out existing file in graftcopies
Closed	marmoute	D9793 copies: fix some comment in _filter

Diff 25299

mercurial/copies.py


	merge.update() will have already marked most copies, but it will only			merge.update() will have already marked most copies, but it will only
	mark copies if it thinks the source files are related (see			mark copies if it thinks the source files are related (see
	merge._related()). It will also not mark copies if the file wasn't modified			merge._related()). It will also not mark copies if the file wasn't modified
	on the local side. This function adds the copies that were "missed"			on the local side. This function adds the copies that were "missed"
	by merge.update().			by merge.update().
	"""			"""
	new_copies = pathcopies(base, ctx)			new_copies = pathcopies(base, ctx)
	_filter(wctx.p1(), wctx, new_copies)			parent = wctx.p1()
				_filter(parent, wctx, new_copies)
				# extra filtering to drop copy information for files that existed before
				# the graft (otherwise we would create merge filelog for non-merge commit
				martinvonzUnsubmitted Not Done That sounds pretty bad (creating a filelog entry with two parents for a non-merge commit), even though I'm not sure what user-visible effect it would have. Could you add a test case demonstrating the bug? martinvonz: That sounds pretty bad (creating a filelog entry with two parents for a non-merge commit), even…
				marmouteAuthorUnsubmitted Done The coverage stayed the same. I am adding new code explicitly catching this before removing older code that handled this at a less adequate location. marmoute: The coverage stayed the same. I am adding new code explicitly catching this before removing…
				martinvonzUnsubmitted Not Done Okay. Is there an existing test for it? Which one? martinvonz: Okay. Is there an existing test for it? Which one?
				marmouteAuthorUnsubmitted Done They are tests related to graft dedicated to this kind of copy tracing dropping that fails, if we take change the other logic without adding this one. I forgot which one exactly. marmoute: They are tests related to graft dedicated to this kind of copy tracing dropping that fails, if…
				martinvonzUnsubmitted Not Done I'm guessing you meant `test-rebase-rename.t`. If that's correct, I don't think the comment here is correct. I'll send a patch to fix it. martinvonz: I'm guessing you meant `test-rebase-rename.t`. If that's correct, I don't think the comment…
				for dest, __ in list(new_copies.items()):
				if dest in parent:
				del new_copies[dest]
	for dst, src in pycompat.iteritems(new_copies):			for dst, src in pycompat.iteritems(new_copies):
	wctx[dst].markcopied(src)			wctx[dst].markcopied(src)