Page MenuHomePhabricator

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

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

Details

Reviewers
marmoute
Group Reviewers
hg-reviewers
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
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

martinvonz created this revision.Tue, Jun 23, 1:03 PM
marmoute requested changes to this revision.Tue, Jul 7, 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
655

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

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.Tue, Jul 7, 9:44 AM
martinvonz marked an inline comment as done.Wed, Jul 8, 2:01 PM
martinvonz added inline comments.
mercurial/merge.py
655

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

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.Wed, Jul 8, 2:50 PM
marmoute added inline comments.
mercurial/merge.py
655
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

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.