Patchwork [1,of,3,stable] extdiff: reintroduce backward compatibility with manual quoting of parameters

login
register
mail settings
Submitter Mads Kiilerich
Date Jan. 27, 2015, 2:15 a.m.
Message ID <aa7caa9b564fca7095f8.1422324950@ssl.google-analytics.com>
Download mbox | patch
Permalink /patch/7556/
State Superseded
Commit 01e5b7323a487e873b728c8b150441685b0f8f2f
Headers show

Comments

Mads Kiilerich - Jan. 27, 2015, 2:15 a.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1422323932 -3600
#      Tue Jan 27 02:58:52 2015 +0100
# Branch stable
# Node ID aa7caa9b564fca7095f860178f18737cf360285d
# Parent  0c4419faacbcca691737b5e25820dcbf4c7150ac
extdiff: reintroduce backward compatibility with manual quoting of parameters

72a89cf86fcd broke things ... and the following cleanups didn't fix all issues.
It didn't work with the diffargs shipped in mergetools.rc with explicit
quoting. Parameters would end up with being quoted twice - especially if they
really needed quoting.

To fix that, look for explicit quotes around the variables that will be
substituted with proper quoting. Also accept an additional prefix so we can
handle both
  --foo='$parent'
and
  '--foo=$parent'
Matt Mackall - Jan. 27, 2015, 10:53 p.m.
On Tue, 2015-01-27 at 03:15 +0100, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1422323932 -3600
> #      Tue Jan 27 02:58:52 2015 +0100
> # Branch stable
> # Node ID aa7caa9b564fca7095f860178f18737cf360285d
> # Parent  0c4419faacbcca691737b5e25820dcbf4c7150ac
> extdiff: reintroduce backward compatibility with manual quoting of parameters

This had troubles:

$ python2.6
Python 2.6.7 (r267:88850, Aug  3 2011, 11:33:52) 
[GCC 4.6.1] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import re
>>> e = '''(['"]?)([^ '"]*)?'''
>>> re.compile(e)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.6/re.py", line 190, in compile
    return _compile(pattern, flags)
  File "/usr/lib/python2.6/re.py", line 245, in _compile
    raise error, v # invalid expression
sre_constants.error: nothing to repeat
>>> re.compile(e[:-1])
<_sre.SRE_Pattern object at 0x7fd8bd6e7128>
>>> 

Seems like the last ? is unnecessary, but I'd like to bounce it back to
you to confirm.
Mads Kiilerich - Jan. 28, 2015, 1:29 a.m.
On 01/27/2015 11:53 PM, Matt Mackall wrote:
> On Tue, 2015-01-27 at 03:15 +0100, Mads Kiilerich wrote:
>> # HG changeset patch
>> # User Mads Kiilerich <madski@unity3d.com>
>> # Date 1422323932 -3600
>> #      Tue Jan 27 02:58:52 2015 +0100
>> # Branch stable
>> # Node ID aa7caa9b564fca7095f860178f18737cf360285d
>> # Parent  0c4419faacbcca691737b5e25820dcbf4c7150ac
>> extdiff: reintroduce backward compatibility with manual quoting of parameters
> This had troubles:
>
> $ python2.6
> Python 2.6.7 (r267:88850, Aug  3 2011, 11:33:52)
> [GCC 4.6.1] on linux2
> Type "help", "copyright", "credits" or "license" for more information.
>>>> import re
>>>> e = '''(['"]?)([^ '"]*)?'''
>>>> re.compile(e)
> Traceback (most recent call last):
>    File "<stdin>", line 1, in <module>
>    File "/usr/lib/python2.6/re.py", line 190, in compile
>      return _compile(pattern, flags)
>    File "/usr/lib/python2.6/re.py", line 245, in _compile
>      raise error, v # invalid expression
> sre_constants.error: nothing to repeat
>>>> re.compile(e[:-1])
> <_sre.SRE_Pattern object at 0x7fd8bd6e7128>
> Seems like the last ? is unnecessary, but I'd like to bounce it back to
> you to confirm.

:-(

Yes, the extra '?' is unnecessary. It was left over from an attempt of 
making sure the 'pre' group only matched when there were leading quotes, 
as in
(?:(['"])([^ '"]*))?(...)\1
but that doesn't work; you can't back reference an optional group. It 
would perhaps be better to have separate expressions for the 3 cases of 
no, single and double quoting.

Also, having thought more about it, I would also like to add '$' to the 
excluded group to minimize the risk of doing the wrong thing on an 
argument with more than one variable reference without whitespace 
separation.

Resending.

/Mads

Patch

diff --git a/hgext/extdiff.py b/hgext/extdiff.py
--- a/hgext/extdiff.py
+++ b/hgext/extdiff.py
@@ -212,13 +212,15 @@  def dodiff(ui, repo, cmdline, pats, opts
                    'clabel': label2, 'child': dir2,
                    'root': repo.root}
         def quote(match):
-            key = match.group()[1:]
+            pre = match.group(2)
+            key = match.group(3)
             if not do3way and key == 'parent2':
-                return ''
-            return util.shellquote(replace[key])
+                return pre
+            return pre + util.shellquote(replace[key])
 
         # Match parent2 first, so 'parent1?' will match both parent1 and parent
-        regex = '\$(parent2|parent1?|child|plabel1|plabel2|clabel|root)'
+        regex = (r'''(['"]?)([^ '"]*)?'''
+                 r'\$(parent2|parent1?|child|plabel1|plabel2|clabel|root)\1')
         if not do3way and not re.search(regex, cmdline):
             cmdline += ' $parent1 $child'
         cmdline = re.sub(regex, quote, cmdline)
diff --git a/tests/test-extdiff.t b/tests/test-extdiff.t
--- a/tests/test-extdiff.t
+++ b/tests/test-extdiff.t
@@ -160,6 +160,28 @@  issue4463: usage of command line configu
   running "echo echo-naked 'being quoted' */a $TESTTMP/a/a" in */extdiff.* (glob)
 #endif
 
+  $ touch 'sp ace'
+  $ hg add 'sp ace'
+  $ hg ci -m 'sp ace'
+  created new head
+  $ echo > 'sp ace'
+
+Test pre-72a89cf86fcd backward compatibility with half-baked manual quoting
+
+  $ cat <<EOF >> $HGRCPATH
+  > [extdiff]
+  > odd =
+  > [merge-tools]
+  > odd.diffargs = --foo='\$clabel' '\$clabel' "--bar=\$clabel" "\$clabel"
+  > odd.executable = echo
+  > EOF
+#if windows
+TODO
+#else
+  $ hg --debug odd | grep '^running'
+  running "/bin/echo --foo='sp ace' 'sp ace' --bar='sp ace' 'sp ace'" in /tmp/extdiff.* (glob)
+#endif
+
 #if execbit
 
 Test extdiff of multiple files in tmp dir: