| 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
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.
> 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.
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 !)