Re: [PATCH] userdiff: add support for Emacs Lisp

[Protesilaos Stavrou <info@protesilaos.com>]

On 2021-02-14, 19:25 +0100, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

> On Sun, Feb 14 2021, Johannes Sixt wrote:
>
>> Am 14.02.21 um 02:41 schrieb Ævar Arnfjörð Bjarmason:
>>> Just a cursory "git log -p -- lisp" in emacs.git with your patch shows
>>> e.g. lisp/thingatpt.el where forms in a "defun" aren't indented (before
>>> it selects the "defun", after with yours it's a "put" in the middle of
>>> the function).
>>
>> Note that negative matches can be specified. We use the feature in the
>> cpp case to exclude public:/protected:/private: and label: that happen 
>> to be not indented. Perhaps that can be useful here?
>>
>> Oh, and BTW, what the patterns treat as "function" must not match what
>> the language treats as function. The purpose of the hunk header is to 
>> spot a place in the source file easily. So, it does not hurt if
>> eval-and-compile forms are captured (as was mentioned in the linked 
>> thread) if desired.
>
> Right, so having lots of test-case is helpful, e.g. for elisp maybe you
> have a top-level defun, maybe not, maybe the top-level is a "(progn" so
> you'd like a second-level more meaningful context, or maybe not...
>
> Obviously these userdiff patterns aren't a general parser and will
> always be hit-and-miss, it's just useful to at least eyeball them
> against in-the-wild test data to check their sanity & add some tests.

Our intent with this patch is to rely on a heuristic that works for most
cases.  There will always be some case that is not covered.  This point
was made explicit in the relevant emacs-devel thread.

> My cursory glance at the emacs.git version v.s. what's being submitted
> here is that this one does a worse job in *very common* cases.
>
>>> Yours also changes it from e.g.:
>>>      @@ -61,7 +61,7 @@ forward-thing
>>> to:
>>>      @@ -61,7 +61,7 @@ (defun forward-thing (thing &optional n)
>>> Is this really desired in elisp?

I think this is a matter of perspective and indeed a valid reason for
doing this in emacs.git first.

For my part, I find the verbose style more informative, especially when
it captures the previous form instead of the one directly around the
diffed lines.

Perhaps one could add to that Emacs' ability to use the same name for
different things, e.g. a function and a variable, where verbosity of
this sort can help with disambiguation.  Granted, we should not really
on such a heading style to aid us in that task, though it may be
something to consider.

>> It's common practice to extract the entire line (sans indentation if
>> applicable), not just the function name. Why would somebody want the 
>> latter? It doesn't carry as much information as could be possible.
>
> Because I'm familiar with the codebase I'm editing and I just need to
> know that the diff I'm viewing is on the "main" function, not that it's
> "main()", "int main(int argv, char **argv)", "int main(const int argv,
> const char *argv[])", or to either have a " {" or not at the end
> depending on the project's coding style.

Oftentimes we produce/read patches for projects that we are not
necessarily acquainted with.  This includes emacs.git and the multitude
of Elisp packages in that milieu.  An extra element of contextuality can
do us good.

It is true that familiarity with the code base will always benefit from
succinct headings, though I feel that whenever we are in doubt we should
err on the side of caution: which means that we must not introduce such
an assumption to the workings of this piece of functionality.

> I know our own userdiff builtin patterns don't do this, but it would be
> very useful to retrofit this capability / maybe make it a configurable
> feature, i.e. have them capture the meaningful part of the line, and you
> could either print it all, or just that part.

It would be nice to have an option that toggles verbosity.  Though I
guess this lies outside the scope of the patch in question.

In conclusion, I think we should decide on the next step forward: if you
think this should be applied to emacs.git before making its way to git
itself, then we can move the discussion there.

Thanks for your time and efforts!
Protesilaos or "Prot"

-- 
Protesilaos Stavrou
protesilaos.com