Re: [PATCH] userdiff: add build-in pattern for shell

Pickfire [off-list ref] writes:

quoted

quoted

+- `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.

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".

quoted

	...[ \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..

quoted

quoted

+	 /* -- */
+	 "(\\$|--?)?([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 ...

quoted

$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.

quoted

quoted

+	 "|\\$[({]|[)}]|[-+*/=!]=?|[\\]&%#/|]{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.

Thanks.