unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* Patch upstream Git for Elisp diff hunk headings
@ 2021-02-11 14:42 Protesilaos Stavrou
  2021-02-11 18:27 ` Adam Spiers
  0 siblings, 1 reply; 8+ messages in thread
From: Protesilaos Stavrou @ 2021-02-11 14:42 UTC (permalink / raw)
  To: emacs-devel; +Cc: Adam Spiers

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Patch upstream Git for Elisp diff hunk headings
  2021-02-11 14:42 Patch upstream Git for Elisp diff hunk headings Protesilaos Stavrou
@ 2021-02-11 18:27 ` Adam Spiers
  2021-02-11 19:05   ` Stefan Kangas
                     ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Adam Spiers @ 2021-02-11 18:27 UTC (permalink / raw)
  To: Protesilaos Stavrou; +Cc: emacs-devel

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Patch upstream Git for Elisp diff hunk headings
  2021-02-11 18:27 ` Adam Spiers
@ 2021-02-11 19:05   ` Stefan Kangas
  2021-02-12 17:39     ` Basil L. Contovounesios
  2021-02-11 19:13   ` Clément Pit-Claudel
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Stefan Kangas @ 2021-02-11 19:05 UTC (permalink / raw)
  To: Adam Spiers, Protesilaos Stavrou; +Cc: emacs-devel

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.



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Patch upstream Git for Elisp diff hunk headings
  2021-02-11 18:27 ` Adam Spiers
  2021-02-11 19:05   ` Stefan Kangas
@ 2021-02-11 19:13   ` Clément Pit-Claudel
  2021-02-11 19:15   ` Óscar Fuentes
  2021-02-11 19:38   ` Stefan Monnier
  3 siblings, 0 replies; 8+ messages in thread
From: Clément Pit-Claudel @ 2021-02-11 19:13 UTC (permalink / raw)
  To: emacs-devel

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?




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Patch upstream Git for Elisp diff hunk headings
  2021-02-11 18:27 ` Adam Spiers
  2021-02-11 19:05   ` Stefan Kangas
  2021-02-11 19:13   ` Clément Pit-Claudel
@ 2021-02-11 19:15   ` Óscar Fuentes
  2021-02-11 19:38   ` Stefan Monnier
  3 siblings, 0 replies; 8+ messages in thread
From: Óscar Fuentes @ 2021-02-11 19:15 UTC (permalink / raw)
  To: emacs-devel

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.




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Patch upstream Git for Elisp diff hunk headings
  2021-02-11 18:27 ` Adam Spiers
                     ` (2 preceding siblings ...)
  2021-02-11 19:15   ` Óscar Fuentes
@ 2021-02-11 19:38   ` Stefan Monnier
  2021-02-13 19:35     ` Adam Spiers
  3 siblings, 1 reply; 8+ messages in thread
From: Stefan Monnier @ 2021-02-11 19:38 UTC (permalink / raw)
  To: Adam Spiers; +Cc: Protesilaos Stavrou, emacs-devel

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

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


        Stefan




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Patch upstream Git for Elisp diff hunk headings
  2021-02-11 19:05   ` Stefan Kangas
@ 2021-02-12 17:39     ` Basil L. Contovounesios
  0 siblings, 0 replies; 8+ messages in thread
From: Basil L. Contovounesios @ 2021-02-12 17:39 UTC (permalink / raw)
  To: Stefan Kangas; +Cc: Adam Spiers, Protesilaos Stavrou, emacs-devel

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Patch upstream Git for Elisp diff hunk headings
  2021-02-11 19:38   ` Stefan Monnier
@ 2021-02-13 19:35     ` Adam Spiers
  0 siblings, 0 replies; 8+ messages in thread
From: Adam Spiers @ 2021-02-13 19:35 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Protesilaos Stavrou, emacs-devel

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 ;-)



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-02-13 19:35 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-02-11 14:42 Patch upstream Git for Elisp diff hunk headings Protesilaos Stavrou
2021-02-11 18:27 ` Adam Spiers
2021-02-11 19:05   ` Stefan Kangas
2021-02-12 17:39     ` Basil L. Contovounesios
2021-02-11 19:13   ` Clément Pit-Claudel
2021-02-11 19:15   ` Óscar Fuentes
2021-02-11 19:38   ` Stefan Monnier
2021-02-13 19:35     ` Adam Spiers

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).