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

[Matt Harbison]

On Wed, 05 Jul 2017 09:01:20 -0400, Yuya Nishihara <yuya at tcha.org> wrote:

> On Tue, 04 Jul 2017 23:39:59 -0400, Matt Harbison wrote:
>> On Tue, 04 Jul 2017 09:13:46 -0400, Yuya Nishihara <yuya at tcha.org>  
>> wrote:
>>
>> > On Tue, 04 Jul 2017 12:42:08 +0900, FUJIWARA Katsunori wrote:
>> >> 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:
>> >> > >> >  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.
>> >
>> > FWIW, I found ntpath.expandvars() does support $name and ${name} on
>> > Windows.
>> >
>> > https://docs.python.org/2/library/os.path.html#os.path.expandvars
>>
>> Thanks, I forgot about that too.  I switched to that, and got some
>> unexpected failures.  I'll have to dig in more, and that should give  
>> time
>> for stable to merge back into default to pick up the help text changes.
>
> Oops, I wanted to tell you that we could copy the expansion rule (and  
> maybe
> a part of the implementation.) Variable expansion should be done only  
> once,
> but cmd.exe will do that again.

I'm having second thoughts about how useful it is to follow their  
expansion rules.  The rules are (slightly reordered):

1) no expansion within single quotes
2) ${varname} is accepted.
3) $varname is accepted.
4) varnames can be made out of letters, digits and the characters '_-'
    (though is not verified in the ${varname} and %varname% cases)

5) '$$' is translated into '$'
6) '%%' is translated into '%' if '%%' are not seen in %var1%%var2%
7) %varname% is accepted.

We can get 2-4 with Foozy's regex.  The first rule is interesting, so it  
might be worth not using the regex.  OTOH, to be useful on Windows, the  
_outer_ single quotes need to be replaced with double quotes.

I don't understand why they implemented 5- '$$' prints the pid of the  
script.  6 we don't need to touch.  I also think it's bad for portability  
that ${foo} is looked up, and left alone if it isn't found, since unix  
shell will substitute an empty string.

>> BTW, if I end up using this, should it be conditionalized to just
>> Windows?  (In theory, it shouldn't matter for non-Windows, but just in
>> case...)
>
> Just Windows. Unix shell has much more complex syntax.