Re: master f2d2fe6fc8: server-execute: Initialize the *scratch* buffer

[Sean Whitton <spwhitton@spwhitton.name>]

Hello,

On Fri 06 May 2022 at 08:40am +03, Eli Zaretskii wrote:

>> From: Sean Whitton <spwhitton@spwhitton.name>
>> Cc: Robert Pluim <rpluim@gmail.com>, emacs-devel@gnu.org,
>>  55257-submitter@debbugs.gnu.org
>> Date: Thu, 05 May 2022 15:07:41 -0700
>>
>> -(eval-when-compile (require 'cl-lib))
>> +(eval-when-compile
>> +  (require 'cl-lib)
>> +  (require 'subr-x))
>
> Why did you need subr-x here?  AFAIR, doing this breaks bootstrap,
> which is why if-let is now in subr.el.

Ah, my mistake, I didn't know it had moved (though I'm going to get rid
of the if-let I think).

>> +(defun get-initial-buffer-create ()
>> +  "Return the \*scratch\* buffer, creating a new one if needed."
>> +  (if-let ((scratch (get-buffer "*scratch*")))
>> +      scratch
>> +    (prog1 (setq scratch (get-buffer-create "*scratch*"))
>> +      (with-current-buffer scratch
>> +        (when initial-scratch-message
>> +          (insert (substitute-command-keys initial-scratch-message))
>> +          (set-buffer-modified-p nil))
>> +        (funcall initial-major-mode)))))
>
> It's somewhat inelegant to explicitly test for the buffer's existence
> before you call get-buffer-create.  Is that only to avoid changing its
> contents?  If so, can't you test for that in some other way?

I had the same intuition at first, but I don't think there is another
way -- the code wants to touch the buffer at all only if it wasn't
already there.  And the code path where it already exists will be by far
the most commonly called, so it seems best to avoid calling
with-current-buffer if we don't have to.

>> +    return call0 (intern ("get-initial-buffer-create"));
>
> Instead of calling intern each time this function is called from C, it
> is better to define a symbol for it, usually named
> Qget_initial_buffer_create, and then call0 it directly.

Will do.

>>  /* The following function is a safe variant of Fother_buffer: It doesn't
>> @@ -1659,15 +1650,7 @@ other_buffer_safely (Lisp_Object buffer)
>>      if (candidate_buffer (buf, buffer))
>>        return buf;
>>
>> -  AUTO_STRING (scratch, "*scratch*");
>> -  buf = Fget_buffer (scratch);
>> -  if (NILP (buf))
>> -    {
>> -      buf = Fget_buffer_create (scratch, Qnil);
>> -      Fset_buffer_major_mode (buf);
>> -    }
>> -
>> -  return buf;
>> +  return call0 (intern ("get-initial-buffer-create"));
>
> get-initial-buffer-create shows the initial-scratch-message, something
> the C code you are replacing didn't do.  This is a change in behavior
> that should at least be documented, if not fixed.

This is deliberate -- to my mind I'm fixing the same bug as the one in
server.el.  other-buffer recreates *scratch* for the same sort of
reasons that 'emacsclient -nc' does.

Where were you thinking it should be documented?  The Emacs Lisp changes
section of NEWS?

> I also wonder whether we should use safe_call in these places.

Could you say more?

-- 
Sean Whitton