bug#68174: 30.0.50; Project and VC disagree about what repository we're in

bug#68174: 30.0.50; Project and VC disagree about what repository we're in

[Sean Whitton]

X-debbugs-cc: juri@linkov.net

Hello,

1. emacs -q
2. (setopt vc-deduce-backend-nonvc-modes t)
3. C-x C-f ~/src/dotfiles/.emacs.d/init.el RET
4. C-x p p ~/src/emacs/ RET o C-x v L
5. (project-root (project-current)) => "~/src/emacs/"
6. C-x v +
   - git pull is attempted in ~/src/dotfiles/.

You're in a *vc-change-log* for emacs.git, and (project-current) returns
the correct project.  But C-x v + operates in ~/src/dotfiles/.

-- 
Sean Whitton

bug#68174: 30.0.50; Project and VC disagree about what repository we're in

[Dmitry Gutov]

Hi!

On 31/12/2023 15:26, Sean Whitton wrote:
> X-debbugs-cc:juri@linkov.net
> 
> Hello,
> 
> 1. emacs -q
> 2. (setopt vc-deduce-backend-nonvc-modes t)
> 3. C-x C-f ~/src/dotfiles/.emacs.d/init.el RET
> 4. C-x p p ~/src/emacs/  RET o C-x v L
> 5. (project-root (project-current)) => "~/src/emacs/"
> 6. C-x v +
>     - git pull is attempted in ~/src/dotfiles/.
> 
> You're in a*vc-change-log*  for emacs.git, and (project-current) returns
> the correct project.  But C-x v + operates in ~/src/dotfiles/.

Thanks for the report.

I general, the problem looks unsolvable (or at least hard) because 
various VC-related buffers look back on the fileset that spawned them 
through the variable vc-parent-buffer. And that one is often just set to 
the buffer that was previous before the current one was created (in 
vc-setup-buffer).

This particular case though seems fixed with this patch, please test:

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 3cd835a9d6b..82051740b8f 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1074,6 +1074,7 @@ vc-expand-dirs

  (defvar vc-dir-backend)
  (defvar log-view-vc-backend)
+(defvar log-view-vc-fileset)
  (defvar log-edit-vc-backend)
  (defvar diff-vc-backend)
  (defvar diff-vc-revisions)
@@ -1155,6 +1156,8 @@ vc-deduce-fileset
  	      (vc-state buffer-file-name)
  	      (vc-checkout-model backend buffer-file-name))
  	(list backend (list buffer-file-name))))
+     ((derived-mode-p 'log-view-mode)
+      (list log-view-vc-backend log-view-vc-fileset))
       ((and (buffer-live-p vc-parent-buffer)
             ;; FIXME: Why this test?  --Stef
             (or (buffer-file-name vc-parent-buffer)

bug#68174: 30.0.50; Project and VC disagree about what repository we're in

[Sean Whitton]

Hello,

On Tue 02 Jan 2024 at 05:31am +02, Dmitry Gutov wrote:

> I general, the problem looks unsolvable (or at least hard) because various
> VC-related buffers look back on the fileset that spawned them through the
> variable vc-parent-buffer. And that one is often just set to the buffer that
> was previous before the current one was created (in vc-setup-buffer).

Hmm.  This would be unfortunate.

> This particular case though seems fixed with this patch, please test:
>
> diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
> index 3cd835a9d6b..82051740b8f 100644
> --- a/lisp/vc/vc.el
> +++ b/lisp/vc/vc.el
> @@ -1074,6 +1074,7 @@ vc-expand-dirs
>
>  (defvar vc-dir-backend)
>  (defvar log-view-vc-backend)
> +(defvar log-view-vc-fileset)
>  (defvar log-edit-vc-backend)
>  (defvar diff-vc-backend)
>  (defvar diff-vc-revisions)
> @@ -1155,6 +1156,8 @@ vc-deduce-fileset
>  	      (vc-state buffer-file-name)
>  	      (vc-checkout-model backend buffer-file-name))
>  	(list backend (list buffer-file-name))))
> +     ((derived-mode-p 'log-view-mode)
> +      (list log-view-vc-backend log-view-vc-fileset))
>       ((and (buffer-live-p vc-parent-buffer)
>             ;; FIXME: Why this test?  --Stef
>             (or (buffer-file-name vc-parent-buffer)

Yup, that works, thanks!

-- 
Sean Whitton

bug#68174: 30.0.50; Project and VC disagree about what repository we're in

[Dmitry Gutov]

On 02/01/2024 10:55, Sean Whitton wrote:
>> This particular case though seems fixed with this patch, please test:
>>
>> diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
>> index 3cd835a9d6b..82051740b8f 100644
>> --- a/lisp/vc/vc.el
>> +++ b/lisp/vc/vc.el
>> @@ -1074,6 +1074,7 @@ vc-expand-dirs
>>
>>   (defvar vc-dir-backend)
>>   (defvar log-view-vc-backend)
>> +(defvar log-view-vc-fileset)
>>   (defvar log-edit-vc-backend)
>>   (defvar diff-vc-backend)
>>   (defvar diff-vc-revisions)
>> @@ -1155,6 +1156,8 @@ vc-deduce-fileset
>>   	      (vc-state buffer-file-name)
>>   	      (vc-checkout-model backend buffer-file-name))
>>   	(list backend (list buffer-file-name))))
>> +     ((derived-mode-p 'log-view-mode)
>> +      (list log-view-vc-backend log-view-vc-fileset))
>>        ((and (buffer-live-p vc-parent-buffer)
>>              ;; FIXME: Why this test?  --Stef
>>              (or (buffer-file-name vc-parent-buffer)
> Yup, that works, thanks!

Very good.

Eli, do you think we could add this to emacs-29 as well?

It would help the users of 29.2 avoid the described bug when/if they 
upgrade to the latest project.el. Probably not very urgent, but OTOH the 
change looks pretty safe.

bug#68174: 30.0.50; Project and VC disagree about what repository we're in

[Eli Zaretskii]

> Date: Tue, 2 Jan 2024 15:10:53 +0200
> Cc: 68174@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 02/01/2024 10:55, Sean Whitton wrote:
> >> This particular case though seems fixed with this patch, please test:
> >>
> >> diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
> >> index 3cd835a9d6b..82051740b8f 100644
> >> --- a/lisp/vc/vc.el
> >> +++ b/lisp/vc/vc.el
> >> @@ -1074,6 +1074,7 @@ vc-expand-dirs
> >>
> >>   (defvar vc-dir-backend)
> >>   (defvar log-view-vc-backend)
> >> +(defvar log-view-vc-fileset)
> >>   (defvar log-edit-vc-backend)
> >>   (defvar diff-vc-backend)
> >>   (defvar diff-vc-revisions)
> >> @@ -1155,6 +1156,8 @@ vc-deduce-fileset
> >>   	      (vc-state buffer-file-name)
> >>   	      (vc-checkout-model backend buffer-file-name))
> >>   	(list backend (list buffer-file-name))))
> >> +     ((derived-mode-p 'log-view-mode)
> >> +      (list log-view-vc-backend log-view-vc-fileset))
> >>        ((and (buffer-live-p vc-parent-buffer)
> >>              ;; FIXME: Why this test?  --Stef
> >>              (or (buffer-file-name vc-parent-buffer)
> > Yup, that works, thanks!
> 
> Very good.
> 
> Eli, do you think we could add this to emacs-29 as well?
> 
> It would help the users of 29.2 avoid the described bug when/if they 
> upgrade to the latest project.el. Probably not very urgent, but OTOH the 
> change looks pretty safe.

If it's safe, you'd need to convince me.  Because just by looking at
it, I don't have a clear idea what else it could affect.  And the use
case is pretty rare, AFAICT.  So I tend to be happier with this on
master.

bug#68174: 30.0.50; Project and VC disagree about what repository we're in

[Dmitry Gutov]

Version: 30.1

On 02/01/2024 15:28, Eli Zaretskii wrote:
>> Date: Tue, 2 Jan 2024 15:10:53 +0200
>> Cc: 68174@debbugs.gnu.org, juri@linkov.net
>> From: Dmitry Gutov <dmitry@gutov.dev>
>>
>> On 02/01/2024 10:55, Sean Whitton wrote:
>>>> This particular case though seems fixed with this patch, please test:
>>>>
>>>> diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
>>>> index 3cd835a9d6b..82051740b8f 100644
>>>> --- a/lisp/vc/vc.el
>>>> +++ b/lisp/vc/vc.el
>>>> @@ -1074,6 +1074,7 @@ vc-expand-dirs
>>>>
>>>>    (defvar vc-dir-backend)
>>>>    (defvar log-view-vc-backend)
>>>> +(defvar log-view-vc-fileset)
>>>>    (defvar log-edit-vc-backend)
>>>>    (defvar diff-vc-backend)
>>>>    (defvar diff-vc-revisions)
>>>> @@ -1155,6 +1156,8 @@ vc-deduce-fileset
>>>>    	      (vc-state buffer-file-name)
>>>>    	      (vc-checkout-model backend buffer-file-name))
>>>>    	(list backend (list buffer-file-name))))
>>>> +     ((derived-mode-p 'log-view-mode)
>>>> +      (list log-view-vc-backend log-view-vc-fileset))
>>>>         ((and (buffer-live-p vc-parent-buffer)
>>>>               ;; FIXME: Why this test?  --Stef
>>>>               (or (buffer-file-name vc-parent-buffer)
>>> Yup, that works, thanks!
>>
>> Very good.
>>
>> Eli, do you think we could add this to emacs-29 as well?
>>
>> It would help the users of 29.2 avoid the described bug when/if they
>> upgrade to the latest project.el. Probably not very urgent, but OTOH the
>> change looks pretty safe.
> 
> If it's safe, you'd need to convince me.  Because just by looking at
> it, I don't have a clear idea what else it could affect.

It's not a trivial transformation, but it's semantically sound: 
log-view-mode saves -- or one might say caches -- the backend and the 
fileset in its own variables locally (referenced in the diff above). The 
commands in log-view.el (of which there are several) use those vars 
rather than look up vc-parent-buffer, so the values should be correct.

> And the use
> case is pretty rare, AFAICT.  So I tend to be happier with this on
> master.

Ok, I'll push it to master for now.

bug#68174: 30.0.50; Project and VC disagree about what repository we're in

[Eli Zaretskii]

> Date: Wed, 3 Jan 2024 02:13:07 +0200
> Cc: spwhitton@spwhitton.name, 68174-done@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> > If it's safe, you'd need to convince me.  Because just by looking at
> > it, I don't have a clear idea what else it could affect.
> 
> It's not a trivial transformation, but it's semantically sound: 
> log-view-mode saves -- or one might say caches -- the backend and the 
> fileset in its own variables locally (referenced in the diff above). The 
> commands in log-view.el (of which there are several) use those vars 
> rather than look up vc-parent-buffer, so the values should be correct.

Thanks.  Let's see if there are some use cases which actually benefit
from an ad-hoc decision at that place instead of using the stashed
values.