Re: [PATCH] Fixing package-initialize, adding early init file

[Stefan Monnier]

> Since it has been more than a month with no response, I am re-posting
> the patch which fixes the problems previously discussed [1] [2] [3]
> with `package-initialize' by adding a second (optional) init-file
> `early-init.el'. This patch includes some refactoring of the code in
> `startup.el' to provide for loading any number of init-files in an
> extensible way. I hope that the change can be included in Emacs 26.2
> or Emacs 27.

Sorry for the delay, here are my comments (I doubt it's appropriate for
Emacs-26.2 since I think 26.N should be kept exclusively for bug-fixes).

Once you fix the two minor problems I mention below, looks good to me,


        Stefan


> * lisp/startup.el (early-init-file): New variable, for the filename of
>   the early init file after it has been loaded.
>
> * lisp/startup.el (load-user-init-file): New function, used to
>   eliminate duplicate code in loading the early and regular init
>   files.
>
> * lisp/startup.el (command-line): Load the early init file using
>   `load-user-init-file'. Move the check for an invalid username to
>   just before that, and move the initialization of the package system
>   to just after. Load the regular init file using
>   `load-user-init-file'.

The format for commit messages wants to combine those as follows
(notice also that we use double-spaces after "."):

    * lisp/startup.el (early-init-file): New variable, for the filename of
    the early init file after it has been loaded.
    (load-user-init-file): New function, used to eliminate duplicate code
    in loading the early and regular init files.
    (command-line): Load the early init file using `load-user-init-file'.
    Move the check for an invalid username to just before that, and move
    the initialization of the package system to just after.
    Load the regular init file using `load-user-init-file'.

> * src/lread.c (Vuser_init_file): Note change in semantics due to its
>   usage while loading the early init file.
>
> * lisp/emacs-lisp/package.el (package--ensure-init-file): Remove
>   definition, usage, and documentation.
>
> * lisp/emacs-lisp/package.el (package--init-file-ensured): Remove
>   definition and usage.

Same here, please combine the two messages about lisp/emacs-lisp/package.el.

> * etc/NEWS: Document changes to startup and package.el.

No need to mention these changes to NEWS here.

> diff --git a/lisp/startup.el b/lisp/startup.el
> index 4575f1f94d..de85933983 100644
> --- a/lisp/startup.el
> +++ b/lisp/startup.el
> @@ -312,6 +312,15 @@ inhibit-startup-hooks
>  Currently this applies to: `emacs-startup-hook', `term-setup-hook',
>  and `window-setup-hook'.")
>  
> +(defvar early-init-file nil
> +  "File name, including directory, of user's early init file.
> +If the file loaded had extension `.elc', and the corresponding
> +source file exists, this variable contains the name of source
> +file, suitable for use by functions like `custom-save-all' which
> +edit the init file.  While Emacs loads and evaluates the init
> +file, value is the real name of the file, regardless of whether
> +or not it has the `.elc' extension.")

This duplicates the doc of user-init-file, basically.  I think it'd be
better to refer to user-init-file and just explains in which way
it's different.

Also, IIUC the above is not true: while loading the early init file,
`early-init-file` will not be bound to the file being loaded, instead it
will be `user-init-file` which will be bound to it (and I'd rather not
document that quirk).

> +(defun load-user-init-file
> +    (compute-filename &optional compute-alternate-filename load-defaults)

We usually use names like `filename-function` rather than
`compute-filename` (the function itself could be named
`compute-filename` since a function *does* something, but a variable
doesn't *do* it just contains a value, in this case a function value).

> +  "Load a user init-file.
> +COMPUTE-FILENAME is called with no arguments and should return
> +the name of the init-file to load. If this file cannot be loaded,
> +and COMPUTE-ALTERNATE-FILENAME is non-nil, then it is called with
> +no arguments and should return the name of an alternate init-file
> +to load. If LOAD-DEFAULTS is non-nil, then load default.el after
> +the init-file.
> +
> +This function sets `user-init-file' to the name of the loaded
> +init-file, or to a default value if loading is not possible."
> +  (let ((debug-on-error-from-init-file nil)
> +        (debug-on-error-should-be-set nil)
> +        (debug-on-error-initial
> +         (if (eq init-file-debug t)
> +             'startup
> +           init-file-debug))
> +        (orig-enable-multibyte (default-value 'enable-multibyte-characters)))
> +    (let ((debug-on-error debug-on-error-initial)
> +          ;; We create an anonymous function here so that we can call
> +          ;; it in different contexts depending on the value of
> +          ;; `debug-on-error'.
> +          (read-init-file
> +           (lambda ()
> +             (when init-file-user
> +               (let ((init-file-name (funcall compute-filename)))
> +
> +                 ;; If `user-init-file' is t, then `load' will store
> +                 ;; the name of the file that it loads into
> +                 ;; `user-init-file'.
> +                 (setq user-init-file t)
> +                 (load init-file-name 'noerror 'nomessage)
> +
> +                 (when (eq user-init-file t)
> +                   (let ((alt-file-name (funcall 
> compute-alternate-filename)))
> +                     (load alt-file-name 'noerror 'nomessage)

You could directly do

    (load (funcall compute-alternate-filename) 'noerror 'nomessage)

here.  But more significantly, compute-alternate-filename is an optional
arg yet you forget to check if it's nil!

> +              ;; If a previous init-file had an error, don't forget
> +              ;; about that.
> +              (unless init-file-had-error
> +                (setq init-file-had-error nil)))

There's a problem here, since this is a nop.