bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Sean Whitton]

X-debbugs-cc: dmitry@gutov.dev, juri@linkov.net, sbaugh@catern.com

Hello,

1. cd /home/spwhitton/src/some-project && emacs -q
2. (setopt project-switch-commands #'project-prefix-or-any-command)
3. (project-remember-project (project-current nil "/home/spwhitton/src/emacs/trunk/))
4. C-x p p /home/spwhitton/src/emacs/trunk RET C-x v L

VC prints the log of /home/spwhitton/some-project, not that of /home/spwhitton/src/emacs/trunk.

-- 
Sean Whitton

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Dmitry Gutov]

Hi!

On 14/11/2023 15:13, Sean Whitton wrote:
> X-debbugs-cc: dmitry@gutov.dev, juri@linkov.net, sbaugh@catern.com
> 
> Hello,
> 
> 1. cd /home/spwhitton/src/some-project && emacs -q
> 2. (setopt project-switch-commands #'project-prefix-or-any-command)
> 3. (project-remember-project (project-current nil "/home/spwhitton/src/emacs/trunk/))
> 4. C-x p p /home/spwhitton/src/emacs/trunk RET C-x v L
> 
> VC prints the log of /home/spwhitton/some-project, not that of /home/spwhitton/src/emacs/trunk.

I'm having difficulty reproducing this.

On step 4, I'm asked for the project root (because *scratch* doesn't 
have a current VC backend), but the input defaults to the directory 
chosen in steps 3 and 4. And if I open a file-visiting buffer first, 
then the prompt is skipped, and I do see the log of the other project. 
Not the one used in step 1.

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Sean Whitton]

Hello,

On Mon 20 Nov 2023 at 04:17am +02, Dmitry Gutov wrote:

> Hi!
>
> On 14/11/2023 15:13, Sean Whitton wrote:
>> X-debbugs-cc: dmitry@gutov.dev, juri@linkov.net, sbaugh@catern.com
>> Hello,
>> 1. cd /home/spwhitton/src/some-project && emacs -q
>> 2. (setopt project-switch-commands #'project-prefix-or-any-command)
>> 3. (project-remember-project (project-current nil "/home/spwhitton/src/emacs/trunk/))
>> 4. C-x p p /home/spwhitton/src/emacs/trunk RET C-x v L
>> VC prints the log of /home/spwhitton/some-project, not that of
>> /home/spwhitton/src/emacs/trunk.
>
> I'm having difficulty reproducing this.

Hmm.  I can't reproduce it with 'emacs -q', but I can still reproduce
with my init.el.  I'm not sure whether something changed or whether I
was wrong before.

> On step 4, I'm asked for the project root (because *scratch* doesn't
> have a current VC backend), but the input defaults to the directory
> chosen in steps 3 and 4. And if I open a file-visiting buffer first,
> then the prompt is skipped, [...]

If I select *Messages*, which has a default-directory of "~/", then
'C-x p p RET ~/src/emacs/trunk RET C-x v L' prompts me for a directory,
but the default value is "~/".  Before I start bisecting my init, does
anything else occur to you?

As an aside, it doesn't seem good that whether or not step four prompts
you for a directory depends on whether the current buffer happens to be
visiting a file or not.  Can we improve that, or is it inherent to the
design of the new feature?

Thanks.

-- 
Sean Whitton

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Juri Linkov]

> As an aside, it doesn't seem good that whether or not step four prompts
> you for a directory depends on whether the current buffer happens to be
> visiting a file or not.  Can we improve that, or is it inherent to the
> design of the new feature?

This was improved recently in bug#67145, so now you can set
'vc-deduce-backend-nonvc-modes' to nil to avoid this prompt.
I'm not sure if nil should be the default value.

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Dmitry Gutov]

On 23/11/2023 19:20, Juri Linkov wrote:
>> As an aside, it doesn't seem good that whether or not step four prompts
>> you for a directory depends on whether the current buffer happens to be
>> visiting a file or not.  Can we improve that, or is it inherent to the
>> design of the new feature?
> This was improved recently in bug#67145, so now you can set
> 'vc-deduce-backend-nonvc-modes' to nil to avoid this prompt.
> I'm not sure if nil should be the default value.

I wonder if the "deduce in all modes" value should be t rather than nil.

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Dmitry Gutov]

On 23/11/2023 17:21, Sean Whitton wrote:

>> On 14/11/2023 15:13, Sean Whitton wrote:
>>> X-debbugs-cc: dmitry@gutov.dev, juri@linkov.net, sbaugh@catern.com
>>> Hello,
>>> 1. cd /home/spwhitton/src/some-project && emacs -q
>>> 2. (setopt project-switch-commands #'project-prefix-or-any-command)
>>> 3. (project-remember-project (project-current nil "/home/spwhitton/src/emacs/trunk/))
>>> 4. C-x p p /home/spwhitton/src/emacs/trunk RET C-x v L
>>> VC prints the log of /home/spwhitton/some-project, not that of
>>> /home/spwhitton/src/emacs/trunk.
>>
>> I'm having difficulty reproducing this.
> 
> Hmm.  I can't reproduce it with 'emacs -q', but I can still reproduce
> with my init.el.  I'm not sure whether something changed or whether I
> was wrong before.
> 
>> On step 4, I'm asked for the project root (because *scratch* doesn't
>> have a current VC backend), but the input defaults to the directory
>> chosen in steps 3 and 4. And if I open a file-visiting buffer first,
>> then the prompt is skipped, [...]
> 
> If I select *Messages*, which has a default-directory of "~/", then
> 'C-x p p RET ~/src/emacs/trunk RET C-x v L' prompts me for a directory,
> but the default value is "~/".  Before I start bisecting my init, does
> anything else occur to you?

That's the thing: I've tried a few different things which could have 
helped reproduce this, but they didn't.

> As an aside, it doesn't seem good that whether or not step four prompts
> you for a directory depends on whether the current buffer happens to be
> visiting a file or not.  Can we improve that, or is it inherent to the
> design of the new feature?

More like it's inherent to how the command you are calling is designed: 
it checks for the backend of the current buffer and behaves differently 
depending on whether one is found.

Since project-prefix-or-any-command works in the current buffer, it 
cannot really reset every such local variable. OTOH, it could 
temporarily switch to an empty buffer. In that case, however, any 
thing-at-point functionality won't work (and the invoked command might 
want to use the symbol at point as some default input or etc). At the 
time we discussed this, the latter seemed like a bigger problem.

Like Juri said, this particular disparity can be configured away, but 
different oddities in that class are bound to come up again.

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Juri Linkov]

>>> As an aside, it doesn't seem good that whether or not step four prompts
>>> you for a directory depends on whether the current buffer happens to be
>>> visiting a file or not.  Can we improve that, or is it inherent to the
>>> design of the new feature?
>> This was improved recently in bug#67145, so now you can set
>> 'vc-deduce-backend-nonvc-modes' to nil to avoid this prompt.
>> I'm not sure if nil should be the default value.
>
> I wonder if the "deduce in all modes" value should be t rather than nil.

Good idea since nil could be used to disable deduction completely.
Then maybe it should be defcustom?

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Dmitry Gutov]

On 24/11/2023 09:46, Juri Linkov wrote:
>>>> As an aside, it doesn't seem good that whether or not step four prompts
>>>> you for a directory depends on whether the current buffer happens to be
>>>> visiting a file or not.  Can we improve that, or is it inherent to the
>>>> design of the new feature?
>>> This was improved recently in bug#67145, so now you can set
>>> 'vc-deduce-backend-nonvc-modes' to nil to avoid this prompt.
>>> I'm not sure if nil should be the default value.
>> I wonder if the "deduce in all modes" value should be t rather than nil.
> Good idea since nil could be used to disable deduction completely.
> Then maybe it should be defcustom?

Why not.

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 678 bytes --]

>>>>> As an aside, it doesn't seem good that whether or not step four prompts
>>>>> you for a directory depends on whether the current buffer happens to be
>>>>> visiting a file or not.  Can we improve that, or is it inherent to the
>>>>> design of the new feature?
>>>> This was improved recently in bug#67145, so now you can set
>>>> 'vc-deduce-backend-nonvc-modes' to nil to avoid this prompt.
>>>> I'm not sure if nil should be the default value.
>>> I wonder if the "deduce in all modes" value should be t rather than nil.
>> Good idea since nil could be used to disable deduction completely.
>> Then maybe it should be defcustom?
>
> Why not.

Maybe something like this:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vc-deduce-backend-nonvc-modes.patch --]
[-- Type: text/x-diff, Size: 1326 bytes --]

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 1bd9ecb2193..bde22536d1f 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1071,19 +1071,23 @@ log-edit-vc-backend
 (defvar diff-vc-backend)
 (defvar diff-vc-revisions)
 
-;; Maybe we could even use comint-mode rather than shell-mode?
-(defvar vc-deduce-backend-nonvc-modes
+(defcustom vc-deduce-backend-nonvc-modes
+  ;; Maybe we could even use comint-mode rather than shell-mode?
   '(dired-mode shell-mode eshell-mode compilation-mode)
   "List of modes not supported by VC where backend should be deduced.
 In these modes the backend is deduced based on `default-directory'.
-When nil, the backend is deduced in all modes.")
+When t, the backend is deduced in all modes."
+  :type '(choice (const :tag "None" nil)
+		 hook
+		 (const :tag "All" t))
+  :version "30.1")
 
 (defun vc-deduce-backend ()
   (cond ((derived-mode-p 'vc-dir-mode)   vc-dir-backend)
 	((derived-mode-p 'log-view-mode) log-view-vc-backend)
 	((derived-mode-p 'log-edit-mode) log-edit-vc-backend)
 	((derived-mode-p 'diff-mode)     diff-vc-backend)
-	((or (null vc-deduce-backend-nonvc-modes)
+	((or (eq vc-deduce-backend-nonvc-modes t)
 	     (derived-mode-p vc-deduce-backend-nonvc-modes))
 	 (ignore-errors (vc-responsible-backend default-directory)))
 	(vc-mode (vc-backend buffer-file-name))))

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Dmitry Gutov]

On 25/11/2023 20:39, Juri Linkov wrote:
> +		 hook

This one looks a bit out of place, but if it simply means "a function", 
that's ok.

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Juri Linkov]

>> +		 hook
>
> This one looks a bit out of place, but if it simply means "a function",
> that's ok.

Actually, it's a shorthand for "a list of functions".

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Eli Zaretskii]

> Cc: 67171@debbugs.gnu.org, sbaugh@catern.com,
>  Sean Whitton <spwhitton@spwhitton.name>
> From: Juri Linkov <juri@linkov.net>
> Date: Sat, 25 Nov 2023 20:39:37 +0200
> 
> +(defcustom vc-deduce-backend-nonvc-modes
> +  ;; Maybe we could even use comint-mode rather than shell-mode?
>    '(dired-mode shell-mode eshell-mode compilation-mode)
>    "List of modes not supported by VC where backend should be deduced.
>  In these modes the backend is deduced based on `default-directory'.
> -When nil, the backend is deduced in all modes.")
> +When t, the backend is deduced in all modes."

Please don't use "when" in these contexts; use "if" instead.  "When"
could be confusing because it has the meaning of time, not only of
condition.

Also, it is better to say "If the value is t" or "If this is t",
instead of just "If t".

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Dmitry Gutov]

On 25/11/2023 21:10, Juri Linkov wrote:
>>> +		 hook
>> This one looks a bit out of place, but if it simply means "a function",
>> that's ok.
> Actually, it's a shorthand for "a list of functions".

Ah, perfect.

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Juri Linkov]

>> +(defcustom vc-deduce-backend-nonvc-modes
>> +  ;; Maybe we could even use comint-mode rather than shell-mode?
>>    '(dired-mode shell-mode eshell-mode compilation-mode)
>>    "List of modes not supported by VC where backend should be deduced.
>>  In these modes the backend is deduced based on `default-directory'.
>> -When nil, the backend is deduced in all modes.")
>> +When t, the backend is deduced in all modes."
>
> Please don't use "when" in these contexts; use "if" instead.  "When"
> could be confusing because it has the meaning of time, not only of
> condition.
>
> Also, it is better to say "If the value is t" or "If this is t",
> instead of just "If t".

Now changed to defcustom with these fixes.

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Sean Whitton]

Hello,

On Mon 20 Nov 2023 at 04:17am +02, Dmitry Gutov wrote:

> On 14/11/2023 15:13, Sean Whitton wrote:
>> X-debbugs-cc: dmitry@gutov.dev, juri@linkov.net, sbaugh@catern.com
>> Hello,
>> 1. cd /home/spwhitton/src/some-project && emacs -q
>> 2. (setopt project-switch-commands #'project-prefix-or-any-command)
>> 3. (project-remember-project (project-current nil "/home/spwhitton/src/emacs/trunk/))
>> 4. C-x p p /home/spwhitton/src/emacs/trunk RET C-x v L
>> VC prints the log of /home/spwhitton/some-project, not that of
>> /home/spwhitton/src/emacs/trunk.
>
> I'm having difficulty reproducing this.
>
> On step 4, I'm asked for the project root (because *scratch* doesn't have a
> current VC backend), but the input defaults to the directory chosen in steps 3
> and 4. And if I open a file-visiting buffer first, then the prompt is skipped,
> and I do see the log of the other project. Not the one used in step 1.

Alright, I've bisected it.  After step 3, additionally eval this form:

    (define-key project-prefix-map "L" #'vc-print-root-log)

I have this because I want to be able to type just L instead of C-x v L.
That doesn't work -- possibly not a bug -- but surely adding that
binding shouldn't affect C-x v L, at least?

-- 
Sean Whitton

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Dmitry Gutov]

On 06/12/2023 00:40, Sean Whitton wrote:
> Alright, I've bisected it.  After step 3, additionally eval this form:
> 
>      (define-key project-prefix-map "L" #'vc-print-root-log)
> 
> I have this because I want to be able to type just L instead of C-x v L.
> That doesn't work -- possibly not a bug -- but surely adding that
> binding shouldn't affect C-x v L, at least?

All right, the full scenario is unexpected, but otherwise it's a 
documented behavior, see the docstring for 'project-any-command'.

We discussed the possibility of the override in the other way (in 
bug#63648, which resulted in this command), but not an opt-out for 
commands in project-prefix-map, yet.

So... something like this?

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index a81bb63fba4..feef7ba5248 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -1861,9 +1861,10 @@ project-any-command
      (when command
        ;; We could also check the command name against "\\`project-",
        ;; and/or (get command 'project-command).
-      (map-keymap
-       (lambda (_evt cmd) (if (eq cmd command) (setq found t)))
-       project-prefix-map)
+      (unless (get command 'project-switch-with-default-directory)
+        (map-keymap
+         (lambda (_evt cmd) (if (eq cmd command) (setq found t)))
+         project-prefix-map))
        (if found
            (let ((project-current-directory-override root))
              (call-interactively command))


Combined with

   (put 'vc-print-root-log 'project-switch-with-default-directory t)

somewhere in your init script.

The alternative would be tagging all project-related commands. Even if 
we also check for the 'project-' prefix in command's name, the 
user-defined commands using the project API will be affected (I don't 
know for how many it would be a problem, but still).

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Sean Whitton]

Hello,

On Wed 06 Dec 2023 at 02:26am +02, Dmitry Gutov wrote:

> On 06/12/2023 00:40, Sean Whitton wrote:
>> Alright, I've bisected it.  After step 3, additionally eval this form:
>>      (define-key project-prefix-map "L" #'vc-print-root-log)
>> I have this because I want to be able to type just L instead of C-x v L.
>> That doesn't work -- possibly not a bug -- but surely adding that
>> binding shouldn't affect C-x v L, at least?
>
> All right, the full scenario is unexpected, but otherwise it's a documented
> behavior, see the docstring for 'project-any-command'.

Thanks, I see what you mean.

> We discussed the possibility of the override in the other way (in
> bug#63648, which resulted in this command), but not an opt-out for
> commands in project-prefix-map, yet.
>
> So... something like this?
>
> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
> index a81bb63fba4..feef7ba5248 100644
> --- a/lisp/progmodes/project.el
> +++ b/lisp/progmodes/project.el
> @@ -1861,9 +1861,10 @@ project-any-command
>      (when command
>        ;; We could also check the command name against "\\`project-",
>        ;; and/or (get command 'project-command).
> -      (map-keymap
> -       (lambda (_evt cmd) (if (eq cmd command) (setq found t)))
> -       project-prefix-map)
> +      (unless (get command 'project-switch-with-default-directory)
> +        (map-keymap
> +         (lambda (_evt cmd) (if (eq cmd command) (setq found t)))
> +         project-prefix-map))
>        (if found
>            (let ((project-current-directory-override root))
>              (call-interactively command))
>
>
> Combined with
>
>   (put 'vc-print-root-log 'project-switch-with-default-directory t)
>
> somewhere in your init script.
>
> The alternative would be tagging all project-related commands. Even if we also
> check for the 'project-' prefix in command's name, the user-defined commands
> using the project API will be affected (I don't know for how many it would be
> a problem, but still).

This solution makes sense.  We definitely want the user to have a way to
tag additional commands.  But couldn't we pre-tag some, like this one,
for example?  It is difficult to think of wanting to not have this one
tagged.  And the user could always remove the tag in their init.

-- 
Sean Whitton

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Juri Linkov]

> So... something like this?
>
> @@ -1861,9 +1861,10 @@ project-any-command
>      (when command
>        ;; We could also check the command name against "\\`project-",
>        ;; and/or (get command 'project-command).
> -      (map-keymap
> -       (lambda (_evt cmd) (if (eq cmd command) (setq found t)))
> -       project-prefix-map)
> +      (unless (get command 'project-switch-with-default-directory)
> +        (map-keymap
> +         (lambda (_evt cmd) (if (eq cmd command) (setq found t)))
> +         project-prefix-map))
>        (if found
>            (let ((project-current-directory-override root))
>              (call-interactively command))

Why not let-bind both variables for all commands:
'project-current-directory-override' and 'default-directory'?

Then project commands will pick up the first:

  (or project-current-directory-override default-directory)

And non-project commands will just ignore
'project-current-directory-override'.

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Dmitry Gutov]

On 06/12/2023 19:10, Juri Linkov wrote:
>> So... something like this?
>>
>> @@ -1861,9 +1861,10 @@ project-any-command
>>       (when command
>>         ;; We could also check the command name against "\\`project-",
>>         ;; and/or (get command 'project-command).
>> -      (map-keymap
>> -       (lambda (_evt cmd) (if (eq cmd command) (setq found t)))
>> -       project-prefix-map)
>> +      (unless (get command 'project-switch-with-default-directory)
>> +        (map-keymap
>> +         (lambda (_evt cmd) (if (eq cmd command) (setq found t)))
>> +         project-prefix-map))
>>         (if found
>>             (let ((project-current-directory-override root))
>>               (call-interactively command))
> Why not let-bind both variables for all commands:
> 'project-current-directory-override' and 'default-directory'?
> 
> Then project commands will pick up the first:
> 
>    (or project-current-directory-override default-directory)
> 
> And non-project commands will just ignore
> 'project-current-directory-override'.

I think that would still regress bug#58784.

And project-current-directory-override was really only added to benefit 
such rare cases.

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Dmitry Gutov]

On 06/12/2023 17:09, Sean Whitton wrote:

>> Combined with
>>
>>    (put 'vc-print-root-log 'project-switch-with-default-directory t)
>>
>> somewhere in your init script.
>>
>> The alternative would be tagging all project-related commands. Even if we also
>> check for the 'project-' prefix in command's name, the user-defined commands
>> using the project API will be affected (I don't know for how many it would be
>> a problem, but still).
> 
> This solution makes sense.  We definitely want the user to have a way to
> tag additional commands.  But couldn't we pre-tag some, like this one,
> for example?  It is difficult to think of wanting to not have this one
> tagged.  And the user could always remove the tag in their init.

That would be a half-measure still. And why this command but not others? 
And if others too, then which ones?

It might seem natural to you, but it never occurred to add 
vc-print-root-log to project-prefix-map to me. What other commands would 
not occur to us both but would to others?

Would it make sense to tag all VC commands? Or just consider the 'vc-' 
prefix as a negative?

To consider the "alternative" approach once more, we could recognize the 
  'project-' commands as the ones that should use 
project-current-directory-override. But the rest would use 
default-directory, unless they have a property 'project-related' or 
something. That would exclude user-defined commands in the beginning, 
but then again, the difference between binding 
project-current-directory-override and default-directory might matter 
only to a small fraction of them.

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Sean Whitton]

Hello,

On Thu 07 Dec 2023 at 02:10am +02, Dmitry Gutov wrote:

> On 06/12/2023 17:09, Sean Whitton wrote:
>
>>> Combined with
>>>
>>>    (put 'vc-print-root-log 'project-switch-with-default-directory t)
>>>
>>> somewhere in your init script.
>>>
>>> The alternative would be tagging all project-related commands. Even if we also
>>> check for the 'project-' prefix in command's name, the user-defined commands
>>> using the project API will be affected (I don't know for how many it would be
>>> a problem, but still).
>> This solution makes sense.  We definitely want the user to have a way to
>> tag additional commands.  But couldn't we pre-tag some, like this one,
>> for example?  It is difficult to think of wanting to not have this one
>> tagged.  And the user could always remove the tag in their init.
>
> That would be a half-measure still. And why this command but not others? And
> if others too, then which ones?
>
> It might seem natural to you, but it never occurred to add vc-print-root-log
> to project-prefix-map to me. What other commands would not occur to us both
> but would to others?
>
> Would it make sense to tag all VC commands? Or just consider the 'vc-' prefix
> as a negative?
>
> To consider the "alternative" approach once more, we could recognize the
> 'project-' commands as the ones that should use
> project-current-directory-override. But the rest would use default-directory,
> unless they have a property 'project-related' or something. That would exclude
> user-defined commands in the beginning, but then again, the difference between
> binding project-current-directory-override and default-directory might matter
> only to a small fraction of them.

I think the half-measure is okay, for it can become a fuller measure
over time.  Let's not do anything blanket for all vc- or project-
commands, but just provide the facility, and pre-tag commands as we
realise it couldn't make sense not to want the facility for those.

-- 
Sean Whitton

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Juri Linkov]

>>> +      (unless (get command 'project-switch-with-default-directory)
>>> +        (map-keymap
>>> +         (lambda (_evt cmd) (if (eq cmd command) (setq found t)))
>>> +         project-prefix-map))
>>>         (if found
>>>             (let ((project-current-directory-override root))
>>>               (call-interactively command))
>> Why not let-bind both variables for all commands:
>> 'project-current-directory-override' and 'default-directory'?
>> Then project commands will pick up the first:
>>    (or project-current-directory-override default-directory)
>> And non-project commands will just ignore
>> 'project-current-directory-override'.
>
> I think that would still regress bug#58784.
>
> And project-current-directory-override was really only added to benefit
> such rare cases.

So the only reason to distinguish project commands from non-project commands
is that 'project-buffers' uses (buffer-local-value 'default-directory buf)?
Then wouldn't it be easier to exclude from setting default-directory
all commands that use 'project-buffers'?

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Dmitry Gutov]

On 07/12/2023 19:22, Juri Linkov wrote:
>>>> +      (unless (get command 'project-switch-with-default-directory)
>>>> +        (map-keymap
>>>> +         (lambda (_evt cmd) (if (eq cmd command) (setq found t)))
>>>> +         project-prefix-map))
>>>>          (if found
>>>>              (let ((project-current-directory-override root))
>>>>                (call-interactively command))
>>> Why not let-bind both variables for all commands:
>>> 'project-current-directory-override' and 'default-directory'?
>>> Then project commands will pick up the first:
>>>     (or project-current-directory-override default-directory)
>>> And non-project commands will just ignore
>>> 'project-current-directory-override'.
>> I think that would still regress bug#58784.
>>
>> And project-current-directory-override was really only added to benefit
>> such rare cases.
> So the only reason to distinguish project commands from non-project commands
> is that 'project-buffers' uses (buffer-local-value 'default-directory buf)?
> Then wouldn't it be easier to exclude from setting default-directory
> all commands that use 'project-buffers'?

Using which method, though?

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Juri Linkov]

>>>>> +      (unless (get command 'project-switch-with-default-directory)
>>>>> +        (map-keymap
>>>>> +         (lambda (_evt cmd) (if (eq cmd command) (setq found t)))
>>>>> +         project-prefix-map))
>>>>>          (if found
>>>>>              (let ((project-current-directory-override root))
>>>>>                (call-interactively command))
>>>> Why not let-bind both variables for all commands:
>>>> 'project-current-directory-override' and 'default-directory'?
>>>> Then project commands will pick up the first:
>>>>     (or project-current-directory-override default-directory)
>>>> And non-project commands will just ignore
>>>> 'project-current-directory-override'.
>>> I think that would still regress bug#58784.
>>>
>>> And project-current-directory-override was really only added to benefit
>>> such rare cases.
>> So the only reason to distinguish project commands from non-project commands
>> is that 'project-buffers' uses (buffer-local-value 'default-directory buf)?
>> Then wouldn't it be easier to exclude from setting default-directory
>> all commands that use 'project-buffers'?
>
> Using which method, though?

Probably by a command symbol property, although I don't quite like such solution.

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Dmitry Gutov]

On 07/12/2023 13:23, Sean Whitton wrote:
> Hello,
> 
> On Thu 07 Dec 2023 at 02:10am +02, Dmitry Gutov wrote:
> 
>> On 06/12/2023 17:09, Sean Whitton wrote:
>>
>>>> Combined with
>>>>
>>>>     (put 'vc-print-root-log 'project-switch-with-default-directory t)
>>>>
>>>> somewhere in your init script.
>>>>
>>>> The alternative would be tagging all project-related commands. Even if we also
>>>> check for the 'project-' prefix in command's name, the user-defined commands
>>>> using the project API will be affected (I don't know for how many it would be
>>>> a problem, but still).
>>> This solution makes sense.  We definitely want the user to have a way to
>>> tag additional commands.  But couldn't we pre-tag some, like this one,
>>> for example?  It is difficult to think of wanting to not have this one
>>> tagged.  And the user could always remove the tag in their init.
>> That would be a half-measure still. And why this command but not others? And
>> if others too, then which ones?
>>
>> It might seem natural to you, but it never occurred to add vc-print-root-log
>> to project-prefix-map to me. What other commands would not occur to us both
>> but would to others?
>>
>> Would it make sense to tag all VC commands? Or just consider the 'vc-' prefix
>> as a negative?
>>
>> To consider the "alternative" approach once more, we could recognize the
>> 'project-' commands as the ones that should use
>> project-current-directory-override. But the rest would use default-directory,
>> unless they have a property 'project-related' or something. That would exclude
>> user-defined commands in the beginning, but then again, the difference between
>> binding project-current-directory-override and default-directory might matter
>> only to a small fraction of them.
> I think the half-measure is okay, for it can become a fuller measure
> over time.  Let's not do anything blanket for all vc- or project-
> commands, but just provide the facility, and pre-tag commands as we
> realise it couldn't make sense not to want the facility for those.

Sorry, but I still don't see how vc-print-root-log is different from 
many others (vc-print-* family of commands, or rgrep, or... commands 
like run-python (ensuring that it runs in a specific dir might mean the 
correct venv and python version), or find-dired, or etc.

So if we tag vc-print-root-log in-tree, it seems like we'd need to add 
properties for all of the above, and more (also in-tree).

I have now pushed the patch with the opposite solution to master.

Its saving grace is that, like Juri also noted, most project-related 
commands wouldn't require this property to be set to function 
adequately. The ones that need the original value of default-directory 
in the current buffer should be in strict minority.

Please check that it works for you, and for any further problems.

This is now in master:

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index a81bb63fba4..7789243cb00 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -1841,10 +1841,12 @@ project-execute-extended-command
  ;;;###autoload
  (defun project-any-command (&optional overriding-map prompt-format)
    "Run the next command in the current project.
-If the command is in `project-prefix-map', it gets passed that
-info with `project-current-directory-override'.  Otherwise,
-`default-directory' is temporarily set to the current project's
-root.
+
+If the command name starts with `project-', or its symbol has
+property `project-related', it gets passed the project to use
+with the variable `project-current-directory-override'.
+Otherwise, `default-directory' is temporarily set to the current
+project's root.

  If OVERRIDING-MAP is non-nil, it will be used as
  `overriding-local-map' to provide shorter bindings from that map
@@ -1856,15 +1858,11 @@ project-any-command
                      (key-binding (read-key-sequence
                                    (format prompt-format (project-root 
pr)))
                                   t)))
-         (root (project-root pr))
-         found)
+         (root (project-root pr)))
      (when command
-      ;; We could also check the command name against "\\`project-",
-      ;; and/or (get command 'project-command).
-      (map-keymap
-       (lambda (_evt cmd) (if (eq cmd command) (setq found t)))
-       project-prefix-map)
-      (if found
+      (if (when (symbolp command)
+            (or (string-prefix-p "project-" (symbol-name command))
+                (get command 'project-command)))
            (let ((project-current-directory-override root))
              (call-interactively command))
          (let ((default-directory root))

bug#67171: 30.0.50; (At least) some VC commands fail with project-prefix-or-any-command

[Sean Whitton]

Hello,

On Fri 08 Dec 2023 at 10:37pm +02, Dmitry Gutov wrote:

> I have now pushed the patch with the opposite solution to master.
>
> Its saving grace is that, like Juri also noted, most project-related commands
> wouldn't require this property to be set to function adequately. The ones that
> need the original value of default-directory in the current buffer should be
> in strict minority.
>
> Please check that it works for you, and for any further problems.

It certainly solves #67171.

I wasn't part of the earlier discussion, so thank you very much for
figuring out a solution that would seem to cover all these use cases.

-- 
Sean Whitton