bug#43192: lisp/files.el; 6d10b607d0 introduced bug that breaks C-x C-c

bug#43192: lisp/files.el; 6d10b607d0 introduced bug that breaks C-x C-c

[Tom Gillespie]

Trying to run save-buffers-kill-terminal from a buffer that
has no buffer-file-name fails with stringp, nil if there is
more than one buffer with the same file name (which appends
<> to the buffer name) and one of those buffers is unsaved.

Here is the repro.
#+begin_src bash
emacs -Q --eval "\
(progn
  (setq auto-save-default nil)
  (ignore-errors (make-directory \"/tmp/a\"))
  (ignore-errors (make-directory \"/tmp/b\"))
  (with-current-buffer (find-file \"/tmp/a/file.ext\")
    (erase-buffer)
    (insert \"\n\")
    (save-buffer))
  (with-current-buffer (find-file \"/tmp/b/file.ext\")
    (erase-buffer)
    (save-buffer)
    (insert \"some text\n\"))
  (switch-to-buffer \"*scratch*\")
  (with-current-buffer \"*scratch*\"
    (save-buffers-kill-terminal)))"
#+end_src

If you add a second (save-buffer) after (insert \"some text\n\")
then emacs will exit as expected. The repro can also be run
in batch mode and will exit with code 255 instead of 0, and
could be added as a test to prevent this in the future.

This happens because the *scratch* buffer does not have a
buffer-file-name, and that branch of the or statement is reached
because I have two files with the same name in different folders open
and thus the buffer file name does not match the buffer name so it
goes and looks at scratch which has no buffer file name at all,
causing the stringp, nil error.

The offending lines from 6d10b607d094bfd29b9ce0c4baf469e3683c3ac6
#+begin_src diff
+                                   (string-match
+                                    (concat "\\<"
+                                            (regexp-quote
+                                             (file-name-nondirectory
+                                              buffer-file-name))
+                                            "<[0-9]+>\\'")
+                                    (buffer-name buffer)))
#+end_src

This is the second statement in a call to `or'. buffer-file-name is
not guaranteed to be non-nil because buffers like *scratch* and
*Messages* exist. In many workflows for emacsclient opening to scratch
and closing again from scratch are common.

I think that putting the string-match inside (if buffer-file-name ...)
should be sufficient to fix the issue, but I don't know what the other
branch should be, if anything. I worry that there may also be other
issues with incorrect assumptions about the relationship between
buffer-name and buffer-file-name when there is more than one file
with the same name open, but I have not checked carefully.

bug#43192: lisp/files.el; 6d10b607d0 introduced bug that breaks C-x C-c

[Lars Ingebrigtsen]

Tom Gillespie <tgbugs@gmail.com> writes:

> #+begin_src diff
> +                                   (string-match
> +                                    (concat "\\<"
> +                                            (regexp-quote
> +                                             (file-name-nondirectory
> +                                              buffer-file-name))
> +                                            "<[0-9]+>\\'")
> +                                    (buffer-name buffer)))
> #+end_src
>
> This is the second statement in a call to `or'. buffer-file-name is
> not guaranteed to be non-nil because buffers like *scratch* and
> *Messages* exist. In many workflows for emacsclient opening to scratch
> and closing again from scratch are common.

I think it's just a typo -- the code should be:

diff --git a/lisp/files.el b/lisp/files.el
index 3403e257a1..5f5902d0cb 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -5574,7 +5574,7 @@ save-some-buffers
                                     (concat "\\<"
                                             (regexp-quote
                                              (file-name-nondirectory
-                                              buffer-file-name))
+                                              (buffer-file-name buffer)))
                                             "<[^>]*>\\'")
                                     (buffer-name buffer)))
                                   ;; The buffer name is similar to the

I've now applied this to Emacs 28, and that fixes the test case in this
bug report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43192: lisp/files.el; 6d10b607d0 introduced bug that breaks C-x C-c

[Tom Gillespie]

Ah, I wondered if that might be the case, everything else was so
consistent, but I thought there might be some reason to use the
variable directly. Since it is not the case my other concerns don't
matter. Confirming fixed on my end. Thanks!
Tom

On Thu, Sep 3, 2020 at 7:59 PM Lars Ingebrigtsen <larsi@gnus.org> wrote:
>
> Tom Gillespie <tgbugs@gmail.com> writes:
>
> > #+begin_src diff
> > +                                   (string-match
> > +                                    (concat "\\<"
> > +                                            (regexp-quote
> > +                                             (file-name-nondirectory
> > +                                              buffer-file-name))
> > +                                            "<[0-9]+>\\'")
> > +                                    (buffer-name buffer)))
> > #+end_src
> >
> > This is the second statement in a call to `or'. buffer-file-name is
> > not guaranteed to be non-nil because buffers like *scratch* and
> > *Messages* exist. In many workflows for emacsclient opening to scratch
> > and closing again from scratch are common.
>
> I think it's just a typo -- the code should be:
>
> diff --git a/lisp/files.el b/lisp/files.el
> index 3403e257a1..5f5902d0cb 100644
> --- a/lisp/files.el
> +++ b/lisp/files.el
> @@ -5574,7 +5574,7 @@ save-some-buffers
>                                      (concat "\\<"
>                                              (regexp-quote
>                                               (file-name-nondirectory
> -                                              buffer-file-name))
> +                                              (buffer-file-name buffer)))
>                                              "<[^>]*>\\'")
>                                      (buffer-name buffer)))
>                                    ;; The buffer name is similar to the
>
> I've now applied this to Emacs 28, and that fixes the test case in this
> bug report.
>
> --
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no