[PATCH] Fix `early-init-file' value when file is missing

classic Classic list List threaded Threaded
19 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Fix `early-init-file' value when file is missing

Radon Rosborough
Dear all,

In Emacs 27, when Emacs starts up without an early init-file (located
by default at ~/.emacs.d/early-init.el), the value of
`early-init-file' is set to ~/.emacs.d/early-init (note the missing
file extension). When the early init-file does exist, the variable is
set to the correct value. I have attached a patch which fixes this
bug.

Feedback is welcome. Please copy me on replies, since I am not
subscribed to emacs-devel.

Best regards,
Radon Rosborough


0001-Fix-early-init-file-value-when-file-is-missing.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix `early-init-file' value when file is missing

Radon Rosborough
> In Emacs 27, when Emacs starts up without an early init-file (located
> by default at ~/.emacs.d/early-init.el), the value of
> `early-init-file' is set to ~/.emacs.d/early-init (note the missing
> file extension). When the early init-file does exist, the variable is
> set to the correct value. I have attached a patch which fixes this
> bug.
>
> Feedback is welcome. Please copy me on replies, since I am not
> subscribed to emacs-devel.

I am following up on this message since it has been one week with no
reply, and I would like to see the patch either merged or discussed.

Thanks,
Radon


0001-Fix-early-init-file-value-when-file-is-missing.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix `early-init-file' value when file is missing

Radon Rosborough
> In Emacs 27, when Emacs starts up without an early init-file (located
> by default at ~/.emacs.d/early-init.el), the value of
> `early-init-file' is set to ~/.emacs.d/early-init (note the missing
> file extension). When the early init-file does exist, the variable is
> set to the correct value. I have attached a patch which fixes this
> bug.
>
> Feedback is welcome. Please copy me on replies, since I am not
> subscribed to emacs-devel.

Hi. I am following up on this again since it has been two weeks with
no reply. Could somebody please let me know that the message was
received?

Thanks,
Radon



0001-Fix-early-init-file-value-when-file-is-missing.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix `early-init-file' value when file is missing

Eli Zaretskii
> From: Radon Rosborough <[hidden email]>
> Date: Tue, 29 Jan 2019 09:24:07 -0800
>
> Could somebody please let me know that the message was received?

The message was received.

Thanks.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix `early-init-file' value when file is missing

Eli Zaretskii
In reply to this post by Radon Rosborough
> From: Radon Rosborough <[hidden email]>
> Date: Tue, 29 Jan 2019 09:24:07 -0800
>
> Hi. I am following up on this again since it has been two weeks with
> no reply. Could somebody please let me know that the message was
> received?

Sorry for the long delay.

This patch has the (possibly unintended) consequence that if ~/.emacs
doesn't exist, user-init-file is set to "~/.emacs.el", something I
don't think we want.  Can you propose a change that will affect only
early-init-file, not any other init file?

Thanks.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix `early-init-file' value when file is missing

Radon Rosborough
> From: Eli Zaretskii <[hidden email]>
> Date: Feb 1, 2019, 1:11 AM
>
> This patch has the (possibly unintended) consequence that if ~/.emacs
> doesn't exist, user-init-file is set to "~/.emacs.el", something I
> don't think we want. Can you propose a change that will affect only
> early-init-file, not any other init file?

Sure, no problem. I apologize for the oversight.

A revised patch which implements the same bugfix without changing the
value of `user-init-file' is attached.

Best,
Radon


0001-Fix-early-init-file-value-when-file-is-missing.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix `early-init-file' value when file is missing

Eli Zaretskii
> From: Radon Rosborough <[hidden email]>
> Date: Fri, 1 Feb 2019 15:10:24 -0800
> Cc: emacs-devel <[hidden email]>
>
> > This patch has the (possibly unintended) consequence that if ~/.emacs
> > doesn't exist, user-init-file is set to "~/.emacs.el", something I
> > don't think we want. Can you propose a change that will affect only
> > early-init-file, not any other init file?
>
> Sure, no problem. I apologize for the oversight.
>
> A revised patch which implements the same bugfix without changing the
> value of `user-init-file' is attached.
>
> Previously, if no early init-file existed in `user-emacs-directory',
> then the value of `early-init-file' after startup would be
> ~/.emacs.d/early-init (note the missing extension).  This commit
> adjusts that value to ~/.emacs.d/early-init.el as desired, while not
> changing other behavior.  Note that when the early init-file did
> exist, then the value of `early-init-file' after startup was already
> correct; this commit fixes a bug that occurred only when the file did
> not exist.
>
> lisp/startup.el (load-user-init-file): Update logic.

Thanks.  However, I would prefer to have this solved outside
load-user-init-file, if feasible.  This function is too central to
Emacs to make non-trivial changes there for such a minor problem's
sake.  Especially since we don't have a test suite for startup
functionalities.  I think the change outside of the function will also
be much simpler and thus easier to grasp.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix `early-init-file' value when file is missing

Radon Rosborough
> From: Eli Zaretskii <[hidden email]>
> Date: Feb 7, 2019, 11:32 PM
>
> However, I would prefer to have this solved outside
> load-user-init-file, if feasible. This function is too central to
> Emacs to make non-trivial changes there for such a minor problem's
> sake.

I am sorry, but I simply cannot see how this change is non-trivial.
There is exactly one logic change that this patch makes: namely, that
the extension of user-init-file is changed to .el, but only in the
case of the early init-file not being found. That change will affect
only one of the two invocations of this function which exist, in only
one case. Everything else in the patch is simply renaming variables
and updating the documentation.

I also cannot see how any other way of solving this bug could be as
clear and straightforward as fixing it directly by updating the code
that was broken.

Could you explain your reasoning?

Thanks,
Radon

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix `early-init-file' value when file is missing

Eli Zaretskii
> From: Radon Rosborough <[hidden email]>
> Date: Fri, 8 Feb 2019 09:34:08 -0800
> Cc: emacs-devel <[hidden email]>
>
> > However, I would prefer to have this solved outside
> > load-user-init-file, if feasible. This function is too central to
> > Emacs to make non-trivial changes there for such a minor problem's
> > sake.
>
> I am sorry, but I simply cannot see how this change is non-trivial.
> There is exactly one logic change that this patch makes: namely, that
> the extension of user-init-file is changed to .el, but only in the
> case of the early init-file not being found.

But to do that, you've modified the API of load-user-init-file,
changing the semantics of its second argument, which then caused the
code of the function to change to accommodate that.  This then
requires to go and check that these changes didn't affect the other
callers of that function in ways we don't want.

> I also cannot see how any other way of solving this bug could be as
> clear and straightforward as fixing it directly by updating the code
> that was broken.

All you want is to set a single variable to a specific value if the
file wasn't found, right?  How hard can it be to do that after
load-user-init-file returns and reports a failure?  Or do that inside
the function, but only if it processes early-init file specifically?

> Could you explain your reasoning?

I tried in the message you responded to, but you've elided all those
explanations.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix `early-init-file' value when file is missing

Radon Rosborough
> you've modified the API of load-user-init-file

But we have never released a version of Emacs that includes this
function -- and it's an internal function. There are only two callers
of this function that exist, and they are both touched by this patch.

But, okay, the only reason I changed the arguments was because I
thought it was an improvement. Would you accept a patch that fixed
this bug without changing the arguments?

> All you want is to set a single variable to a specific value if the
> file wasn't found, right?

Not quite -- load-user-init-file *already* sets the variable to a
specific value, and the value is wrong. This patch changes the
specific value from the wrong value to the correct value.

> How hard can it be to do that after load-user-init-file returns and
> reports a failure?

I do not know how to do this, because it doesn't seem to me that
load-user-init-file reports failures at all. Instead, it handles them
itself. Hence why changing load-user-init-file is required.

> Or do that inside the function, but only if it processes early-init
> file specifically?

I really thought that this was exactly what my patch did. There are
exactly two places where this internal function is called, and
checking whether the optional argument is provided is the way to tell
whether the early init-file is being processed.

---

In conclusion, if I remove all the changes in this patch except for
the '(setq user-init-file ...)' line, would you accept that?

Best,
Radon

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix `early-init-file' value when file is missing

Stefan Monnier
> But we have never released a version of Emacs that includes this
> function -- and it's an internal function.

Good point: could you put a "--" in its name to clarify this?


        Stefan


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix `early-init-file' value when file is missing

Eli Zaretskii
In reply to this post by Radon Rosborough
> From: Radon Rosborough <[hidden email]>
> Date: Sun, 10 Feb 2019 15:04:59 -0800
> Cc: emacs-devel <[hidden email]>
>
> > you've modified the API of load-user-init-file
>
> But we have never released a version of Emacs that includes this
> function -- and it's an internal function. There are only two callers
> of this function that exist, and they are both touched by this patch.

True, but that's not what bothered me.  What bothered me was that
changes in the interface and the implementation of the function might
adversely affect the other caller of this function in startup.el, and
verifying that no use case will become broken because of that is not a
trivial task.  For a significant improvement ion functionality, such
risks might be justified, but here we are talking about a minor
cleanup, so I'd prefer a safer change.

> > Or do that inside the function, but only if it processes early-init
> > file specifically?
>
> I really thought that this was exactly what my patch did. There are
> exactly two places where this internal function is called, and
> checking whether the optional argument is provided is the way to tell
> whether the early init-file is being processed.

Such a test is too indirect, and will cease to DTRT if we ever need to
pass a second function in the case of early-init or not pass it in the
other case.  I hope we can make a more direct test.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix `early-init-file' value when file is missing

Radon Rosborough
> I hope we can make a more direct test.

Okay, but as I mentioned in my previous message, I could amend this
patch to not change the signature of `load-user-init-file' (even
though the change has no logical impact whatsoever that could change
user-facing behavior). Would you accept the patch if it didn't change
the signature of `load-user-init-file'?

Thanks,
Radon

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix `early-init-file' value when file is missing

Eli Zaretskii
> From: Radon Rosborough <[hidden email]>
> Date: Mon, 11 Feb 2019 21:38:36 -0800
> Cc: emacs-devel <[hidden email]>
>
> > I hope we can make a more direct test.
>
> Okay, but as I mentioned in my previous message, I could amend this
> patch to not change the signature of `load-user-init-file' (even
> though the change has no logical impact whatsoever that could change
> user-facing behavior). Would you accept the patch if it didn't change
> the signature of `load-user-init-file'?

I think so, but it's hard to say without actually seeing the patch, or
at least hearing the idea which you intend to implement in a bit more
detail.

Let me just be clear: the most important aspect for me in this regard
is to be able to tell easily that the change affects only the
early-init file and cannot possibly affect the other caller, and also
that the change is obviously correct.  So if you can come up with such
a safe change, I will of course gladly accept it.

Thanks in advance, and sorry for causing you extra work.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix `early-init-file' value when file is missing

Radon Rosborough
> if you can come up with such a safe change, I will of course gladly
> accept it.

It is attached.

0001-Fix-early-init-file-value-when-file-is-missing.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix `early-init-file' value when file is missing

Eli Zaretskii
> From: Radon Rosborough <[hidden email]>
> Date: Tue, 12 Feb 2019 18:36:20 -0800
> Cc: emacs-devel <[hidden email]>
>
> > if you can come up with such a safe change, I will of course gladly
> > accept it.
>
> It is attached.

Thanks.  This still uses the indirect evidence of the second function
argument.

Maybe all of this will be much simpler if in this fragment:

  ;; Load the early init file, if found.
  (load-user-init-file
   (lambda ()
     (expand-file-name
      "early-init"
      (file-name-as-directory
       (concat "~" init-file-user "/.emacs.d")))))

we replace "early-init" with "early-init.el"?  Does that solve the
problem?  If so, do you see any downsides with such a change?  We
didn't advertise the support for "~/.emacs.d/early-init", with no
extension.

WDYT?

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix `early-init-file' value when file is missing

Radon Rosborough
> Does that solve the problem?

Yes.

> If so, do you see any downsides with such a change?

Yes -- it lacks consistency and is therefore confusing -- but arguing
about this is tiring. Let's just do it that way. It's better than
leaving things as they are now.

Okay?

Best,
Radon

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix `early-init-file' value when file is missing

Eli Zaretskii
> From: Radon Rosborough <[hidden email]>
> Date: Fri, 15 Feb 2019 16:47:27 -0800
> Cc: emacs-devel <[hidden email]>
>
> > Does that solve the problem?
>
> Yes.
>
> > If so, do you see any downsides with such a change?
>
> Yes -- it lacks consistency and is therefore confusing

I added a comment explaining why we do that, to hopefully alleviate at
least some of that confusion.

>  but arguing about this is tiring. Let's just do it that way. It's
> better than leaving things as they are now.
>
> Okay?

Done.  Thanks for the feedback, and for reporting the problem to begin
with.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix `early-init-file' value when file is missing

Radon Rosborough
> Done. Thanks for the feedback, and for reporting the problem to begin
> with.

Much appreciated.

Best,
Radon