This is an archive of the discontinued Mercurial Phabricator instance.

copies: stop recording buggy file merge when new file overwrite an old one
AbandonedPublic

Authored by marmoute on Mar 5 2020, 1:14 PM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Summary

The BD/DB and BF/FB cases in tests/test-copies-chain-merge.t shown an
inconsistent behavior when merging two branches where one plainly replace a file
untouched on the other side.

We do the minimal amount of work in the commit code to fix this bug. The case is
narrow enough that the performance impact does not scare me right now.

My priority is to get the behavior fixed with the minimal code changes. (Because
I need to behavior to be right to make more progress on the changeset centric
algorithm). We can iterate later on a more efficient approach (updating the
merge state to record the situation at the time it is initially detected).

The tests/test-convert-hg-source.t is affected because it also created such
graphs.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

marmoute created this revision.Mar 5 2020, 1:14 PM
marmoute updated this revision to Diff 20550.Mar 6 2020, 5:21 AM
martinvonz added inline comments.
mercurial/localrepo.py
2871–2877

Is this correct when grafting (not merging), such as when rebasing? Is it worth having a test case for that? Is it even possible to get here in that case? Maybe when rebasing a merge commit?

tests/test-copies-chain-merge.t
602

Does the removal of this comment mean that it's no longer correct? I don't think that's what you meant, so should the comment remain (or even not have been added until now)?

marmoute added inline comments.Mar 6 2020, 4:42 PM
tests/test-copies-chain-merge.t
602

The previous output is no longer wrong, so we no longer need to specify the next one is wrong.

martinvonz added inline comments.Mar 6 2020, 4:45 PM
tests/test-copies-chain-merge.t
602

Ah, so that comment meant to say that that "unlike the test case just before, this one is correct". That wasn't clear to me. But no need to change anything since it's going away anyway.

marmoute retitled this revision from copies: stop recoding buggy file merge when new file overwrite an old one to copies: stop recording buggy file merge when new file overwrite an old one.Mar 6 2020, 6:52 PM
marmoute updated this revision to Diff 20595.

series update after Martin feedback

marmoute added inline comments.Mar 6 2020, 7:50 PM
mercurial/localrepo.py
2871–2877

I don't know, but not existing test broke. Consolidating test for the plain merge case already took a long time. Not having any test failure from exisitng rebase/graft test seemed enough to me while making progress on the correctness of this.

marmoute updated this revision to Diff 20755.Mar 11 2020, 6:02 PM

FYI, I'd like to look at this patch, but I'm busy with other things right now (switching back and forth between browser tabs to delay the interview feedback that I'm actually supposed to write), so it will take a bit longer before I'll find time for this one.

This is pretty ugly and it doesn't seem that the next patch depends on it. You said you'll soon clean it up anyway, so I wonder if should just wait for the better solution instead. It doesn't seem like this fixes a serious bug so we have to rush it. Thoughts?

This is pretty ugly and it doesn't seem that the next patch depends on it. You said you'll soon clean it up anyway, so I wonder if should just wait for the better solution instead. It doesn't seem like this fixes a serious bug so we have to rush it. Thoughts?

My initial motivation to rush the ugly way was "getting the behavior right to compare with the changeset centric one and being able to test performance improvement for the changeset centric one while having access to a specific repository". However, cancelling of all travel has cancelled the window to access that repo. I'll resubmit a cleaner versions soon.

since you did not commented on it, I assume the new behavior is fine by you, right?

This is pretty ugly and it doesn't seem that the next patch depends on it. You said you'll soon clean it up anyway, so I wonder if should just wait for the better solution instead. It doesn't seem like this fixes a serious bug so we have to rush it. Thoughts?

My initial motivation to rush the ugly way was "getting the behavior right to compare with the changeset centric one and being able to test performance improvement for the changeset centric one while having access to a specific repository". However, cancelling of all travel has cancelled the window to access that repo. I'll resubmit a cleaner versions soon.
since you did not commented on it, I assume the new behavior is fine by you, right?

You mean the new behavior in this patch? Yes, I think I'm fine with it. I initially wasn't sure if the old behavior should be considered a bug, but I think you're right that it should.

durin42 requested changes to this revision.Apr 1 2020, 3:01 PM
durin42 added a subscriber: durin42.

Per irc checkin, we expect changes on this before review.

This revision now requires changes to proceed.Apr 1 2020, 3:01 PM
pulkit added a subscriber: pulkit.Apr 10 2020, 7:39 AM

D8392 is an attempt to fix the same issue by storing extra information in mergestate.

D8392 is an attempt to fix the same issue by storing extra information in mergestate.

The approach (storing in the merge state) is the one I planned to implement before handling the task to Pulkit. so +1 on the approach.

(feel free to reuse that very same diff for it.)

marmoute abandoned this revision.Apr 10 2020, 1:46 PM

superseeded by D8392