Page MenuHomePhabricator

[RFC] mergestate: store about files resolved in favour of other
ClosedPublic

Authored by pulkit on Apr 10 2020, 6:43 AM.

Details

Summary

Committing a merge sometimes wrongly creates a new filenode where it can re-use
an existing one. This happens because the commit code does it's own calculation
and does not know what happened on merge.

This starts storing information in mergestate about files which were
automatically merged and the other/remote version of file was used.
We need this information at commit to pick the filenode parent for the new
commit.

This issue was found by Pierre-Yves David and idea to store the relevant parts
in mergestate is also suggested by him.

Somethings which can be further investigated are:

  1. refactoring of commit logic more to depend on this information
  2. maybe a more generic solution?

Diff Detail

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

Event Timeline

pulkit created this revision.Apr 10 2020, 6:43 AM

This attempts to fix the issue which D8243 tries to fix by storing extra information in mergestate.

marmoute accepted this revision.Apr 10 2020, 7:56 AM
marmoute added a subscriber: marmoute.

The approach seems good and the test change seems consistent with D8243 (@pulkit can you confirm there are no difference?)

This looks good to me.

pulkit edited the summary of this revision. (Show Details)Apr 10 2020, 10:28 AM
durin42 accepted this revision as: durin42.

I like it, but want to get @martinvonz to look too.

martinvonz accepted this revision.Apr 10 2020, 12:11 PM
This revision is now accepted and ready to land.Apr 10 2020, 12:11 PM
This revision was automatically updated to reflect the committed changes.
martinvonz added inline comments.Oct 6 2020, 4:28 PM
tests/test-copies-chain-merge.t
311–313

Could you send a follow-up updating this?

367

The hash no longer exists, and maybe it's no longer wrong?

381

And I don't think this is wrong anymore (i.e. the comment is now wrong)

388

Would be clearer to update this too. Either simply delete it or say what it should show in order to be correct (e.g. "log output should not include the merge commit").

549

Is this still true?

555

delete

583

And this comment also seems wrong now

593

As above, delete to clarify

pulkit added a comment.Oct 7 2020, 3:59 AM

@martinvonz sent D9168 as followup for all the comments.