This is an archive of the discontinued Mercurial Phabricator instance.

Differential D10115

copies: choose target directory based on longest match
ClosedPublic

Authored by martinvonz on Mar 4 2021, 7:24 PM.

Download Raw Diff

Details

Reviewers

pulkit

Group Reviewers

hg-reviewers

Commits

rHGad30b29bc23d: copies: choose target directory based on longest match

Summary

If one side of a merge renames dir1/ to dir2/ and the subdirectory
dir1/subdir1/ to dir2/subdir2/, and the other side of the merge
adds a file in dir1/subdir1/, we should clearly move that into
dir2/subdir2/. We already detect the directories correctly before
this patch, but we iterate over them in arbitrary order. That results
in the new file sometimes ending up in dir2/subdir1/ instead. This
patch fixes it by iterating over the source directories by visiting
subdirectories first. That's achieved by simply iterating over them in
reverse lexicographical order.

Without the fix, the test case still passes on Python 2 but fails on
Python 3. It depends on the iteration order of the dict. I did not
look into how it's built up and why it behaved differently before the
fix. I could probably have gotten it to fail on Python 2 as well by
choosing different directory names.

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

martinvonz created this revision.Mar 4 2021, 7:24 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptMar 4 2021, 7:24 PM

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

pulkit accepted this revision.Mar 5 2021, 3:34 AM

This revision is now accepted and ready to land.Mar 5 2021, 3:34 AM

martinvonz added a commit: rHGad30b29bc23d: copies: choose target directory based on longest match.Mar 5 2021, 4:34 AM

Closed by commit rHGad30b29bc23d: copies: choose target directory based on longest match (authored by martinvonz). · Explain Why

This revision was automatically updated to reflect the committed changes.

Is this also going to affect case where entry are nto childre of each other but still have different side ? for example if we had ['a/short', 'a/longer'] with a prefix matching on 'a' this change alter the behavior, don't it ?

In D10115#154126, @marmoute wrote:

Is this also going to affect case where entry are nto childre of each other but still have different side ? for example if we had ['a/short', 'a/longer'] with a prefix matching on 'd' this change alter the behavior, don't it ?

I don't understand. d doesn't match either of those. Did you mean a? Even assuming that, I'm not sure what you mean, but maybe line 1092 answers your question?

In D10115#154127, @martinvonz wrote:

In D10115#154126, @marmoute wrote:

Is this also going to affect case where entry are nto childre of each other but still have different side ? for example if we had ['a/short', 'a/longer'] with a prefix matching on 'd' this change alter the behavior, don't it ?

I don't understand. d doesn't match either of those. Did you mean a? Even assuming that, I'm not sure what you mean, but maybe line 1092 answers your question?

Ah, perhaps you just saw "longest match" in the title and thought that that's literally what the code does. Perhaps I should have said "deepest match".

Yeah I meant 'a', sorry about that (the 'd' comes from a copies discussion I had with Pulkit in //). Line 192 does not help me much since we still seems to be sorting the longest first as far as I undersant (which result in the deepest coming first).
However it seems like we were previously interating over a dictionnary so we unstable order, right ?

In D10115#154171, @marmoute wrote:

Yeah I meant 'a', sorry about that (the 'd' comes from a copies discussion I had with Pulkit in //). Line 192 does not help me much since we still seems to be sorting the longest first as far as I undersant (which result in the deepest coming first).

I guess I still don't know what your question is then. You said something about this change altering behavior. What I didn't understand is how you think it alters the behavior. Can you elaborate?

However it seems like we were previously interating over a dictionnary so we unstable order, right ?

Yes.

In D10115#154172, @martinvonz wrote:

In D10115#154171, @marmoute wrote:

Yeah I meant 'a', sorry about that (the 'd' comes from a copies discussion I had with Pulkit in //). Line 192 does not help me much since we still seems to be sorting the longest first as far as I undersant (which result in the deepest coming first).

I guess I still don't know what your question is then. You said something about this change altering behavior. What I didn't understand is how you think it alters the behavior. Can you elaborate?

My question was "If you change the ordering, are we sure we don't break things break previous behavior that could make sense?"

However it seems like we were previously interating over a dictionnary so we unstable order, right ?

Yes.

Since the previous ordering seems to be "no stable ordering" I don't think this diff could have break any behavior relying on it ☺

Revision Contents
Changeset List

			Path	Packages
M			mercurial/copies.py (8 lines)
M			tests/test-rename-dir-merge.t (42 lines)

Diff 26100

mercurial/copies.py


	dirmove = {k + b"/": v + b"/" for k, v in pycompat.iteritems(dirmove)}			dirmove = {k + b"/": v + b"/" for k, v in pycompat.iteritems(dirmove)}

	for d in dirmove:			for d in dirmove:
	repo.ui.debug(			repo.ui.debug(
	b" discovered dir src: '%s' -> dst: '%s'\n" % (d, dirmove[d])			b" discovered dir src: '%s' -> dst: '%s'\n" % (d, dirmove[d])
	)			)

				# Sort the directories in reverse order, so we find children first
				# For example, if dir1/ was renamed to dir2/, and dir1/subdir1/
				# was renamed to dir2/subdir2/, we want to move dir1/subdir1/file
				# to dir2/subdir2/file (not dir2/subdir1/file)
				dirmove_children_first = sorted(dirmove, reverse=True)

	movewithdir = {}			movewithdir = {}
	# check unaccounted nonoverlapping files against directory moves			# check unaccounted nonoverlapping files against directory moves
	for f in addedfilesfn():			for f in addedfilesfn():
	if f not in fullcopy:			if f not in fullcopy:
	for d in dirmove:			for d in dirmove_children_first:
	if f.startswith(d):			if f.startswith(d):
	# new file added in a directory that was moved, move it			# new file added in a directory that was moved, move it
	df = dirmove[d] + f[len(d) :]			df = dirmove[d] + f[len(d) :]
	if df not in copy:			if df not in copy:
	movewithdir[f] = df			movewithdir[f] = df
	repo.ui.debug(			repo.ui.debug(
	b" pending file src: '%s' -> dst: '%s'\n"			b" pending file src: '%s' -> dst: '%s'\n"
	% (f, df)			% (f, df)

tests/test-rename-dir-merge.t

	$ hg merge			$ hg merge
	2 files updated, 0 files merged, 2 files removed, 0 files unresolved			2 files updated, 0 files merged, 2 files removed, 0 files unresolved
	(branch merge, don't forget to commit)			(branch merge, don't forget to commit)
	$ hg st --copies			$ hg st --copies
	M s/s			M s/s
	M t/t			M t/t
	R a/s			R a/s
	R a/t			R a/t

				$ cd ..


				Test that files are moved to a new directory based on the path prefix that
				matches the most. dir1/ below gets renamed to dir2/, and dir1/subdir1/ gets
				renamed to dir2/subdir2/. We want dir1/subdir1/newfile to move to
				dir2/subdir2/ (not to dir2/subdir1/ as we would infer based on just the rename
				of dir1/ to dir2/).

				$ hg init nested-renames
				$ cd nested-renames
				$ mkdir dir1
				$ echo a > dir1/file1
				$ echo b > dir1/file2
				$ mkdir dir1/subdir1
				$ echo c > dir1/subdir1/file3
				$ echo d > dir1/subdir1/file4
				$ hg ci -Aqm initial
				$ hg mv dir1 dir2
				moving dir1/file1 to dir2/file1
				moving dir1/file2 to dir2/file2
				moving dir1/subdir1/file3 to dir2/subdir1/file3
				moving dir1/subdir1/file4 to dir2/subdir1/file4
				$ hg mv dir2/subdir1 dir2/subdir2
				moving dir2/subdir1/file3 to dir2/subdir2/file3
				moving dir2/subdir1/file4 to dir2/subdir2/file4
				$ hg ci -m 'move dir1/ to dir2/ and dir1/subdir1/ to dir2/subdir2/'
				$ hg co 0
				4 files updated, 0 files merged, 4 files removed, 0 files unresolved
				$ echo e > dir1/subdir1/file5
				$ hg ci -Aqm 'add file in dir1/subdir1/'
				$ hg merge 1
				5 files updated, 0 files merged, 4 files removed, 0 files unresolved
				(branch merge, don't forget to commit)
				$ hg files
				dir2/file1
				dir2/file2
				dir2/subdir2/file3
				dir2/subdir2/file4
				dir2/subdir2/file5
				$ cd ..

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	26098		Mar 4 2021, 7:24 PM	★	★
Diff 2	26100	rHGad30b29bc23de9533cc7367b5cdc6a925c96898d	Mar 4 2021, 7:06 PM	★	★