GNU bug report logs - #73801
31.0.50; project-try-vc sometimes set wrong cache project-vc-extra-root-markers

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 73801 in the body.
You can then email your comments to 73801 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Zhengyi Fu <i <at> fuzy.me>
To: bug-gnu-emacs <at> gnu.org
Subject: 31.0.50; project-try-vc sometimes set wrong cache
 project-vc-extra-root-markers
Date: Mon, 14 Oct 2024 14:29:48 +0800

Consider the following project:

   test-project/
   ├── .git/
   └── subproject/
       ├── marker
       └── subdir/

If `project-vc-extra-root-markers' is set to `("marker")' and
`project-try-vc' is invoked with `test-project/subproject/subdir', it will set the `project-vc'
VC property of the `test-project/subproject' directory to
`(".../test-project" project-vc nil)', so if later `project-try-vc' is
invoked with that directory, it will return a wrong result.

This is because project-vc tries to detect the VC backend by invoking
`project-try-vc' on the subproject while let binding
`project-vc-extra-root-markers' to nil and the result is cached.

The following patch can fix the problem:

--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -595,7 +595,7 @@ project-try-vc
             (let* ((project-vc-extra-root-markers nil)
                    ;; Avoid submodules scan.
                    (enable-dir-local-variables nil)
-                   (parent (project-try-vc root)))
+                   (parent (project-try-vc dir)))
               (and parent (setq backend (nth 1 parent)))))
           (setq project (list 'vc backend root))
           ;; FIXME: Cache for a shorter time.

Message #8 received at 73801 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Zhengyi Fu <i <at> fuzy.me>, Philip Kaludercic <philipk <at> posteo.net>
Cc: 73801 <at> debbugs.gnu.org
Subject: Re: bug#73801: 31.0.50; project-try-vc sometimes set wrong cache
 project-vc-extra-root-markers
Date: Sun, 27 Oct 2024 12:31:40 +0200

> From: Zhengyi Fu <i <at> fuzy.me>
> Date: Mon, 14 Oct 2024 14:29:48 +0800
> 
> Consider the following project:
> 
>    test-project/
>    ├── .git/
>    └── subproject/
>        ├── marker
>        └── subdir/
> 
> If `project-vc-extra-root-markers' is set to `("marker")' and
> `project-try-vc' is invoked with `test-project/subproject/subdir', it will set the `project-vc'
> VC property of the `test-project/subproject' directory to
> `(".../test-project" project-vc nil)', so if later `project-try-vc' is
> invoked with that directory, it will return a wrong result.
> 
> This is because project-vc tries to detect the VC backend by invoking
> `project-try-vc' on the subproject while let binding
> `project-vc-extra-root-markers' to nil and the result is cached.
> 
> The following patch can fix the problem:
> 
> --- a/lisp/progmodes/project.el
> +++ b/lisp/progmodes/project.el
> @@ -595,7 +595,7 @@ project-try-vc
>              (let* ((project-vc-extra-root-markers nil)
>                     ;; Avoid submodules scan.
>                     (enable-dir-local-variables nil)
> -                   (parent (project-try-vc root)))
> +                   (parent (project-try-vc dir)))
>                (and parent (setq backend (nth 1 parent)))))
>            (setq project (list 'vc backend root))
>            ;; FIXME: Cache for a shorter time.

Philip, could you please look into this?

Message #11 received at 73801 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Zhengyi Fu <i <at> fuzy.me>, 73801 <at> debbugs.gnu.org
Subject: Re: bug#73801: 31.0.50; project-try-vc sometimes set wrong cache
 project-vc-extra-root-markers
Date: Mon, 28 Oct 2024 06:06:17 +0200

Hi! Thanks for the report.

On 14/10/2024 09:29, Zhengyi Fu wrote:
> Consider the following project:
> 
>     test-project/
>     ├── .git/
>     └── subproject/
>         ├── marker
>         └── subdir/
> 
> If `project-vc-extra-root-markers' is set to `("marker")' and
> `project-try-vc' is invoked with `test-project/subproject/subdir', it will set the `project-vc'
> VC property of the `test-project/subproject' directory to
> `(".../test-project" project-vc nil)', so if later `project-try-vc' is
> invoked with that directory, it will return a wrong result.
> 
> This is because project-vc tries to detect the VC backend by invoking
> `project-try-vc' on the subproject while let binding
> `project-vc-extra-root-markers' to nil and the result is cached.
> 
> The following patch can fix the problem:
> 
> --- a/lisp/progmodes/project.el
> +++ b/lisp/progmodes/project.el
> @@ -595,7 +595,7 @@ project-try-vc
>               (let* ((project-vc-extra-root-markers nil)
>                      ;; Avoid submodules scan.
>                      (enable-dir-local-variables nil)
> -                   (parent (project-try-vc root)))
> +                   (parent (project-try-vc dir)))
>                 (and parent (setq backend (nth 1 parent)))))
>             (setq project (list 'vc backend root))

I've pushed a different fix in commit 29b30eb49f8 (with slightly better 
performance, I think).

Please try it when you can.

It would be nice to get either of the patches into Emacs 30, too, but it 
might be a little late given where it is in the pretest.

Message #14 received at 73801 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: 73801 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Cc: Zhengyi Fu <i <at> fuzy.me>
Subject: Re: bug#73801: 31.0.50; project-try-vc sometimes set wrong cache
 project-vc-extra-root-markers
Date: Tue, 29 Oct 2024 04:44:13 +0200

Since I see some changes added to the release branch still,

On 28/10/2024 06:06, Dmitry Gutov wrote:
> It would be nice to get either of the patches into Emacs 30, too, but it 
> might be a little late given where it is in the pretest.

Eli, could we install either of the fixes for this bug to emacs-30 too?

The one I installed on master is longer but should result in less I/O, 
while the patch by Zhengyi Fu is a one-liner, which might feel a little 
safer.

Message #17 received at 73801 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: i <at> fuzy.me, 73801 <at> debbugs.gnu.org
Subject: Re: bug#73801: 31.0.50; project-try-vc sometimes set wrong cache
 project-vc-extra-root-markers
Date: Tue, 29 Oct 2024 15:33:36 +0200

> Date: Tue, 29 Oct 2024 04:44:13 +0200
> From: Dmitry Gutov <dmitry <at> gutov.dev>
> Cc: Zhengyi Fu <i <at> fuzy.me>
> 
> Since I see some changes added to the release branch still,
> 
> On 28/10/2024 06:06, Dmitry Gutov wrote:
> > It would be nice to get either of the patches into Emacs 30, too, but it 
> > might be a little late given where it is in the pretest.
> 
> Eli, could we install either of the fixes for this bug to emacs-30 too?
> 
> The one I installed on master is longer but should result in less I/O, 
> while the patch by Zhengyi Fu is a one-liner, which might feel a little 
> safer.

I don't understand the implications of that one-line (nor, TBH, the
analysis of the original problem), so I'm not sure these changes are
safe.  How do we know that catering to this corner case will not screw
other corner cases?  It isn't the first time project.el needs to
tiptoe between several valid outcomes using some pretty ad-hoc
heuristic.

Message #20 received at 73801 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: i <at> fuzy.me, 73801 <at> debbugs.gnu.org
Subject: Re: bug#73801: 31.0.50; project-try-vc sometimes set wrong cache
 project-vc-extra-root-markers
Date: Tue, 29 Oct 2024 22:31:04 +0200

On 29/10/2024 15:33, Eli Zaretskii wrote:
>> Date: Tue, 29 Oct 2024 04:44:13 +0200
>> From: Dmitry Gutov <dmitry <at> gutov.dev>
>> Cc: Zhengyi Fu <i <at> fuzy.me>
>>
>> Since I see some changes added to the release branch still,
>>
>> On 28/10/2024 06:06, Dmitry Gutov wrote:
>>> It would be nice to get either of the patches into Emacs 30, too, but it
>>> might be a little late given where it is in the pretest.
>>
>> Eli, could we install either of the fixes for this bug to emacs-30 too?
>>
>> The one I installed on master is longer but should result in less I/O,
>> while the patch by Zhengyi Fu is a one-liner, which might feel a little
>> safer.
> 
> I don't understand the implications of that one-line (nor, TBH, the
> analysis of the original problem), so I'm not sure these changes are
> safe.

The original problem was due to project-try-vc being invoked recursively 
on a parent directory while a variable that affects its computation is 
bound to nil. The function itself (project-try-vc) memoizes its return 
value. As a result, any subsequent call to it with the same argument 
outside of the said binding could return wrong result.

The first fix (one-liner) made sure that we're calling it with the same 
argument that is passed to the current call, ensuring that the cache 
will be rewritten after it returns.

The second fix (mine) was to extract the value computation into a helper 
function, making the recursive call to not be memoized.

> How do we know that catering to this corner case will not screw
> other corner cases?

Difficult to guarantee that 100%, but this specific case seems important 
enough, while at the same time we can infer that the change won't affect 
the majority scenario because the code is guarded by these conditions:

    (when root
      (when (not backend)
         ...

Message #23 received at 73801 <at> debbugs.gnu.org (full text, mbox):

From: Zhengyi Fu <i <at> fuzy.me>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: Zhengyi Fu <i <at> fuzy.me>, 73801 <at> debbugs.gnu.org
Subject: Re: bug#73801: 31.0.50; project-try-vc sometimes set wrong cache
 project-vc-extra-root-markers
Date: Thu, 31 Oct 2024 11:15:09 +0800

Hi!  I've tried your patch and it works for me.  Thanks!

Message #26 received at 73801 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: i <at> fuzy.me, 73801 <at> debbugs.gnu.org
Subject: Re: bug#73801: 31.0.50; project-try-vc sometimes set wrong cache
 project-vc-extra-root-markers
Date: Thu, 31 Oct 2024 12:06:31 +0200

> Date: Tue, 29 Oct 2024 22:31:04 +0200
> Cc: 73801 <at> debbugs.gnu.org, i <at> fuzy.me
> From: Dmitry Gutov <dmitry <at> gutov.dev>
> 
> On 29/10/2024 15:33, Eli Zaretskii wrote:
> >> Date: Tue, 29 Oct 2024 04:44:13 +0200
> >> From: Dmitry Gutov <dmitry <at> gutov.dev>
> >> Cc: Zhengyi Fu <i <at> fuzy.me>
> >>
> >> Since I see some changes added to the release branch still,
> >>
> >> On 28/10/2024 06:06, Dmitry Gutov wrote:
> >>> It would be nice to get either of the patches into Emacs 30, too, but it
> >>> might be a little late given where it is in the pretest.
> >>
> >> Eli, could we install either of the fixes for this bug to emacs-30 too?
> >>
> >> The one I installed on master is longer but should result in less I/O,
> >> while the patch by Zhengyi Fu is a one-liner, which might feel a little
> >> safer.
> > 
> > I don't understand the implications of that one-line (nor, TBH, the
> > analysis of the original problem), so I'm not sure these changes are
> > safe.
> 
> The original problem was due to project-try-vc being invoked recursively 
> on a parent directory while a variable that affects its computation is 
> bound to nil. The function itself (project-try-vc) memoizes its return 
> value. As a result, any subsequent call to it with the same argument 
> outside of the said binding could return wrong result.
> 
> The first fix (one-liner) made sure that we're calling it with the same 
> argument that is passed to the current call, ensuring that the cache 
> will be rewritten after it returns.
> 
> The second fix (mine) was to extract the value computation into a helper 
> function, making the recursive call to not be memoized.
> 
> > How do we know that catering to this corner case will not screw
> > other corner cases?
> 
> Difficult to guarantee that 100%, but this specific case seems important 
> enough, while at the same time we can infer that the change won't affect 
> the majority scenario because the code is guarded by these conditions:
> 
>      (when root
>        (when (not backend)
>           ...

FWIW, I have a bad feeling about this, but if you are confident, feel
free to backport.

Message #31 received at 73801-done <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: i <at> fuzy.me, 73801-done <at> debbugs.gnu.org
Subject: Re: bug#73801: 31.0.50; project-try-vc sometimes set wrong cache
 project-vc-extra-root-markers
Date: Fri, 1 Nov 2024 02:49:24 +0200

Version: 30.1

On 31/10/2024 12:06, Eli Zaretskii wrote:
>>> How do we know that catering to this corner case will not screw
>>> other corner cases?
>> Difficult to guarantee that 100%, but this specific case seems important
>> enough, while at the same time we can infer that the change won't affect
>> the majority scenario because the code is guarded by these conditions:
>>
>>       (when root
>>         (when (not backend)
>>            ...
> FWIW, I have a bad feeling about this, but if you are confident, feel
> free to backport.

Thanks, I've cherry-picked the longer (but more transparent) fix and a 
new regression test as well, to the release branch.

And thanks to Zhengyi Fu for trying it out.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.