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.
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?
>
> 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.
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.
>> I also note how our tests in
>> t4018-diff-funcname.sh are really bad in not testing for this at
>> all. I.e. we just test that we match the right line, not how we extract
>> a match from it.
>
> Oh, well. These are "semi-automated" tests cases. If you have an idea
> how to achieve the goal without burdening test writers with lots of
> subtleties, be my guest. ;)
I'll do that soon.
>> 1. https://lists.gnu.org/archive/html/emacs-devel/2021-02/msg00739.html
>>
>
> -- Hannes
