Patchwork [4,of,4] windows: replace single quote with double quote when translating to cmd.exe

login
register
mail settings
Submitter Matt Harbison
Date July 9, 2018, 3:27 p.m.
Message ID <53901aad0dbbf6d194a5.1531150067@Envy>
Download mbox | patch
Permalink /patch/32714/
State Accepted
Headers show

Comments

Matt Harbison - July 9, 2018, 3:27 p.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1531026341 14400
#      Sun Jul 08 01:05:41 2018 -0400
# Node ID 53901aad0dbbf6d194a502ec437e4ef7d285e3fe
# Parent  6c1736648339979cd4950d435cd696f0d4a4a186
windows: replace single quote with double quote when translating to cmd.exe

Since cmd.exe doesn't understand single quotes, single quotes to prevent $var
expansion is basically unusable without this.

Translation is disabled in test-bookmarks.t because the hook is `sh -c "..."`,
and while many of the arguments in the ellipsis are escaped double quotes, there
are a few single quotes that get rewritten as unescaped double quotes, throwing
off the whole command.  It seems more important that the native cmd.exe work out
of the box than sh.exe.
Yuya Nishihara - July 10, 2018, 12:43 p.m.
On Mon, 09 Jul 2018 11:27:47 -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1531026341 14400
> #      Sun Jul 08 01:05:41 2018 -0400
> # Node ID 53901aad0dbbf6d194a502ec437e4ef7d285e3fe
> # Parent  6c1736648339979cd4950d435cd696f0d4a4a186
> windows: replace single quote with double quote when translating to cmd.exe
> 
> Since cmd.exe doesn't understand single quotes, single quotes to prevent $var
> expansion is basically unusable without this.

What if a user expects <'> will be passed as <'> itself?

I think it's okay to enable the $var translation by default because it's limited
to the set of the known variable names, but for <'>, there's nothing to prevent
unexpected translation. If we want this, the "tonative" option should be off by
default.
Matt Harbison - July 10, 2018, 3:02 p.m.
> On Jul 10, 2018, at 8:43 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> 
>> On Mon, 09 Jul 2018 11:27:47 -0400, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1531026341 14400
>> #      Sun Jul 08 01:05:41 2018 -0400
>> # Node ID 53901aad0dbbf6d194a502ec437e4ef7d285e3fe
>> # Parent  6c1736648339979cd4950d435cd696f0d4a4a186
>> windows: replace single quote with double quote when translating to cmd.exe
>> 
>> Since cmd.exe doesn't understand single quotes, single quotes to prevent $var
>> expansion is basically unusable without this.
> 
> What if a user expects <'> will be passed as <'> itself?
> 
> I think it's okay to enable the $var translation by default because it's limited
> to the set of the known variable names, but for <'>, there's nothing to prevent
> unexpected translation. If we want this, the "tonative" option should be off by
> default.

Since single quote isn’t allowed in a path name and isn’t recognized as a shell quote, I figured nobody would use it (except to run sh.exe, but that probably should disable translating anyway).

I’d also like to change ~/ to %USERPROFILE%/.  So if you think it’s a problem, I’m ok with defaulting to off.
Yuya Nishihara - July 10, 2018, 3:27 p.m.
On Tue, 10 Jul 2018 11:02:42 -0400, Matt Harbison wrote:
> 
> > On Jul 10, 2018, at 8:43 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > 
> >> On Mon, 09 Jul 2018 11:27:47 -0400, Matt Harbison wrote:
> >> # HG changeset patch
> >> # User Matt Harbison <matt_harbison@yahoo.com>
> >> # Date 1531026341 14400
> >> #      Sun Jul 08 01:05:41 2018 -0400
> >> # Node ID 53901aad0dbbf6d194a502ec437e4ef7d285e3fe
> >> # Parent  6c1736648339979cd4950d435cd696f0d4a4a186
> >> windows: replace single quote with double quote when translating to cmd.exe
> >> 
> >> Since cmd.exe doesn't understand single quotes, single quotes to prevent $var
> >> expansion is basically unusable without this.
> > 
> > What if a user expects <'> will be passed as <'> itself?
> > 
> > I think it's okay to enable the $var translation by default because it's limited
> > to the set of the known variable names, but for <'>, there's nothing to prevent
> > unexpected translation. If we want this, the "tonative" option should be off by
> > default.
> 
> Since single quote isn’t allowed in a path name and isn’t recognized as a shell quote, I figured nobody would use it (except to run sh.exe, but that probably should disable translating anyway).

I don't have access to Windows machine right now, but isn't a single quote
just allowed in command string? My point is that a single character will be
more likely to conflict with a valid use of it than $varname.

> I’d also like to change ~/ to %USERPROFILE%/.  So if you think it’s a problem,
> I’m ok with defaulting to off.

Default off seems safer. Not all Mercurial users would use Windows as Unix.

Patch

diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt
--- a/mercurial/help/config.txt
+++ b/mercurial/help/config.txt
@@ -890,9 +890,10 @@  be ``$HG_HOOKTYPE=incoming`` and ``$HG_H
 
   Some basic Unix syntax is supported for portability, including ``$VAR``
   and ``${VAR}`` style variables.  To use a literal ``$``, it must be
-  escaped with a back slash or inside of a strong quote.  This can be
-  disabled by adding a prefix of ``tonative.`` to the hook name on a new
-  line, and setting it to ``False``.  For example::
+  escaped with a back slash or inside of a strong quote.  Strong quotes
+  will be replaced by double quotes after processing.  This translation
+  can be disabled by adding a prefix of ``tonative.`` to the hook name on
+  a new line, and setting it to ``False``.  For example::
 
     [hooks]
     incoming.autobuild = /my/build/hook
diff --git a/mercurial/windows.py b/mercurial/windows.py
--- a/mercurial/windows.py
+++ b/mercurial/windows.py
@@ -268,7 +268,7 @@  def shelltocmdexe(path, env):
     'cmd %var1% %var2% %var3% $missing ${missing} %missing%'
     >>> # Single quote prevents expansion, as does \$ escaping
     >>> shelltocmdexe(b"cmd '$var1 ${var2} %var3%' \$var1 \${var2} \\", e)
-    "cmd '$var1 ${var2} %var3%' $var1 ${var2} \\"
+    'cmd "$var1 ${var2} %var3%" $var1 ${var2} \\'
     >>> # $$ is not special. %% is not special either, but can be the end and
     >>> # start of consecutive variables
     >>> shelltocmdexe(b"cmd $$ %% %var1%%var2%", e)
@@ -292,7 +292,7 @@  def shelltocmdexe(path, env):
             pathlen = len(path)
             try:
                 index = path.index(b'\'')
-                res += b'\'' + path[:index + 1]
+                res += b'"' + path[:index] + b'"'
             except ValueError:
                 res += c + path
                 index = pathlen - 1
diff --git a/tests/test-bookmarks.t b/tests/test-bookmarks.t
--- a/tests/test-bookmarks.t
+++ b/tests/test-bookmarks.t
@@ -1096,7 +1096,9 @@  add hooks:
   $ cat << EOF >> .hg/hgrc
   > [hooks]
   > pretxnclose-bookmark.force-public  = sh -c "(echo \$HG_BOOKMARK| grep -v NEW > /dev/null) || [ -z \"\$HG_NODE\" ] || (hg log -r \"\$HG_NODE\" -T '{phase}' | grep public > /dev/null)"
+  > tonative.pretxnclose-bookmark.force-public = False
   > pretxnclose-bookmark.force-forward = sh -c "(echo \$HG_BOOKMARK| grep -v NEW > /dev/null) || [ -z \"\$HG_NODE\" ] || (hg log -r \"max(\$HG_OLDNODE::\$HG_NODE)\" -T 'MATCH' | grep MATCH > /dev/null)"
+  > tonative.pretxnclose-bookmark.force-forward = False
   > EOF
 
   $ hg log -G -T phases
diff --git a/tests/test-histedit-fold.t b/tests/test-histedit-fold.t
--- a/tests/test-histedit-fold.t
+++ b/tests/test-histedit-fold.t
@@ -498,16 +498,15 @@  Test unix -> windows style variable subs
   > post-add = echo ran $HG_ARGS, literal \$non-var, 'also $non-var', $HG_RESULT
   > EOF
 
-TODO: Windows should output double quotes around "also $non-var"
   $ echo "foo" > amended.txt
   $ HGRCPATH=$TESTTMP/tmp.hgrc hg add -v amended.txt
   running hook pre-add: echo no variables
   no variables
   adding amended.txt
   converting hook "post-add" to native (windows !)
-  running hook post-add: echo ran %HG_ARGS%, literal $non-var, 'also $non-var', %HG_RESULT% (windows !)
+  running hook post-add: echo ran %HG_ARGS%, literal $non-var, "also $non-var", %HG_RESULT% (windows !)
   running hook post-add: echo ran $HG_ARGS, literal \$non-var, 'also $non-var', $HG_RESULT (no-windows !)
-  ran add -v amended.txt, literal $non-var, 'also $non-var', 0 (windows !)
+  ran add -v amended.txt, literal $non-var, "also $non-var", 0 (windows !)
   ran add -v amended.txt, literal $non-var, also $non-var, 0 (no-windows !)
   $ hg ci -q --config extensions.largefiles= --amend -I amended.txt
   The fsmonitor extension is incompatible with the largefiles extension and has been disabled. (fsmonitor !)