From: Ivan Tham <pickfire@riseup.net>
Date: Thu, 30 Mar 2017 07:20:38 GMT
Subject: Re: [PATCH] userdiff: add build-in pattern for shell
Message-ID: <20170330072038.GA17735@riseup.net>
In-Reply-To: <xmqqefxf7038.fsf@gitster.mtv.corp.google.com>

On Wed, Mar 29, 2017 at 11:25:15PM -0700, Junio C Hamano wrote:
>Pickfire <pickfire@riseup.net> writes:
>
>>> > +- `sh` suitable for source code in POSIX-compatible shells.
>>>
>>> The new test you added seems to show that this is not limited to
>>> POSIX shells but also understands bashisms like ${x//x/x}.  Perhaps
>>> drop "POSIX-compatible" from here
>>
>> Those shells are still POSIX-compatible so I think it is true to put
>> that or otherwise, something like fish shell will break since it is
>> as well a shell but the syntax is totally different.

Okay, I will change it from POSIX-compatible to POSIX-like.

>Scripts with bash-isms are not necessarily usable by POSIX compatible
>shells (think "dash") and this highlighter recognises bash specific
>enhancements (which by the way is a plus), so if you absolutely want to
>say "POSIX something" in order to clarify that csh and friends do not
>apply, say "POSIX-like".
>
>>> 	...[ \t]*\\(\\)[\t]*....
>>
>> Ah, I think I forgot to escape the quoting of ( and ). I will send in
>> another patch for that.
>
>OK.  Note that we usually avoid applying a patch whose brokenness was
>noticed while review (which then necessitates a follow up patch "oops,
>the previous was botched; here is a fix-up").  The "another patch"
>needs to be a v2, i.e. pretending as if the version of the patch we are
>discussing never happened, not an incremental on top of the patch we
>are discussing..

Yes, I will put in a V2 which comes with "[PATCH v2]" in reply to this
thread.

>>> > +	 /* -- */ +
>>> > "(\\$|--?)?([a-zA-Z_][a-zA-Z0-9._]*|[0-9]+|#)|--" /* command/param
>>> > */
>>>
>>> TBH, I have no idea what this line-noise is doing.
>>
>> That breaks word into "a", "$a" and "-a" as well as "$1" and "$#". I
>> tried supporting $? by adding +|#|\\?)--" but it doesn't seemed like
>> it is working.
>
>This ...

???

>>> $foobar, $4, --foobar, foobar, 123 and -- can be seen easily out of
>>> these patterns.  I am not sure what --# would be (perhaps you meant
>>> to only catch $# and --# is included by accident, in which case it
>>> is understandable).  It feels a bit strange to see that $# is
>>> supported but not $?; --foo but not --foo=bar; foobar but not "foo
>>> bar" inside a dq-pair.
>>
>> Yes, getting --# will be very rare in shell. I think it is better to
>> seperate the --foo=bar into --foo and bar. I don't get what you man
>> by the dq-pair.
>
>These design decisions (e.g. what you decided are the tokens to be
>taken as a word---taking "--foo" and "bar" as separate things when
>given "--foo=bar" is a good example but with this regexp you are making
>many other design decisions) need to be explained in the log message.
>A string inside a double-quote pair is taken as a single parameter to
>the shell, e.g.
>
>    cmd "arg that is quoted inside double-quote pair" $#
>
>It is unclear what your regexp is doing to such an argument.

Okay, I will put that into the log. I still don't quite know what you
want to achieve with:

    cmd "arg that is quoted inside double-quote pair" $#

If I am correct, you are probably talking about:

    "cmd "arg that is quoted inside double-quote pair" $#"

That will be handled with other words together.

>>> > +	 "|\\$[({]|[)}]|[-+*/=!]=?|[\\]&%#/|]{1,2}|[<>]{1,3}|[ \t]#.*"),
>>>
>>> And this one is even more dense.
>
>FYI, this is also pointing out the need to explain what kind of things
>you wanted to recognise as words; explaining in a reply message is a
>good first step, as the questioner may find the explanation in your
>response still inadequate, in which case you have a chance to refine
>it, but the ultimate goal is to put the polished explanation that would
>help people who later want to understand what you added to the codebase
>by describing what you wanted to do with the change in either in-code
>comment or commit log message when you send an updated patch.

Ah, I can point it out here:

\\$[({]           start of $( or ${
[)}]              ends ^
[-+*/=!]=?        operators
[\\]&%#/|]{1,2}   pipes and stuff like ${a##a} or &&
[<>]{1,3}         io redirections
[ \t]#.*          comments

I hope that makes it clear and concise.

-- 
Do what you like, like what you do.  -- Pickfire