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" <bug-gnu-emacs@gnu.org>]

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