Patchwork [1,of,4] tests: add newfilenode test case in test-merge-changedelete.t

login
register
mail settings
Submitter Pulkit Goyal
Date Sept. 30, 2020, 1:13 p.m.
Message ID <aa8ac5a5deb695a48917.1601471602@workspace>
Download mbox | patch
Permalink /patch/47348/
State Accepted
Headers show

Comments

Pulkit Goyal - Sept. 30, 2020, 1:13 p.m.
# HG changeset patch
# User Pulkit Goyal <7895pulkit@gmail.com>
# Date 1601458765 -19800
#      Wed Sep 30 15:09:25 2020 +0530
# Node ID aa8ac5a5deb695a489172726016d3be57ad17cc1
# Parent  db11f8f39cafb7973f1cc7076c5e6e595440501e
# EXP-Topic merge-newnode-final
tests: add newfilenode test case in test-merge-changedelete.t
Yuya Nishihara - Oct. 2, 2020, 11:52 a.m.
On Wed, 30 Sep 2020 18:43:22 +0530, Pulkit Goyal wrote:
> # HG changeset patch
> # User Pulkit Goyal <7895pulkit@gmail.com>
> # Date 1601458765 -19800
> #      Wed Sep 30 15:09:25 2020 +0530
> # Node ID aa8ac5a5deb695a489172726016d3be57ad17cc1
> # Parent  db11f8f39cafb7973f1cc7076c5e6e595440501e
> # EXP-Topic merge-newnode-final
> tests: add newfilenode test case in test-merge-changedelete.t

Queued, thanks.
Yuya Nishihara - Oct. 2, 2020, 11:59 a.m.
On Wed, 30 Sep 2020 18:43:23 +0530, Pulkit Goyal wrote:
> # HG changeset patch
> # User Pulkit Goyal <7895pulkit@gmail.com>
> # Date 1601461014 -19800
> #      Wed Sep 30 15:46:54 2020 +0530
> # Node ID 39f0021504cfdd65db22223de938cf6a123d18f8
> # Parent  aa8ac5a5deb695a489172726016d3be57ad17cc1
> # EXP-Topic merge-newnode-final
> merge: if DELETED_CHANGED and GET are in actions, choose DELETED_CHANGED
> 
> ACTION_GET represents that either the file is created on remote or it's newer on
> the remote side. However, since we have a ACTION_DELETE_CHANGED too, it means
> the file is not present locally and ACTION_GET is representing that file was
> created on remote.
> 
> Having both ACTION_GET and ACTION_DELETED_CHANGED is conflicting because one
> says that file was created on remote and other says file has delete-changed
> conflicts.
> 
> Let's choose ACTION_DELETED_CHANGED which will result in conflicts and make user
> choose the right way forward.
> 
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -1220,6 +1220,21 @@ def calculateupdates(
>                  repo.ui.note(_(b" %s: picking 'keep new' action\n") % f)
>                  mresult.addfile(f, *bids[mergestatemod.ACTION_KEEP_NEW][0])
>                  continue
> +            # ACTION_GET and ACTION_DELETE_CHANGED are conflicting actions as
> +            # one action states the file is newer/created on remote side and
> +            # other states that file is deleted locally and changed on remote
> +            # side. Let's fallback and rely on a conflicting action to let user
> +            # do the right thing
> +            if (
> +                mergestatemod.ACTION_DELETED_CHANGED in bids
> +                and mergestatemod.ACTION_GET in bids
> +                and len(bids) == 2
> +            ):

I think len(bids) > 2 would be moot, but I feel this "len(bids) == 2" is rather
harmful. Picking DELETE_CHANGED should be better than blindly take GET action.
Pulkit Goyal - Oct. 2, 2020, 1 p.m.
On Fri, Oct 2, 2020 at 5:29 PM Yuya Nishihara <yuya@tcha.org> wrote:
>
> On Wed, 30 Sep 2020 18:43:23 +0530, Pulkit Goyal wrote:
> > # HG changeset patch
> > # User Pulkit Goyal <7895pulkit@gmail.com>
> > # Date 1601461014 -19800
> > #      Wed Sep 30 15:46:54 2020 +0530
> > # Node ID 39f0021504cfdd65db22223de938cf6a123d18f8
> > # Parent  aa8ac5a5deb695a489172726016d3be57ad17cc1
> > # EXP-Topic merge-newnode-final
> > merge: if DELETED_CHANGED and GET are in actions, choose DELETED_CHANGED
> >
> > ACTION_GET represents that either the file is created on remote or it's newer on
> > the remote side. However, since we have a ACTION_DELETE_CHANGED too, it means
> > the file is not present locally and ACTION_GET is representing that file was
> > created on remote.
> >
> > Having both ACTION_GET and ACTION_DELETED_CHANGED is conflicting because one
> > says that file was created on remote and other says file has delete-changed
> > conflicts.
> >
> > Let's choose ACTION_DELETED_CHANGED which will result in conflicts and make user
> > choose the right way forward.
> >
> > diff --git a/mercurial/merge.py b/mercurial/merge.py
> > --- a/mercurial/merge.py
> > +++ b/mercurial/merge.py
> > @@ -1220,6 +1220,21 @@ def calculateupdates(
> >                  repo.ui.note(_(b" %s: picking 'keep new' action\n") % f)
> >                  mresult.addfile(f, *bids[mergestatemod.ACTION_KEEP_NEW][0])
> >                  continue
> > +            # ACTION_GET and ACTION_DELETE_CHANGED are conflicting actions as
> > +            # one action states the file is newer/created on remote side and
> > +            # other states that file is deleted locally and changed on remote
> > +            # side. Let's fallback and rely on a conflicting action to let user
> > +            # do the right thing
> > +            if (
> > +                mergestatemod.ACTION_DELETED_CHANGED in bids
> > +                and mergestatemod.ACTION_GET in bids
> > +                and len(bids) == 2
> > +            ):
>
> I think len(bids) > 2 would be moot, but I feel this "len(bids) == 2" is rather
> harmful. Picking DELETE_CHANGED should be better than blindly take GET action.

I completely agree with you, will send a followup. Going further, I
even wrote a patch[1] that always gives preference to delete-changed
or changed-delete actions over these non-conflicting actions. However,
I am still pondering whether that's a good idea or not.

[1]: https://foss.heptapod.net/octobus/mercurial-devel/-/merge_requests/42/diffs?commit_id=7ab70b0b6511e9ee808af71d95d7e40a6e6e95fe

Patch

diff --git a/tests/test-merge-changedelete.t b/tests/test-merge-changedelete.t
--- a/tests/test-merge-changedelete.t
+++ b/tests/test-merge-changedelete.t
@@ -1,3 +1,15 @@ 
+#testcases newfilenode old
+
+#if newfilenode
+Enable the config option
+------------------------
+
+  $ cat >> $HGRCPATH <<EOF
+  > [experimental]
+  > merge-track-salvaged = True
+  > EOF
+#endif
+
 Tests for change/delete conflicts, including:
 b5605d88dc27: Make ui.prompt repeat on "unrecognized response" again
  (issue897)