| marmoute |
| hg-reviewers |
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.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
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. | |
| mercurial/merge.py | ||
|---|---|---|
| 831 |
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.
It's what you would probably expect:
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. | |
| mercurial/merge.py | ||
|---|---|---|
| 831 |
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.
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. | |
| 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 (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). | |
| mercurial/merge.py | ||
|---|---|---|
| 831 |
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? | |
| 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. | |
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 ?