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

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

[Zhengyi Fu]

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.

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

[Eli Zaretskii]

> From: Zhengyi Fu <i@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?

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

[Dmitry Gutov]

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.

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

[Dmitry Gutov]

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.

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

[Eli Zaretskii]

> Date: Tue, 29 Oct 2024 04:44:13 +0200
> From: Dmitry Gutov <dmitry@gutov.dev>
> Cc: Zhengyi Fu <i@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.

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

[Dmitry Gutov]

On 29/10/2024 15:33, Eli Zaretskii wrote:
>> Date: Tue, 29 Oct 2024 04:44:13 +0200
>> From: Dmitry Gutov <dmitry@gutov.dev>
>> Cc: Zhengyi Fu <i@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)
          ...

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

[Zhengyi Fu]

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

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

[Eli Zaretskii]

> Date: Tue, 29 Oct 2024 22:31:04 +0200
> Cc: 73801@debbugs.gnu.org, i@fuzy.me
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 29/10/2024 15:33, Eli Zaretskii wrote:
> >> Date: Tue, 29 Oct 2024 04:44:13 +0200
> >> From: Dmitry Gutov <dmitry@gutov.dev>
> >> Cc: Zhengyi Fu <i@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.

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

[Dmitry Gutov]

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.