Should diff.elisp.xfuncname match cl-lib macros?

Should diff.elisp.xfuncname match cl-lib macros?

[Basil L. Contovounesios]

The current git-config setting diff.elisp.xfuncname generated by
autogen.sh matches definitions beginning with '(def', but not '(cl-def'.

Given the increasing presence of cl-defuns, cl-defgenerics, etc. in the
emacs.git sources, would it be welcome to additionally match cl-lib
macros?  Or should that customisation be left up to the individual?

Thanks,

-- 
Basil

Re: Should diff.elisp.xfuncname match cl-lib macros?

[Stefan Monnier]

> Given the increasing presence of cl-defuns, cl-defgenerics, etc. in the
> emacs.git sources, would it be welcome to additionally match cl-lib
> macros?

Yes, please,


        Stefan

Re: Should diff.elisp.xfuncname match cl-lib macros?

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

What is the feature for which people typically use cl-defun?
Maybe we should make it a standard, builtin feature of defun.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: Should diff.elisp.xfuncname match cl-lib macros?

[Basil L. Contovounesios]

[-- Attachment #1: Type: text/plain, Size: 356 bytes --]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Given the increasing presence of cl-defuns, cl-defgenerics, etc. in the
>> emacs.git sources, would it be welcome to additionally match cl-lib
>> macros?
>
> Yes, please,

I can think of three ways to achieve this given the lack of shy groups
in EREs:

0. Match "def" anywhere within the form's name:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: prefix.patch --]
[-- Type: text/x-diff, Size: 539 bytes --]

diff --git a/autogen.sh b/autogen.sh
index 40d0c37b11..c931103249 100755
--- a/autogen.sh
+++ b/autogen.sh
@@ -317,7 +317,7 @@ hooks=
 # Configure 'git diff' hunk header format.
 
 git_config diff.elisp.xfuncname \
-	   '^\(def[^[:space:]]+[[:space:]]+([^()[:space:]]+)'
+           '^\([^[:space:]]*def[^[:space:]]+[[:space:]]+([^()[:space:]]+)'
 git_config 'diff.m4.xfuncname' '^((m4_)?define|A._DEFUN(_ONCE)?)\([^),]*'
 git_config 'diff.make.xfuncname' \
 	   '^([$.[:alnum:]_].*:|[[:alnum:]_]+[[:space:]]*([*:+]?[:?]?|!?)=|define .*)'

[-- Attachment #3: Type: text/plain, Size: 30 bytes --]


1. Match any column 0 form:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: any.patch --]
[-- Type: text/x-diff, Size: 523 bytes --]

diff --git a/autogen.sh b/autogen.sh
index 40d0c37b11..860d980c95 100755
--- a/autogen.sh
+++ b/autogen.sh
@@ -317,7 +317,7 @@ hooks=
 # Configure 'git diff' hunk header format.
 
 git_config diff.elisp.xfuncname \
-	   '^\(def[^[:space:]]+[[:space:]]+([^()[:space:]]+)'
+           '^\([^[:space:]]+[[:space:]]+([^()[:space:]]+)'
 git_config 'diff.m4.xfuncname' '^((m4_)?define|A._DEFUN(_ONCE)?)\([^),]*'
 git_config 'diff.make.xfuncname' \
 	   '^([$.[:alnum:]_].*:|[[:alnum:]_]+[[:space:]]*([*:+]?[:?]?|!?)=|define .*)'

[-- Attachment #5: Type: text/plain, Size: 56 bytes --]


1. Capture the whole header line, including the form:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: whole.patch --]
[-- Type: text/x-diff, Size: 532 bytes --]

diff --git a/autogen.sh b/autogen.sh
index 40d0c37b11..c28c8928ed 100755
--- a/autogen.sh
+++ b/autogen.sh
@@ -317,7 +317,7 @@ hooks=
 # Configure 'git diff' hunk header format.
 
 git_config diff.elisp.xfuncname \
-	   '^\(def[^[:space:]]+[[:space:]]+([^()[:space:]]+)'
+           '^(\((cl-)?def[^[:space:]]+[[:space:]]+[^()[:space:]]+)'
 git_config 'diff.m4.xfuncname' '^((m4_)?define|A._DEFUN(_ONCE)?)\([^),]*'
 git_config 'diff.make.xfuncname' \
 	   '^([$.[:alnum:]_].*:|[[:alnum:]_]+[[:space:]]*([*:+]?[:?]?|!?)=|define .*)'

[-- Attachment #7: Type: text/plain, Size: 65 bytes --]


Which is preferred?  Is there a better way?

Thanks,

-- 
Basil

cl-defun vs defun (was: Should diff.elisp.xfuncname match cl-lib macros?)

[Basil L. Contovounesios]

Richard Stallman <rms@gnu.org> writes:

> What is the feature for which people typically use cl-defun?
> Maybe we should make it a standard, builtin feature of defun.

As described under "(cl) Argument Lists", cl-defun extends defun to
support CL argument lists (with default values for optional arguments,
keyword arguments, etc.) and enclose the function body in an implicit
block.  In my limited experience, the former is by far the more
frequently used feature.

I, for one, would welcome support for CL argument lists in vanilla
defun.  Has this been discussed before?

-- 
Basil

Re: Should diff.elisp.xfuncname match cl-lib macros?

[Eli Zaretskii]

> From: "Basil L. Contovounesios" <contovob@tcd.ie>
> Date: Sat, 16 Mar 2019 15:30:22 +0000
> Cc: emacs-devel@gnu.org
> 
> >> Given the increasing presence of cl-defuns, cl-defgenerics, etc. in the
> >> emacs.git sources, would it be welcome to additionally match cl-lib
> >> macros?
> >
> > Yes, please,
> 
> I can think of three ways to achieve this given the lack of shy groups
> in EREs:

Is this feature even useful with Lisp?  I find it mostly useless,
here's a typical example:

  git log -L :next-line:lisp/simple.el

Type this command in your repository, then watch the fun.  More fun is
available when Git for some reason decides you asked about some
variable, not a function (and in Lisp we frequently have variables by
the same name as a function).

So I think until we fix this basic deficiency, extending that to more
symbols will just add to the mess.  YMMV.

Re: Should diff.elisp.xfuncname match cl-lib macros?

[Stefan Monnier]

> Is this feature even useful with Lisp?  I find it mostly useless,
> here's a typical example:
>
>   git log -L :next-line:lisp/simple.el

Hmm... didn't know about this feature.  AFAIK xfuncname is mostly used
to add the "containing function name" on the header of each diff hunk.


        Stefan

Re: Should diff.elisp.xfuncname match cl-lib macros?

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sat, 16 Mar 2019 13:12:04 -0400
> 
> AFAIK xfuncname is mostly used to add the "containing function name"
> on the header of each diff hunk.

That's a million times less important to me than "git log -L".

Re: Should diff.elisp.xfuncname match cl-lib macros?

[Stefan Monnier]

>> AFAIK xfuncname is mostly used to add the "containing function name"
>> on the header of each diff hunk.
> That's a million times less important to me than "git log -L".

Indeed, it's not an important feature at all.  It's just a nice "frill".

Given the inevitable unreliability of the info it gets, I'm surprised it
grew to take a more significant role in -L.  I guess it's still
worthwhile in the sense that it's good when it works and when it doesn't
you're no worse off than if you didn't have that feature.


        Stefan

Re: Should diff.elisp.xfuncname match cl-lib macros?

[Basil L. Contovounesios]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: "Basil L. Contovounesios" <contovob@tcd.ie>
>> Date: Sat, 16 Mar 2019 15:30:22 +0000
>> Cc: emacs-devel@gnu.org
>> 
>> >> Given the increasing presence of cl-defuns, cl-defgenerics, etc. in the
>> >> emacs.git sources, would it be welcome to additionally match cl-lib
>> >> macros?
>> >
>> > Yes, please,
>> 
>> I can think of three ways to achieve this given the lack of shy groups
>> in EREs:
>
> Is this feature even useful with Lisp?  I find it mostly useless,
> here's a typical example:
>
>   git log -L :next-line:lisp/simple.el
>
> Type this command in your repository, then watch the fun.  More fun is
> available when Git for some reason decides you asked about some
> variable, not a function (and in Lisp we frequently have variables by
> the same name as a function).

I don't think Git ever tries to distinguish between variables and
functions, despite the name of xfuncname.

> So I think until we fix this basic deficiency, extending that to more
> symbols will just add to the mess.  YMMV.

Which deficiency do you mean?  That 'git log -L' picks up
next-line-or-history-element instead of next-line in your example,
i.e. that it's currently inaccurate?  How do you usually use this
feature?  What kind of fix do you envision?  (Sorry, I don't have
experience using this Git feature.)

-- 
Basil

Re: Should diff.elisp.xfuncname match cl-lib macros?

[Basil L. Contovounesios]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> AFAIK xfuncname is mostly used to add the "containing function name"
>>> on the header of each diff hunk.
>> That's a million times less important to me than "git log -L".
>
> Indeed, it's not an important feature at all.  It's just a nice "frill".
>
> Given the inevitable unreliability of the info it gets, I'm surprised it
> grew to take a more significant role in -L.  I guess it's still
> worthwhile in the sense that it's good when it works and when it doesn't
> you're no worse off than if you didn't have that feature.

I agree, FWIW.  My main use for xfuncname is as a manual alternative to
add-change-log-entry, by using diff hunk headers to know where a change
has been made.  When searching Git logs I tend to use --grep or -G
rather than -L.

-- 
Basil

Re: cl-defun vs defun (was: Should diff.elisp.xfuncname match cl-lib macros?)

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > As described under "(cl) Argument Lists", cl-defun extends defun to
  > support CL argument lists (with default values for optional arguments,
  > keyword arguments, etc.)

Long ago, adding these features to Emacs would have been bloat and would
have caused problems for users.  But not nowadays.  So I see no reason
to oppose it.


-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: Should diff.elisp.xfuncname match cl-lib macros?

[Eli Zaretskii]

> From: "Basil L. Contovounesios" <contovob@tcd.ie>
> Cc: <monnier@iro.umontreal.ca>,  <emacs-devel@gnu.org>
> Date: Sat, 16 Mar 2019 21:54:55 +0000
> 
> > So I think until we fix this basic deficiency, extending that to more
> > symbols will just add to the mess.  YMMV.
> 
> Which deficiency do you mean?  That 'git log -L' picks up
> next-line-or-history-element instead of next-line in your example,
> i.e. that it's currently inaccurate?

Yes.

> How do you usually use this feature?

With commands such as the one I show above.  It's supposed to show the
history of changes in the named function, but instead it almost always
shows unrelated history.

Re: Should diff.elisp.xfuncname match cl-lib macros?

[Andreas Schwab]

On Mär 17 2019, Eli Zaretskii <eliz@gnu.org> wrote:

> With commands such as the one I show above.  It's supposed to show the
> history of changes in the named function, but instead it almost always
> shows unrelated history.

In which way is it unrelated?  You asked for the history of any function
matching `next-line', and you got it.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: Should diff.elisp.xfuncname match cl-lib macros?

[Basil L. Contovounesios]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: "Basil L. Contovounesios" <contovob@tcd.ie>
>> Cc: <monnier@iro.umontreal.ca>,  <emacs-devel@gnu.org>
>> Date: Sat, 16 Mar 2019 21:54:55 +0000
>> 
>> > So I think until we fix this basic deficiency, extending that to more
>> > symbols will just add to the mess.  YMMV.
>> 
>> Which deficiency do you mean?  That 'git log -L' picks up
>> next-line-or-history-element instead of next-line in your example,
>> i.e. that it's currently inaccurate?
>
> Yes.
>
>> How do you usually use this feature?
>
> With commands such as the one I show above.  It's supposed to show the
> history of changes in the named function, but instead it almost always
> shows unrelated history.

I'm not aware of any way around this other than through restricting the
search regexp, as Andreas alluded to:

git log -L ':next-line[^-]:lisp/simple.el'

Are you?  Either way, I'm not sure the proposed change to xfuncname will
make 'git log -L' any harder to use in practice.  On the contrary, it
will allow using 'git log -L' to search for cl-defuns, etc., where this
is currently not possible.

-- 
Basil

Re: Should diff.elisp.xfuncname match cl-lib macros?

[Juri Linkov]

> Is this feature even useful with Lisp?  I find it mostly useless,
> here's a typical example:
>
>   git log -L :next-line:lisp/simple.el
>
> Type this command in your repository, then watch the fun.  More fun is
> available when Git for some reason decides you asked about some
> variable, not a function (and in Lisp we frequently have variables by
> the same name as a function).

Since it uses the first match, a workaround would be first to find
a match immediately before 'next-line', e.g. 'next-line-add-newlines',
then starting from its position continue the search for 'next-line':

git log -L :next-line-add-newlines:lisp/simple.el -L :next-line:lisp/simple.el

Maybe this feature could be used by ‘C-x v h’ (vc-region-history)
in case when there is no active region.  Currently it signals
an error "The mark is not active now", but could use the name
of the current defun for '-L :defun:filename'.

PS: Regarding the change in diff.elisp.xfuncname, can we also have
an appropriate regexp for diff.c.xfuncname as well, to find
DEFUN in C code?

Re: Should diff.elisp.xfuncname match cl-lib macros?

[Stefan Monnier]

> Maybe this feature could be used by ‘C-x v h’ (vc-region-history)
> in case when there is no active region.  Currently it signals
> an error "The mark is not active now", but could use the name
> of the current defun for '-L :defun:filename'.

Wouldn't mark-defun give a better result (i.e. let the major mode
determine the corresponding region rather than rely on Git's xfuncname)?


        Stefan

Re: Should diff.elisp.xfuncname match cl-lib macros?

[Juri Linkov]

>> Maybe this feature could be used by ‘C-x v h’ (vc-region-history)
>> in case when there is no active region.  Currently it signals
>> an error "The mark is not active now", but could use the name
>> of the current defun for '-L :defun:filename'.
>
> Wouldn't mark-defun give a better result (i.e. let the major mode
> determine the corresponding region rather than rely on Git's xfuncname)?

I can confirm that 'C-M-h C-x v h' produces the same result as
'-L :funcname:filename' for most cases, and is more reliable
for ambiguous names like 'next-line'.

Re: Should diff.elisp.xfuncname match cl-lib macros?

[Basil L. Contovounesios]

[-- Attachment #1: Type: text/plain, Size: 345 bytes --]

Juri Linkov <juri@linkov.net> writes:

> PS: Regarding the change in diff.elisp.xfuncname, can we also have
> an appropriate regexp for diff.c.xfuncname as well, to find
> DEFUN in C code?

I'm not sure we need to define a new pattern for C files distinct from
the built-in cpp pattern, but here's a patch extending the cpp and elisp
patterns:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: autogen.diff --]
[-- Type: text/x-diff, Size: 967 bytes --]

diff --git a/autogen.sh b/autogen.sh
index 40d0c37b11..b5722bdb12 100755
--- a/autogen.sh
+++ b/autogen.sh
@@ -316,8 +316,16 @@ hooks=
 
 # Configure 'git diff' hunk header format.
 
+# This xfuncname is based on Git's built-in 'cpp' pattern.
+# The first line rejects jump targets and access declarations.
+# The second line matches top-level functions and methods.
+# The third line matches preprocessor and DEFUN macros.
+git_config diff.cpp.xfuncname \
+'!^[ \t]*[A-Za-z_][A-Za-z_0-9]*:[[:space:]]*($|/[/*])
+^((::[[:space:]]*)?[A-Za-z_][A-Za-z_0-9]*[[:space:]]*\(.*)$
+^((#define[[:space:]]|DEFUN).*)$'
 git_config diff.elisp.xfuncname \
-	   '^\(def[^[:space:]]+[[:space:]]+([^()[:space:]]+)'
+           '^\([^[:space:]]*def[^[:space:]]+[[:space:]]+([^()[:space:]]+)'
 git_config 'diff.m4.xfuncname' '^((m4_)?define|A._DEFUN(_ONCE)?)\([^),]*'
 git_config 'diff.make.xfuncname' \
 	   '^([$.[:alnum:]_].*:|[[:alnum:]_]+[[:space:]]*([*:+]?[:?]?|!?)=|define .*)'

[-- Attachment #3: Type: text/plain, Size: 27 bytes --]


WDYT?

Thanks,

-- 
Basil

Re: Should diff.elisp.xfuncname match cl-lib macros?

[Basil L. Contovounesios]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> Juri Linkov <juri@linkov.net> writes:
>
>> PS: Regarding the change in diff.elisp.xfuncname, can we also have
>> an appropriate regexp for diff.c.xfuncname as well, to find
>> DEFUN in C code?
>
> I'm not sure we need to define a new pattern for C files distinct from
> the built-in cpp pattern, but here's a patch extending the cpp and elisp
> patterns:
>
> diff --git a/autogen.sh b/autogen.sh
> index 40d0c37b11..b5722bdb12 100755
> --- a/autogen.sh
> +++ b/autogen.sh
> @@ -316,8 +316,16 @@ hooks=
>  
>  # Configure 'git diff' hunk header format.
>  
> +# This xfuncname is based on Git's built-in 'cpp' pattern.
> +# The first line rejects jump targets and access declarations.
> +# The second line matches top-level functions and methods.
> +# The third line matches preprocessor and DEFUN macros.
> +git_config diff.cpp.xfuncname \
> +'!^[ \t]*[A-Za-z_][A-Za-z_0-9]*:[[:space:]]*($|/[/*])
> +^((::[[:space:]]*)?[A-Za-z_][A-Za-z_0-9]*[[:space:]]*\(.*)$
> +^((#define[[:space:]]|DEFUN).*)$'
>  git_config diff.elisp.xfuncname \
> -	   '^\(def[^[:space:]]+[[:space:]]+([^()[:space:]]+)'
> +           '^\([^[:space:]]*def[^[:space:]]+[[:space:]]+([^()[:space:]]+)'
>  git_config 'diff.m4.xfuncname' '^((m4_)?define|A._DEFUN(_ONCE)?)\([^),]*'
>  git_config 'diff.make.xfuncname' \
>  	   '^([$.[:alnum:]_].*:|[[:alnum:]_]+[[:space:]]*([*:+]?[:?]?|!?)=|define .*)'
>
> WDYT?

No further comments, so I pushed this to master[1].

[1: d3a0ddedba]: Improve C and Elisp Git diff hunk headers
  2019-05-20 16:02:11 +0100
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=d3a0ddedba53b9e2c99274c8ec125d53f991da5d

Thanks,

-- 
Basil