On Wed, Mar 29, 2017 at 11:25:15PM -0700, Junio C Hamano wrote:
Pickfire <pickfire@xxxxxxxxxx> 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