Patchwork [4,of,4] windows: expand '~/' and '~\' to %USERPROFILE% when translating to cmd.exe

login
register
mail settings
Submitter Matt Harbison
Date July 16, 2018, 9:31 p.m.
Message ID <54611420fcf7868ee195.1531776695@mharbison-pc.attotech.com>
Download mbox | patch
Permalink /patch/32881/
State Accepted
Headers show

Comments

Matt Harbison - July 16, 2018, 9:31 p.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1531715553 14400
#      Mon Jul 16 00:32:33 2018 -0400
# Node ID 54611420fcf7868ee195e9fa3070efc0d57e9757
# Parent  e71e478d5e49768357287a2f181e8a2b23213239
windows: expand '~/' and '~\' to %USERPROFILE% when translating to cmd.exe

It's convenient to be able to reference hooks in a portable location on any
platform.
Yuya Nishihara - July 17, 2018, 12:27 p.m.
On Mon, 16 Jul 2018 17:31:35 -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1531715553 14400
> #      Mon Jul 16 00:32:33 2018 -0400
> # Node ID 54611420fcf7868ee195e9fa3070efc0d57e9757
> # Parent  e71e478d5e49768357287a2f181e8a2b23213239
> windows: expand '~/' and '~\' to %USERPROFILE% when translating to cmd.exe

Queued, thanks.

> +        elif (c == b'~' and index + 1 < pathlen
> +              and path[index + 1] in (b'\\', b'/')):
> +            res += "%USERPROFILE%"

Nit: ~/ is substituted only at the beginning. ('"~/"' and 'foo~/' shouldn't
be expanded.)
Matt Harbison - July 17, 2018, 2:42 p.m.
> On Jul 17, 2018, at 8:27 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> 
>> On Mon, 16 Jul 2018 17:31:35 -0400, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1531715553 14400
>> #      Mon Jul 16 00:32:33 2018 -0400
>> # Node ID 54611420fcf7868ee195e9fa3070efc0d57e9757
>> # Parent  e71e478d5e49768357287a2f181e8a2b23213239
>> windows: expand '~/' and '~\' to %USERPROFILE% when translating to cmd.exe
> 
> Queued, thanks.
> 
>> +        elif (c == b'~' and index + 1 < pathlen
>> +              and path[index + 1] in (b'\\', b'/')):
>> +            res += "%USERPROFILE%"
> 
> Nit: ~/ is substituted only at the beginning. ('"~/"' and 'foo~/' shouldn't
> be expanded.)

Good catch.

Is there a way to mark this (tilde expansion) experimental?  (Or just undocumented it?)  I see we use os.path.expanduser() for various things, which uses a slightly more elaborate scheme.  The downside is it checks HOME first, which has a different definition under msys.  The config files under ~/ in msys gets read before the regular Windows user’s mercurial.ini, which means there’s no way to disable expansion under msys, but leave it enabled when run with cmd.exe.  So it seems to boil down to consistency vs portability, and I’d probably lean slightly to portable, based on my usage.
Yuya Nishihara - July 18, 2018, 12:13 p.m.
On Tue, 17 Jul 2018 10:42:33 -0400, Matt Harbison wrote:
> > On Jul 17, 2018, at 8:27 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> >> On Mon, 16 Jul 2018 17:31:35 -0400, Matt Harbison wrote:
> >> # HG changeset patch
> >> # User Matt Harbison <matt_harbison@yahoo.com>
> >> # Date 1531715553 14400
> >> #      Mon Jul 16 00:32:33 2018 -0400
> >> # Node ID 54611420fcf7868ee195e9fa3070efc0d57e9757
> >> # Parent  e71e478d5e49768357287a2f181e8a2b23213239
> >> windows: expand '~/' and '~\' to %USERPROFILE% when translating to cmd.exe
> > 
> > Queued, thanks.
> > 
> >> +        elif (c == b'~' and index + 1 < pathlen
> >> +              and path[index + 1] in (b'\\', b'/')):
> >> +            res += "%USERPROFILE%"
> > 
> > Nit: ~/ is substituted only at the beginning. ('"~/"' and 'foo~/' shouldn't
> > be expanded.)
> 
> Good catch.
> 
> Is there a way to mark this (tilde expansion) experimental?  (Or just undocumented it?)  I see we use os.path.expanduser() for various things, which uses a slightly more elaborate scheme.  The downside is it checks HOME first, which has a different definition under msys.  The config files under ~/ in msys gets read before the regular Windows user’s mercurial.ini, which means there’s no way to disable expansion under msys, but leave it enabled when run with cmd.exe.  So it seems to boil down to consistency vs portability, and I’d probably lean slightly to portable, based on my usage.

Should I drop this for now?

I think this patch is good enough, and the excessive tilde expansion can
be fixed later.
Matt Harbison - July 18, 2018, 12:32 p.m.
> On Jul 18, 2018, at 8:13 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> On Tue, 17 Jul 2018 10:42:33 -0400, Matt Harbison wrote:
>>>> On Jul 17, 2018, at 8:27 AM, Yuya Nishihara <yuya@tcha.org> wrote:
>>>> On Mon, 16 Jul 2018 17:31:35 -0400, Matt Harbison wrote:
>>>> # HG changeset patch
>>>> # User Matt Harbison <matt_harbison@yahoo.com>
>>>> # Date 1531715553 14400
>>>> #      Mon Jul 16 00:32:33 2018 -0400
>>>> # Node ID 54611420fcf7868ee195e9fa3070efc0d57e9757
>>>> # Parent  e71e478d5e49768357287a2f181e8a2b23213239
>>>> windows: expand '~/' and '~\' to %USERPROFILE% when translating to cmd.exe
>>> 
>>> Queued, thanks.
>>> 
>>>> +        elif (c == b'~' and index + 1 < pathlen
>>>> +              and path[index + 1] in (b'\\', b'/')):
>>>> +            res += "%USERPROFILE%"
>>> 
>>> Nit: ~/ is substituted only at the beginning. ('"~/"' and 'foo~/' shouldn't
>>> be expanded.)
>> 
>> Good catch.
>> 
>> Is there a way to mark this (tilde expansion) experimental?  (Or just undocumented it?)  I see we use os.path.expanduser() for various things, which uses a slightly more elaborate scheme.  The downside is it checks HOME first, which has a different definition under msys.  The config files under ~/ in msys gets read before the regular Windows user’s mercurial.ini, which means there’s no way to disable expansion under msys, but leave it enabled when run with cmd.exe.  So it seems to boil down to consistency vs portability, and I’d probably lean slightly to portable, based on my usage.
> 
> Should I drop this for now?
> 
> I think this patch is good enough, and the excessive tilde expansion can
> be fixed later.

If you think this can be fixed later, it’s probably OK to keep.  I was just toying with expanduser(), and wondering out loud if we would be able to shift to approximately that after releasing this.
Yuya Nishihara - July 18, 2018, 1:12 p.m.
On Wed, 18 Jul 2018 08:32:32 -0400, Matt Harbison wrote:
> 
> > On Jul 18, 2018, at 8:13 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > 
> > On Tue, 17 Jul 2018 10:42:33 -0400, Matt Harbison wrote:
> >>>> On Jul 17, 2018, at 8:27 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> >>>> On Mon, 16 Jul 2018 17:31:35 -0400, Matt Harbison wrote:
> >>>> # HG changeset patch
> >>>> # User Matt Harbison <matt_harbison@yahoo.com>
> >>>> # Date 1531715553 14400
> >>>> #      Mon Jul 16 00:32:33 2018 -0400
> >>>> # Node ID 54611420fcf7868ee195e9fa3070efc0d57e9757
> >>>> # Parent  e71e478d5e49768357287a2f181e8a2b23213239
> >>>> windows: expand '~/' and '~\' to %USERPROFILE% when translating to cmd.exe
> >>> 
> >>> Queued, thanks.
> >>> 
> >>>> +        elif (c == b'~' and index + 1 < pathlen
> >>>> +              and path[index + 1] in (b'\\', b'/')):
> >>>> +            res += "%USERPROFILE%"
> >>> 
> >>> Nit: ~/ is substituted only at the beginning. ('"~/"' and 'foo~/' shouldn't
> >>> be expanded.)
> >> 
> >> Good catch.
> >> 
> >> Is there a way to mark this (tilde expansion) experimental?  (Or just undocumented it?)  I see we use os.path.expanduser() for various things, which uses a slightly more elaborate scheme.  The downside is it checks HOME first, which has a different definition under msys.  The config files under ~/ in msys gets read before the regular Windows user’s mercurial.ini, which means there’s no way to disable expansion under msys, but leave it enabled when run with cmd.exe.  So it seems to boil down to consistency vs portability, and I’d probably lean slightly to portable, based on my usage.
> > 
> > Should I drop this for now?
> > 
> > I think this patch is good enough, and the excessive tilde expansion can
> > be fixed later.
> 
> If you think this can be fixed later, it’s probably OK to keep.  I was just toying with expanduser(), and wondering out loud if we would be able to shift to approximately that after releasing this.

Leave it as is then. It should be okay to fix this to do more correct
substitution.

Patch

diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt
--- a/mercurial/help/config.txt
+++ b/mercurial/help/config.txt
@@ -892,9 +892,11 @@  be ``$HG_HOOKTYPE=incoming`` and ``$HG_H
 .. container:: windows
 
   Some basic Unix syntax can be enabled 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.  Strong quotes
-  will be replaced by double quotes after processing.
+  and ``${VAR}`` style variables.  A ``~`` followed by ``\`` or ``/`` will
+  be expanded to ``%USERPROFILE%`` to simulate a subset of tilde expansion
+  on Unix.  To use a literal ``$`` or ``~``, it must be escaped with a back
+  slash or inside of a strong quote.  Strong quotes will be replaced by
+  double quotes after processing.
 
   This feature is enabled by adding a prefix of ``tonative.`` to the hook
   name on a new line, and setting it to ``True``.  For example::
diff --git a/mercurial/windows.py b/mercurial/windows.py
--- a/mercurial/windows.py
+++ b/mercurial/windows.py
@@ -276,8 +276,11 @@  def shelltocmdexe(path, env):
     >>> # No double substitution
     >>> shelltocmdexe(b"$var1 %var1%", {b'var1': b'%var2%', b'var2': b'boom'})
     '%var1% %var1%'
+    >>> # Tilde expansion
+    >>> shelltocmdexe(b"~/dir ~\dir2 ~tmpfile \~/", {})
+    '%USERPROFILE%/dir %USERPROFILE%\\dir2 ~tmpfile ~/'
     """
-    if not any(c in path for c in b"$'"):
+    if not any(c in path for c in b"$'~"):
         return path
 
     varchars = pycompat.sysbytes(string.ascii_letters + string.digits) + b'_-'
@@ -344,9 +347,13 @@  def shelltocmdexe(path, env):
 
                 if c != '':
                     index -= 1
-        elif c == b'\\' and index + 1 < pathlen and path[index + 1] == b'$':
-            # Skip '\', but only if it is escaping $
-            res += b'$'
+        elif (c == b'~' and index + 1 < pathlen
+              and path[index + 1] in (b'\\', b'/')):
+            res += "%USERPROFILE%"
+        elif (c == b'\\' and index + 1 < pathlen
+              and path[index + 1] in (b'$', b'~')):
+            # Skip '\', but only if it is escaping $ or ~
+            res += path[index + 1]
             index += 1
         else:
             res += c