bug#64071: 28.2; smerge-diff creates *vc-diff* without setting it up

bug#64071: 28.2; smerge-diff creates *vc-diff* without setting it up

[Spencer Baugh]

1. emacs -Q
2. Create a buffer containing:
<<<<<<< left
foo
=======
bar
>>>>>>> right
3. M-x smerge-mode
4. M-x smerge-diff-upper-lower
5. The resulting *vc-diff* buffer is in diff-mode but it's not
read-only, which is unusual for a *vc-diff* buffer.  Probably there is
also other setup missing.

Relatedly, if a *vc-diff* buffer already exists, smerge-mode will use it
without updating the mode-line, so it will say, for example, "Diff from
*vc-change-log*" despite the diff being from smerge conflict resolution.

This bug is also present in Emacs 29 and trunk.

The below diff resolves the read-only part.  But maybe we want to be
creating the *vc-diff* by calling into vc, which would also fix the
mode-line part?

diff --git a/lisp/vc/smerge-mode.el b/lisp/vc/smerge-mode.el
index c39a9cc2f22..65d93c17a64 100644
--- a/lisp/vc/smerge-mode.el
+++ b/lisp/vc/smerge-mode.el
@@ -1259,6 +1259,7 @@ smerge-diff
 	      (if (eq status 0) (insert "No differences found.\n"))))
 	  (goto-char (point-min))
 	  (diff-mode)
+          (read-only-mode)
 	  (display-buffer (current-buffer) t))
       (delete-file file1)
       (delete-file file2))))


In GNU Emacs 28.2 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo version 1.15.12, Xaw scroll bars)
 of 2023-02-09 built on 
Repository revision: 739b5d0e52d83ec567bd61a5a49ac0e93e0eb469
Repository branch: HEAD
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: CentOS Linux 7 (Core)

Configured using:
 'configure --with-x-toolkit=lucid --without-gpm --without-gconf
 --without-selinux --without-imagemagick --with-modules --with-gif=no
 --with-cairo --with-rsvg'


Configured features:
CAIRO DBUS FREETYPE GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON LIBXML2
MODULES NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND THREADS TIFF
TOOLKIT_SCROLL_BARS X11 XDBE XIM XPM LUCID ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Text

Minor modes in effect:
  smerge-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny dired dired-loaddefs
rfc822 mml mml-sec epa derived epg rfc6068 epg-config gnus-util rmail
rmail-loaddefs auth-source cl-seq eieio eieio-core cl-macs
eieio-loaddefs password-cache json map text-property-search time-date
subr-x seq byte-opt gv bytecomp byte-compile cconv mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader cl-loaddefs
cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils
smerge-mode diff-mode easy-mmode diff iso-transl tooltip eldoc paren
electric uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel
term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer cl-generic cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite emoji-zwj charscript charprop case-table
epa-hook jka-cmpr-hook help simple abbrev obarray cl-preloaded nadvice
button loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote threads dbusbind inotify
dynamic-setting system-font-setting font-render-setting cairo x-toolkit
x multi-tty make-network-process emacs)

Memory information:
((conses 16 80621 6370)
 (symbols 48 11294 1)
 (strings 32 28957 2105)
 (string-bytes 1 956297)
 (vectors 16 13522)
 (vector-slots 8 187949 10390)
 (floats 8 30 39)
 (intervals 56 256 0)
 (buffers 992 12)
 (heap 1024 13825 2912))

bug#64071: 28.2; smerge-diff creates *vc-diff* without setting it up

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Date: Wed, 14 Jun 2023 17:41:39 -0400
> 
> 1. emacs -Q
> 2. Create a buffer containing:
> <<<<<<< left
> foo
> =======
> bar
> >>>>>>> right
> 3. M-x smerge-mode
> 4. M-x smerge-diff-upper-lower
> 5. The resulting *vc-diff* buffer is in diff-mode but it's not
> read-only, which is unusual for a *vc-diff* buffer.

That command enters diff-mode, and diff-mode doesn't force read-only
status on the current buffer.  Why should Smerge force that?

> Probably there is also other setup missing.

Which ones, and to what mode they belong?

> Relatedly, if a *vc-diff* buffer already exists, smerge-mode will use it
> without updating the mode-line, so it will say, for example, "Diff from
> *vc-change-log*" despite the diff being from smerge conflict resolution.

This should probably be easy to fix, and the fix should be safe enough
for the emacs-29 branch.  How about proposing such a fix, and leaving
the more controversial aspects of this alone for now?

Thanks.

bug#64071: 28.2; smerge-diff creates *vc-diff* without setting it up

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Date: Wed, 14 Jun 2023 17:41:39 -0400
>> 
>> 1. emacs -Q
>> 2. Create a buffer containing:
>> <<<<<<< left
>> foo
>> =======
>> bar
>> >>>>>>> right
>> 3. M-x smerge-mode
>> 4. M-x smerge-diff-upper-lower
>> 5. The resulting *vc-diff* buffer is in diff-mode but it's not
>> read-only, which is unusual for a *vc-diff* buffer.
>
> That command enters diff-mode, and diff-mode doesn't force read-only
> status on the current buffer.  Why should Smerge force that?

Buffer named "*vc-diff*" are universally read-only, except if they are
created through this path.  (Because they're otherwise only created by
vc, which marks them read-only.)  It breaks user expectations for a
*vc-diff* buffer to not be read-only.

>> Probably there is also other setup missing.
>
> Which ones, and to what mode they belong?

Setup done by vc when it creates *vc-diff* buffers.

After doing a little investigation, it looks like the read-only setting
is the most important divergence after all.

Maybe we should be calling diff-setup-whitespace, and maybe we should be
running vc-diff-finish-functions.  Possibly not though.

>> Relatedly, if a *vc-diff* buffer already exists, smerge-mode will use it
>> without updating the mode-line, so it will say, for example, "Diff from
>> *vc-change-log*" despite the diff being from smerge conflict resolution.
>
> This should probably be easy to fix, and the fix should be safe enough
> for the emacs-29 branch.  How about proposing such a fix, and leaving
> the more controversial aspects of this alone for now?

Hm, honestly I only care about the read-only-mode, that's by far most
confusing thing about this.  I've gotten user complaints about that
specifically being confusing, so that's what I'd like to fix.  Stefan
likes my simple patch, so what's wrong with it?

bug#64071: 28.2; smerge-diff creates *vc-diff* without setting it up

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  64071@debbugs.gnu.org
> Date: Tue, 27 Jun 2023 16:17:45 -0400
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > That command enters diff-mode, and diff-mode doesn't force read-only
> > status on the current buffer.  Why should Smerge force that?
> 
> Buffer named "*vc-diff*" are universally read-only, except if they are
> created through this path.

But users can legitimately make a *vc-diff* buffer modifiable, no?
And your patch silently makes it read-only again, behind the user's
back.  That's hardly nice, is it?

> Stefan likes my simple patch, so what's wrong with it?

See above: we shouldn't change the read-only attribute that was set by
the user.

bug#64071: 28.2; smerge-diff creates *vc-diff* without setting it up

[Spencer Baugh]

[-- Attachment #1: Type: text/plain, Size: 850 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  64071@debbugs.gnu.org
>> Date: Tue, 27 Jun 2023 16:17:45 -0400
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > That command enters diff-mode, and diff-mode doesn't force read-only
>> > status on the current buffer.  Why should Smerge force that?
>> 
>> Buffer named "*vc-diff*" are universally read-only, except if they are
>> created through this path.
>
> But users can legitimately make a *vc-diff* buffer modifiable, no?
> And your patch silently makes it read-only again, behind the user's
> back.  That's hardly nice, is it?
>
>> Stefan likes my simple patch, so what's wrong with it?
>
> See above: we shouldn't change the read-only attribute that was set by
> the user.

Good point.  How about this?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-newly-created-smerge-diff-buffers-read-only.patch --]
[-- Type: text/x-patch, Size: 1374 bytes --]

From 4de911b6ed34ee1a728e1c57a65e9314f3785dc4 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Wed, 28 Jun 2023 08:48:01 -0400
Subject: [PATCH] Make newly-created smerge-diff-buffers read-only

Buffers name *vc-diff* are usually created by vc, which makes them
read-only.  If we create such a buffer, let's make it read-only too.
If the buffer already exists, though, don't change that since the user
might have deliberately made it writable.

* lisp/vc/smerge-mode.el (smerge-diff): Make newly-created
smerge-diff-buffers read-only. (bug#64071)
---
 lisp/vc/smerge-mode.el | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lisp/vc/smerge-mode.el b/lisp/vc/smerge-mode.el
index c39a9cc2f22..524a042fb55 100644
--- a/lisp/vc/smerge-mode.el
+++ b/lisp/vc/smerge-mode.el
@@ -1243,7 +1243,11 @@ smerge-diff
     (write-region beg1 end1 file1 nil 'nomessage)
     (write-region beg2 end2 file2 nil 'nomessage)
     (unwind-protect
-	(with-current-buffer (get-buffer-create smerge-diff-buffer-name)
+	(save-current-buffer
+          (if-let (buffer (get-buffer smerge-diff-buffer-name))
+              (set-buffer buffer)
+            (set-buffer (get-buffer-create smerge-diff-buffer-name))
+            (setq buffer-read-only t))
 	  (setq default-directory dir)
 	  (let ((inhibit-read-only t))
 	    (erase-buffer)
-- 
2.39.3

bug#64071: 28.2; smerge-diff creates *vc-diff* without setting it up

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: monnier@iro.umontreal.ca,  64071@debbugs.gnu.org
> Date: Wed, 28 Jun 2023 08:48:38 -0400
> 
> > See above: we shouldn't change the read-only attribute that was set by
> > the user.
> 
> Good point.  How about this?

Better, thanks.

bug#64071: 28.2; smerge-diff creates *vc-diff* without setting it up

[sbaugh]

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Cc: monnier@iro.umontreal.ca,  64071@debbugs.gnu.org
>> Date: Wed, 28 Jun 2023 08:48:38 -0400
>> 
>> > See above: we shouldn't change the read-only attribute that was set by
>> > the user.
>> 
>> Good point.  How about this?
>
> Better, thanks.

Is this patch OK to apply, then?

bug#64071: 28.2; smerge-diff creates *vc-diff* without setting it up

[Eli Zaretskii]

> From: sbaugh@catern.com
> Date: Sat, 19 Aug 2023 12:32:01 +0000 (UTC)
> Cc: Spencer Baugh <sbaugh@janestreet.com>, monnier@iro.umontreal.ca,
> 	64071@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> >> From: Spencer Baugh <sbaugh@janestreet.com>
> >> Cc: monnier@iro.umontreal.ca,  64071@debbugs.gnu.org
> >> Date: Wed, 28 Jun 2023 08:48:38 -0400
> >> 
> >> > See above: we shouldn't change the read-only attribute that was set by
> >> > the user.
> >> 
> >> Good point.  How about this?
> >
> > Better, thanks.
> 
> Is this patch OK to apply, then?

Yes, I think so.

bug#64071: 28.2; smerge-diff creates *vc-diff* without setting it up

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:
>> From: sbaugh@catern.com
>> Date: Sat, 19 Aug 2023 12:32:01 +0000 (UTC)
>> Cc: Spencer Baugh <sbaugh@janestreet.com>, monnier@iro.umontreal.ca,
>> 	64071@debbugs.gnu.org
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> >> From: Spencer Baugh <sbaugh@janestreet.com>
>> >> Cc: monnier@iro.umontreal.ca,  64071@debbugs.gnu.org
>> >> Date: Wed, 28 Jun 2023 08:48:38 -0400
>> >> 
>> >> > See above: we shouldn't change the read-only attribute that was set by
>> >> > the user.
>> >> 
>> >> Good point.  How about this?
>> >
>> > Better, thanks.
>> 
>> Is this patch OK to apply, then?
>
> Yes, I think so.

Could it be applied, then?

bug#64071: 28.2; smerge-diff creates *vc-diff* without setting it up

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: sbaugh@catern.com,  monnier@iro.umontreal.ca,  64071@debbugs.gnu.org
> Date: Wed, 04 Oct 2023 10:05:26 -0400
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> >> From: sbaugh@catern.com
> >> Date: Sat, 19 Aug 2023 12:32:01 +0000 (UTC)
> >> Cc: Spencer Baugh <sbaugh@janestreet.com>, monnier@iro.umontreal.ca,
> >> 	64071@debbugs.gnu.org
> >> 
> >> Eli Zaretskii <eliz@gnu.org> writes:
> >> >> From: Spencer Baugh <sbaugh@janestreet.com>
> >> >> Cc: monnier@iro.umontreal.ca,  64071@debbugs.gnu.org
> >> >> Date: Wed, 28 Jun 2023 08:48:38 -0400
> >> >> 
> >> >> > See above: we shouldn't change the read-only attribute that was set by
> >> >> > the user.
> >> >> 
> >> >> Good point.  How about this?
> >> >
> >> > Better, thanks.
> >> 
> >> Is this patch OK to apply, then?
> >
> > Yes, I think so.
> 
> Could it be applied, then?

Installed on master, and closing the bug.

bug#64071: 28.2; smerge-diff creates *vc-diff* without setting it up

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 1. emacs -Q
> 2. Create a buffer containing:
> <<<<<<< left
> foo
> =======
> bar
>>>>>>>> right
> 3. M-x smerge-mode
> 4. M-x smerge-diff-upper-lower
> 5. The resulting *vc-diff* buffer is in diff-mode but it's not
> read-only, which is unusual for a *vc-diff* buffer.  Probably there is
> also other setup missing.

Good catch, indeed.

> Relatedly, if a *vc-diff* buffer already exists, smerge-mode will use it
> without updating the mode-line, so it will say, for example, "Diff from
> *vc-change-log*" despite the diff being from smerge conflict resolution.

Hmm... that's also a bug, indeed.  I'm tempted to say that maybe the bug
is in the code which changed the `mode-line-format` (it should arguably
either not change it, or arrange to have it reset when it doesn't match
the buffer's contents any more).

> This bug is also present in Emacs 29 and trunk.
>
> The below diff resolves the read-only part.  But maybe we want to be
> creating the *vc-diff* by calling into vc, which would also fix the
> mode-line part?
>
> diff --git a/lisp/vc/smerge-mode.el b/lisp/vc/smerge-mode.el
> index c39a9cc2f22..65d93c17a64 100644
> --- a/lisp/vc/smerge-mode.el
> +++ b/lisp/vc/smerge-mode.el
> @@ -1259,6 +1259,7 @@ smerge-diff
>  	      (if (eq status 0) (insert "No differences found.\n"))))
>  	  (goto-char (point-min))
>  	  (diff-mode)
> +          (read-only-mode)
>  	  (display-buffer (current-buffer) t))
>        (delete-file file1)
>        (delete-file file2))))

LGTM.


        Stefan