Patchwork [2,of,3,STABLE?] filemerge: treat EOF at prompt as fail, not abort

login
register
mail settings
Submitter Siddharth Agarwal
Date Nov. 11, 2015, 1:20 a.m.
Message ID <0829cdf6d0d3da14077f.1447204828@dev666.prn1.facebook.com>
Download mbox | patch
Permalink /patch/11361/
State Accepted
Commit 33eb8a56d0c993bce4bda8f07325d49f6fa40dd0
Headers show

Comments

Siddharth Agarwal - Nov. 11, 2015, 1:20 a.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1447204396 28800
#      Tue Nov 10 17:13:16 2015 -0800
# Node ID 0829cdf6d0d3da14077f4de0d73c8bddf6190f06
# Parent  625d11b1fc9b3709335b885ccdd753e3062f1ab3
filemerge: treat EOF at prompt as fail, not abort

Previously we'd abort the merge entirely if there was an EOF at the prompt.
This is unnecessary -- it's much better to simply fail and treat the file as
unresolved instead.
Martin von Zweigbergk - Nov. 11, 2015, 6:12 p.m.
On Tue, Nov 10, 2015 at 5:23 PM Siddharth Agarwal <sid0@fb.com> wrote:

> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1447204396 28800
> #      Tue Nov 10 17:13:16 2015 -0800
> # Node ID 0829cdf6d0d3da14077f4de0d73c8bddf6190f06
> # Parent  625d11b1fc9b3709335b885ccdd753e3062f1ab3
> filemerge: treat EOF at prompt as fail, not abort
>
> Previously we'd abort the merge entirely if there was an EOF at the prompt.
> This is unnecessary -- it's much better to simply fail and treat the file
> as
> unresolved instead.
>

Great. Thanks for fixing this. I also ran into this case when I was working
on change/delete conflicts.


>
> diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
> --- a/mercurial/filemerge.py
> +++ b/mercurial/filemerge.py
> @@ -175,15 +175,19 @@ def _iprompt(repo, mynode, orig, fcd, fc
>      ui = repo.ui
>      fd = fcd.path()
>
> -    index = ui.promptchoice(_(" no tool found to merge %s\n"
> -                              "keep (l)ocal or take (o)ther?"
> -                              "$$ &Local $$ &Other") % fd, 0)
> -    choice = ['local', 'other'][index]
> +    try:
> +        index = ui.promptchoice(_(" no tool found to merge %s\n"
> +                                  "keep (l)ocal or take (o)ther?"
> +                                  "$$ &Local $$ &Other") % fd, 0)
> +        choice = ['local', 'other'][index]
>
> -    if choice == 'other':
> -        return _iother(repo, mynode, orig, fcd, fco, fca, toolconf)
> -    else:
> -        return _ilocal(repo, mynode, orig, fcd, fco, fca, toolconf)
> +        if choice == 'other':
> +            return _iother(repo, mynode, orig, fcd, fco, fca, toolconf)
> +        else:
> +            return _ilocal(repo, mynode, orig, fcd, fco, fca, toolconf)
> +    except error.ResponseExpected:
>

Aha, here's the ResponseExpected! I would have been much less mystified if
that patch had come in this series instead of on its own. (I would probably
have called it "ResponseMissing" or "NoInput" or similar. The last line
there now reads almost "except if a response is expected".)


> +        ui.write("\n")
> +        return 1
>
>  @internaltool('local', nomerge)
>  def _ilocal(repo, mynode, orig, fcd, fco, fca, toolconf):
> diff --git a/tests/test-merge-tools.t b/tests/test-merge-tools.t
> --- a/tests/test-merge-tools.t
> +++ b/tests/test-merge-tools.t
> @@ -544,6 +544,41 @@ ui.merge specifies internal:prompt:
>    # hg resolve --list
>    R f
>
> +prompt with EOF
> +
> +  $ beforemerge
> +  [merge-tools]
> +  false.whatever=
>
+  true.priority=1
> +  true.executable=cat
>
+  # hg update -C 1
> +  $ hg merge -r 2 --config ui.merge=internal:prompt --config
> ui.interactive=true


Could use "--tool internal:prompt" instead of config here and below? But
not worth resending for anyway.


> +   no tool found to merge f
>

This line is a  little misleading, but obviously not this patch's fault.

Also, I'll send a patch for removing the leading space some day. I don't
see a reason for it to be there.


> +  keep (l)ocal or take (o)ther?
> +  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]
> +  $ aftermerge
> +  # cat f
> +  revision 1
> +  space
> +  # hg stat
> +  M f
> +  # hg resolve --list
> +  U f
> +  $ hg resolve --all --config ui.merge=internal:prompt --config
> ui.interactive=true
> +   no tool found to merge f
> +  keep (l)ocal or take (o)ther?
> +  [1]
> +  $ aftermerge
> +  # cat f
> +  revision 1
> +  space
> +  # hg stat
> +  M f
> +  ? f.orig
> +  # hg resolve --list
> +  U f
>  ui.merge specifies internal:dump:
>
>    $ beforemerge
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
>

Patch

diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
--- a/mercurial/filemerge.py
+++ b/mercurial/filemerge.py
@@ -175,15 +175,19 @@  def _iprompt(repo, mynode, orig, fcd, fc
     ui = repo.ui
     fd = fcd.path()
 
-    index = ui.promptchoice(_(" no tool found to merge %s\n"
-                              "keep (l)ocal or take (o)ther?"
-                              "$$ &Local $$ &Other") % fd, 0)
-    choice = ['local', 'other'][index]
+    try:
+        index = ui.promptchoice(_(" no tool found to merge %s\n"
+                                  "keep (l)ocal or take (o)ther?"
+                                  "$$ &Local $$ &Other") % fd, 0)
+        choice = ['local', 'other'][index]
 
-    if choice == 'other':
-        return _iother(repo, mynode, orig, fcd, fco, fca, toolconf)
-    else:
-        return _ilocal(repo, mynode, orig, fcd, fco, fca, toolconf)
+        if choice == 'other':
+            return _iother(repo, mynode, orig, fcd, fco, fca, toolconf)
+        else:
+            return _ilocal(repo, mynode, orig, fcd, fco, fca, toolconf)
+    except error.ResponseExpected:
+        ui.write("\n")
+        return 1
 
 @internaltool('local', nomerge)
 def _ilocal(repo, mynode, orig, fcd, fco, fca, toolconf):
diff --git a/tests/test-merge-tools.t b/tests/test-merge-tools.t
--- a/tests/test-merge-tools.t
+++ b/tests/test-merge-tools.t
@@ -544,6 +544,41 @@  ui.merge specifies internal:prompt:
   # hg resolve --list
   R f
 
+prompt with EOF
+
+  $ beforemerge
+  [merge-tools]
+  false.whatever=
+  true.priority=1
+  true.executable=cat
+  # hg update -C 1
+  $ hg merge -r 2 --config ui.merge=internal:prompt --config ui.interactive=true
+   no tool found to merge f
+  keep (l)ocal or take (o)ther? 
+  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]
+  $ aftermerge
+  # cat f
+  revision 1
+  space
+  # hg stat
+  M f
+  # hg resolve --list
+  U f
+  $ hg resolve --all --config ui.merge=internal:prompt --config ui.interactive=true
+   no tool found to merge f
+  keep (l)ocal or take (o)ther? 
+  [1]
+  $ aftermerge
+  # cat f
+  revision 1
+  space
+  # hg stat
+  M f
+  ? f.orig
+  # hg resolve --list
+  U f
 ui.merge specifies internal:dump:
 
   $ beforemerge