Patchwork [STABLE,checkunknown] merge: respect checkunknown/checkignored when force is True

login
register
mail settings
Submitter Siddharth Agarwal
Date Jan. 22, 2016, 10:08 p.m.
Message ID <ab22c1449e8591635e54.1453500513@dev666.prn1.facebook.com>
Download mbox | patch
Permalink /patch/12872/
State Changes Requested
Headers show

Comments

Siddharth Agarwal - Jan. 22, 2016, 10:08 p.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1453500273 28800
#      Fri Jan 22 14:04:33 2016 -0800
# Branch stable
# Node ID ab22c1449e8591635e545b9edea6c8d023b6a606
# Parent  158bdc8965720ca4061f8f8d806563cfc7cdb62e
# Available At http://42.netv6.net/sid0-wip/hg/
#              hg pull http://42.netv6.net/sid0-wip/hg/ -r ab22c1449e85
merge: respect checkunknown/checkignored when force is True

checkunknown and checkignored are currently respected for updates and regular
merges, but not for certain kinds of rebases. To be precise, they aren't
respected for rebases when:

(1) we're rebasing while currently on the destination commit, and
(2) an untracked or ignored file F is currently in the working copy, and
(3) the same file F is in a source commit, and
(4) F has different contents in the source commit.

This happens because rebases set force to True when calling merge.update.
Setting force to True makes a lot of sense in general, but it turns out the
force option is overloaded: there's a deprecated '--force' option in merge that
allows you to merge in outstanding changes, including changes in untracked
files. Those are the semantics that _checkunknownfiles follows, and that's
covered by the 'cm' action.

I think the behavior during rebases when checkunknown is 'abort' (the default)
is wrong -- we should abort on or overwrite differing untracked files, not try
to merge them in. However that would likely be a pretty big BC change, so I'm
disinclined to do that for stable. I think getting the semantics for 'warn' and
'ignore' correct before they're set in stone is more important.
Pierre-Yves David - Jan. 23, 2016, 12:16 a.m.
On 01/22/2016 02:08 PM, Siddharth Agarwal wrote:
> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1453500273 28800
> #      Fri Jan 22 14:04:33 2016 -0800
> # Branch stable
> # Node ID ab22c1449e8591635e545b9edea6c8d023b6a606
> # Parent  158bdc8965720ca4061f8f8d806563cfc7cdb62e
> # Available At http://42.netv6.net/sid0-wip/hg/
> #              hg pull http://42.netv6.net/sid0-wip/hg/ -r ab22c1449e85
> merge: respect checkunknown/checkignored when force is True
>
> checkunknown and checkignored are currently respected for updates and regular
> merges, but not for certain kinds of rebases. To be precise, they aren't
> respected for rebases when:
>
> (1) we're rebasing while currently on the destination commit, and
> (2) an untracked or ignored file F is currently in the working copy, and
> (3) the same file F is in a source commit, and
> (4) F has different contents in the source commit.
>
> This happens because rebases set force to True when calling merge.update.
> Setting force to True makes a lot of sense in general, but it turns out the
> force option is overloaded: there's a deprecated '--force' option in merge that
> allows you to merge in outstanding changes, including changes in untracked
> files. Those are the semantics that _checkunknownfiles follows, and that's
> covered by the 'cm' action.
>
> I think the behavior during rebases when checkunknown is 'abort' (the default)
> is wrong -- we should abort on or overwrite differing untracked files, not try
> to merge them in. However that would likely be a pretty big BC change, so I'm
> disinclined to do that for stable. I think getting the semantics for 'warn' and
> 'ignore' correct before they're set in stone is more important.

 From reading this, it seems like `hg merge --force` (deprecated) and 
`hg rebase` both us a `force=True` parameters down the stack to carry 
sightly different meaning.

The current behavior is built around the `hg merge --force` use case.
You series seems to be changing the behavior of this `force=True` to 
match something fitting the `hg rebase needs`. Correct?

If so, could we not get a `forcemerge=True` and a `force=True` parameter 
that strictly disambiguates the intent here?
Siddharth Agarwal - Jan. 23, 2016, 12:54 a.m.
On 1/22/16 16:16, Pierre-Yves David wrote:
>
>
> On 01/22/2016 02:08 PM, Siddharth Agarwal wrote:
>> # HG changeset patch
>> # User Siddharth Agarwal <sid0@fb.com>
>> # Date 1453500273 28800
>> #      Fri Jan 22 14:04:33 2016 -0800
>> # Branch stable
>> # Node ID ab22c1449e8591635e545b9edea6c8d023b6a606
>> # Parent  158bdc8965720ca4061f8f8d806563cfc7cdb62e
>> # Available At http://42.netv6.net/sid0-wip/hg/
>> #              hg pull http://42.netv6.net/sid0-wip/hg/ -r ab22c1449e85
>> merge: respect checkunknown/checkignored when force is True
>>
>> checkunknown and checkignored are currently respected for updates and 
>> regular
>> merges, but not for certain kinds of rebases. To be precise, they aren't
>> respected for rebases when:
>>
>> (1) we're rebasing while currently on the destination commit, and
>> (2) an untracked or ignored file F is currently in the working copy, and
>> (3) the same file F is in a source commit, and
>> (4) F has different contents in the source commit.
>>
>> This happens because rebases set force to True when calling 
>> merge.update.
>> Setting force to True makes a lot of sense in general, but it turns 
>> out the
>> force option is overloaded: there's a deprecated '--force' option in 
>> merge that
>> allows you to merge in outstanding changes, including changes in 
>> untracked
>> files. Those are the semantics that _checkunknownfiles follows, and 
>> that's
>> covered by the 'cm' action.
>>
>> I think the behavior during rebases when checkunknown is 'abort' (the 
>> default)
>> is wrong -- we should abort on or overwrite differing untracked 
>> files, not try
>> to merge them in. However that would likely be a pretty big BC 
>> change, so I'm
>> disinclined to do that for stable. I think getting the semantics for 
>> 'warn' and
>> 'ignore' correct before they're set in stone is more important.
>
> From reading this, it seems like `hg merge --force` (deprecated) and 
> `hg rebase` both us a `force=True` parameters down the stack to carry 
> sightly different meaning.
>
> The current behavior is built around the `hg merge --force` use case.
> You series seems to be changing the behavior of this `force=True` to 
> match something fitting the `hg rebase needs`. Correct?

Yes, that's exactly correct.

> If so, could we not get a `forcemerge=True` and a `force=True` 
> parameter that strictly disambiguates the intent here?

That's my plan for after the freeze. I think doing this before the 
freeze is going to be too disruptive.
Pierre-Yves David - Jan. 30, 2016, 2:23 p.m.
On 01/23/2016 01:54 AM, Siddharth Agarwal wrote:
> On 1/22/16 16:16, Pierre-Yves David wrote:
>>
>>
>> On 01/22/2016 02:08 PM, Siddharth Agarwal wrote:
>>> # HG changeset patch
>>> # User Siddharth Agarwal <sid0@fb.com>
>>> # Date 1453500273 28800
>>> #      Fri Jan 22 14:04:33 2016 -0800
>>> # Branch stable
>>> # Node ID ab22c1449e8591635e545b9edea6c8d023b6a606
>>> # Parent  158bdc8965720ca4061f8f8d806563cfc7cdb62e
>>> # Available At http://42.netv6.net/sid0-wip/hg/
>>> #              hg pull http://42.netv6.net/sid0-wip/hg/ -r ab22c1449e85
>>> merge: respect checkunknown/checkignored when force is True
>>>
>>> checkunknown and checkignored are currently respected for updates and
>>> regular
>>> merges, but not for certain kinds of rebases. To be precise, they aren't
>>> respected for rebases when:
>>>
>>> (1) we're rebasing while currently on the destination commit, and
>>> (2) an untracked or ignored file F is currently in the working copy, and
>>> (3) the same file F is in a source commit, and
>>> (4) F has different contents in the source commit.
>>>
>>> This happens because rebases set force to True when calling
>>> merge.update.
>>> Setting force to True makes a lot of sense in general, but it turns
>>> out the
>>> force option is overloaded: there's a deprecated '--force' option in
>>> merge that
>>> allows you to merge in outstanding changes, including changes in
>>> untracked
>>> files. Those are the semantics that _checkunknownfiles follows, and
>>> that's
>>> covered by the 'cm' action.
>>>
>>> I think the behavior during rebases when checkunknown is 'abort' (the
>>> default)
>>> is wrong -- we should abort on or overwrite differing untracked
>>> files, not try
>>> to merge them in. However that would likely be a pretty big BC
>>> change, so I'm
>>> disinclined to do that for stable. I think getting the semantics for
>>> 'warn' and
>>> 'ignore' correct before they're set in stone is more important.
>>
>> From reading this, it seems like `hg merge --force` (deprecated) and
>> `hg rebase` both us a `force=True` parameters down the stack to carry
>> sightly different meaning.
>>
>> The current behavior is built around the `hg merge --force` use case.
>> You series seems to be changing the behavior of this `force=True` to
>> match something fitting the `hg rebase needs`. Correct?
>
> Yes, that's exactly correct.
>
>> If so, could we not get a `forcemerge=True` and a `force=True`
>> parameter that strictly disambiguates the intent here?
>
> That's my plan for after the freeze. I think doing this before the
> freeze is going to be too disruptive.

For the record, we talked to each other in person and we decided to 
undocument the new options until the behavior have been fixed (in 3.8).

Patch undocumenting them have landed.

Patch

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -582,17 +582,17 @@  def _checkunknownfiles(repo, wctx, mctx,
     choose a different action.
     """
     conflicts = set()
+    warnconflicts = set()
+    unknownconfig = _getcheckunknownconfig(repo, 'merge', 'checkunknown')
+    ignoredconfig = _getcheckunknownconfig(repo, 'merge', 'checkignored')
     if not force:
         abortconflicts = set()
-        warnconflicts = set()
         def collectconflicts(conflicts, config):
             if config == 'abort':
                 abortconflicts.update(conflicts)
             elif config == 'warn':
                 warnconflicts.update(conflicts)
 
-        unknownconfig = _getcheckunknownconfig(repo, 'merge', 'checkunknown')
-        ignoredconfig = _getcheckunknownconfig(repo, 'merge', 'checkignored')
         for f, (m, args, msg) in actions.iteritems():
             if m in ('c', 'dc'):
                 if _checkunknownfile(repo, wctx, mctx, f):
@@ -611,23 +611,44 @@  def _checkunknownfiles(repo, wctx, mctx,
         if abortconflicts:
             raise error.Abort(_("untracked files in working directory "
                                 "differ from files in requested revision"))
+    else:
+        # 'cm' should only appear when doing a force merge
+        for f, (m, args, msg) in actions.iteritems():
+            if m == 'cm':
+                fl2, anc = args
+                different = _checkunknownfile(repo, wctx, mctx, f)
+                if repo.dirstate._ignore(f):
+                    config = ignoredconfig
+                else:
+                    config = unknownconfig
 
-        for f in sorted(warnconflicts):
-            repo.ui.warn(_("%s: replacing untracked file\n") % f)
+                # The behavior when force is True is described by this table:
+                #  config  different  |    action    backup
+                #    *         n      |      get        n
+                #   abort      y      |     merge       -
+                #   warn       y      |  warn + get     y
+                #  ignore      y      |      get        y
+                #
+                # TODO: For rebases, where force is True, config = abort should
+                # probably abort on differing untracked files, not merge.
+                if not different:
+                    actions[f] = ('g', (fl2, False), "remote created")
+                elif config == 'abort':
+                    actions[f] = ('m', (f, f, None, False, anc),
+                                  "remote differs from untracked local")
+                else:
+                    if config == 'warn':
+                        warnconflicts.add(f)
+                    actions[f] = ('g', (fl2, True), "remote created")
+
+    for f in sorted(warnconflicts):
+        repo.ui.warn(_("%s: replacing untracked file\n") % f)
 
     for f, (m, args, msg) in actions.iteritems():
         backup = f in conflicts
         if m == 'c':
             flags, = args
             actions[f] = ('g', (flags, backup), msg)
-        elif m == 'cm':
-            fl2, anc = args
-            different = _checkunknownfile(repo, wctx, mctx, f)
-            if different:
-                actions[f] = ('m', (f, f, None, False, anc),
-                              "remote differs from untracked local")
-            else:
-                actions[f] = ('g', (fl2, backup), "remote created")
 
 def _forgetremoved(wctx, mctx, branchmerge):
     """
diff --git a/tests/test-rebase-scenario-global.t b/tests/test-rebase-scenario-global.t
--- a/tests/test-rebase-scenario-global.t
+++ b/tests/test-rebase-scenario-global.t
@@ -25,7 +25,8 @@ 
 
 Rebasing
 D onto H - simple rebase:
-(this also tests that editor is invoked if '--edit' is specified)
+(this also tests that editor is invoked if '--edit' is specified, and that we
+can warn for colliding untracked files)
 
   $ hg clone -q -u . a a1
   $ cd a1
@@ -50,8 +51,10 @@  D onto H - simple rebase:
 
   $ hg status --rev "3^1" --rev 3
   A D
-  $ HGEDITOR=cat hg rebase -s 3 -d 7 --edit
+  $ echo collide > D
+  $ HGEDITOR=cat hg rebase -s 3 -d 7 --edit --config merge.checkunknown=warn
   rebasing 3:32af7686d403 "D"
+  D: replacing untracked file
   D
   
   
@@ -62,6 +65,9 @@  D onto H - simple rebase:
   HG: branch 'default'
   HG: added D
   saved backup bundle to $TESTTMP/a1/.hg/strip-backup/32af7686d403-6f7dface-backup.hg (glob)
+  $ cat D.orig
+  collide
+  $ rm D.orig
 
   $ hg tglog
   o  7: 'D'
@@ -84,14 +90,19 @@  D onto H - simple rebase:
 
 
 D onto F - intermediate point:
-(this also tests that editor is not invoked if '--edit' is not specified)
+(this also tests that editor is not invoked if '--edit' is not specified, and
+that we can ignore for colliding untracked files)
 
   $ hg clone -q -u . a a2
   $ cd a2
+  $ echo collide > D
 
-  $ HGEDITOR=cat hg rebase -s 3 -d 5
+  $ HGEDITOR=cat hg rebase -s 3 -d 5 --config merge.checkunknown=ignore
   rebasing 3:32af7686d403 "D"
   saved backup bundle to $TESTTMP/a2/.hg/strip-backup/32af7686d403-6f7dface-backup.hg (glob)
+  $ cat D.orig
+  collide
+  $ rm D.orig
 
   $ hg tglog
   o  7: 'D'
@@ -114,15 +125,21 @@  D onto F - intermediate point:
 
 
 E onto H - skip of G:
+(this also tests that we can overwrite untracked files and don't create backups
+if they have the same contents)
 
   $ hg clone -q -u . a a3
   $ cd a3
+  $ hg cat -r 4 E | tee E
+  E
 
   $ hg rebase -s 4 -d 7
   rebasing 4:9520eea781bc "E"
   rebasing 6:eea13746799a "G"
   note: rebase of 6:eea13746799a created no changes to commit
   saved backup bundle to $TESTTMP/a3/.hg/strip-backup/9520eea781bc-fcd8edd4-backup.hg (glob)
+  $ f E.orig
+  E.orig: file not found
 
   $ hg tglog
   o  6: 'E'