all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#73801: 31.0.50; project-try-vc sometimes set wrong cache project-vc-extra-root-markers
@ 2024-10-14  6:29 Zhengyi Fu
  2024-10-27 10:31 ` Eli Zaretskii
  2024-10-28  4:06 ` Dmitry Gutov
  0 siblings, 2 replies; 9+ messages in thread
From: Zhengyi Fu @ 2024-10-14  6:29 UTC (permalink / raw)
  To: 73801

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.





^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#73801: 31.0.50; project-try-vc sometimes set wrong cache project-vc-extra-root-markers
  2024-10-14  6:29 bug#73801: 31.0.50; project-try-vc sometimes set wrong cache project-vc-extra-root-markers Zhengyi Fu
@ 2024-10-27 10:31 ` Eli Zaretskii
  2024-10-28  4:06 ` Dmitry Gutov
  1 sibling, 0 replies; 9+ messages in thread
From: Eli Zaretskii @ 2024-10-27 10:31 UTC (permalink / raw)
  To: Zhengyi Fu, Philip Kaludercic; +Cc: 73801

> 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?





^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#73801: 31.0.50; project-try-vc sometimes set wrong cache project-vc-extra-root-markers
  2024-10-14  6:29 bug#73801: 31.0.50; project-try-vc sometimes set wrong cache project-vc-extra-root-markers Zhengyi Fu
  2024-10-27 10:31 ` Eli Zaretskii
@ 2024-10-28  4:06 ` Dmitry Gutov
  2024-10-29  2:44   ` Dmitry Gutov
  2024-10-31  3:15   ` Zhengyi Fu
  1 sibling, 2 replies; 9+ messages in thread
From: Dmitry Gutov @ 2024-10-28  4:06 UTC (permalink / raw)
  To: Zhengyi Fu, 73801

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.





^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#73801: 31.0.50; project-try-vc sometimes set wrong cache project-vc-extra-root-markers
  2024-10-28  4:06 ` Dmitry Gutov
@ 2024-10-29  2:44   ` Dmitry Gutov
  2024-10-29 13:33     ` Eli Zaretskii
  2024-10-31  3:15   ` Zhengyi Fu
  1 sibling, 1 reply; 9+ messages in thread
From: Dmitry Gutov @ 2024-10-29  2:44 UTC (permalink / raw)
  To: 73801, Eli Zaretskii; +Cc: Zhengyi Fu

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.





^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#73801: 31.0.50; project-try-vc sometimes set wrong cache project-vc-extra-root-markers
  2024-10-29  2:44   ` Dmitry Gutov
@ 2024-10-29 13:33     ` Eli Zaretskii
  2024-10-29 20:31       ` Dmitry Gutov
  0 siblings, 1 reply; 9+ messages in thread
From: Eli Zaretskii @ 2024-10-29 13:33 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: i, 73801

> 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.





^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#73801: 31.0.50; project-try-vc sometimes set wrong cache project-vc-extra-root-markers
  2024-10-29 13:33     ` Eli Zaretskii
@ 2024-10-29 20:31       ` Dmitry Gutov
  2024-10-31 10:06         ` Eli Zaretskii
  0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Gutov @ 2024-10-29 20:31 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: i, 73801

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)
          ...





^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#73801: 31.0.50; project-try-vc sometimes set wrong cache project-vc-extra-root-markers
  2024-10-28  4:06 ` Dmitry Gutov
  2024-10-29  2:44   ` Dmitry Gutov
@ 2024-10-31  3:15   ` Zhengyi Fu
  1 sibling, 0 replies; 9+ messages in thread
From: Zhengyi Fu @ 2024-10-31  3:15 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: Zhengyi Fu, 73801

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





^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#73801: 31.0.50; project-try-vc sometimes set wrong cache project-vc-extra-root-markers
  2024-10-29 20:31       ` Dmitry Gutov
@ 2024-10-31 10:06         ` Eli Zaretskii
  2024-11-01  0:49           ` Dmitry Gutov
  0 siblings, 1 reply; 9+ messages in thread
From: Eli Zaretskii @ 2024-10-31 10:06 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: i, 73801

> 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.





^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#73801: 31.0.50; project-try-vc sometimes set wrong cache project-vc-extra-root-markers
  2024-10-31 10:06         ` Eli Zaretskii
@ 2024-11-01  0:49           ` Dmitry Gutov
  0 siblings, 0 replies; 9+ messages in thread
From: Dmitry Gutov @ 2024-11-01  0:49 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: i, 73801-done

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.





^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2024-11-01  0:49 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-14  6:29 bug#73801: 31.0.50; project-try-vc sometimes set wrong cache project-vc-extra-root-markers Zhengyi Fu
2024-10-27 10:31 ` Eli Zaretskii
2024-10-28  4:06 ` Dmitry Gutov
2024-10-29  2:44   ` Dmitry Gutov
2024-10-29 13:33     ` Eli Zaretskii
2024-10-29 20:31       ` Dmitry Gutov
2024-10-31 10:06         ` Eli Zaretskii
2024-11-01  0:49           ` Dmitry Gutov
2024-10-31  3:15   ` Zhengyi Fu

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.