bug#59575: 29.0.50; add-log-current-defun-header-regexp matches Windows drive letter

bug#59575: 29.0.50; add-log-current-defun-header-regexp matches Windows drive letter

[Juanma Barranquero]

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

If you have an *xref* buffer with absolute Windows filenames, like

~/.emacs.d/init.el
  93:             server-name    (replace-regexp-in-string "\\\\+" "\\"
serv t t)
1102:   (let ((s (when server-name
1104:                (string-match (rx (+ (not (any ?\\))) line-end)
server-name)
1105:                (upcase (match-string 0 server-name))))))
d:/Devel/emacs/repo/trunk/lisp/erc/erc-backend.el
1820:   (pcase-let ((`(,server-name ,server-version)
1823:     (setq erc-server-announced-name server-name)
1827:      's004 ?s server-name ?v server-version

and put the cursor in an absolute filename line (like the one
d:/Devel/[etc] above), calling `add-log-current-defun' returns the drive
letter "d", because it matches a-l-c-d-header-regexp.

The effect is visible when you have which-function-mode enabled, because
the function returns non-nil, so which-function does not resort to imenu,
and you end with "d" in the mode-line.

I suppose this should be fixed in xref.el, which apparently assumes that
file name lines will be either relative or Unix-style (/path/file works
correctly, it's just d:/path/file that fails) and the add-log heuristics
will always fail.

[-- Attachment #2: Type: text/html, Size: 1684 bytes --]

bug#59575: 29.0.50; add-log-current-defun-header-regexp matches Windows drive letter

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Fri, 25 Nov 2022 16:53:51 +0100
> 
> If you have an *xref* buffer with absolute Windows filenames, like
> 
> ~/.emacs.d/init.el
>   93:             server-name    (replace-regexp-in-string "\\\\+" "\\" serv t t)
> 1102:   (let ((s (when server-name
> 1104:                (string-match (rx (+ (not (any ?\\))) line-end) server-name)
> 1105:                (upcase (match-string 0 server-name))))))
> d:/Devel/emacs/repo/trunk/lisp/erc/erc-backend.el
> 1820:   (pcase-let ((`(,server-name ,server-version)
> 1823:     (setq erc-server-announced-name server-name)
> 1827:      's004 ?s server-name ?v server-version
> 
> and put the cursor in an absolute filename line (like the one d:/Devel/[etc] above), calling
> `add-log-current-defun' returns the drive letter "d", because it matches a-l-c-d-header-regexp.
> 
> The effect is visible when you have which-function-mode enabled, because the function returns non-nil, so
> which-function does not resort to imenu, and you end with "d" in the mode-line.
> 
> I suppose this should be fixed in xref.el, which apparently assumes that file name lines will be either relative
> or Unix-style (/path/file works correctly, it's just d:/path/file that fails) and the add-log heuristics will always
> fail.

xref.el doesn't know anything about add-log, and AFAICT doesn't customize it
in any way, shape, or form.  So I think this should be fixed in add-log.el.
Its regexp is too naïve, and should be beefed-up not to fail in this way.
For example, is it really reasonable to accept "defuns" whose name is a
single letter?  Or if it's impossible to do that in the regexp, then we
should reject such "matches" in add-log-current-defun instead.

It is also possible to have xref.el define its customized
add-log-current-defun-function, but that sounds like overkill to me.
Dmitry, WDYT?

bug#59575: 29.0.50; add-log-current-defun-header-regexp matches Windows drive letter

[Juanma Barranquero]

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

On Sat, Nov 26, 2022 at 2:03 PM Eli Zaretskii <eliz@gnu.org> wrote:

> It is also possible to have xref.el define its customized
> add-log-current-defun-function, but that sounds like overkill to me.

That's what I meant, because it seemed to me less fragile than fiddling
with the regexp. But I don't have a strong preference, as long as it is
fixed.

[-- Attachment #2: Type: text/html, Size: 723 bytes --]

bug#59575: 29.0.50; add-log-current-defun-header-regexp matches Windows drive letter

[Dmitry Gutov]

On 26/11/22 15:03, Eli Zaretskii wrote:
> It is also possible to have xref.el define its customized
> add-log-current-defun-function, but that sounds like overkill to me.
> Dmitry, WDYT?

It seems like

   (setq add-log-current-defun-function nil)

should suffice.

Is that the intended behavior, to report the absence of the current defun?

bug#59575: 29.0.50; add-log-current-defun-header-regexp matches Windows drive letter

[Eli Zaretskii]

> Date: Sun, 27 Nov 2022 03:54:27 +0200
> Cc: 59575@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> On 26/11/22 15:03, Eli Zaretskii wrote:
> > It is also possible to have xref.el define its customized
> > add-log-current-defun-function, but that sounds like overkill to me.
> > Dmitry, WDYT?
> 
> It seems like
> 
>    (setq add-log-current-defun-function nil)
> 
> should suffice.

The value is already nil in the *xref* buffer, so I'm unsure how will the
above help.

bug#59575: 29.0.50; add-log-current-defun-header-regexp matches Windows drive letter

[Juanma Barranquero]

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

On Sun, Nov 27, 2022 at 2:54 AM Dmitry Gutov <dgutov@yandex.ru> wrote:
> It seems like
>
>    (setq add-log-current-defun-function nil)
>
> should suffice.

That does not stop add-log-current-defun for working, in absence of an
a-l-c-d-function it just uses the regexp, which is what is returning the
spurious name.

Either adding a custom add-log-current-defun-function, or setting
add-log-current-defun-header-regexp to a buffer-local value so it
recognizes filenames would work. (Buffer-locally, because the variable is
in fact customizable so we can't be sure what its global value will be.)

This works, for example:

(defun xref--add-log-current-defun ()
  (if-let (item (xref--item-at-point))
      (xref-file-location-file (xref-match-item-location item))
    (xref--imenu-extract-index-name)))

(setq-local add-log-current-defun-function #'xref--add-log-current-defun)

> Is that the intended behavior, to report the absence of the current defun?

The final intended behavior, IIUC, is that the filenames in the
xref--xref-buffer-mode get passed to which-function as "defun" names.

[-- Attachment #2: Type: text/html, Size: 2303 bytes --]

bug#59575: 29.0.50; add-log-current-defun-header-regexp matches Windows drive letter

[Juanma Barranquero]

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

On Sun, Nov 27, 2022 at 9:29 AM Juanma Barranquero <lekktu@gmail.com> wrote:
>
> This works, for example:
>
> (defun xref--add-log-current-defun ()
>   (if-let (item (xref--item-at-point))
>       (xref-file-location-file (xref-match-item-location item))
>     (xref--imenu-extract-index-name)))
>
> (setq-local add-log-current-defun-function #'xref--add-log-current-defun)

In fact, to respect the value of `xref-file-name-display' (which seems a
good idea) a bit more complexity is required:

(defun xref--add-log-current-defun ()
  "Return the string used to group a set of locations.
This function is used as a value for `add-log-current-defun-function'."
  (xref--group-name-for-display
   (if-let (item (xref--item-at-point))
       (xref-location-group (xref-match-item-location item))
     (xref--imenu-extract-index-name))
   (xref--project-root (project-current))))

but that uncovers a different bug in xref--group-name-for-display:

   (cl-ecase xref-file-name-display
     (abs group)
     (nondirectory
      (if (string-match-p "\\`~?/" group)
          (file-name-nondirectory group)
        group))

that is, for the 'nondirectory case it tries to match against ~/filename or
/filename, but (again) ignores absolute Windows paths with a drive letter.

That should be changed to

   (cl-ecase xref-file-name-display
     (abs group)
     (nondirectory
-     (if (string-match-p "\\`~?/" group)
+     (if (string-match-p "\\`\\(~\\|[A-Za-z]:\\)?/" group)
          (file-name-nondirectory group)
        group))
     (project-relative

or something similar.

[-- Attachment #2: Type: text/html, Size: 2505 bytes --]

bug#59575: 29.0.50; add-log-current-defun-header-regexp matches Windows drive letter

[Juanma Barranquero]

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

On Sun, Nov 27, 2022 at 12:11 PM Juanma Barranquero <lekktu@gmail.com>
wrote:


> That should be changed to
>
>    (cl-ecase xref-file-name-display
>      (abs group)
>      (nondirectory
> -     (if (string-match-p "\\`~?/" group)
> +     (if (string-match-p "\\`\\(~\\|[A-Za-z]:\\)?/" group)
>           (file-name-nondirectory group)
>         group))
>      (project-relative
>
> or something similar.
>

Silly me, that should just use `file-name-absolute-p'.

OK to fix this, regardless of what's decided for the original bug?

[-- Attachment #2: Type: text/html, Size: 1317 bytes --]

bug#59575: 29.0.50; add-log-current-defun-header-regexp matches Windows drive letter

[Dmitry Gutov]

On 27/11/22 08:49, Eli Zaretskii wrote:
>> Date: Sun, 27 Nov 2022 03:54:27 +0200
>> Cc:59575@debbugs.gnu.org
>> From: Dmitry Gutov<dgutov@yandex.ru>
>>
>> On 26/11/22 15:03, Eli Zaretskii wrote:
>>> It is also possible to have xref.el define its customized
>>> add-log-current-defun-function, but that sounds like overkill to me.
>>> Dmitry, WDYT?
>> It seems like
>>
>>     (setq add-log-current-defun-function nil)
>>
>> should suffice.
> The value is already nil in the*xref*  buffer, so I'm unsure how will the
> above help.

Sorry, brain fart. I meant

   (setq add-log-current-defun-function #'ignore)

bug#59575: 29.0.50; add-log-current-defun-header-regexp matches Windows drive letter

[Juanma Barranquero]

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

On Sun, Nov 27, 2022 at 1:33 PM Dmitry Gutov <dgutov@yandex.ru> wrote:

> Sorry, brain fart. I meant
>
>   (setq add-log-current-defun-function #'ignore)

That "works", in the sense that now which-func gets "n/a" in the xref
results buffer instead of a bogus name. But which-func can be useful in
xref result buffers.

[-- Attachment #2: Type: text/html, Size: 843 bytes --]

bug#59575: 29.0.50; add-log-current-defun-header-regexp matches Windows drive letter

[Dmitry Gutov]

On 27/11/22 13:22, Juanma Barranquero wrote:
> On Sun, Nov 27, 2022 at 12:11 PM Juanma Barranquero <lekktu@gmail.com 
> <mailto:lekktu@gmail.com>> wrote:
> 
>     That should be changed to
> 
>       (cl-ecase xref-file-name-display
>           (abs group)
>           (nondirectory
>     -     (if (string-match-p "\\`~?/" group)
>     +     (if (string-match-p "\\`\\(~\\|[A-Za-z]:\\)?/" group)
>                (file-name-nondirectory group)
>              group))
>           (project-relative
> 
>     or something similar.
> 
> 
> Silly me, that should just use `file-name-absolute-p'.
> 
> OK to fix this, regardless of what's decided for the original bug?

Yes, please. The original idea was to avoid costly operations (in case 
the list of matches is long), but looks like file-name-absolute-p 
doesn't hit the disk.

bug#59575: 29.0.50; add-log-current-defun-header-regexp matches Windows drive letter

[Dmitry Gutov]

On 27/11/22 10:29, Juanma Barranquero wrote:
>> Is that the intended behavior, to report the absence of the current defun?
> 
> The final intended behavior, IIUC, is that the filenames in the 
> xref--xref-buffer-mode get passed to which-function as "defun" names.

I don't mind this approach either, but given that the "definitions" in 
Xref buffers are often only 1 line long, or maybe a few, wouldn't 
showing the file name again somewhere nearby (e.g. the header bar) just 
be redundant?

bug#59575: 29.0.50; add-log-current-defun-header-regexp matches Windows drive letter

[Juanma Barranquero]

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

On Sun, Nov 27, 2022 at 2:05 PM Dmitry Gutov <dgutov@yandex.ru> wrote:

> I don't mind this approach either, but given that the "definitions" in
> Xref buffers are often only 1 line long, or maybe a few, wouldn't
> showing the file name again somewhere nearby (e.g. the header bar) just
> be redundant?

Sometimes you've got a lot of matches. Also, I tend to display the output
of xref (and, generally speaking, of all kinds of "locating"/"matching"
functions, like occur, etc) in small windows, usually less than ten lines
high. And anyway, this won't bother anyone who's not already using
which-func.

[-- Attachment #2: Type: text/html, Size: 1320 bytes --]

bug#59575: 29.0.50; add-log-current-defun-header-regexp matches Windows drive letter

[Juanma Barranquero]

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

On Sun, Nov 27, 2022 at 2:04 PM Dmitry Gutov <dgutov@yandex.ru> wrote:

> Yes, please. The original idea was to avoid costly operations (in case
> the list of matches is long), but looks like file-name-absolute-p
> doesn't hit the disk.

Done in commit 41d2365d58 of 2022-11-27:
Fix xref to correctly display Windows absolute filenames

[-- Attachment #2: Type: text/html, Size: 738 bytes --]

bug#59575: 29.0.50; add-log-current-defun-header-regexp matches Windows drive letter

[Dmitry Gutov]

On 27/11/22 15:18, Juanma Barranquero wrote:
> On Sun, Nov 27, 2022 at 2:05 PM Dmitry Gutov <dgutov@yandex.ru 
> <mailto:dgutov@yandex.ru>> wrote:
> 
>> I don't mind this approach either, but given that the "definitions" in
>> Xref buffers are often only 1 line long, or maybe a few, wouldn't
>> showing the file name again somewhere nearby (e.g. the header bar) just
>> be redundant?
> 
> Sometimes you've got a lot of matches. Also, I tend to display the 
> output of xref (and, generally speaking, of all kinds of 
> "locating"/"matching" functions, like occur, etc) in small windows, 
> usually less than ten lines high. And anyway, this won't bother anyone 
> who's not already using which-func.

Yeah, okay. Sounds good.

bug#59575: 29.0.50; add-log-current-defun-header-regexp matches Windows drive letter

[Juanma Barranquero]

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

Fixed in commit 31cfd6d311 of 2022-11-27
Fix xref interaction with which-func (bug#59575)

[-- Attachment #2: Type: text/html, Size: 188 bytes --]