Patchwork [2,of,3,v2] resolve: print a warning when marking a file with conflict markers

login
register
mail settings
Submitter Gregory Szorc
Date Aug. 31, 2014, 7:56 a.m.
Message ID <35c1d268bbd092ffb15b.1409471773@gps-mbp.local>
Download mbox | patch
Permalink /patch/5648/
State Accepted
Headers show

Comments

Gregory Szorc - Aug. 31, 2014, 7:56 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1409419638 -7200
#      Sat Aug 30 19:27:18 2014 +0200
# Node ID 35c1d268bbd092ffb15b85605b4f5d36c3d08986
# Parent  55c81bbb9b76fe941faea87e77b0a5c95d4425f2
resolve: print a warning when marking a file with conflict markers

If you are using internal:merge or any mergetool that inserts
conflict markers and use `hg resolve -m`, there's a chance you may
accidentally mark a file with conflict markers as resolved. This
is almost always unintended.

This patch adds a warning so fewer people will shoot themselves with
this footgun.
Mads Kiilerich - Aug. 31, 2014, 1:35 p.m.
On 08/31/2014 09:56 AM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1409419638 -7200
> #      Sat Aug 30 19:27:18 2014 +0200
> # Node ID 35c1d268bbd092ffb15b85605b4f5d36c3d08986
> # Parent  55c81bbb9b76fe941faea87e77b0a5c95d4425f2
> resolve: print a warning when marking a file with conflict markers
>
> If you are using internal:merge or any mergetool that inserts
> conflict markers and use `hg resolve -m`, there's a chance you may
> accidentally mark a file with conflict markers as resolved. This
> is almost always unintended.
>
> This patch adds a warning so fewer people will shoot themselves with
> this footgun.

I think one root cause of that problem is that -m defaults to marking 
all files as resolved. Adressing that root cause would be better.

I think -m should require either a filename or --all. That would be the 
good kind of non-backward compatible change.

Anyway, a better implementation of this would perhaps be to avoid the 
layering violation and let resolve -m call filemerge in some way and let 
it run the check.conflicts check if it is configured for the current tool.

/Mads
Pierre-Yves David - Sept. 2, 2014, 7:11 p.m.
On 08/31/2014 03:35 PM, Mads Kiilerich wrote:
> On 08/31/2014 09:56 AM, Gregory Szorc wrote:
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc@gmail.com>
>> # Date 1409419638 -7200
>> #      Sat Aug 30 19:27:18 2014 +0200
>> # Node ID 35c1d268bbd092ffb15b85605b4f5d36c3d08986
>> # Parent  55c81bbb9b76fe941faea87e77b0a5c95d4425f2
>> resolve: print a warning when marking a file with conflict markers
>>
>> If you are using internal:merge or any mergetool that inserts
>> conflict markers and use `hg resolve -m`, there's a chance you may
>> accidentally mark a file with conflict markers as resolved. This
>> is almost always unintended.
>>
>> This patch adds a warning so fewer people will shoot themselves with
>> this footgun.
>
> I think one root cause of that problem is that -m defaults to marking
> all files as resolved. Adressing that root cause would be better.

Actually there is there multiple way to get that:

My main common cause for this is me missing a second markers after 
resolving the first one. (or me forgetting to drop the leading << or 
closing >> when dropping the rest of the markers.)

(I'm much less often bitten by that as I now use contrib/editmerge that 
happily reopen the file again if it detect any markers.)

I think this proposal is a good improvement.

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -16,8 +16,9 @@  import archival, changegroup, cmdutil, h
 import sshserver, hgweb, commandserver
 import extensions
 from hgweb import server as hgweb_server
 import merge as mergemod
+import filemerge
 import minirst, revset, fileset
 import dagparser, context, simplemerge, graphmod
 import random
 import setdiscovery, treediscovery, dagutil, pvec, localrepo
@@ -5079,9 +5080,10 @@  def resolve(ui, repo, *pats, **opts):
         if not ms.active() and not show:
             raise util.Abort(
                 _('resolve command not applicable when not merging'))
 
-        m = scmutil.match(repo[None], pats, opts)
+        wctx = repo[None]
+        m = scmutil.match(wctx, pats, opts)
         ret = 0
         didwork = False
 
         for f in ms:
@@ -5097,8 +5099,11 @@  def resolve(ui, repo, *pats, **opts):
                     ui.write("%s %s\n" % (ms[f].upper(), f),
                              label='resolve.' +
                              {'u': 'unresolved', 'r': 'resolved'}[ms[f]])
             elif mark:
+                if filemerge.hasconflictmarkers(wctx.filectx(f)):
+                    ui.warn(_('(%s appears to contain conflict markers; '
+                              'did you intend to mark as resolved?)\n') % f)
                 ms.mark(f, "r")
             elif unmark:
                 ms.mark(f, "u")
             else:
diff --git a/tests/test-add.t b/tests/test-add.t
--- a/tests/test-add.t
+++ b/tests/test-add.t
@@ -106,8 +106,9 @@  should fail
   $ hg st
   M a
   ? a.orig
   $ hg resolve -m a
+  (a appears to contain conflict markers; did you intend to mark as resolved?)
   (no more unresolved files)
   $ hg ci -m merge
 
 Issue683: peculiarity with hg revert of an removed then added file
diff --git a/tests/test-commit-amend.t b/tests/test-commit-amend.t
--- a/tests/test-commit-amend.t
+++ b/tests/test-commit-amend.t
@@ -590,8 +590,9 @@  Amend a merge changeset (with renames an
   warning: conflicts during merge.
   merging cc incomplete! (edit conflicts, then use 'hg resolve --mark')
   [1]
   $ hg resolve -m cc
+  (cc appears to contain conflict markers; did you intend to mark as resolved?)
   (no more unresolved files)
   $ hg ci -m 'merge bar'
   $ hg log --config diff.git=1 -pr .
   changeset:   23:93cd4445f720
diff --git a/tests/test-mq-qnew.t b/tests/test-mq-qnew.t
--- a/tests/test-mq-qnew.t
+++ b/tests/test-mq-qnew.t
@@ -157,8 +157,9 @@  plain headers
   warning: conflicts during merge.
   merging a incomplete! (edit conflicts, then use 'hg resolve --mark')
   0 files updated, 0 files merged, 0 files removed, 1 files unresolved
   use 'hg resolve' to retry unresolved file merges or 'hg update -C .' to abandon
+  (a appears to contain conflict markers; did you intend to mark as resolved?)
   (no more unresolved files)
   abort: cannot manage merge changesets
   $ rm -r sandbox
 
@@ -231,8 +232,9 @@  hg headers
   warning: conflicts during merge.
   merging a incomplete! (edit conflicts, then use 'hg resolve --mark')
   0 files updated, 0 files merged, 0 files removed, 1 files unresolved
   use 'hg resolve' to retry unresolved file merges or 'hg update -C .' to abandon
+  (a appears to contain conflict markers; did you intend to mark as resolved?)
   (no more unresolved files)
   abort: cannot manage merge changesets
   $ rm -r sandbox
 
diff --git a/tests/test-resolve.t b/tests/test-resolve.t
--- a/tests/test-resolve.t
+++ b/tests/test-resolve.t
@@ -59,5 +59,26 @@  test crashed merge with empty mergestate
 resolve -l, should be empty
 
   $ hg resolve -l
 
-  $ cd ..
+conflict markers in file marked as resolved will emit a warning
+
+  $ hg up -r 0
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ echo another-merge >> file
+  $ hg commit -m 'test conflict markers'
+  created new head
+  $ hg merge --tool=internal:fail
+  0 files updated, 0 files merged, 0 files removed, 1 files unresolved
+  use 'hg resolve' to retry unresolved file merges or 'hg update -C .' to abandon
+  [1]
+
+  $ cat >> file << EOF
+  > <<<<<<< dest
+  > foo
+  > ======= source
+  > bar
+  > >>>>>>>
+  > EOF
+  $ hg resolve -m file
+  (file appears to contain conflict markers; did you intend to mark as resolved?)
+  (no more unresolved files)
diff --git a/tests/test-status-color.t b/tests/test-status-color.t
--- a/tests/test-status-color.t
+++ b/tests/test-status-color.t
@@ -311,8 +311,9 @@  test 'resolve -l'
   0 files updated, 0 files merged, 0 files removed, 2 files unresolved
   use 'hg resolve' to retry unresolved file merges or 'hg update -C .' to abandon
   [1]
   $ hg resolve -m b
+  (b appears to contain conflict markers; did you intend to mark as resolved?)
 
 hg resolve with one unresolved, one resolved:
 
   $ hg resolve --color=always -l