Re: [PATCH] userdiff: add support for Emacs Lisp
flat view
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-02-14 01:58:38
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.
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 :)
quoted hunk ↗ jump to hunk
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 javadiff --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