bug#55778: 29.0.50; [PATCH] M-. into a .gz; we've all been there.

[Eli Zaretskii]

> From: dick.r.chiang@gmail.com
> Date: Fri, 03 Jun 2022 02:27:59 -0400
> 
> * lisp/emacs-lisp/find-func.el (find-function-regexp-alist):
> Was crashing on this missing entry in find-function-regexp-alist.
> (find-function-C-source-directory): Style.
> (find-function-C-source): Style.
> (find-function-search-for-symbol): English.
> * lisp/help-fns.el (help-C-file-name): English.
> (find-lisp-object-file-name): Use it.
> * lisp/help-mode.el (xref): Require.
> (help-function-def--button-function): Use it.
> (help-face-def): Use it.
> * lisp/progmodes/elisp-mode.el (xref-backend-definitions): Style.
> (elisp--xref-find-definitions): Style.
> (xref-location-marker): Use it.
> * lisp/progmodes/project.el (project-root): Snuff bytecomp warning.
> * lisp/progmodes/xref.el (xref-prefer-source-directory): New defcustom.
> (xref-preferred-message): Message user.
> (xref-preferred-source): Act on new defcustom.
> (xref-show-definitions-buffer): Style.
> (xref--create-fetcher): Style.
> * src/emacs.c (syms_of_emacs): English.
> * src/lread.c (load_path_default): English.
> (init_lread): Style.
> (syms_of_lread): New defvar.
> * test/lisp/progmodes/elisp-mode-tests.el (xref-tests-prefer-source):
> Test it.
> * test/lisp/progmodes/xref-tests.el
> (xref-matches-in-directory-finds-none-for-some-regexp): Style.
> (xref--buf-pairs-iterator-groups-markers-by-buffers-1): Style.
> (xref--buf-pairs-iterator-groups-markers-by-buffers-2): Style.
> (xref--buf-pairs-iterator-cleans-up-markers): Style.

Please separate the real changes from refactoring of existing code and
from documentation changes that are unrelated to code changes.  They
each should be reviewed and judged separately and with different
criteria in mind.

> @@ -257,7 +258,7 @@ find-library--from-load-history
>  
>  (defvar find-function-C-source-directory
>    (let ((dir (expand-file-name "src" source-directory)))
> -    (if (file-accessible-directory-p dir) dir))
> +    (when (file-accessible-directory-p dir) dir))
>    "Directory where the C source files of Emacs can be found.
>  If nil, do not try to find the source code of functions and variables
>  defined in C.")
> @@ -283,8 +284,8 @@ find-function-C-source
>                   (read-directory-name "Emacs C source dir: " nil nil t))))
>      (setq file (expand-file-name file dir))
>      (if (file-readable-p file)
> -        (if (null find-function-C-source-directory)
> -            (setq find-function-C-source-directory dir))
> +        (unless find-function-C-source-directory
> +          (setq find-function-C-source-directory dir))
>        (error "The C source file %s is not available"
>               (file-name-nondirectory file))))

This seems to be some effort to use 'when' and 'unless' instead of
'if'?  If so, please don't: I see no reason for such changes.

> +(defcustom xref-prefer-source-directory nil
> +  "If non-nil, jump to the file in `source-directory' that
> +corresponds to the target."

"if that exists", right?  And what does "jump" mean? who is it that
will "jump"?

Also, the first line of a doc string should be a complete sentence,
for the benefit of 'apropos' commands.

>    DEFVAR_LISP ("installation-directory", Vinstallation_directory,
> -            doc: /* A directory within which to look for the `lib-src' and 
> `etc' directories.
> -In an installed Emacs, this is normally nil.  It is non-nil if
> -both `lib-src' (on MS-DOS, `info') and `etc' directories are found
> -within the variable `invocation-directory' or its parent.  For example,
> -this is the case when running an uninstalled Emacs executable from its
> -build directory.  */);
> +            doc: /* Should have been named build-directory.
> +Counter-intuitively, this variable is nil when Emacs is invoked from
> +its `make install` executable.  It normally takes on the value of
> +`source-directory' when Emacs is invoked from its within-repo `make`
> +executable.  Its primary use is locating the lib-src and etc
> +subdirectories of the build.  Not to be confused with
> +`installed-directory'.  */);

Please keep your controversial opinions out of the Emacs sources, and
definitely out of the doc strings.  Your intuition is wrong here.

> -/* Return the default load-path, to be used if EMACSLOADPATH is unset.
> -   This does not include the standard site-lisp directories
> -   under the installation prefix (i.e., PATH_SITELOADSEARCH),
> -   but it does (unless no_site_lisp is set) include site-lisp
> -   directories in the source/build directories if those exist and we
> -   are running uninstalled.
> -
> -   Uses the following logic:
> -   If !will_dump: Use PATH_LOADSEARCH.
> -   The remainder is what happens when dumping is about to happen:
> -   If dumping, just use PATH_DUMPLOADSEARCH.
> -   Otherwise use PATH_LOADSEARCH.
> -
> -   If !initialized, then just return PATH_DUMPLOADSEARCH.
> -   If initialized:
> -   If Vinstallation_directory is not nil (ie, running uninstalled):
> -   If installation-dir/lisp exists and not already a member,
> -   we must be running uninstalled.  Reset the load-path
> -   to just installation-dir/lisp.  (The default PATH_LOADSEARCH
> -   refers to the eventual installation directories.  Since we
> -   are not yet installed, we should not use them, even if they exist.)
> -   If installation-dir/lisp does not exist, just add
> -   PATH_DUMPLOADSEARCH at the end instead.
> -   Add installation-dir/site-lisp (if !no_site_lisp, and exists
> -   and not already a member) at the front.
> -   If installation-dir != source-dir (ie running an uninstalled,
> -   out-of-tree build) AND install-dir/src/Makefile exists BUT
> -   install-dir/src/Makefile.in does NOT exist (this is a sanity
> -   check), then repeat the above steps for source-dir/lisp, site-lisp.  */
> +/* Dig toplevel LOAD-PATH out of epaths.h.  */

Fat chance we will ever accept such "changes".  These details might
bore you, but they are there for a reason: so that people who need to
make changes in this tricky code could be aware of all the pitfalls
that others in their good time fell into.

>  static Lisp_Object
>  load_path_default (void)
>  {
>    if (will_dump_p ())
> -    /* PATH_DUMPLOADSEARCH is the lisp dir in the source directory.
> -       We used to add ../lisp (ie the lisp dir in the build
> -       directory) at the front here, but that should not be
> -       necessary, since in out of tree builds lisp/ is empty, save
> -       for Makefile.  */
> +    /* PATH_DUMPLOADSEARCH is the lisp dir in the source directory.  */
>      return decode_env_path (0, PATH_DUMPLOADSEARCH, 0);

And this.

> -  Lisp_Object lpath = Qnil;
> -
> -  lpath = decode_env_path (0, PATH_LOADSEARCH, 0);
> +  Lisp_Object lpath = decode_env_path (0, PATH_LOADSEARCH, 0);
>  
> +  /* Counter-intuitively Vinstallation_directory is nil for
> +     invocations of the `make install` executable, and is
> +     Vsource_directory for invocations of the within-repo `make`
> +     executable.
> +  */

And this.  (The comment style is also not our style.)

> +  DEFVAR_LISP ("installed-directory", Vinstalled_directory,
> +            doc: /* Install path of built-in lisp libraries.
> +This directory contains the `etc`, `lisp`, and `site-lisp`
> +installables, and is determined at configure time in the epaths-force
> +make target.  Not to be confused with the legacy
> +`installation-directory' nor `invocation-directory'.  */);
> +  Vinstalled_directory
> +    = Fexpand_file_name (build_string ("../"),
> +                      Fcar (decode_env_path (0, PATH_LOADSEARCH, 0)));

This won't work reliably, because it assumes PATH_LOADSEARCH is
determined at build time.  That is only true for some builds, but not
for others.  It also disregards EMACSLOADPATH.  To be reliable, the
variable's value needs to be recalculated at startup, each time anew.