Re: [PATCH v4 1/1][GSOC] userdiff: extend Bash pattern to cover more shell function forms

From: Johannes Sixt <j6t@kdbg.org>
Date: May 6, 2025, 16:30 UTC
Message-ID: <9d548d94-eed8-434a-a9ef-67df694c5c79@kdbg.org>
In-Reply-To: <20250330134018.9662-2-dhar61595@gmail.com>

Am 30.03.25 um 15:39 schrieb Moumita:

Show 9 quoted lines

> From: Moumita Dhar <dhar61595@gmail.com>
> 
> The previous function regex required explicit matching of function
> bodies using `{`, `(`, `((`, or `[[`, which caused several issues:
> 
> - It failed to capture valid functions where `{` was on the next line
>   due to line continuation (`\`).
> - It did not recognize functions with single  command body, such as
>   `x () echo hello`.

Good.

> Replacing the function body matching logic with `.*$`, ensures
> that everything on the function definition line is captured,
> aligning with other userdiff drivers and improving hunk headers in
> `git diff`.

Personally, I am a bit allergic against marketing speak. *Of course* do we intend improve the code. No need to mention it.

> Additionally, the word regex is refined to better recognize shell
> syntax, including additional parameter expansion operators and
> command-line options, improving syntax-aware diffs.

Good. (But see above about "improving".)

> 
> Signed-off-by: Moumita Dhar <dhar61595@gmail.com>
> ---

Show 12 quoted lines

> diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
> index f51d3557f1..0be647c2fb 100755
> --- a/t/t4034-diff-words.sh
> +++ b/t/t4034-diff-words.sh
> @@ -320,6 +320,7 @@ test_expect_success 'unset default driver' '
>  
>  test_language_driver ada
>  test_language_driver bibtex
> +test_language_driver bash
>  test_language_driver cpp
>  test_language_driver csharp
>  test_language_driver css

Including a test for word-diff is very much appreciated!

Show 15 quoted lines

> diff --git a/t/t4034/bash/expect b/t/t4034/bash/expect
> new file mode 100644
> index 0000000000..a0f7cbd5a3
> --- /dev/null
> +++ b/t/t4034/bash/expect
> @@ -0,0 +1,30 @@
> +<BOLD>diff --git a/pre b/post<RESET>
> +<BOLD>index 09ac008..60ba6a2 100644<RESET>
> +<BOLD>--- a/pre<RESET>
> +<BOLD>+++ b/post<RESET>
> +<CYAN>@@ -1,25 +1,25 @@<RESET>
> +<RED>my_var<RESET><GREEN>new_var<RESET>=10
> +x=<RED>123<RESET><GREEN>456<RESET>
> +y=<RED>3.14<RESET><GREEN>2.71<RESET>
> +z=<RED>.5<RESET><GREEN>.75<RESET>

OK.

> +echo <RED>$USER<RESET><GREEN>$USERNAME<RESET>

This tests a typical variable expansion. Good.

> +${<RED>HOME<RESET><GREEN>HOMEDIR<RESET>}

A more elaborate variable expansion does not include the surrounding ${ }. Good.

> +if [ "<RED>$a<RESET><GREEN>$x<RESET>" == "<RED>$b<RESET><GREEN>$y<RESET>" ] || [ "<RED>$c<RESET><GREEN>$x<RESET>" != "<RED>$d<RESET><GREEN>$y<RESET>" ]; then echo "OK"; fi

This line also only tests variable expansions and is quite redundant. It could test the operators that we see, but it doesn't. See below for ideas how to do that.

And all from here...

Show 12 quoted lines

> +((<RED>a<RESET><GREEN>x<RESET>+=<RED>b<RESET><GREEN>y<RESET>))
> +((<RED>a<RESET><GREEN>x<RESET>-=<RED>b<RESET><GREEN>y<RESET>))
> +$((<RED>a<RESET><GREEN>x<RESET><<<RED>b<RESET><GREEN>y<RESET>))
> +$((<RED>a<RESET><GREEN>x<RESET>>><RED>b<RESET><GREEN>y<RESET>))
> +${<RED>a<RESET><GREEN>x<RESET>:-<RED>b<RESET><GREEN>y<RESET>}
> +${<RED>a<RESET><GREEN>x<RESET>:=<RED>b<RESET><GREEN>y<RESET>}
> +${<RED>a<RESET><GREEN>x<RESET>##*/}
> +${<RED>a<RESET><GREEN>x<RESET>%.*}
> +${<RED>a<RESET><GREEN>x<RESET>%%.*}
> +${<RED>a<RESET><GREEN>x<RESET>^^}
> +${<RED>a<RESET><GREEN>x<RESET>,}
> +${<RED>a<RESET><GREEN>x<RESET>,,}

... to here also only test variable expansions, but not the operators. As written, they are totally redundant and should be dropped or improved.

To test, for example, that '##' is kept together as an operator, you have to have

   ${x#*/}

in the pre-image and

   ${x##*/}

in the post-image, so that we see

   ${x<RED>#<RESET><GREEN>##<RESET>*/}

in the result. If '##' were two tokens (due to a bug in the pattern), we would see

   ${x#<GREEN>#<RESET>*/}

in the result.

You should go through all not-single-character operators and have a pre-image and a post-image where one characters is added or removed. The following is not a correct test:

pre: ((x+=b)) post: ((x-=b)) result: ((x<RED>+=<RESET><GREEN>-=<RESET>))

because we would see this result even if the pattern has a bug that recognizes only one of += or -=, but would split the other one. A correct test would be:

pre: ((x+b)) post: ((x+=b)) result: ((x<RED>+<RESET><GREEN>+=<RESET>))

> +${!<RED>a<RESET><GREEN>x<RESET>}

Here, you should have no '!' in the pre-image and the '!' in the post image and not change the name. Then the test demonstrates that '!' is its own token and not merged with '${' (if that is the intent of the test; otherwise it is a redunant test).

> +${<RED>a<RESET><GREEN>x<RESET>[@]}

If you want to test that '@' is not merged with the brackets, then you can have, for example '*' in the pre-image and '@' in the post image.

> +${<RED>a<RESET><GREEN>x<RESET>:?error message}

Either redundant or another two-character operator to test.

> +${<RED>a<RESET><GREEN>x<RESET>:2:3}

Redundant.

> +ls <RED>-a<RESET><GREEN>-x<RESET>
> +ls <RED>--a<RESET><GREEN>--x<RESET>

Both are good tests.

Show 14 quoted lines

> diff --git a/userdiff.c b/userdiff.c
> index 340c4eb4f7..4c77c7e0f6 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -64,15 +64,27 @@ PATTERNS("bash",
>  	     /* Bashism identifier with optional parentheses */
>  	     "(function[ \t]+[a-zA-Z_][a-zA-Z0-9_]*(([ \t]*\\([ \t]*\\))|([ \t]+))"
>  	 ")"
> -	 /* Optional whitespace */
> -	 "[ \t]*"
> -	 /* Compound command starting with `{`, `(`, `((` or `[[` */
> -	 "(\\{|\\(\\(?|\\[\\[)"
> +	 /* Everything after the function header is captured  */
> +	 ".*$"

I remember suggesting to capture everything after the function header. However, If I am not mistaken, this does not do what I intended (and as written it means that a pointless matching operation happens). The hunk header shows everything that is in the outermost parentheses (group). This catch-all, however, is even outside the outermost group and not captured. It should be above the closing parenthesis that we see in the context.

To test for this, you can have one test where "RIGHT" is not the function name, but in the comment on the function header line.

Show 17 quoted lines

>  	 /* End of captured text */
>  	 ")",
>  	 /* -- */
> -	 /* Characters not in the default $IFS value */
> -	 "[^ \t]+"),
> +	 /* Identifiers: variable and function names */
> +	  "[a-zA-Z_][a-zA-Z0-9_]*"
> +	 /* Numeric constants: integers and decimals */
> +	  "|[0-9]+(\\.[0-9]*)?|[-+]?\\.[0-9]+"
> +	 /* Shell variables: $VAR, ${VAR} */
> +	  "|\\$[a-zA-Z_][a-zA-Z0-9_]*|\\$\\{"
> +	  /* Logical and comparison operators */
> +	 "|\\|\\||&&|<<|>>|==|!=|<=|>="
> +	 /* Assignment and arithmetic operators */
> +	 "|[-+*/%&|^!=<>]=?"
> +	 /* Additional parameter expansion operators */
> +	 "|:?=|:-|:\\+|:\\?|:|#|##|%|%%|/[a-zA-Z0-9_-]+|\\^\\^?|,|,,?|!|@|:[0-9]+(:[0-9]+)?"

What is the purpose of this "/[a-zA-Z0-9_-]+"? It would mean, for example that changes in alphanumeric path names would include the slash in the changed part. I don't think this should be here.

Show 7 quoted lines

> +	 /* Command-line options (to avoid splitting -option) */
> +	 "|--?[a-zA-Z0-9_-]+"
> +	 /* Brackets and grouping symbols */
> +	 "|\\(|\\)|\\{|\\}|\\[|\\]"),
>  PATTERNS("bibtex",
>  	 "(@[a-zA-Z]{1,}[ \t]*\\{{0,1}[ \t]*[^ \t\"@',\\#}{~%]*).*$",
>  	 /* -- */

-- Hannes