Re: [PATCH] userdiff: add support for Emacs Lisp

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 14, 2021, 01:41 UTC
Message-ID: <87wnvbbf2y.fsf@evledraar.gmail.com>
In-Reply-To: <20210213192447.6114-1-git@adamspiers.org>

On Sat, Feb 13 2021, Adam Spiers wrote:

> Add a diff driver which recognises Elisp top-level forms and outline
> headings for display in hunk headers, and which correctly renders word
> diffs.

Neat, I for one would find this useful.

Show 11 quoted lines

> This approach was previously discussed on the emacs-devel mailing list:
>
>    https://lists.gnu.org/archive/html/emacs-devel/2021-02/msg00705.html
>
> * userdiff.c (builtin_drivers): Provide regexen for Elisp top level
>   forms and outline headings, and for word diffs.
> * t/t4018-diff-funcname.sh (diffpatterns): Add test for elisp driver
> * t/t4018/elisp-outline-heading: Test fixture for outline headings
> * t/t4018/elisp-top-level-form: Test fixture for top level forms
> * t/t4034-diff-words.sh: Add test for elisp driver
> * t/t4034/elisp/*: Test fixtures for word diffs

Please no on the overly verbose emacs.git convention of per-file changelogs in commit messages :)

Show 92 quoted lines

> diff --git a/t/t4018/elisp-outline-heading b/t/t4018/elisp-outline-heading
> new file mode 100644
> index 0000000000..c13bdafafe
> --- /dev/null
> +++ b/t/t4018/elisp-outline-heading
> @@ -0,0 +1,6 @@
> +;;; A top-level outline heading
> +;;;; A second-level outline heading RIGHT
> +
> +;; This is a ChangeMe comment outside top-level forms
> +(defun foo ()
> +  (bar 1 2 3)
> diff --git a/t/t4018/elisp-top-level-form b/t/t4018/elisp-top-level-form
> new file mode 100644
> index 0000000000..683f7ffcf1
> --- /dev/null
> +++ b/t/t4018/elisp-top-level-form
> @@ -0,0 +1,7 @@
> +;;; Outline heading
> +
> +;; This is a comment
> +(RIGHT
> +  (list 1 2 3)
> +  ChangeMe
> +  (list a b c))
> diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
> index 0c8fb39ced..a546ee831a 100755
> --- a/t/t4034-diff-words.sh
> +++ b/t/t4034-diff-words.sh
> @@ -315,6 +315,7 @@ test_language_driver cpp
>  test_language_driver csharp
>  test_language_driver css
>  test_language_driver dts
> +test_language_driver elisp
>  test_language_driver fortran
>  test_language_driver html
>  test_language_driver java
> diff --git a/t/t4034/elisp/expect b/t/t4034/elisp/expect
> new file mode 100644
> index 0000000000..29a6ef2520
> --- /dev/null
> +++ b/t/t4034/elisp/expect
> @@ -0,0 +1,9 @@
> +<BOLD>diff --git a/pre b/post<RESET>
> +<BOLD>index 4a39df8..6619e96 100644<RESET>
> +<BOLD>--- a/pre<RESET>
> +<BOLD>+++ b/post<RESET>
> +<CYAN>@@ -1,4 +1,4 @@<RESET>
> +(defun <RED>myfunc<RESET><GREEN>my-func<RESET> (<RED>a b<RESET><GREEN>first second<RESET>)
> +  "This is a <RED>really<RESET><GREEN>(moderately)<RESET> cool function."
> +  (let ((c (<RED>+ a b<RESET><GREEN>1+ first<RESET>)))
> +    (format "one more than the total is %d" (<RED>1+<RESET><GREEN>+<RESET> c <GREEN>second<RESET>))))
> diff --git a/t/t4034/elisp/post b/t/t4034/elisp/post
> new file mode 100644
> index 0000000000..6619e96657
> --- /dev/null
> +++ b/t/t4034/elisp/post
> @@ -0,0 +1,4 @@
> +(defun my-func (first second)
> +  "This is a (moderately) cool function."
> +  (let ((c (1+ first)))
> +    (format "one more than the total is %d" (+ c second))))
> diff --git a/t/t4034/elisp/pre b/t/t4034/elisp/pre
> new file mode 100644
> index 0000000000..4a39df8ffb
> --- /dev/null
> +++ b/t/t4034/elisp/pre
> @@ -0,0 +1,4 @@
> +(defun myfunc (a b)
> +  "This is a really cool function."
> +  (let ((c (+ a b)))
> +    (format "one more than the total is %d" (1+ c))))
> diff --git a/userdiff.c b/userdiff.c
> index 3f81a2261c..292e51674a 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -53,6 +53,15 @@ PATTERNS("dts",
>  	 /* Property names and math operators */
>  	 "[a-zA-Z0-9,._+?#-]+"
>  	 "|[-+*/%&^|!~]|>>|<<|&&|\\|\\|"),
> +PATTERNS("elisp",
> +	 /* Top level forms and outline headings */
> +	 "^((\\(|;;;+ +).+)",
> +	 /*
> +	  * Emacs Lisp allows symbol names containing any characters.
> +	  * However spaces within the symbol must be escaped.
> +	  */
> +	 "(\\.|[^ ()])+"
> +	 ),
>  PATTERNS("elixir",
>  	 "^[ \t]*((def(macro|module|impl|protocol|p)?|test)[ \t].*)$",
>  	 /* -- */

I think this patch would benefit from first being dogfooded in the emacs-devel community. There's an existing regex in emacs.git's autoconf.sh which anyone developing emacs.git is using.

If you run:

    diff -u <(git -c diff.elisp.xfuncname='^\(def[^[:space:]]+[[:space:]]+([^()[:space:]]+)' log -p -- lisp | grep -m 100 @@) \
	    <(git -c diff.elisp.xfuncname='^((\(|;;;+ +).*)$' log -p -- lisp | grep -m 100 @@) \
    | less

You can see how it differs from yours, and that also neatly answers the question[1] you had in the linked thread about why these patterns tend to be explicitly scoped to how you define a function/package/whatever in the language in question.

Just a cursory "git log -p -- lisp" in emacs.git with your patch shows e.g. lisp/thingatpt.el where forms in a "defun" aren't indented (before it selects the "defun", after with yours it's a "put" in the middle of the function).

Yours also changes it from e.g.:

    @@ -61,7 +61,7 @@ forward-thing

to:

    @@ -61,7 +61,7 @@ (defun forward-thing (thing &optional n)

Is this really desired in elisp? I also note how our tests in t4018-diff-funcname.sh are really bad in not testing for this at all. I.e. we just test that we match the right line, not how we extract a match from it.

1. https://lists.gnu.org/archive/html/emacs-devel/2021-02/msg00739.html