diff: "lisp" userdiff_driver #2000
Conversation
Welcome to GitGitGadgetHi @slburson, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that either:
You can CC potential reviewers by adding a footer to the PR description with the following syntax: NOTE: DO NOT copy/paste your CC list from a previous GGG PR's description, Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join [email protected], where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
|
@dscho Could I trouble you for a |
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. Signed-off-by: Scott L. Burson <[email protected]>
slburson
force-pushed
the
lisp-userdiff_driver
branch
from
ccf37a5 to
da99bb0
Compare
|
/allow |
|
User slburson is now allowed to use GitGitGadget. |
|
@dscho Thanks! |
|
/preview |
|
Preview email sent as [email protected] |
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Johannes Sixt wrote (reply to this): [Cc the author of the Scheme driver]
Am 15.11.25 um 11:17 schrieb Scott L. Burson via GitGitGadget:
> From: "Scott L. Burson" <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
|
|
On the Git mailing list, "Scott L. Burson" wrote (reply to this): On Sat, Nov 15, 2025 at 9:06 AM Johannes Sixt <[email protected]> wrote:
>
> [Cc the author of the Scheme driver]
>
> Am 15.11.25 um 11:17 schrieb Scott L. Burson via GitGitGadget:
> > From: "Scott L. Burson" <[email protected]>
>
> Please
> add a lot more details why the Scheme driver is unsuitable for Lisp and
> why a new driver is needed.
Here is text I propose for the commit message:
----
Common Lisp has top-level forms 'defun' and 'deftype' that are not
matched by the current Scheme pattern. Also, it is more common when
defining user macros intended as top-level forms to prefix their names
with "def" instead of "define"; such forms are also not matched. And
some such forms don't even begin with "def".
On the other hand, it is an established formatting convention in the
Lisp community that only top-level forms start at the left margin. So
matching any unindented line starting with an open parenthesis is an
acceptable heuristic; false positives will be rare.
However, there are also cases where notionally top-level forms are
grouped together within some containing form. At least in the Common
Lisp community, it is conventional to indent these by two spaces, or
sometimes one. But matching just an open parenthesis indented by two
spaces would be too broad; so the pattern added by this commit
requires an indented form to start with "(def". It is believed that
this strikes a good balance between potential false positives and
false negatives.
----
I discussed the pattern with some other experienced Common Lisp
developers on a mailing list, and this is what I settled on after
incorporating their feedback.
> 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.
Will do.
> >
> > Signed-off-by: Scott L. Burson <[email protected]>
> > ---
> > 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?
That is a good question. I don't think so, but I don't work in Scheme.
I see that you have CC'ed Atharva Raykar; let's see whether he would
have any objection.
I would point out that Scheme is a dialect of Lisp, not the other way
around. (Lisp is unusual in being a family of languages, rather than a
single language.) And having a separate "lisp" driver might aid
discoverability.
But I understand: Scheme got their driver in first, and you have to fight
against the tendency of the driver list to grow unboundedly.
Ooh, that reminds me: if we do decide to add a "lisp" driver, I'll also need
to add it to 'Documentation/gitattributes.adoc'.
> The string literal for hunk headers can contain "\n"
Noted.
> > + /* 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.)
It's not important, but technically, Common Lisp allows an escaped
backslash between vertical bars, but the R7RS formal grammar does not.
However, I just tried Chicken Scheme, which claims to be at least
partially R7RS compliant, and it does accept the escaped backslash. I
am left to conclude that Scheme implementors think that the omission
of the escaped backslash from the R7RS formal grammar is an oversight
(I think so too).
Of course, no one would actually write a symbol name with an escaped
backslash in it unless they were submitting to an obfuscated Lisp
contest. So we are really being pedantic here. Still, may as well allow
it.
Atharva, any comments?
-- Scott |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Johannes Sixt <[email protected]> writes:
>> + /* 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?
As we generally assume that the file being edited is syntactically
sound, even if one lisp variant understands "(deffoo" and others do
not, it should be generally fine for the pattern to say something
like "at the beginning of the line, optionally following a few
spaces, four-letter sequence '(def' is likely to be the beginning of
a function definition", as long as there is some convention that
user defined functions and macros, unless they are to behave
similarly to "(defun", would not be named so confusingly to start
with d-e-f.
It would be nice if a single set of rules can cover what existing
scheme patterns cover, Emacs lisp, and Common lisp.
|
2 participants
CC: Junio C Hamano [email protected], Johannes Sixt [email protected], Ævar Arnfjörð Bjarmason [email protected], Jaydeep P Das [email protected]