bug#74504: 31.0.50; Wrong source directory in *Help*

bug#74504: 31.0.50; Wrong source directory in *Help*

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

bug#74504: 31.0.50; Wrong source directory in *Help*

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 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)

bug#74504: 31.0.50; Wrong source directory in *Help*

[Eli Zaretskii]

> Cc: 74504@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@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?

bug#74504: 31.0.50; Wrong source directory in *Help*

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 74504@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@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

bug#74504: 31.0.50; Wrong source directory in *Help*

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 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

bug#74504: 31.0.50; Wrong source directory in *Help*

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

close 74504 31.1
quit

Stefan Monnier <monnier@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