Page MenuHomePhabricator

copies: handle more cases where a file got replaced by a copy
ClosedPublic

Authored by martinvonz on Jun 23 2020, 1:03 PM.

Details

Summary

This patch fixes the changeset-centric version in a pretty
straight-forward way. It fixes it to automatically resolve the
conflict, which is better than resulting in a modify/delete conflict
as it was before b4057d001760 (merge: when rename was made on both
sides, use ancestor as merge base, 2020-01-22).

I'll leave it for later to test and explicitly handle cases where
files have been renamed to the same target on different sides of the
merge.

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.Jun 23 2020, 1:03 PM
marmoute requested changes to this revision.Jul 7 2020, 9:44 AM
marmoute added a subscriber: marmoute.

I have enough question I am I moving this to "change requested" I might move it to "accepted" withotu actual change depending of the answers.

mercurial/merge.py
831

If I read them correctly, these merge are not merge since on side is unchanged. Should we be using a get here?

What happens if some change happens to y after the rename. This is a case were we need an actual merge. What is the current behavior ? What is the behavior after the patch ?

Do we have a test for this case ?

tests/test-copies.t
499–500

We should probably specify that the "BROKEN" part will remain as such (compared to changeset centric one who can detect it). Otherwise, it might looks like an in-progress fix that never landed.

This revision now requires changes to proceed.Jul 7 2020, 9:44 AM
martinvonz marked an inline comment as done.Jul 8 2020, 2:01 PM
martinvonz added inline comments.
mercurial/merge.py
831

If I read them correctly, these merge are not merge since on side is unchanged. Should we be using a get here?

I don't think ACTION_GET lets you get from a different path name. It looks like all the other actions we create when a file was copied are using ACTION_MERGE, even if some could probably be "optimized" for the case where no change was made on one side.

What happens if some change happens to y after the rename. This is a case were we need an actual merge. What is the current behavior ? What is the behavior after the patch ?

It's what you would probably expect:

  • Before this patch, the copy tracing doesn't work and the changes to x are not propagated to y. Therefore, no merge conflict happens.
  • After this patch, the copy tracing works and the changes to x are propagated to y. Therefore, there will be a merge conflict (if nearby lines are modified, of course).

Do we have a test for this case ?

I can change the existing test case if you prefer, but it seems excessive to have tests for both cases.

tests/test-copies.t
499–500

We already have several tests of broken behavior in the test suite (just grep for "BROKEN") , so I think that's fine.

marmoute added a subscriber: pulkit.Jul 8 2020, 2:50 PM
marmoute added inline comments.
mercurial/merge.py
831
If I read them correctly, these merge are not merge since on side is unchanged. Should we be using a get here?

I don't think ACTION_GET lets you get from a different path name. It looks like all the other actions we create when a file was copied are using ACTION_MERGE, even if some could probably be "optimized" for the case where no change was made on one side.

I see. @pulkit is starting a refactoring of the action declaration that might lift this limitation (or make it trivial to lift it). Can you distinct between the two case in your code (using the current option, ACTION_MERGE, in both) to make it simpler to update this code in the future.

Do we have a test for this case ?

I can change the existing test case if you prefer, but it seems excessive to have tests for both cases.

They are two case, one where a merge is necessary, one where no merge is needed. So we must have a case for both. Please update your series with such test coverage.

tests/test-copies.t
499–500

The main different I see here is that in the existing case, the BROKEN case have a hope to get fixed (unless I understand them wrong). While this one is hopelessly wrong and won't be fixed (with this copy tracing strategy). So I would not use BROKEN for that case.

martinvonz marked an inline comment as done.Jul 16 2020, 1:02 PM
martinvonz added a subscriber: durin42.
martinvonz added inline comments.
mercurial/merge.py
831

Do we have a test for this case ?

I can change the existing test case if you prefer, but it seems excessive to have tests for both cases.

They are two case, one where a merge is necessary, one where no merge is needed. So we must have a case for both. Please update your series with such test coverage.

I didn't reply to this for a long time because the categorical "So we must have a case for both. Please update your series with such test coverage." made me feel like you didn't want to have a discussion about this. I talked to @durin42 about this a bit. He pointed out that maybe the root cause of the disagreement was that my "seems excessive to have tests for both cases" above was not motivated, because I didn't realize then that you thought that we really should needed testing of both cases. My reason for not testing both is that there's no different code branches in this patch that would be tested by duplicating the tests (unlike your guess on #mercurial, it's not that I argued for "intentionally lower the test coverage"). I think the code branches that would be covered by the two tests you suggest are in filemerge.py. I think we should have more specific tests if we want to make sure that that code works well when one side of the code is unchanged, but that seems outside the scope of this patch. Or did you consider that and you see a particular risk between interaction between the code I'm adding here and the filemerge code?

tests/test-copies.t
499–500

Yeah, that's fair -- we're probably never going to make that work (kind of like how status between two commits will include changed-and-then-reverted files).

marmoute added inline comments.Jul 31 2020, 12:58 PM
mercurial/merge.py
831

I didn't reply to this for a long time because the categorical "So we must have a case for both. Please update your series with such test coverage." made me feel like you didn't want to have a discussion about this. I talked to @durin42 about this a bit. He pointed out that maybe the root cause of the disagreement was that my "seems excessive to have tests for both cases" above was not motivated, because I didn't realize then that you thought that we really should needed testing of both cases
My reason for not testing both is that there's no different code branches in this patch that would be tested by duplicating the tests

Doing a merge involves many layers of code (computing the merge, applying working copy action, applying dirstate action, actually creating commit, etc). Small difference in the merge cases pass through the same code path at one level, but in different code paths in others. Overall the whole merge stack has been a prolific source of bugs for the last handful of months, and we know that small differences such as the one we are discussing right now may be source of bugs. In addition, the code paths of today are not the code paths of tomorrow, and this code is already actively being refactored.

You successfully isolated a new family of cases where Mercurial misbehave and there are two clear sub-cases introduced by a small variations. It seems a small price (mostly duplicating the test entries with the small variation) coming with a big benefit (increasing the number of cases covered by our tests for an important, complicated and unfortunately often buggy logic).

So could you please add explicit testing for all the new merge cases?

martinvonz added inline comments.Oct 29 2020, 10:20 AM
mercurial/merge.py
831

I don't think additional testing is warranted. I'll let reviewers decide if they want to queue this or if I should drop the patch.

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.