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

On 2021-02-14, 19:25 +0100, Ævar Arnfjörð Bjarmason [off-list ref] wrote:

On Sun, Feb 14 2021, Johannes Sixt wrote:

quoted

Am 14.02.21 um 02:41 schrieb Ævar Arnfjörð Bjarmason:

quoted

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.

quoted

quoted

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.

quoted

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