[PATCH] hooks: allow Unix style environment variables on external Windows hooks

[FUJIWARA Katsunori]

At Mon, 03 Jul 2017 21:57:01 -0400,
Matt Harbison wrote:
> 
> On Mon, 03 Jul 2017 12:15:49 -0400, FUJIWARA Katsunori  
> <foozy at lares.dti.ne.jp> wrote:
> 
> > At Mon, 3 Jul 2017 22:32:32 +0900,
> > Yuya Nishihara wrote:
> >>
> >> On Sun, 02 Jul 2017 01:45:00 -0400, Matt Harbison wrote:
> >> > # HG changeset patch
> >> > # User Matt Harbison <matt_harbison at yahoo.com>
> >> > # Date 1498969929 14400
> >> > #      Sun Jul 02 00:32:09 2017 -0400
> >> > # Node ID 5684913c02965f4a6bb43320e8aa189333962dd1
> >> > # Parent  61ed2cc98fd17c50c7adf634d777d946781a9fc1
> >> > hooks: allow Unix style environment variables on external Windows  
> >> hooks
> >> >
> >> > This will help making common hooks between Windows and non-Windows  
> >> platforms.
> >> > The hook is launched via cmd.exe, which doesn't understand $var, nor  
> >> single
> >> > quotes.  Therefore, all that is handled is converting $var to %var%,  
> >> and
> >> > allowing a literal '$' if escaped with '\'.  Like Unix variables,  
> >> Windows will
> >> > substitute in the middle of a word (e.g. foo%bar%), so I don't think  
> >> there are
> >> > any other special cases to handle.
> >>
> >> (+CC foozy, another Windows expert)
> >>
> >> Perhaps we'll need to update the help. '$' could be included in a valid
> >> filename, which now escape is required.
> >>
> >> > --- a/mercurial/hook.py
> >> > +++ b/mercurial/hook.py
> >> > @@ -8,6 +8,7 @@
> >> >  from __future__ import absolute_import
> >> >
> >> >  import os
> >> > +import re
> >> >  import sys
> >> >
> >> >  from .i18n import _
> >> > @@ -116,6 +117,17 @@
> >> >      return r, False
> >> >
> >> >  def _exthook(ui, repo, htype, name, cmd, args, throw):
> >> > +    if pycompat.osname == 'nt':
> >> > +        # Replace Unix style environment variables with Windows  
> >> style variables,
> >> > +        # for replacement by cmd.exe.  Treat '\$' as a literal $.
> >> > +        def replacer(m):
> >> > +            prefix = m.group(1)
> >> > +            if prefix == '\\':
> >> > +                return '$' + m.group(2)
> >> > +            return prefix + '%' + m.group(2) + '%'
> >> > +
> >> > +        cmd = re.sub(br'(.?)\$(.+?)\b', replacer, cmd)
> >
> > How about '(.?)\$([A-Za-z_]\w*)' for more strict matching against
> > variable name?
> 
> Unfortunately, that strictness is... optimistic:
> 
> https://ss64.com/nt/syntax-variables.html
> 
> Names can include spaces, '{', '}', and '$'.  Ugh.
> 
> Or are you saying we don't need to support those characters, because they  
> wouldn't be portable anyway?  (The fact that there are built-ins with some  
> non portable characters like 'COMMONPROGRAMFILES(X86)' gives me slight  
> pause, though I've never seen any of the other characters in practice.   
> And mostly the convenience is when using variables exported by hg.)

Yes, I think that platform specific characters and features
(substitutions, and so on) can be omitted, because main concern of
this patch is portability of external hook command line between
platforms.

> > BTW, is supporting simple "{}" surround over engineering in this case?
> 
> Seems easy to do, and reasonable.
> 
> >     def replacer(m):
> >         prefix = m.group(1)
> >         if prefix == '\\':
> >             return '$' + m.group(2)
> >         varname = m.group(2)
> >         if varname.startswith('{'):
> >             return prefix + '%' + m.group(2)[1:-1] + '%'
> >         return prefix + '%' + m.group(2) + '%'
> >
> >     cmd = re.sub(br'(.?)\$({[A-Za-z_]\w*}|[A-Za-z_]\w*)', replacer, cmd)
> >
> > This allows concatenation of environment variables and normal texts
> > (e.g. ${FOO}BAR).
> >
> >> Can you move this to windows.py, which hosts several shell-related  
> >> functions.
> >> I think the function can have some doctests.
> >>
> 

-- 
----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy at lares.dti.ne.jp