Patch upstream Git for Elisp diff hunk headings

Patch upstream Git for Elisp diff hunk headings

[Protesilaos Stavrou]

Dear members,

Myself and Adam Spiers (in cc) have been discussing the possibility of
patching Git so that it can handle Emacs Lisp diff hunk headings
natively.  Those headings consist of the text that follows the line
ranges that diff outputs.  So this:

  @@ -389,7 +390,7 @@ HEADING HERE

Git produces those headings using language-specific regular expressions.
Elisp is not covered.  Users must thus define their own Git attributes.
Not doing so results in practically useless text for the heading; text
that does not provide an accurate sense of context.

I have noticed that Emacs' git repo includes the file ".gitattributes"
which instructs diffs inside of that repo to use an Elisp-aware regexp
present in "autogen.sh":

  git_config diff.elisp.xfuncname \
             '^\([^[:space:]]*def[^[:space:]]+[[:space:]]+([^()[:space:]]+)'

This is an improvement over the out-of-the-box Git experience.

Now the questions to this list:

1. What do you think about only targeting top-level forms?

2. How about the comments that are interpreted by outline-minor-mode as
   headings?  So the three or more ";;;" at the beginning of a line.
   Would it not be right to match those as well, since they are supposed
   to be 'headings' in Elisp source code?

Thank you in advance!
Protesilaos or "Prot"

-- 
Protesilaos Stavrou
protesilaos.com

Re: Patch upstream Git for Elisp diff hunk headings

[Adam Spiers]

On Thu, 11 Feb 2021 at 14:42, Protesilaos Stavrou <info@protesilaos.com> wrote: 
>Dear members, 
>
>Myself and Adam Spiers (in cc) have been discussing the possibility of 
>patching Git so that it can handle Emacs Lisp diff hunk headings 
>natively.  Those headings consist of the text that follows the line 
>ranges that diff outputs.  So this: 
>
>   @@ -389,7 +390,7 @@ HEADING HERE
>
>Git produces those headings using language-specific regular expressions. 
>Elisp is not covered.  Users must thus define their own Git attributes. 
>Not doing so results in practically useless text for the heading; text 
>that does not provide an accurate sense of context. 
>
>I have noticed that Emacs' git repo includes the file ".gitattributes" 
>which instructs diffs inside of that repo to use an Elisp-aware regexp 
>present in "autogen.sh": 
>
>   git_config diff.elisp.xfuncname \
>              '^\([^[:space:]]*def[^[:space:]]+[[:space:]]+([^()[:space:]]+)'
>
>This is an improvement over the out-of-the-box Git experience. 
>
>Now the questions to this list: 
>
>1. What do you think about only targeting top-level forms? 

Thanks Prot.  To add a little bit of colour to this particular 
question, I'm thinking of cases like 

    (use-package some-package
       :config
       [... many lines of config code ...])

or many other top-level forms commonly found in users' emacs init 
files.

These would not match the regexp above, and since clearly it is 
possible to define any function or macro, the only way I can think of 
to teach git a line-based regexp for recognising the start of 
top-level forms is to make the assumption that they are not indented, 
e.g.

    git_config diff.elisp.xfuncname "^(\\(.*)$"

Indeed this is what Prot initially suggested in his original blog post 
which sparked this discussion: 

    https://protesilaos.com/codelog/2021-01-26-git-diff-hunk-elisp-org/

While of course no one is claiming that this is technically correct in 
all cases, I don't think I can actually recall ever seeing a single 
instance of a top-level form which *was* indented.  So presumably it's 
a good enough heuristic to be helpful in the vast majority of cases. 
Or at least it's more likely to be right than a heuristic which 
assumes that all top-level forms include "def" in the first symbol in 
the form, since the latter obviously fails in many common scenarios 
including the init files one mentioned above. 

Would be grateful to hear other views on this.

Thanks,
Adam

Re: Patch upstream Git for Elisp diff hunk headings

[Stefan Kangas]

Adam Spiers <emacs-devel@adamspiers.org> writes:

> While of course no one is claiming that this is technically correct in
> all cases, I don't think I can actually recall ever seeing a single
> instance of a top-level form which *was* indented.  So presumably it's
> a good enough heuristic to be helpful in the vast majority of cases.
> Or at least it's more likely to be right than a heuristic which
> assumes that all top-level forms include "def" in the first symbol in
> the form, since the latter obviously fails in many common scenarios
> including the init files one mentioned above.

(It also fails with `ert-deftest', and probably also other unit test
frameworks in popular use.)

I'd say go for it.  The heuristics seems like a clear improvement and
this feature would be appreciated.

Thanks for working on this.

Re: Patch upstream Git for Elisp diff hunk headings

[Clément Pit-Claudel]

On 2/11/21 1:27 PM, Adam Spiers wrote:
> While of course no one is claiming that this is technically correct in 
> all cases, I don't think I can actually recall ever seeing a single 
> instance of a top-level form which *was* indented.

This heuristic is going to break for `eval-and-compile` forms, which are reasonably common.
Maybe combine both heuristics and detect both `(def` at any indentation and `(` in column 0?

Re: Patch upstream Git for Elisp diff hunk headings

[Óscar Fuentes]

Adam Spiers <emacs-devel@adamspiers.org> writes:

>     git_config diff.elisp.xfuncname "^(\\(.*)$"
>
> Indeed this is what Prot initially suggested in his original blog post 
> which sparked this discussion: 
>
>     https://protesilaos.com/codelog/2021-01-26-git-diff-hunk-elisp-org/
>
> While of course no one is claiming that this is technically correct in 
> all cases, I don't think I can actually recall ever seeing a single 
> instance of a top-level form which *was* indented.  So presumably it's 
> a good enough heuristic to be helpful in the vast majority of cases. 
> Or at least it's more likely to be right than a heuristic which 
> assumes that all top-level forms include "def" in the first symbol in 
> the form, since the latter obviously fails in many common scenarios 
> including the init files one mentioned above. 
>
> Would be grateful to hear other views on this.

Indeed, "^(\\(.*)$" is good enough IMHO, and I do not understand the
advantages of the second variant that Prot mentioned on his blog.

Re: Patch upstream Git for Elisp diff hunk headings

[Stefan Monnier]

>     git_config diff.elisp.xfuncname "^(\\(.*)$"

LGTM.
But as you suggested, it would be good to add something like
"^;;;;* [^[:space:]]" to it.


        Stefan

Re: Patch upstream Git for Elisp diff hunk headings

[Basil L. Contovounesios]

Stefan Kangas <stefankangas@gmail.com> writes:

> Adam Spiers <emacs-devel@adamspiers.org> writes:
>
>> While of course no one is claiming that this is technically correct in
>> all cases, I don't think I can actually recall ever seeing a single
>> instance of a top-level form which *was* indented.  So presumably it's
>> a good enough heuristic to be helpful in the vast majority of cases.
>> Or at least it's more likely to be right than a heuristic which
>> assumes that all top-level forms include "def" in the first symbol in
>> the form, since the latter obviously fails in many common scenarios
>> including the init files one mentioned above.
>
> (It also fails with `ert-deftest', and probably also other unit test
> frameworks in popular use.)

The current regexp doesn't fail on ert-deftest.

-- 
Basil

Re: Patch upstream Git for Elisp diff hunk headings

[Adam Spiers]

On Thu, 11 Feb 2021 at 19:38, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> >     git_config diff.elisp.xfuncname "^(\\(.*)$"
>
> LGTM.
> But as you suggested, it would be good to add something like
> "^;;;;* [^[:space:]]" to it.

Thanks Stefan and everyone else for your feedback.

This has now been submitted to the git mailing list:

    https://public-inbox.org/git/20210213192447.6114-1-git@adamspiers.org/

so if you have further feedback, please reply there.

(I thought about cross-posting, but that usually angers at least one person ;-)