bug#60897: 29.0.60; vc-revert fails to revert buffer

bug#60897: 29.0.60; vc-revert fails to revert buffer

[Eric Gillespie]

1. git clone git://git.sv.gnu.org/emacs.git
2. emacs -Q emacs/BUGS
3. Make any change at all.  Save it.
4. C-x v =
5. C-x v u

Step 4 switches to the diff buffer, as it always has (good).

If you switch back to the file buffer and C-x v u there, the file
is reverted and with it the buffer.

But we're staying in the diff buffer:

Expected behavior:
File and buffer both reverted.

Actual behavior:
File is reverted but buffer is NOT.


bisect results:

4803fba487d41f0817feab48b5095ef4b4940ff6 is the first bad commit
commit 4803fba487d41f0817feab48b5095ef4b4940ff6
Author: Juri Linkov <juri@linkov.net>
Date:   Sun Aug 28 22:38:51 2022 +0300

    'C-x v v' on a diff buffer commits it as a patch (bug#52349)



In GNU Emacs 29.0.60 (build 4, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.16.0) of 2023-01-17 built on dogato
Repository revision: 56d69c2fc4782dc23bd79ddcbccfbae9b263ecac
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12003000
System Description: openSUSE Leap 15.4

Configured using:
 'configure -C --prefix=/home/epg/.local --disable-silent-rules
 --without-libsystemd --without-xft --without-m17n-flt
 --without-toolkit-scroll-bars --without-gpm --without-dbus
 --without-gsettings --without-selinux --without-compress-install
 --without-tree-sitter --with-x CC=gcc-11'

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

Important settings:
  value of $LC_CTYPE: en_US.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Fundamental

Minor modes in effect:
  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
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny rfc822
mml mml-sec password-cache epa derived epg rfc6068 epg-config gnus-util
text-property-search time-date subr-x mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils vc-hg vc-bzr vc-src
vc-sccs vc-svn vc-cvs vc-rcs log-view pcvs-util smerge-mode diff
whitespace vc vc-git diff-mode easy-mmode vc-dispatcher dired-aux
cl-loaddefs cl-lib dired dired-loaddefs rmc iso-transl tooltip cconv
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 nadvice seq simple cl-generic
indonesian philippine 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 abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads inotify dynamic-setting
font-render-setting cairo x-toolkit xinput2 x multi-tty
make-network-process emacs)

Memory information:
((conses 16 55463 8329)
 (symbols 48 6885 0)
 (strings 32 21222 2121)
 (string-bytes 1 592863)
 (vectors 16 13020)
 (vector-slots 8 183606 11153)
 (floats 8 27 40)
 (intervals 56 545 0)
 (buffers 976 17))

bug#60897: 29.0.60; vc-revert fails to revert buffer

[Juri Linkov]

> 2. emacs -Q emacs/BUGS
> 3. Make any change at all.  Save it.
> 4. C-x v =
> 5. C-x v u
>
> Step 4 switches to the diff buffer, as it always has (good).
>
> If you switch back to the file buffer and C-x v u there, the file
> is reverted and with it the buffer.
>
> But we're staying in the diff buffer:
>
> Expected behavior:
> File and buffer both reverted.
>
> Actual behavior:
> File is reverted but buffer is NOT.

The behavior of C-x v u in the diff buffer was improved in emacs-29
to revert all files mentioned in the diff.  In emacs-28 it reverted
only one file, but in emacs-29 when the diff buffer shows many files,
such as after C-x v D, all these files are reverted.

But as you noticed it doesn't refresh the buffer.  This is because
vc-deduce-fileset in the diff buffer now returns relative file names,
not absolute as vc-resynch-buffer expects.

Expanding relative file names to absolute is not possible in
diff-vc-deduce-fileset, because then C-x v v will fail in
vc-default-checkin-patch that expects relative file names.

So I think the right place to do this is before trying to compare
relative names with (string= buffer-file-name file):

```
diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
index e1a3eff448d..3f9c39f2aff 100644
--- a/lisp/vc/vc-dispatcher.el
+++ b/lisp/vc/vc-dispatcher.el
@@ -643,6 +643,8 @@ vc-resynch-buffers-in-directory
 
 (defun vc-resynch-buffer (file &optional keep noquery reset-vc-info)
   "If FILE is currently visited, resynch its buffer."
+  (unless (file-name-absolute-p file)
+    (setq file (expand-file-name file (vc-root-dir))))
   (if (string= buffer-file-name file)
       (vc-resynch-window file keep noquery reset-vc-info)
     (if (file-directory-p file)
```

bug#60897: 29.0.60; vc-revert fails to revert buffer

[Eric Gillespie]

Juri Linkov <juri@linkov.net> writes:

> The behavior of C-x v u in the diff buffer was improved in emacs-29
> to revert all files mentioned in the diff.  In emacs-28 it reverted
> only one file, but in emacs-29 when the diff buffer shows many files,
> such as after C-x v D, all these files are reverted.

Interesting, I hadn't noticed.  Actually, I just tried reverting
two files from the diff buffer in 28.2 and it worked.

I've been regretting not finding the time to report the vc bug I
ran across in emacs 28, but then when
4803fba487d41f0817feab48b5095ef4b4940ff6 showed up and fixed what
I HAD noticed, I stopped worrying about it.  I'm getting worried
again :)

Here's what I see:

1. edit two versioned files
2. mark them both in vc-dired
3. = to show the diff for the two marked files
4. C-x v u

In emacs 27 and 28, both files are reverted.

Now, do the same first 3 steps again but now:
4. C-x v v
5. write a log message
6. C-c C-c

In 23.1 and 27: commits, as it has since vc-dired was born in
23.1 (I have 23.1 handy and just confirmed I was not
misremembering).

In 28, you get this baffling prompt:

'Buffer *vc-diff* modified; save it?  (yes or no)'

Neither option commits :)

It seemed to go back to normal with commit
4803fba487d41f0817feab48b5095ef4b4940ff6 on my daily driver
openSUSE laptop, but it's not working on my home FreeBSD box!
I'll look into that more...

I can't seem to make that fail a second time.  But this was the
error message I got:

vc-do-command: Failed (status 128): git --no-pager apply --cached ../../../../tmp/git-patch11lenk

Which is a pretty strange path and which does not exist.  If you
add one more ../ (five total) you will indeed reach /tmp but that
would be a strange way to reference it.

Thanks!

bug#60897: 29.0.60; vc-revert fails to revert buffer

[Juri Linkov]

> Here's what I see:
>
> 1. edit two versioned files
> 2. mark them both in vc-dired
> 3. = to show the diff for the two marked files
> 4. C-x v u
>
> In emacs 27 and 28, both files are reverted.

In emacs 27 and 28 reverting was unpredictable
and depended on the parent buffer of *vc-diff*.
So for example, after typing = at the top of the vc-dir buffer
without marking files, C-x v u failed with such error:

  (file-missing "Cannot open load file" "No such file or directory" "vc-nil")

> It seemed to go back to normal with commit
> 4803fba487d41f0817feab48b5095ef4b4940ff6 on my daily driver
> openSUSE laptop, but it's not working on my home FreeBSD box!
> I'll look into that more...
>
> I can't seem to make that fail a second time.  But this was the
> error message I got:
>
> vc-do-command: Failed (status 128): git --no-pager apply --cached ../../../../tmp/git-patch11lenk

Did it pop up a buffer with an error output message?

> Which is a pretty strange path and which does not exist.  If you
> add one more ../ (five total) you will indeed reach /tmp but that
> would be a strange way to reference it.

Hmm, the error comes from (vc-git-command nil 0 patch-file "apply" "--cached")
where patch-file is created by (make-nearby-temp-file "git-patch").
I guess make-nearby-temp-file makes such a relative name that looks strange.

bug#60897: 29.0.60; vc-revert fails to revert buffer

[Juri Linkov]

> But as you noticed it doesn't refresh the buffer.  This is because
> vc-deduce-fileset in the diff buffer now returns relative file names,
> not absolute as vc-resynch-buffer expects.
>
> Expanding relative file names to absolute is not possible in
> diff-vc-deduce-fileset, because then C-x v v will fail in
> vc-default-checkin-patch that expects relative file names.
>
> So I think the right place to do this is before trying to compare
> relative names with (string= buffer-file-name file):
>
> diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
> @@ -643,6 +643,8 @@ vc-resynch-buffers-in-directory
>  
>  (defun vc-resynch-buffer (file &optional keep noquery reset-vc-info)
>    "If FILE is currently visited, resynch its buffer."
> +  (unless (file-name-absolute-p file)
> +    (setq file (expand-file-name file (vc-root-dir))))
>    (if (string= buffer-file-name file)
>        (vc-resynch-window file keep noquery reset-vc-info)
>      (if (file-directory-p file)

Eli, is this ok for the emacs-29 branch?  I'm unsure because
vc-resynch-buffer is used in too many places, so this change
is not localized to vc-revert.

bug#60897: 29.0.60; vc-revert fails to revert buffer

[Eli Zaretskii]

> Cc: 60897@debbugs.gnu.org
> From: Juri Linkov <juri@linkov.net>
> Date: Sun, 22 Jan 2023 19:17:21 +0200
> 
> > But as you noticed it doesn't refresh the buffer.  This is because
> > vc-deduce-fileset in the diff buffer now returns relative file names,
> > not absolute as vc-resynch-buffer expects.
> >
> > Expanding relative file names to absolute is not possible in
> > diff-vc-deduce-fileset, because then C-x v v will fail in
> > vc-default-checkin-patch that expects relative file names.
> >
> > So I think the right place to do this is before trying to compare
> > relative names with (string= buffer-file-name file):
> >
> > diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
> > @@ -643,6 +643,8 @@ vc-resynch-buffers-in-directory
> >  
> >  (defun vc-resynch-buffer (file &optional keep noquery reset-vc-info)
> >    "If FILE is currently visited, resynch its buffer."
> > +  (unless (file-name-absolute-p file)
> > +    (setq file (expand-file-name file (vc-root-dir))))
> >    (if (string= buffer-file-name file)
> >        (vc-resynch-window file keep noquery reset-vc-info)
> >      (if (file-directory-p file)
> 
> Eli, is this ok for the emacs-29 branch?  I'm unsure because
> vc-resynch-buffer is used in too many places, so this change
> is not localized to vc-revert.

You could use the expanded file name only for the purposes of the
string= call, no?  Then the effect of this change would be very local
and thus safer.

bug#60897: 29.0.60; vc-revert fails to revert buffer

[Juri Linkov]

>> >  (defun vc-resynch-buffer (file &optional keep noquery reset-vc-info)
>> >    "If FILE is currently visited, resynch its buffer."
>> > +  (unless (file-name-absolute-p file)
>> > +    (setq file (expand-file-name file (vc-root-dir))))
>> >    (if (string= buffer-file-name file)
>> >        (vc-resynch-window file keep noquery reset-vc-info)
>> >      (if (file-directory-p file)
>> 
>> Eli, is this ok for the emacs-29 branch?  I'm unsure because
>> vc-resynch-buffer is used in too many places, so this change
>> is not localized to vc-revert.
>
> You could use the expanded file name only for the purposes of the
> string= call, no?  Then the effect of this change would be very local
> and thus safer.

There is another string= call that fails inside vc-resynch-window
called from vc-resynch-buffer.

bug#60897: 29.0.60; vc-revert fails to revert buffer

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: epg@pretzelnet.org,  60897@debbugs.gnu.org
> Date: Mon, 23 Jan 2023 09:51:30 +0200
> 
> >> >  (defun vc-resynch-buffer (file &optional keep noquery reset-vc-info)
> >> >    "If FILE is currently visited, resynch its buffer."
> >> > +  (unless (file-name-absolute-p file)
> >> > +    (setq file (expand-file-name file (vc-root-dir))))
> >> >    (if (string= buffer-file-name file)
> >> >        (vc-resynch-window file keep noquery reset-vc-info)
> >> >      (if (file-directory-p file)
> >> 
> >> Eli, is this ok for the emacs-29 branch?  I'm unsure because
> >> vc-resynch-buffer is used in too many places, so this change
> >> is not localized to vc-revert.
> >
> > You could use the expanded file name only for the purposes of the
> > string= call, no?  Then the effect of this change would be very local
> > and thus safer.
> 
> There is another string= call that fails inside vc-resynch-window
> called from vc-resynch-buffer.

How about replacing string= in these two cases with something smarter,
which would expand-file-name?  Comparing file names as simple strings
is wrought with trouble anyway.

bug#60897: 29.0.60; vc-revert fails to revert buffer

[Juri Linkov]

>> >> >  (defun vc-resynch-buffer (file &optional keep noquery reset-vc-info)
>> >> >    "If FILE is currently visited, resynch its buffer."
>> >> > +  (unless (file-name-absolute-p file)
>> >> > +    (setq file (expand-file-name file (vc-root-dir))))
>> >> >    (if (string= buffer-file-name file)
>> >> >        (vc-resynch-window file keep noquery reset-vc-info)
>> >> >      (if (file-directory-p file)
>>
>> There is another string= call that fails inside vc-resynch-window
>> called from vc-resynch-buffer.
>
> How about replacing string= in these two cases with something smarter,
> which would expand-file-name?  Comparing file names as simple strings
> is wrought with trouble anyway.

This works as well:

diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
index e1a3eff448d..8a0cdef4726 100644
--- a/lisp/vc/vc-dispatcher.el
+++ b/lisp/vc/vc-dispatcher.el
@@ -608,7 +608,7 @@ vc-resynch-window
 difference between the buffer and the file is due to
 modifications by the dispatcher client code, rather than user
 editing!"
-  (and (string= buffer-file-name file)
+  (and (string-prefix-p file buffer-file-name)
        (if keep
 	   (when (file-exists-p file)
 	     (when reset-vc-info
@@ -643,7 +643,7 @@ vc-resynch-buffers-in-directory
 
 (defun vc-resynch-buffer (file &optional keep noquery reset-vc-info)
   "If FILE is currently visited, resynch its buffer."
-  (if (string= buffer-file-name file)
+  (if (string-prefix-p file buffer-file-name)
       (vc-resynch-window file keep noquery reset-vc-info)
     (if (file-directory-p file)
 	(vc-resynch-buffers-in-directory file keep noquery reset-vc-info)

bug#60897: 29.0.60; vc-revert fails to revert buffer

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: epg@pretzelnet.org,  60897@debbugs.gnu.org
> Date: Tue, 24 Jan 2023 19:46:51 +0200
> 
> > How about replacing string= in these two cases with something smarter,
> > which would expand-file-name?  Comparing file names as simple strings
> > is wrought with trouble anyway.
> 
> This works as well:
> 
> diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
> index e1a3eff448d..8a0cdef4726 100644
> --- a/lisp/vc/vc-dispatcher.el
> +++ b/lisp/vc/vc-dispatcher.el
> @@ -608,7 +608,7 @@ vc-resynch-window
>  difference between the buffer and the file is due to
>  modifications by the dispatcher client code, rather than user
>  editing!"
> -  (and (string= buffer-file-name file)
> +  (and (string-prefix-p file buffer-file-name)
>         (if keep
>  	   (when (file-exists-p file)
>  	     (when reset-vc-info
> @@ -643,7 +643,7 @@ vc-resynch-buffers-in-directory
>  
>  (defun vc-resynch-buffer (file &optional keep noquery reset-vc-info)
>    "If FILE is currently visited, resynch its buffer."
> -  (if (string= buffer-file-name file)
> +  (if (string-prefix-p file buffer-file-name)
>        (vc-resynch-window file keep noquery reset-vc-info)
>      (if (file-directory-p file)
>  	(vc-resynch-buffers-in-directory file keep noquery reset-vc-info)

I'm confused: I though the problem was that FILE was some relative
form of buffer-file-name, which is why you wanted to run it through
expand-file-name, to make it absolute.  But if this is the case,
i.e. you have file = "foo" and buffer-file-name = /path/to/foo, then
string-prefix-p will not do the job, right?  And even if it did, you
could have false positives.

Maybe I just don't understand what this if clause is trying to test?

bug#60897: 29.0.60; vc-revert fails to revert buffer

[Juri Linkov]

>>  (defun vc-resynch-buffer (file &optional keep noquery reset-vc-info)
>>    "If FILE is currently visited, resynch its buffer."
>> -  (if (string= buffer-file-name file)
>> +  (if (string-prefix-p file buffer-file-name)
>>        (vc-resynch-window file keep noquery reset-vc-info)
>>      (if (file-directory-p file)
>>  	(vc-resynch-buffers-in-directory file keep noquery reset-vc-info)
>
> I'm confused: I though the problem was that FILE was some relative
> form of buffer-file-name, which is why you wanted to run it through
> expand-file-name, to make it absolute.  But if this is the case,
> i.e. you have file = "foo" and buffer-file-name = /path/to/foo, then
> string-prefix-p will not do the job, right?  And even if it did, you
> could have false positives.
>
> Maybe I just don't understand what this if clause is trying to test?

I tried to avoid expand-file-name, but maybe it's unavoidable.
In this case I can't find a solution simpler than this:

diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
index e1a3eff448d..fd5f655a0f6 100644
--- a/lisp/vc/vc-dispatcher.el
+++ b/lisp/vc/vc-dispatcher.el
@@ -608,7 +608,10 @@ vc-resynch-window
 difference between the buffer and the file is due to
 modifications by the dispatcher client code, rather than user
 editing!"
-  (and (string= buffer-file-name file)
+  (and (string= buffer-file-name
+                (if (file-name-absolute-p file)
+                    file
+                  (expand-file-name file (vc-root-dir))))
        (if keep
 	   (when (file-exists-p file)
 	     (when reset-vc-info
@@ -643,7 +646,10 @@ vc-resynch-buffers-in-directory
 
 (defun vc-resynch-buffer (file &optional keep noquery reset-vc-info)
   "If FILE is currently visited, resynch its buffer."
-  (if (string= buffer-file-name file)
+  (if (string= buffer-file-name
+               (if (file-name-absolute-p file)
+                   file
+                 (expand-file-name file (vc-root-dir))))
       (vc-resynch-window file keep noquery reset-vc-info)
     (if (file-directory-p file)
 	(vc-resynch-buffers-in-directory file keep noquery reset-vc-info)

bug#60897: 29.0.60; vc-revert fails to revert buffer

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: epg@pretzelnet.org,  60897@debbugs.gnu.org
> Date: Tue, 24 Jan 2023 20:18:32 +0200
> 
> I tried to avoid expand-file-name, but maybe it's unavoidable.
> In this case I can't find a solution simpler than this:

This is fine by me, thanks.

bug#60897: 29.0.60; vc-revert fails to revert buffer

[Juri Linkov]

close 60897 29.0.60
thanks

>> I tried to avoid expand-file-name, but maybe it's unavoidable.
>> In this case I can't find a solution simpler than this:
>
> This is fine by me, thanks.

Thus fixed in emacs-29 and closed.