GNU bug report logs - #74504
31.0.50; Wrong source directory in *Help*

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 74504 in the body.
You can then email your comments to 74504 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Eshel Yaron <me <at> eshelyaron.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 31.0.50; Wrong source directory in *Help*
Date: Sun, 24 Nov 2024 08:12:39 +0100

Hi,

I think that commit e807d62c leads to the following regression:

1. Build Emacs
2. Run src/emacs -Q
3. In the *scratch* buffer say M-x cd /tmp
   (/tmp here is just an arbitrary directory that is not the Emacs
   sources directory)
4. C-h f make-overlay
   Now the first line in the *Help* buffer says:
   "make-overlay is a primitive-function in ‘C source code’."
   So far so good.
5. Switch to the *Help* buffer and type s, this finds buffer.c
6. Switch back to the *Help* buffer and type g
   The first line in *Help* now says:
   "make-overlay is a primitive-function in ‘/tmp/src/buffer.c’."
                                             ^^^^^ <-- Wrong!
                            
Note that the reported file name starts with whatever default-directory
happens to be in the *Help* buffer, which is incorrect.  Before commit
e807d62c, we'd get a file name relative to source-directory, so just
src/buffer.c in this case.


Best,

Eshel

Message #8 received at 74504 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: 74504 <at> debbugs.gnu.org
Subject: Re: bug#74504: 31.0.50; Wrong source directory in *Help*
Date: Mon, 25 Nov 2024 18:26:12 -0500

> 1. Build Emacs
> 2. Run src/emacs -Q
> 3. In the *scratch* buffer say M-x cd /tmp
>    (/tmp here is just an arbitrary directory that is not the Emacs
>    sources directory)
> 4. C-h f make-overlay
>    Now the first line in the *Help* buffer says:
>    "make-overlay is a primitive-function in ‘C source code’."
>    So far so good.
> 5. Switch to the *Help* buffer and type s, this finds buffer.c
> 6. Switch back to the *Help* buffer and type g
>    The first line in *Help* now says:
>    "make-overlay is a primitive-function in ‘/tmp/src/buffer.c’."
>                                              ^^^^^ <-- Wrong!

Hmm... my code assumed the file was always absolute, but apparently
that's not always the case.

The patch below seems to give us back the previous behavior, but maybe
we should change some other part of the code so
`help-fns-short-filename` is not used at all for the C sources (since
it's specifically trying to give a name relative to `load-path`, which
is nonsensical for our C files).


        Stefan


diff --git a/lisp/help-fns.el b/lisp/help-fns.el
index c87c86bae84..266cf8eb4a9 100644
--- a/lisp/help-fns.el
+++ b/lisp/help-fns.el
@@ -1052,7 +1052,8 @@ help-fns--radix-trees
   "Cache of radix-tree representation of `load-path'.")
 
 (defun help-fns--filename (file)
-  (let ((f (abbreviate-file-name (expand-file-name file))))
+  (let ((f (abbreviate-file-name
+            (if (file-name-absolute-p file) (expand-file-name file) file))))
     (if (file-name-case-insensitive-p f) (downcase f) f)))
 
 (defun help-fns--radix-tree (dirs)

Message #11 received at 74504 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: me <at> eshelyaron.com, 74504 <at> debbugs.gnu.org
Subject: Re: bug#74504: 31.0.50; Wrong source directory in *Help*
Date: Sat, 07 Dec 2024 14:26:56 +0200

> Cc: 74504 <at> debbugs.gnu.org
> Date: Mon, 25 Nov 2024 18:26:12 -0500
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> > 1. Build Emacs
> > 2. Run src/emacs -Q
> > 3. In the *scratch* buffer say M-x cd /tmp
> >    (/tmp here is just an arbitrary directory that is not the Emacs
> >    sources directory)
> > 4. C-h f make-overlay
> >    Now the first line in the *Help* buffer says:
> >    "make-overlay is a primitive-function in ‘C source code’."
> >    So far so good.
> > 5. Switch to the *Help* buffer and type s, this finds buffer.c
> > 6. Switch back to the *Help* buffer and type g
> >    The first line in *Help* now says:
> >    "make-overlay is a primitive-function in ‘/tmp/src/buffer.c’."
> >                                              ^^^^^ <-- Wrong!
> 
> Hmm... my code assumed the file was always absolute, but apparently
> that's not always the case.
> 
> The patch below seems to give us back the previous behavior, but maybe
> we should change some other part of the code so
> `help-fns-short-filename` is not used at all for the C sources (since
> it's specifically trying to give a name relative to `load-path`, which
> is nonsensical for our C files).
> 
> 
>         Stefan
> 
> 
> diff --git a/lisp/help-fns.el b/lisp/help-fns.el
> index c87c86bae84..266cf8eb4a9 100644
> --- a/lisp/help-fns.el
> +++ b/lisp/help-fns.el
> @@ -1052,7 +1052,8 @@ help-fns--radix-trees
>    "Cache of radix-tree representation of `load-path'.")
>  
>  (defun help-fns--filename (file)
> -  (let ((f (abbreviate-file-name (expand-file-name file))))
> +  (let ((f (abbreviate-file-name
> +            (if (file-name-absolute-p file) (expand-file-name file) file))))
>      (if (file-name-case-insensitive-p f) (downcase f) f)))
>  
>  (defun help-fns--radix-tree (dirs)

Ping!  Can we make further progress with this bug?

Message #14 received at 74504 <at> debbugs.gnu.org (full text, mbox):

From: Eshel Yaron <me <at> eshelyaron.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 74504 <at> debbugs.gnu.org
Subject: Re: bug#74504: 31.0.50; Wrong source directory in *Help*
Date: Sun, 08 Dec 2024 08:35:34 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

>> Cc: 74504 <at> debbugs.gnu.org
>> Date: Mon, 25 Nov 2024 18:26:12 -0500
>> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>> 
>> > 1. Build Emacs
>> > 2. Run src/emacs -Q
>> > 3. In the *scratch* buffer say M-x cd /tmp
>> >    (/tmp here is just an arbitrary directory that is not the Emacs
>> >    sources directory)
>> > 4. C-h f make-overlay
>> >    Now the first line in the *Help* buffer says:
>> >    "make-overlay is a primitive-function in ‘C source code’."
>> >    So far so good.
>> > 5. Switch to the *Help* buffer and type s, this finds buffer.c
>> > 6. Switch back to the *Help* buffer and type g
>> >    The first line in *Help* now says:
>> >    "make-overlay is a primitive-function in ‘/tmp/src/buffer.c’."
>> >                                              ^^^^^ <-- Wrong!
>> 
>> Hmm... my code assumed the file was always absolute, but apparently
>> that's not always the case.
>> 
>> The patch below seems to give us back the previous behavior, but maybe
>> we should change some other part of the code so
>> `help-fns-short-filename` is not used at all for the C sources (since
>> it's specifically trying to give a name relative to `load-path`, which
>> is nonsensical for our C files).
>> 
>> 
>>         Stefan
>> 
>> 
>> diff --git a/lisp/help-fns.el b/lisp/help-fns.el
>> index c87c86bae84..266cf8eb4a9 100644
>> --- a/lisp/help-fns.el
>> +++ b/lisp/help-fns.el
>> @@ -1052,7 +1052,8 @@ help-fns--radix-trees
>>    "Cache of radix-tree representation of `load-path'.")
>>  
>>  (defun help-fns--filename (file)
>> -  (let ((f (abbreviate-file-name (expand-file-name file))))
>> +  (let ((f (abbreviate-file-name
>> +            (if (file-name-absolute-p file) (expand-file-name file) file))))
>>      (if (file-name-case-insensitive-p f) (downcase f) f)))
>>  
>>  (defun help-fns--radix-tree (dirs)
>
> Ping!  Can we make further progress with this bug?

I think that the workaround Stefan suggests makes sense.  It doesn't
make the code (and the implicit assumptions about when a file name is
absolute and when it isn't) any clearer though.  Another option may be
something like the following, where we explicitly look only for .el
files under load-path:

diff --git a/lisp/help-fns.el b/lisp/help-fns.el
index c87c86bae84..b57201c6100 100644
--- a/lisp/help-fns.el
+++ b/lisp/help-fns.el
@@ -1064,22 +1064,14 @@ help-fns--radix-tree
       rt)))
 
 (defun help-fns-short-filename (filename)
-  (let* ((short (help-fns--filename filename))
-         (prefixes (radix-tree-prefixes (help-fns--radix-tree load-path)
-                                        (file-name-directory short))))
-    (if (not prefixes)
-        ;; The file is not inside the `load-path'.
-        ;; FIXME: Here's the old code (too slow, bug#73766),
-        ;; which used to try and shorten it with "../" as well.
-        ;; (dolist (dir load-path)
-        ;;   (let ((rel (file-relative-name filename dir)))
-        ;;     (if (< (length rel) (length short))
-        ;;         (setq short rel)))
-        ;;   (let ((rel (file-relative-name abbrev dir)))
-        ;;     (if (< (length rel) (length short))
-        ;;         (setq short rel))))
-        short
-      (file-relative-name short (caar prefixes)))))
+  (cond
+   ((eq filename 'C-source) "C source code")
+   ((equal (file-name-extension filename) "el")
+    (let* ((short (help-fns--filename filename))
+           (prefixes (radix-tree-prefixes (help-fns--radix-tree load-path)
+                                          (file-name-directory short))))
+      (if prefixes (file-relative-name short (caar prefixes)) short)))
+   (t filename)))
 
 (defun help-fns--analyze-function (function)
   ;; FIXME: Document/explain the differences between FUNCTION,
@@ -1203,10 +1195,7 @@ help-fns-function-description-header
             (setq help-mode--current-data (list :symbol function)))
 	;; We used to add .el to the file name,
 	;; but that's completely wrong when the user used load-file.
-	(princ (format-message " in `%s'"
-                               (if (eq file-name 'C-source)
-                                   "C source code"
-                                 (help-fns-short-filename file-name))))
+	(princ (format-message " in `%s'" (help-fns-short-filename file-name)))
 	;; Make a hyperlink to the library.
 	(with-current-buffer standard-output
           (setq help-mode--current-data (list :symbol function
@@ -1399,9 +1388,7 @@ describe-variable
 		         (progn
 		           (princ (format-message
                                    " is a variable defined in `%s'.\n\n"
-                                   (if (eq file-name 'C-source)
-                                       "C source code"
-                                     (help-fns-short-filename file-name))))
+                                   (help-fns-short-filename file-name)))
 		           (with-current-buffer standard-output
                              (setq help-mode--current-data
                                    (list :symbol variable
@@ -2146,9 +2133,7 @@ describe-keymap
                   (princ ".\n\n"))
               (princ (format-message
                       " defined in `%s'.\n\n"
-                      (if (eq file-name 'C-source)
-                          "C source code"
-                        (help-fns-short-filename file-name))))
+                      (help-fns-short-filename file-name)))
               (save-excursion
                 (re-search-backward (substitute-command-keys "`\\([^`']+\\)'"))
                 (setq help-mode--current-data (list :symbol keymap

Message #17 received at 74504 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 74504 <at> debbugs.gnu.org
Subject: Re: bug#74504: 31.0.50; Wrong source directory in *Help*
Date: Fri, 13 Dec 2024 17:51:31 -0500

> I think that the workaround Stefan suggests makes sense.

Pushed a slightly tweaked patch to `master`.

> It doesn't make the code (and the implicit assumptions about when
> a file name is absolute and when it isn't) any clearer though.
> Another option may be something like the following, where we
> explicitly look only for .el files under load-path:

AFAICT the other parts of the code treat specially the C files rather
than the ELisp files.  Not sure if it matters (e.g. for `.so` Emacs
modules?), but I'd rather try and use the same hack here as elsewhere.

FWIW, the other place I find this distinction is
`find-function-search-for-symbol` where we do:

    (if (string-match "\\`src/\\(.*\\.\\(c\\|m\\)\\)\\'" library)
        (find-function-C-source symbol (match-string 1 library) type)

And the `src/*.c` strings come from `help-C-file-name`.

We should label those strings better in order to know more reliably
whether they're supposed to be searched in `load-path` or in
`find-function-S-source-directory` or god knows where else.

But in the mean time, this specific bug is fixed.


        Stefan

Message #20 received at 74504 <at> debbugs.gnu.org (full text, mbox):

From: Eshel Yaron <me <at> eshelyaron.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 74504 <at> debbugs.gnu.org
Subject: Re: bug#74504: 31.0.50; Wrong source directory in *Help*
Date: Sat, 14 Dec 2024 12:48:38 +0100

close 74504 31.1
quit

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

>> I think that the workaround Stefan suggests makes sense.
>
> Pushed a slightly tweaked patch to `master`.
>
>> It doesn't make the code (and the implicit assumptions about when
>> a file name is absolute and when it isn't) any clearer though.
>> Another option may be something like the following, where we
>> explicitly look only for .el files under load-path:
>
> AFAICT the other parts of the code treat specially the C files rather
> than the ELisp files.  Not sure if it matters (e.g. for `.so` Emacs
> modules?), but I'd rather try and use the same hack here as elsewhere.
>
> FWIW, the other place I find this distinction is
> `find-function-search-for-symbol` where we do:
>
>     (if (string-match "\\`src/\\(.*\\.\\(c\\|m\\)\\)\\'" library)
>         (find-function-C-source symbol (match-string 1 library) type)
>
> And the `src/*.c` strings come from `help-C-file-name`.
>
> We should label those strings better in order to know more reliably
> whether they're supposed to be searched in `load-path` or in
> `find-function-S-source-directory` or god knows where else.
>
> But in the mean time, this specific bug is fixed.

Confirmed, closing.  Thank you!

Eshel

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.