Re: [PATCH v2 2/2] merge with Scheme regexp; fix bugs

From: Johannes Sixt <j6t@kdbg.org>
Date: Nov 27, 2025, 16:09 UTC
Message-ID: <b6656e6d-d1e8-4ebe-821f-9211643a71ab@kdbg.org>
In-Reply-To: <86315aa3e36afa1ee741a2c9b9e95a71ca569302.1764211096.git.gitgitgadget@gmail.com>

Am 27.11.25 um 03:38 schrieb Scott L. Burson via GitGitGadget:

Show 31 quoted lines

> From: "Scott L. Burson" <Scott@sympoiesis.com>
> 
> This commit merges (by disjoining) the new generic Lisp regexp into
> the existing Scheme regexp.  It also fixes two bugs: the new regexp
> was unintentionally allowing tabs, and the matching of "(def" should
> be case-insensitive.
> 
> Signed-off-by: Scott L. Burson <Scott@sympoiesis.com>
> ---
>  userdiff.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/userdiff.c b/userdiff.c
> index e127b4a1f1..b67dfddbef 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -249,14 +249,6 @@ 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).*)$",
> -	 /* Common Lisp symbol syntax allows arbitrary strings between vertical bars */
> -	 "\\|([^\\\\]|\\\\\\\\|\\\\\\|)*\\|"
> -	 /* All other words are delimited by spaces or parentheses/brackets/braces */
> -	 "|([^][(){} \t])+"),
>  PATTERNS("markdown",
>  	 "^ {0,3}#{1,6}[ \t].*",
>  	 /* -- */

You made this commit a fixup commit of the commit from the first round. This isn't desirable as long as the earlier patch has not been integrated in "next", yet.

You should have squashed the commits into one. The cover letter gives a really good justification for this change and should be the commit's message (with its subject line, ie, the PR title). However, don't write "This commit does X", but write "Do X" instead: you give someone an order to change the code. (Also, after squashing there is no bug to fix anymore, of course.)

Show 7 quoted lines

> @@ -352,14 +344,21 @@ PATTERNS("rust",
>  	 "|[0-9][0-9_a-fA-Fiosuxz]*(\\.([0-9]*[eE][+-]?)?[0-9_fF]*)?"
>  	 "|[-+*\\/<>%&^|=!:]=|<<=?|>>=?|&&|\\|\\||->|=>|\\.{2}=|\\.{3}|::"),
>  PATTERNS("scheme",
> -	 "^[\t ]*(\\(((define|def(struct|syntax|class|method|rules|record|proto|alias)?)[-*/ \t]|(library|module|struct|class)[*+ \t]).*)$",
> +	 /* A possibly indented left paren followed by a Scheme keyword. */
> +	 "^[\t ]*(\\(((define|def(struct|syntax|class|method|rules|record|proto|alias)?)[-*/ \t]|(library|module|struct|class)[*+ \t]).*)$\n"

Mental note how this RE is nested:

	[\t ]*(
		\((
			(
				define|def(
					struct|syntax|class|method
					|rules|record|proto|alias
				)?
			)[-*/ \t]
			|
			(
				library|module|struct|class
			)[*+ \t]
		).*
	)$

Show 5 quoted lines

> +	 /*
> +	  * For other Lisp dialects: either an unindented left paren, or a
> +	  * slightly indented line starting with "(def".
> +	  */
> +	 "^((\\(| {1,2}\\([Dd][Ee][Ff]).*)$",

Here you are adding a very generous new pattern, the opening parenthesis without indentation. This will not only apply to "other Lisp dialects", as the comment says, but also Scheme code and will produce new matches. It does not change the test cases in t/t4018/scheme-*, because all have additional matches later.

As such it would possibly be more honest to extract it out into its own (first) pattern and marked as applying to all dialects:

	/*
	 * An unindented opening parenthesis identifies a top-level
         * structure in all Lisp dialects.
	 */
	"^(\\(.*)$\n",

Note that the Scheme pattern excludes the indentation from the capture. You may want to do so here, too (and simplify "one or two spaces" like this):

	"^  ?(\\([Dd][Ee][Ff].*)$",

Would it be possible to have test cases of Lisp code that is not covered by the Scheme pattern?

Show 9 quoted lines

>  	 /*
> -	  * R7RS valid identifiers include any sequence enclosed
> -	  * within vertical lines having no backslashes
> +	  * The union of R7RS and Common Lisp symbol syntax: allows arbitrary
> +	  * strings between vertical bars, including escaped backslashes and
> +	  * vertical bars.
>  	  */
> -	 "\\|([^\\\\]*)\\|"
> +	 "\\|([^\\\\]|\\\\\\\\|\\\\\\|)*\\|"

Without the C quoting we have

	\|([^\\]|\\\\|\\\|)*\|

>  	 /* All other words should be delimited by spaces or parentheses */
> -	 "|([^][)(}{[ \t])+"),
> +	 "|([^][)(}{ \t])+"),

Here we have a single bracket expression. The removed opening [ does not begin a new one, but is a duplicated character. Good.

-- Hannes