( )⚙ D10115 copies: choose target directory based on longest match

This is an archive of the discontinued Mercurial Phabricator instance.

copies: choose target directory based on longest match
ClosedPublic

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

Details

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
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
This revision was automatically updated to reflect the committed changes.
marmoute added a subscriber: marmoute.EditedMar 9 2021, 12:52 PM

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 ?

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?

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 ?

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.

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 ☺