From: Johannes Sixt Date: Sat, 15 Nov 2025 17:06:44 GMT Subject: Re: [PATCH] diff: "lisp" userdiff_driver Message-ID: <773d3233-c890-4df9-8f7e-32ff8a48651e@kdbg.org> In-Reply-To: [Cc the author of the Scheme driver] Am 15.11.25 um 11:17 schrieb Scott L. Burson via GitGitGadget: > From: "Scott L. Burson" > > The "scheme" driver doesn't quite work for Common Lisp. This driver > is very generic and should work for almost any dialect of Lisp, > including Common Lisp. "Doesn't quite work" is unfortunately a description of the problem that does not help a reviewer decide whether this change is justified. Please add a lot more details why the Scheme driver is unsuitable for Lisp and why a new driver is needed. It is customary to mark changes to the drivers in the subject line with "userdiff:". Have a look at `git log userdiff.c`. It would be appreciated to stay away from nerdy tokens like "userdiff_driver" when the change can be summarized in plain English language. > > Signed-off-by: Scott L. Burson > --- > diff: "lisp" userdiff_driver > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2000%2Fslburson%2Flisp-userdiff_driver-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2000/slburson/lisp-userdiff_driver-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/2000 > > userdiff.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/userdiff.c b/userdiff.c > index fe710a68bf..e127b4a1f1 100644 > --- a/userdiff.c > +++ b/userdiff.c > @@ -249,6 +249,14 @@ PATTERNS("kotlin", > "|[.][0-9][0-9_]*([Ee][-+]?[0-9]+)?[fFlLuU]?" > /* unary and binary operators */ > "|[-+*/<>%&^|=!]==?|--|\\+\\+|<<=|>>=|&&|\\|\\||->|\\.\\*|!!|[?:.][.:]"), > +PATTERNS("lisp", > + /* Either an unindented left paren, or a slightly indented line > + * starting with "(def" */ > + "^((\\(|:space:{1,2}\\(def).*)$", Compared to the Scheme driver, this regular expression is - more restrictive because it does not permit arbitrary indentation; - less restrictive because it permits everything that begins with "(def". What would happen if this regular expression were added to the Scheme driver? Would it pick up additional and unwanted hunk headers is typical Scheme code? Just so you know: The string literal for hunk headers can contain "\n" to separate regular expressions. For a given line, they are attempted to match in order until the first match is found. > + /* Common Lisp symbol syntax allows arbitrary strings between vertical bars */ > + "\\|([^\\\\]|\\\\\\\\|\\\\\\|)*\\|" The Scheme driver has an similar description of this word token, but it has only half as many backslashes. Is the difference necessary? Isn't actually one or the other incorrect? (I did not try to understand what this version here does.) > + /* All other words are delimited by spaces or parentheses/brackets/braces */ > + "|([^][(){} \t])+"), This one looks identical to the same line in Scheme with a slightly altered comment. In summary, I feel like this could be unified with the Scheme driver, except when awkward false positive hunk headers in existing Scheme code are to be expected. That's a question that I cannot answer. -- Hannes