Re: [PATCH] userdiff: support Bash

[Junio C Hamano <gitster@pobox.com>]

Victor Engmark <victor@engmark.name> writes:

It's a bit troubling not to see the most basic form, i.e.

	RIGHT () {
		: ChangeMe
	}

in the set of tests.

> diff --git a/t/t4018/bash-trailing-comment b/t/t4018/bash-trailing-comment
> new file mode 100644
> index 0000000000..f1edbeda31
> --- /dev/null
> +++ b/t/t4018/bash-trailing-comment
> @@ -0,0 +1,4 @@
> +RIGHT() { # Comment
> +
> +    ChangeMe
> +}

This is the closest, but it tests "# with comment" at the same time.

> diff --git a/userdiff.c b/userdiff.c
> index fde02f225b..9de0497007 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -23,6 +23,12 @@ IPATTERN("ada",
>  	 "[a-zA-Z][a-zA-Z0-9_]*"
>  	 "|[-+]?[0-9][0-9#_.aAbBcCdDeEfF]*([eE][+-]?[0-9_]+)?"
>  	 "|=>|\\.\\.|\\*\\*|:=|/=|>=|<=|<<|>>|<>"),
> +PATTERNS("bash",
> +	 /* POSIX & bashism form; all four compound command types */
> +	 "^[ \t]*((function[ \t]*)?[a-zA-Z_][a-zA-Z0-9_]*(\\(\\))?[ \t]*(\\{|\\(\\(?|\\[\\[))",

We allow an optional leading indent, an optional noiseword
"function" plus a run of whitespaces, and then an identifier
followed by "line noise".

The pattern makes the run of whitespaces after "function" entirely
optional, which makes "functionabc" be taken as a single identifier
without noiseword "function", but it's not like we are parsing and
painting only the identifer in a color different from the keyword,
so it is OK.  Using [ \t]+ instead of [ \t]* after "function" would
probably make the result more clear, even though it does not make a
practical difference.

Then the "line noise" part.  I am not sure if I follow this part
correctly:

	"(\\(\\))?[ \t]*(\\{|\\(\\(?|\\[\\[)"

is what follows the identifier that is the function name.  We may
have 0 or 1 "()", followed by an optional run of whitespaces.  And
then one of '{', '(', '((', '[[' would come.

Did I read it correctly?

Even though it may be unusual to write open and close parentheses
not directly next to each other, or with a space after the name of
the function, i.e.

	RIGHT ( ) {

would also be a valid header, no?  The way I read the pattern in the
above, it should not hit, as the pattern does not allow anything but
"()" as the "the previous identifier is the name of the function we
are defining" sign, and it does not allow any whitespace between the
identifier and "()".

But it does, and the reason why this most basic form happens to work
is because it relies on the support for "bash-ism" forms.  Even if
the "()" after identifier is missing, you'll match as long as the
identifier is followed by an optional run of whitespace and then an
open paren, brace or bracket:

	"[ \t]*(\\{|\\(\\(?|\\[\\[)"

And I do not like code or pattern that happens to appear to work by
accident.  Can we tighten it a bit?

	"^[ \t]*"		     /* (op) leading indent */
	"("			     /* here comes the whole thing */
	"(function[ \t]+)?"	     /* (op) noiseword "function" */
	"[a-zA-Z_][a-zA-Z0-9_]*"     /* identifier - function name */
	"[ \t]*(\\([ \t]*\\))?"      /* (op) start of func () */
	"[ \t]*(\\{|\\(\\(?|\\[\\[)" /* various "opening" of body */
	")",

is my attempt to break it down to make it readable and more correct.

> +	 /* -- */
> +	 /* Characters not in the default $IFS value */
> +	 "[^ \t]+"),
>  PATTERNS("dts",
>  	 "!;\n"
>  	 "!=\n"

Thanks.