Re: [PATCH v2] userdiff: support Bash

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 21, 2020, 18:39 UTC
Message-ID: <xmqq8sbz8lvb.fsf@gitster.c.googlers.com>
In-Reply-To: <a07042af-d16c-1975-c0d1-f22f4fec5827@kdbg.org>

Johannes Sixt <j6t@kdbg.org> writes:

Show 23 quoted lines

>> diff --git a/t/t4018/bash-missing-parentheses b/t/t4018/bash-missing-parentheses
>> new file mode 100644
>> index 0000000000..d112761300
>> --- /dev/null
>> +++ b/t/t4018/bash-missing-parentheses
>> @@ -0,0 +1,4 @@
>> +functionRIGHT { # non-match
>> +    :
>> +    echo 'ChangeMe'
>> +}
>
> To check for a non-match, you write the test like this:
>
> 	function RIGHT () {
> 	}
> 	# the following must not be picked up:
> 	functionWrong {
> 		:
> 		ChangeMe
> 	}
>
> That is, you present a positive match, and then expect that the
> subsequent negative match is not picked up.

All good suggestions, but I especially appreciate this one ;-).

Show 16 quoted lines

>> diff --git a/userdiff.c b/userdiff.c
>> index fde02f225b..8830019f05 100644
>> --- a/userdiff.c
>> +++ b/userdiff.c
>> @@ -23,6 +23,28 @@ IPATTERN("ada",
>>  	 "[a-zA-Z][a-zA-Z0-9_]*"
>>  	 "|[-+]?[0-9][0-9#_.aAbBcCdDeEfF]*([eE][+-]?[0-9_]+)?"
>>  	 "|=>|\\.\\.|\\*\\*|:=|/=|>=|<=|<<|>>|<>"),
>> +PATTERNS("bash",
>> +	 /* Optional leading indentation */
>> +	 "^[ \t]*"
>> +	 /* Start of function definition */
>> +	 "("
>
> The purpose of this outer-most pair of parentheses is actually to mark
> the captured text, not so much "the function definition".

This, too (I called it "here comes the whole thing" in my suggested version ).

Show 12 quoted lines

>> +	 /* Start of POSIX/Bashism grouping */
>> +	 "("
>
> You could omit the comment if you indent the parts that are inside the
> parentheses:
>
> 	"("
> 		"..."
> 	"|"
> 		"..."
> 	")"
>

An excellent readability suggestion.

Thanks for a review. Especially the parts that mine didn't touch (i.e. the proposed log message).

Show 18 quoted lines

> (But perhaps don't indent between the outer-most parentheses; it would
> get us too far to the right. But judge yourself.)
>
>> +	 /* POSIX identifier with mandatory parentheses */
>> +	 "[a-zA-Z_][a-zA-Z0-9_]*[ \t]*\\([ \t]*\\))"
>> +	 /* Bashism identifier with optional parentheses */
>> +	 "|(function[ \t]+[a-zA-Z_][a-zA-Z0-9_]*(([ \t]*\\([ \t]*\\))|([ \t]+))"
>> +	 /* End of POSIX/Bashism grouping */
>> +	 ")"
>> +	 /* Optional whitespace */
>> +	 "[ \t]*"
>> +	 /* Compound command starting with `{`, `(`, `((` or `[[` */
>> +	 "(\\{|\\(\\(?|\\[\\[)"
>> +	 /* End of function definition */
>> +	 ")",
>> +	 /* -- */
>> +	 /* Characters not in the default $IFS value */
>> +	 "[^ \t]+"),