This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8940

merge: store ACTION_KEEP_ABSENT when we are keeping the file absent locally
ClosedPublic

Authored by pulkit on Aug 24 2020, 10:14 AM.

Details

Reviewers
marmoute
Group Reviewers
hg-reviewers
Commits
rHG09edbff6ae8d: merge: store ACTION_KEEP_ABSENT when we are keeping the file absent locally
Summary

If a file is not present on the local side, and it's unchanged between other
merge parent and ancestor, we don't use any action, neither we had a if-else
branch for that condition.

This leads to bid-merge missing that there is a such action possible which can
be performed. As test changes demonstrate, we now choose the locally deleted
side instead of choosing the remote one.

This whole logic is not acurrate as we should prompt user on what to do
when this kind of criss-cross merge is in play.

Diff Detail

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

Event Timeline

pulkit created this revision.Aug 24 2020, 10:14 AM
marmoute added a subscriber: marmoute.Aug 26 2020, 12:51 PM

Using ACTION_KEEP for a file that does not exist seems a bti scary. Did we not refactor the merge code to allow to express this ? (Do not touch working copy and dirstate, but something happened?)

pulkit added a comment.Aug 27 2020, 4:53 AM
In D8940#134074, @marmoute wrote:

Using ACTION_KEEP for a file that does not exist seems a bti scary. Did we not refactor the merge code to allow to express this ? (Do not touch working copy and dirstate, but something happened?)

ACTION_KEEP is used for the same purpose: Do not touch working copy and dirstate, but something happened.

We can have a more dedicated ACTION_KEEP_DELETED

pulkit updated this revision to Diff 22473.Aug 27 2020, 5:38 AM
marmoute added a comment.Sep 1 2020, 4:55 AM

Having a dedicated action would seems cleaner.

pulkit edited parent revisions, added: D8974: merge: add `ACTION_KEEP_ABSENT` to represent files we want to keep absent; removed: D8939: tests: add criss cross merging tests whose behavior need to be fixed.Sep 1 2020, 8:13 AM
pulkit retitled this revision from merge: store ACTION_KEEP when we are keeping the file deleted locally to merge: store ACTION_KEEP_DELETED when we are keeping the file deleted locally.
pulkit edited the summary of this revision. (Show Details)
pulkit updated this revision to Diff 22498.
pulkit added a comment.Sep 1 2020, 8:13 AM
In D8940#134348, @marmoute wrote:

Having a dedicated action would seems cleaner.

Added a dedicated action ACTION_KEEP_DELETED and used it.

pulkit retitled this revision from merge: store ACTION_KEEP_DELETED when we are keeping the file deleted locally to merge: store ACTION_KEEP_ABSENT when we are keeping the file absent locally.Sep 2 2020, 7:53 AM
pulkit updated this revision to Diff 22509.
marmoute added a comment.Sep 11 2020, 4:59 AM

So right now the bid merge still behave in a bad way (delete the file instead of conflicting, but at least not it consistently does so regardless of where the merge is started?

If so, please update the changeset description to point this out.

I am also pondering if we should have a summary of the current issues within the test file, so that we can clean up while fixing this.

marmoute requested changes to this revision.Sep 11 2020, 5:40 AM
This revision now requires changes to proceed.Sep 11 2020, 5:40 AM
pulkit added a commit: rHG09edbff6ae8d: merge: store ACTION_KEEP_ABSENT when we are keeping the file absent locally.Sep 16 2020, 7:49 AM
This revision was not accepted when it landed; it landed in state Needs Revision.
Closed by commit rHG09edbff6ae8d: merge: store ACTION_KEEP_ABSENT when we are keeping the file absent locally (authored by pulkit). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/merge.py (7 lines)
Mtests/test-merge-criss-cross.t (18 lines)

Diff 22509

mercurial/merge.py

) )
else: else:
mresult.addfile( mresult.addfile(
f, f,
mergestatemod.ACTION_DELETED_CHANGED, mergestatemod.ACTION_DELETED_CHANGED,
(None, f, f, False, pa.node()), (None, f, f, False, pa.node()),
b'prompt deleted/changed', b'prompt deleted/changed',
) )
else:
mresult.addfile(
f,
mergestatemod.ACTION_KEEP_ABSENT,
None,
b'local not present, remote unchanged',
)
if repo.ui.configbool(b'experimental', b'merge.checkpathconflicts'): if repo.ui.configbool(b'experimental', b'merge.checkpathconflicts'):
# If we are merging, look for path conflicts. # If we are merging, look for path conflicts.
checkpathconflicts(repo, wctx, p2, mresult) checkpathconflicts(repo, wctx, p2, mresult)
narrowmatch = repo.narrowmatch() narrowmatch = repo.narrowmatch()
if not narrowmatch.always(): if not narrowmatch.always():
# Updates "actions" in place # Updates "actions" in place

tests/test-merge-criss-cross.t

other-file other-file
(merging a deletion with keeping → conflict) (merging a deletion with keeping → conflict)
BROKEN: this should result in conflict BROKEN: this should result in conflict
$ hg update --clean 'desc("merge-deleting-the-file-from-deleted")' $ hg update --clean 'desc("merge-deleting-the-file-from-deleted")'
0 files updated, 0 files merged, 0 files removed, 0 files unresolved 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg merge 'desc("merge-keeping-the-file-from-deleted")' $ hg merge 'desc("merge-keeping-the-file-from-deleted")'
1 files updated, 0 files merged, 0 files removed, 0 files unresolved 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
(branch merge, don't forget to commit) (branch merge, don't forget to commit)
$ ls -1 $ ls -1
other-file other-file
the-file
(merging a deletion with keeping → conflict) (merging a deletion with keeping → conflict)
BROKEN: this should result in conflict BROKEN: this should result in conflict
$ hg update --clean 'desc("merge-deleting-the-file-from-deleted")' $ hg update --clean 'desc("merge-deleting-the-file-from-deleted")'
0 files updated, 0 files merged, 1 files removed, 0 files unresolved 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg merge 'desc("merge-keeping-the-file-from-updated")' $ hg merge 'desc("merge-keeping-the-file-from-updated")'
1 files updated, 0 files merged, 0 files removed, 0 files unresolved 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
(branch merge, don't forget to commit) (branch merge, don't forget to commit)
$ ls -1 $ ls -1
other-file other-file
the-file
(merging two deletion together → no conflict) (merging two deletion together → no conflict)
$ hg update --clean 'desc("merge-deleting-the-file-from-updated")' $ hg update --clean 'desc("merge-deleting-the-file-from-updated")'
0 files updated, 0 files merged, 1 files removed, 0 files unresolved 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg merge 'desc("merge-deleting-the-file-from-deleted")' $ hg merge 'desc("merge-deleting-the-file-from-deleted")'
0 files updated, 0 files merged, 0 files removed, 0 files unresolved 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
(branch merge, don't forget to commit) (branch merge, don't forget to commit)
$ ls -1 $ ls -1
other-file other-file
(merging a deletion with keeping → conflict) (merging a deletion with keeping → conflict)
BROKEN: this should result in conflict BROKEN: this should result in conflict
$ hg update --clean 'desc("merge-deleting-the-file-from-updated")' $ hg update --clean 'desc("merge-deleting-the-file-from-updated")'
0 files updated, 0 files merged, 0 files removed, 0 files unresolved 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg merge 'desc("merge-keeping-the-file-from-deleted")' $ hg merge 'desc("merge-keeping-the-file-from-deleted")'
1 files updated, 0 files merged, 0 files removed, 0 files unresolved 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
(branch merge, don't forget to commit) (branch merge, don't forget to commit)
$ ls -1 $ ls -1
other-file other-file
the-file
(merging a deletion with keeping → conflict) (merging a deletion with keeping → conflict)
BROKEN: this should result in conflict BROKEN: this should result in conflict
$ hg update --clean 'desc("merge-deleting-the-file-from-updated")' $ hg update --clean 'desc("merge-deleting-the-file-from-updated")'
0 files updated, 0 files merged, 1 files removed, 0 files unresolved 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg merge 'desc("merge-keeping-the-file-from-updated")' $ hg merge 'desc("merge-keeping-the-file-from-updated")'
1 files updated, 0 files merged, 0 files removed, 0 files unresolved 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
(branch merge, don't forget to commit) (branch merge, don't forget to commit)
$ ls -1 $ ls -1
other-file other-file
the-file
(merging two "keeping" together → no conflict) (merging two "keeping" together → no conflict)
$ hg update --clean 'desc("merge-keeping-the-file-from-updated")' $ hg update --clean 'desc("merge-keeping-the-file-from-updated")'
1 files updated, 0 files merged, 0 files removed, 0 files unresolved 1 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg merge 'desc("merge-keeping-the-file-from-deleted")' $ hg merge 'desc("merge-keeping-the-file-from-deleted")'
0 files updated, 0 files merged, 0 files removed, 0 files unresolved 0 files updated, 0 files merged, 0 files removed, 0 files unresolved
(branch merge, don't forget to commit) (branch merge, don't forget to commit)