Re: master 101f3cf5b9: Add support for user edits to VC command arguments

Re: master 101f3cf5b9: Add support for user edits to VC command arguments

[Robert Pluim]

>>>>> On Wed, 21 Sep 2022 15:33:02 -0400 (EDT), Sean Whitton <spwhitton@spwhitton.name> said:

 
    Sean> +(defvar vc-want-edit-command-p nil
    Sean> +  "If non-nil, let user edit the VC shell command before running it.")
    Sean> +
    Sean>  ;; Common command execution logic

Is the intent that someone would setq this to t? If so it should be a
defcustom. If thatʼs not the intent, but itʼs just being used to track
the prefix args to the various changed commands, then I think it
shouldnʼt appear in the docstring.

Robert
-- 

Re: master 101f3cf5b9: Add support for user edits to VC command arguments

[Stefan Monnier]

Robert Pluim [2022-09-22 11:25:31] wrote:
>     Sean> +(defvar vc-want-edit-command-p nil
>     Sean> +  "If non-nil, let user edit the VC shell command before running it.")

BTW, `-p` stands for "predicate", IOW *functions* that return booleans.


        Stefan

Re: master 101f3cf5b9: Add support for user edits to VC command arguments

[Sean Whitton]

Hello,

On Thu 22 Sep 2022 at 09:24AM -04, Stefan Monnier wrote:

> Robert Pluim [2022-09-22 11:25:31] wrote:
>>     Sean> +(defvar vc-want-edit-command-p nil
>>     Sean> +  "If non-nil, let user edit the VC shell command before running it.")
>
> BTW, `-p` stands for "predicate", IOW *functions* that return booleans.

I thought that in Lisp we often call boolean values predicates too, even
though that violates the usage of 'predicate' in mathematics.

Is there some other convention for booleans like this one?

-- 
Sean Whitton

Re: master 101f3cf5b9: Add support for user edits to VC command arguments

[Stefan Monnier]

>> Robert Pluim [2022-09-22 11:25:31] wrote:
>>>     Sean> +(defvar vc-want-edit-command-p nil
>>>     Sean> +  "If non-nil, let user edit the VC shell command before running it.")
>>
>> BTW, `-p` stands for "predicate", IOW *functions* that return booleans.
>
> I thought that in Lisp we often call boolean values predicates too, even
> though that violates the usage of 'predicate' in mathematics.
>
> Is there some other convention for booleans like this one?

Usually we don't need it because the name says it, like
`vc-let-user-edit-commands`.

There is a convention to use `-flag` for boolean variables.  I have
a strong dislike of that convention, tho, because all too often what
starts as a boolean var is later extended to allow for more variety
of values.

There is another convention the use `-mode` for boolean variables, when
they are (very) exposed to the user and defined via `define-minor-mode`.
I like this convention but I don't think it should apply here.


        Stefan

Re: master 101f3cf5b9: Add support for user edits to VC command arguments

[Sean Whitton]

Hello,

On Thu 22 Sep 2022 at 12:55PM -04, Stefan Monnier wrote:

>>> Robert Pluim [2022-09-22 11:25:31] wrote:
>>>>     Sean> +(defvar vc-want-edit-command-p nil
>>>>     Sean> +  "If non-nil, let user edit the VC shell command before running it.")
>>>
>>> BTW, `-p` stands for "predicate", IOW *functions* that return booleans.
>>
>> I thought that in Lisp we often call boolean values predicates too, even
>> though that violates the usage of 'predicate' in mathematics.
>>
>> Is there some other convention for booleans like this one?
>
> Usually we don't need it because the name says it, like
> `vc-let-user-edit-commands`.

I'd be happy to change it to this (minus the plural) if you and/or
others would like.

-- 
Sean Whitton

Re: master 101f3cf5b9: Add support for user edits to VC command arguments

[Juri Linkov]

>>>>>     Sean> +(defvar vc-want-edit-command-p nil
>>>>>     Sean> +  "If non-nil, let user edit the VC shell command before running it.")
>>>>
>>>> BTW, `-p` stands for "predicate", IOW *functions* that return booleans.
>>>
>>> I thought that in Lisp we often call boolean values predicates too, even
>>> though that violates the usage of 'predicate' in mathematics.
>>>
>>> Is there some other convention for booleans like this one?
>>
>> Usually we don't need it because the name says it, like
>> `vc-let-user-edit-commands`.
>
> I'd be happy to change it to this (minus the plural) if you and/or
> others would like.

Why not plural?  Some vc commands run several shell commands.
For example, 'vc-git-log-incoming' runs two shell commands:

  (vc-git-command "fetch" ...
  (vc-git-command "log" ...

We need first to try this feature for more vc commands to see
if some filtering is needed to not edit secondary shell commands,
and then maybe allow customization via regexps or symbol properties
which commands need editing in case of multiple shell commands.

RE: [External] : Re: master 101f3cf5b9: Add support for user edits to VC command arguments

[Drew Adams]

> I thought that in Lisp we often call boolean values predicates too,

I do.  But I think it's become verboten for Emacs.

> even though that violates the usage of 'predicate' in mathematics.

No, it doesn't, actually.  A programming "variable"
can be likened to a nullary function.  That we also
have another kind of nullary function doesn't change
this.

(And a predicate "in mathematics" is not necessarily
a function at all.  There are several (related) uses
of the term.)

> Is there some other convention for booleans like this one?

I don't think I've seen one adopted by Emacs.  But
someone will correct me...

There is the `-flag' suffix convention for user
options (not other variables), which Stefan mentioned.
RMS promotes that.  I use it, but I may be the only
one who does.  I think it's helpful to have a separate
such suffix for options, even if that one is a bit long.

The argument Stefan makes against any such convention
is reasonable.  It's definitely the case that a var
often starts out as Boolean and later allows for more
values.  Renaming isn't such a big deal for user code.
But it's a pain for vanilla Emacs code (i.e., a big
user base).

This is a real consideration, no doubt.  But hey, this
is Lisp - schema-flexible, etc.  And we have aliasing.
Que demande le peuple ?

That worry/argument, BTW, doesn't apply to local vars,
e.g., let-bound vars and function parameters.  I often
use suffix `-p' in those contexts.  It's a very short
suffix, and it makes the code/intention clearer, IMHO.

Re: master 101f3cf5b9: Add support for user edits to VC command arguments

[Sean Whitton]

Hello,

On Thu 22 Sep 2022 at 11:25AM +02, Robert Pluim wrote:

>>>>>> On Wed, 21 Sep 2022 15:33:02 -0400 (EDT), Sean Whitton <spwhitton@spwhitton.name> said:
>
>
>     Sean> +(defvar vc-want-edit-command-p nil
>     Sean> +  "If non-nil, let user edit the VC shell command before running it.")
>     Sean> +
>     Sean>  ;; Common command execution logic
>
> Is the intent that someone would setq this to t? If so it should be a
> defcustom. If thatʼs not the intent, but itʼs just being used to track
> the prefix args to the various changed commands, then I think it
> shouldnʼt appear in the docstring.

It's the latter, it's not meant to be setq.

What exactly do you think shouldn't appear in the docstring?

-- 
Sean Whitton

Re: master 101f3cf5b9: Add support for user edits to VC command arguments

[Robert Pluim]

>>>>> On Thu, 22 Sep 2022 09:06:48 -0700, Sean Whitton <spwhitton@spwhitton.name> said:

    Sean> Hello,
    Sean> On Thu 22 Sep 2022 at 11:25AM +02, Robert Pluim wrote:

    >>>>>>> On Wed, 21 Sep 2022 15:33:02 -0400 (EDT), Sean Whitton <spwhitton@spwhitton.name> said:
    >> 
    >> 
    Sean> +(defvar vc-want-edit-command-p nil
    Sean> +  "If non-nil, let user edit the VC shell command before running it.")
    Sean> +
    Sean> ;; Common command execution logic
    >> 
    >> Is the intent that someone would setq this to t? If so it should be a
    >> defcustom. If thatʼs not the intent, but itʼs just being used to track
    >> the prefix args to the various changed commands, then I think it
    >> shouldnʼt appear in the docstring.

    Sean> It's the latter, it's not meant to be setq.

    Sean> What exactly do you think shouldn't appear in the docstring?

Any reference to `vc-want-edit-command-p', if it truly is just an
internal implementation detail.

Robert
-- 

Re: master 101f3cf5b9: Add support for user edits to VC command arguments

[Sean Whitton]

Hello,

On Fri 23 Sep 2022 at 10:03AM +02, Robert Pluim wrote:

>>>>>> On Thu, 22 Sep 2022 09:06:48 -0700, Sean Whitton <spwhitton@spwhitton.name> said:
>
>     Sean> Hello,
>     Sean> On Thu 22 Sep 2022 at 11:25AM +02, Robert Pluim wrote:
>
>     >>>>>>> On Wed, 21 Sep 2022 15:33:02 -0400 (EDT), Sean Whitton <spwhitton@spwhitton.name> said:
>     >>
>     >>
>     Sean> +(defvar vc-want-edit-command-p nil
>     Sean> +  "If non-nil, let user edit the VC shell command before running it.")
>     Sean> +
>     Sean> ;; Common command execution logic
>     >>
>     >> Is the intent that someone would setq this to t? If so it should be a
>     >> defcustom. If thatʼs not the intent, but itʼs just being used to track
>     >> the prefix args to the various changed commands, then I think it
>     >> shouldnʼt appear in the docstring.
>
>     Sean> It's the latter, it's not meant to be setq.
>
>     Sean> What exactly do you think shouldn't appear in the docstring?
>
> Any reference to `vc-want-edit-command-p', if it truly is just an
> internal implementation detail.

Well, I was thinking that vc-do-command is for people implementing other
vc-* functions, right?  They might want to bind it.

-- 
Sean Whitton

Re: master 101f3cf5b9: Add support for user edits to VC command arguments

[Stefan Monnier]

> @@ -296,8 +305,27 @@ FILE-OR-LIST is the name of a working file; it may be a list of
>  files or be nil (to execute commands that don't expect a file
>  name or set of files).  If an optional list of FLAGS is present,
>  that is inserted into the command line before the filename.
> +
> +If `vc-want-edit-command-p' is non-nil, prompt the user to edit
> +COMMAND and FLAGS before execution.

This prompting seems a bit too specific to particular callers of this
function, so I suggest you replace it with some kind of hook which takes
the command as arg and returns the command to use instead.  It should be
a `<foo>-function` with a default value of `identity`.

>  Return the return value of the slave command in the synchronous
>  case, and the process object in the asynchronous case."
> +  (when vc-want-edit-command-p
> +    (let* ((files-separator-p (string= "--" (car (last flags))))
> +           (edited (split-string-and-unquote
> +                    (read-shell-command
> +                     (format "Edit VC command & arguments%s: "
> +                             (if file-or-list
> +                                 " (files list to be appended)"
> +                               ""))
> +                     (combine-and-quote-strings
> +                      (cons command (remq nil (if files-separator-p
> +                                                  (butlast flags)
> +                                                flags))))))))
> +      (setq command (car edited)
> +            flags (nconc (cdr edited)
> +                         (and files-separator-p '("--"))))))

So this code can then be conveniently placed in a separate function that
callers can simply use in `<foo>-function` variable.

> @@ -327,6 +355,8 @@ case, and the process object in the asynchronous case."
>  		       (string= (buffer-name) buffer))
>  		  (eq buffer (current-buffer)))
>  	(vc-setup-buffer buffer))
> +      (run-hook-with-args 'vc-pre-command-functions
> +		          command file-or-list flags)
>        ;; If there's some previous async process still running, just kill it.
>        (let ((squeezed (remq nil flags))
>  	    (inhibit-read-only t)

Any chance this hook can be merged with the one I propose above?
If not, please clarify (in their doc) how&why they differ.

> @@ -386,22 +416,25 @@ Send the output to BUFFER, which should be a buffer or the name
>  of a buffer, which is created.
>  ROOT should be the directory in which the command should be run.
>  Display the buffer in some window, but don't select it."
> -  (let* ((dir default-directory)
> -	 (inhibit-read-only t)
> -	 window new-window-start)
> +  (letrec ((dir default-directory)
> +	   (inhibit-read-only t)
> +           (fun (lambda (command _ args)
> +                  (remove-hook 'vc-pre-command-functions fun)
> +                  (goto-char (point-max))
> +                  (unless (eq (point) (point-min))
> +	            (insert "\f\n"))
> +                  (setq new-window-start (point))
> +                  (insert "Running \"" command)
> +                  (dolist (arg args)
> +	            (insert " " arg))
> +                  (insert "\"...\n")))
> +	   (window nil) (new-window-start nil))
>      (setq buffer (get-buffer-create buffer))
>      (if (get-buffer-process buffer)
>  	(error "Another VC action on %s is running" root))
>      (with-current-buffer buffer
>        (setq default-directory root)
> -      (goto-char (point-max))
> -      (unless (eq (point) (point-min))
> -	(insert "\f\n"))
> -      (setq new-window-start (point))
> -      (insert "Running \"" command)
> -      (dolist (arg args)
> -	(insert " " arg))
> -      (insert "\"...\n")
> +      (add-hook 'vc-pre-command-functions fun)
>        ;; Run in the original working directory.
>        (let ((default-directory dir))
>  	(apply #'vc-do-command t 'async command nil args)))

Your commit message says:

    (vc-do-async-command): Use the new hook to insert into BUFFER the
    command that's next to be run.

but I don't understand what this changes does.
Wasn't it already inserted into BUFFER?  What's changed?

>  (defun vc-git--pushpull (command prompt extra-args)
>    "Run COMMAND (a string; either push or pull) on the current Git branch.
>  If PROMPT is non-nil, prompt for the Git command to run."
>    (let* ((root (vc-git-root default-directory))
>  	 (buffer (format "*vc-git : %s*" (expand-file-name root)))
> -	 (git-program vc-git-program)
> -	 args)
> -    ;; If necessary, prompt for the exact command.
> -    ;; TODO if pushing, prompt if no default push location - cf bzr.
> -    (when prompt
> -      (setq args (split-string
> -		  (read-shell-command
> -                   (format "Git %s command: " command)
> -                   (format "%s %s" git-program command)
> -                   'vc-git-history)
> -		  " " t))
> -      (setq git-program (car  args)
> -	    command     (cadr args)
> -	    args        (cddr args)))
> -    (setq args (nconc args extra-args))
> +         ;; TODO if pushing, prompt if no default push location - cf bzr.
> +         (vc-want-edit-command-p prompt))
>      (require 'vc-dispatcher)
> -    (apply #'vc-do-async-command buffer root git-program command args)
> +    (when vc-want-edit-command-p
> +      (with-current-buffer (get-buffer-create buffer)
> +        (add-hook 'vc-pre-command-functions
> +                  (pcase-lambda (_ _ `(,new-command . ,new-args))
> +                    (setq command new-command extra-args new-args))
> +                  nil t)))
> +    (apply #'vc-do-async-command
> +           buffer root vc-git-program command extra-args)
>      (with-current-buffer buffer
>        (vc-run-delayed
>          (vc-compilation-mode 'git)
>          (setq-local compile-command
> -                    (concat git-program " " command " "
> -                            (mapconcat #'identity args " ")))
> +                    (concat vc-git-program " " command " "
> +                            (mapconcat #'identity extra-args " ")))
>          (setq-local compilation-directory root)
>          ;; Either set `compilation-buffer-name-function' locally to nil
>          ;; or use `compilation-arguments' to set `name-function'.

IIUC the (git-program vc-git-program) binding allowed that var to have
different values in different buffers.  This change instead presumes
`vc-git-program` is the same in all buffers.  Not sure it matters, but
we've been bitten by similar things in VC.


        Stefan

Re: master 101f3cf5b9: Add support for user edits to VC command arguments

[Sean Whitton]

Hello Stefan,

Thank you for reviewing.

On Thu 22 Sep 2022 at 02:45PM -04, Stefan Monnier wrote:

>> @@ -296,8 +305,27 @@ FILE-OR-LIST is the name of a working file; it may be a list of
>>  files or be nil (to execute commands that don't expect a file
>>  name or set of files).  If an optional list of FLAGS is present,
>>  that is inserted into the command line before the filename.
>> +
>> +If `vc-want-edit-command-p' is non-nil, prompt the user to edit
>> +COMMAND and FLAGS before execution.
>
> This prompting seems a bit too specific to particular callers of this
> function, so I suggest you replace it with some kind of hook which takes
> the command as arg and returns the command to use instead.  It should be
> a `<foo>-function` with a default value of `identity`.

Each command that uses this would have to ensure that the prompting
function is removed again once used, right?  How would you suggest
handling that?  Perhaps we could have a macro doing something like what
we see in vc-exec-after, `add-once-function' or something.

>> @@ -327,6 +355,8 @@ case, and the process object in the asynchronous case."
>>  		       (string= (buffer-name) buffer))
>>  		  (eq buffer (current-buffer)))
>>  	(vc-setup-buffer buffer))
>> +      (run-hook-with-args 'vc-pre-command-functions
>> +		          command file-or-list flags)
>>        ;; If there's some previous async process still running, just kill it.
>>        (let ((squeezed (remq nil flags))
>>  	    (inhibit-read-only t)
>
> Any chance this hook can be merged with the one I propose above?
> If not, please clarify (in their doc) how&why they differ.

Possibly vc-do-async-command and vc-git--pushpull can invoke the
prompting function themselves, thereby obtaining the new command and
arguments, and then add-function a constant function so that
vc-do-command gets the new values.  Is that something like what you have
in mind?

> Your commit message says:
>
>     (vc-do-async-command): Use the new hook to insert into BUFFER the
>     command that's next to be run.
>
> but I don't understand what this changes does.
> Wasn't it already inserted into BUFFER?  What's changed?

Previously it was inserted without going via the hook.

>>  (defun vc-git--pushpull (command prompt extra-args)
>>    "Run COMMAND (a string; either push or pull) on the current Git branch.
>>  If PROMPT is non-nil, prompt for the Git command to run."
>>    (let* ((root (vc-git-root default-directory))
>>  	 (buffer (format "*vc-git : %s*" (expand-file-name root)))
>> -	 (git-program vc-git-program)
>> -	 args)
>> -    ;; If necessary, prompt for the exact command.
>> -    ;; TODO if pushing, prompt if no default push location - cf bzr.
>> -    (when prompt
>> -      (setq args (split-string
>> -		  (read-shell-command
>> -                   (format "Git %s command: " command)
>> -                   (format "%s %s" git-program command)
>> -                   'vc-git-history)
>> -		  " " t))
>> -      (setq git-program (car  args)
>> -	    command     (cadr args)
>> -	    args        (cddr args)))
>> -    (setq args (nconc args extra-args))
>> +         ;; TODO if pushing, prompt if no default push location - cf bzr.
>> +         (vc-want-edit-command-p prompt))
>>      (require 'vc-dispatcher)
>> -    (apply #'vc-do-async-command buffer root git-program command args)
>> +    (when vc-want-edit-command-p
>> +      (with-current-buffer (get-buffer-create buffer)
>> +        (add-hook 'vc-pre-command-functions
>> +                  (pcase-lambda (_ _ `(,new-command . ,new-args))
>> +                    (setq command new-command extra-args new-args))
>> +                  nil t)))
>> +    (apply #'vc-do-async-command
>> +           buffer root vc-git-program command extra-args)
>>      (with-current-buffer buffer
>>        (vc-run-delayed
>>          (vc-compilation-mode 'git)
>>          (setq-local compile-command
>> -                    (concat git-program " " command " "
>> -                            (mapconcat #'identity args " ")))
>> +                    (concat vc-git-program " " command " "
>> +                            (mapconcat #'identity extra-args " ")))
>>          (setq-local compilation-directory root)
>>          ;; Either set `compilation-buffer-name-function' locally to nil
>>          ;; or use `compilation-arguments' to set `name-function'.
>
> IIUC the (git-program vc-git-program) binding allowed that var to have
> different values in different buffers.  This change instead presumes
> `vc-git-program` is the same in all buffers.  Not sure it matters, but
> we've been bitten by similar things in VC.

It's only the use of vc-git-program in compile-command that now works
differently, right?  The value passed to vc-do-async-command should
still be the same buffer-local value if there is one.

-- 
Sean Whitton

Re: master 101f3cf5b9: Add support for user edits to VC command arguments

[Stefan Monnier]

>> This prompting seems a bit too specific to particular callers of this
>> function, so I suggest you replace it with some kind of hook which takes
>> the command as arg and returns the command to use instead.  It should be
>> a `<foo>-function` with a default value of `identity`.
>
> Each command that uses this would have to ensure that the prompting
> function is removed again once used, right?  How would you suggest
> handling that?  Perhaps we could have a macro doing something like what
> we see in vc-exec-after, `add-once-function' or something.

I was thinking of having the caller simply let-bind the variable, or, as
your code does, have the function set the var's value back to
`identity`.

>>> @@ -327,6 +355,8 @@ case, and the process object in the asynchronous case."
>>>  		       (string= (buffer-name) buffer))
>>>  		  (eq buffer (current-buffer)))
>>>  	(vc-setup-buffer buffer))
>>> +      (run-hook-with-args 'vc-pre-command-functions
>>> +		          command file-or-list flags)
>>>        ;; If there's some previous async process still running, just kill it.
>>>        (let ((squeezed (remq nil flags))
>>>  	    (inhibit-read-only t)
>>
>> Any chance this hook can be merged with the one I propose above?
>> If not, please clarify (in their doc) how&why they differ.
>
> Possibly vc-do-async-command and vc-git--pushpull can invoke the
> prompting function themselves, thereby obtaining the new command and
> arguments, and then add-function a constant function so that
> vc-do-command gets the new values.  Is that something like what you have
> in mind?

I don't have anything in mind, I just see that you added two "hooks",
one specifically for prompting the user to modify the command and another
more open ended.  Currently neither subsumes the other, but I wonder if
we couldn't provide a single hook that could satisfy both needs.

>> Your commit message says:
>>
>>     (vc-do-async-command): Use the new hook to insert into BUFFER the
>>     command that's next to be run.
>>
>> but I don't understand what this changes does.
>> Wasn't it already inserted into BUFFER?  What's changed?
>
> Previously it was inserted without going via the hook.

But why did you change the code so it goes via the hook?

>> IIUC the (git-program vc-git-program) binding allowed that var to have
>> different values in different buffers.  This change instead presumes
>> `vc-git-program` is the same in all buffers.  Not sure it matters, but
>> we've been bitten by similar things in VC.
>
> It's only the use of vc-git-program in compile-command that now works
> differently, right?  The value passed to vc-do-async-command should
> still be the same buffer-local value if there is one.

I think so, yes.


        Stefan

Re: master 101f3cf5b9: Add support for user edits to VC command arguments

[Sean Whitton]

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

Hello,

On Thu 22 Sep 2022 at 06:41PM -04, Stefan Monnier wrote:

>>> This prompting seems a bit too specific to particular callers of this
>>> function, so I suggest you replace it with some kind of hook which takes
>>> the command as arg and returns the command to use instead.  It should be
>>> a `<foo>-function` with a default value of `identity`.
>>
>> Each command that uses this would have to ensure that the prompting
>> function is removed again once used, right?  How would you suggest
>> handling that?  Perhaps we could have a macro doing something like what
>> we see in vc-exec-after, `add-once-function' or something.
>
> I was thinking of having the caller simply let-bind the variable, or, as
> your code does, have the function set the var's value back to
> `identity`.
>

[...]

>
> I don't have anything in mind, I just see that you added two "hooks",
> one specifically for prompting the user to modify the command and another
> more open ended.  Currently neither subsumes the other, but I wonder if
> we couldn't provide a single hook that could satisfy both needs.

I'm attaching a refactoring attempt for review.

>>> Your commit message says:
>>>
>>>     (vc-do-async-command): Use the new hook to insert into BUFFER the
>>>     command that's next to be run.
>>>
>>> but I don't understand what this changes does.
>>> Wasn't it already inserted into BUFFER?  What's changed?
>>
>> Previously it was inserted without going via the hook.
>
> But why did you change the code so it goes via the hook?

So that it uses the user-updated value of the command, rather than the
original value.  Hopefully that is clearer in the attached.

-- 
Sean Whitton

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-vc-git-pushpull-Restore-handling-of-vc-git-program.patch --]
[-- Type: text/x-patch, Size: 2151 bytes --]

From b0fcba8791a1702a41df0d4f12bb47d151eec5b9 Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Fri, 23 Sep 2022 10:43:31 -0700
Subject: [PATCH 1/2] vc-git--pushpull: Restore handling of vc-git-program

* lisp/vc/vc-git.el (vc-git--pushpull): Restore handling of
vc-git-program before recent change: respect a buffer-local value of
vc-git-program, and don't ignore user edits to the git program name
when PROMPT.
---
 lisp/vc/vc-git.el | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 2228cf8665..3816d323e6 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1096,22 +1096,25 @@ vc-git--pushpull
 If PROMPT is non-nil, prompt for the Git command to run."
   (let* ((root (vc-git-root default-directory))
 	 (buffer (format "*vc-git : %s*" (expand-file-name root)))
+         (git-program vc-git-program)
          ;; TODO if pushing, prompt if no default push location - cf bzr.
          (vc-want-edit-command-p prompt))
     (require 'vc-dispatcher)
     (when vc-want-edit-command-p
       (with-current-buffer (get-buffer-create buffer)
         (add-hook 'vc-pre-command-functions
-                  (pcase-lambda (_ _ `(,new-command . ,new-args))
-                    (setq command new-command extra-args new-args))
+                  (lambda (&rest args)
+                    (setq git-program (car args)
+                          command (caaddr args)
+                          extra-args (cdaddr args)))
                   nil t)))
     (apply #'vc-do-async-command
-           buffer root vc-git-program command extra-args)
+           buffer root git-program command extra-args)
     (with-current-buffer buffer
       (vc-run-delayed
         (vc-compilation-mode 'git)
         (setq-local compile-command
-                    (concat vc-git-program " " command " "
+                    (concat git-program " " command " "
                             (mapconcat #'identity extra-args " ")))
         (setq-local compilation-directory root)
         ;; Either set `compilation-buffer-name-function' locally to nil
-- 
2.30.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Rework-user-edits-to-VC-commands-to-use-a-more-funct.patch --]
[-- Type: text/x-patch, Size: 16106 bytes --]

From 594ff0b4bdad1f756614556f3487b98ad632c617 Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Fri, 23 Sep 2022 10:09:34 -0700
Subject: [PATCH 2/2] Rework user edits to VC commands to use a more functional
 style

* lisp/vc/vc-dispatcher.el (vc-pre-command-functions): Delete.
(vc-filter-command-function): New variable.
(vc-user-edit-command): Factor out of vc-do-command.
(vc-do-command, vc-do-async-command):
* lisp/vc/vc-git.el (vc-git--pushpull):
* lisp/vc/vc.el (vc-print-branch-log): Use vc-filter-command-function
in place of vc-pre-command-functions and vc-pre-command-functions.
---
 lisp/vc/vc-dispatcher.el | 250 +++++++++++++++++++++------------------
 lisp/vc/vc-git.el        |  21 ++--
 lisp/vc/vc.el            |   4 +-
 3 files changed, 147 insertions(+), 128 deletions(-)

diff --git a/lisp/vc/vc-dispatcher.el b/lisp/vc/vc-dispatcher.el
index 459c2ae103..b931184e35 100644
--- a/lisp/vc/vc-dispatcher.el
+++ b/lisp/vc/vc-dispatcher.el
@@ -109,6 +109,8 @@
 ;; TODO:
 ;; - log buffers need font-locking.
 
+(require 'cl-lib)
+
 ;; General customization
 
 (defcustom vc-logentry-check-hook nil
@@ -265,11 +267,12 @@ vc-run-delayed
   (declare (indent 0) (debug (def-body)))
   `(vc-exec-after (lambda () ,@body)))
 
-(defvar vc-pre-command-functions nil
-  "Hook run at the beginning of `vc-do-command'.
-Each function is called inside the buffer in which the command
-will be run and is passed 3 arguments: the COMMAND, the FILES and
-the FLAGS.")
+(defvar vc-filter-command-function (lambda (&rest args) args)
+  "Function called to transform VC commands before execution.
+The function is called inside the buffer in which the command
+will be run and is passed the COMMAND, FILE-OR-LIST and FLAGS
+arguments to `vc-do-command'.  It should return a list with the
+same structure containing new values for these arguments.")
 
 (defvar vc-post-command-functions nil
   "Hook run at the end of `vc-do-command'.
@@ -291,6 +294,23 @@ vc-tor
   :version "27.1"
   :group 'vc)
 
+(defun vc-user-edit-command (command file-or-list &rest flags)
+  "Prompt the user to edit VC command COMMAND and FLAGS.
+Intended to be used as the value of `vc-filter-command-function'."
+  (let* ((files-separator-p (string= "--" (car (last flags))))
+         (edited (split-string-and-unquote
+                  (read-shell-command
+                   (format "Edit VC command & arguments%s: "
+                           (if file-or-list
+                               " (files list to be appended)"
+                             ""))
+                   (combine-and-quote-strings
+                    (cons command (remq nil (if files-separator-p
+                                                (butlast flags)
+                                              flags))))))))
+    (cl-list* (car edited) file-or-list
+              (nconc (cdr edited) (and files-separator-p '("--"))))))
+
 ;;;###autoload
 (defun vc-do-command (buffer okstatus command file-or-list &rest flags)
   "Execute a slave command, notifying user and checking for errors.
@@ -306,109 +326,102 @@ vc-do-command
 name or set of files).  If an optional list of FLAGS is present,
 that is inserted into the command line before the filename.
 
-If `vc-want-edit-command-p' is non-nil, prompt the user to edit
-COMMAND and FLAGS before execution.
-
 Return the return value of the slave command in the synchronous
 case, and the process object in the asynchronous case."
-  (when vc-want-edit-command-p
-    (let* ((files-separator-p (string= "--" (car (last flags))))
-           (edited (split-string-and-unquote
-                    (read-shell-command
-                     (format "Edit VC command & arguments%s: "
-                             (if file-or-list
-                                 " (files list to be appended)"
-                               ""))
-                     (combine-and-quote-strings
-                      (cons command (remq nil (if files-separator-p
-                                                  (butlast flags)
-                                                flags))))))))
-      (setq command (car edited)
-            flags (nconc (cdr edited)
-                         (and files-separator-p '("--"))))))
-  (when vc-tor
-    (push command flags)
-    (setq command "torsocks"))
-  ;; FIXME: file-relative-name can return a bogus result because
-  ;; it doesn't look at the actual file-system to see if symlinks
-  ;; come into play.
-  (let* ((files
-	  (mapcar (lambda (f) (file-relative-name (expand-file-name f)))
-		  (if (listp file-or-list) file-or-list (list file-or-list))))
-	 ;; Keep entire commands in *Messages* but avoid resizing the
-	 ;; echo area.  Messages in this function are formatted in
-	 ;; a such way that the important parts are at the beginning,
-	 ;; due to potential truncation of long messages.
-	 (message-truncate-lines t)
-	 (full-command
-	  (concat (if (string= (substring command -1) "\n")
-		      (substring command 0 -1)
-		    command)
-		  " " (vc-delistify flags)
-		  " " (vc-delistify files)))
-	 (vc-inhibit-message
-	  (or (eq vc-command-messages 'log)
-	      (eq (selected-window) (active-minibuffer-window)))))
+  (let (;; Keep entire commands in *Messages* but avoid resizing the
+	;; echo area.  Messages in this function are formatted in
+	;; a such way that the important parts are at the beginning,
+	;; due to potential truncation of long messages.
+	(message-truncate-lines t)
+        (vc-inhibit-message
+	 (or (eq vc-command-messages 'log)
+	     (eq (selected-window) (active-minibuffer-window)))))
     (save-current-buffer
       (unless (or (eq buffer t)
 		  (and (stringp buffer)
 		       (string= (buffer-name) buffer))
 		  (eq buffer (current-buffer)))
-	(vc-setup-buffer buffer))
-      (run-hook-with-args 'vc-pre-command-functions
-		          command file-or-list flags)
-      ;; If there's some previous async process still running, just kill it.
-      (let ((squeezed (remq nil flags))
-	    (inhibit-read-only t)
-	    (status 0))
-	(when files
-	  (setq squeezed (nconc squeezed files)))
-	(let (;; Since some functions need to parse the output
-	      ;; from external commands, set LC_MESSAGES to C.
-	      (process-environment (cons "LC_MESSAGES=C" process-environment))
-	      (w32-quote-process-args t))
-	  (if (eq okstatus 'async)
-	      ;; Run asynchronously.
-	      (let ((proc
-		     (let ((process-connection-type nil))
-		       (apply #'start-file-process command (current-buffer)
-                              command squeezed))))
-		(when vc-command-messages
-		  (let ((inhibit-message vc-inhibit-message))
-		    (message "Running in background: %s" full-command)))
-                ;; Get rid of the default message insertion, in case we don't
-                ;; set a sentinel explicitly.
-		(set-process-sentinel proc #'ignore)
-		(set-process-filter proc #'vc-process-filter)
-		(setq status proc)
-		(when vc-command-messages
-		  (vc-run-delayed
-		    (let ((message-truncate-lines t)
-			  (inhibit-message vc-inhibit-message))
-		      (message "Done in background: %s" full-command)))))
-	    ;; Run synchronously
-	    (when vc-command-messages
-	      (let ((inhibit-message vc-inhibit-message))
-		(message "Running in foreground: %s" full-command)))
-	    (let ((buffer-undo-list t))
-	      (setq status (apply #'process-file command nil t nil squeezed)))
-	    (when (and (not (eq t okstatus))
-		       (or (not (integerp status))
-			   (and okstatus (< okstatus status))))
-              (unless (eq ?\s (aref (buffer-name (current-buffer)) 0))
-                (pop-to-buffer (current-buffer))
-                (goto-char (point-min))
-                (shrink-window-if-larger-than-buffer))
-	      (error "Failed (%s): %s"
-		     (if (integerp status) (format "status %d" status) status)
-		     full-command))
-	    (when vc-command-messages
-	      (let ((inhibit-message vc-inhibit-message))
-		(message "Done (status=%d): %s" status full-command)))))
-	(vc-run-delayed
-	  (run-hook-with-args 'vc-post-command-functions
-			      command file-or-list flags))
-	status))))
+        (vc-setup-buffer buffer))
+      (cl-destructuring-bind (command file-or-list &rest flags)
+          (apply vc-filter-command-function command file-or-list flags)
+        (when vc-tor
+          (push command flags)
+          (setq command "torsocks"))
+        (let* (;; FIXME: file-relative-name can return a bogus result
+               ;; because it doesn't look at the actual file-system to
+               ;; see if symlinks come into play.
+               (files
+	        (mapcar (lambda (f)
+                          (file-relative-name (expand-file-name f)))
+		        (if (listp file-or-list)
+                            file-or-list
+                          (list file-or-list))))
+	       (full-command
+	        (concat (if (string= (substring command -1) "\n")
+		            (substring command 0 -1)
+		          command)
+		        " " (vc-delistify flags)
+		        " " (vc-delistify files)))
+               (squeezed (remq nil flags))
+	       (inhibit-read-only t)
+	       (status 0))
+          ;; If there's some previous async process still running,
+          ;; just kill it.
+          (when files
+	    (setq squeezed (nconc squeezed files)))
+	  (let (;; Since some functions need to parse the output
+	        ;; from external commands, set LC_MESSAGES to C.
+	        (process-environment
+                 (cons "LC_MESSAGES=C" process-environment))
+	        (w32-quote-process-args t))
+	    (if (eq okstatus 'async)
+	        ;; Run asynchronously.
+	        (let ((proc
+		       (let ((process-connection-type nil))
+		         (apply #'start-file-process command
+                                (current-buffer) command squeezed))))
+		  (when vc-command-messages
+		    (let ((inhibit-message vc-inhibit-message))
+		      (message "Running in background: %s"
+                               full-command)))
+                  ;; Get rid of the default message insertion, in case
+                  ;; we don't set a sentinel explicitly.
+		  (set-process-sentinel proc #'ignore)
+		  (set-process-filter proc #'vc-process-filter)
+		  (setq status proc)
+		  (when vc-command-messages
+		    (vc-run-delayed
+		      (let ((message-truncate-lines t)
+			    (inhibit-message vc-inhibit-message))
+		        (message "Done in background: %s"
+                                 full-command)))))
+	      ;; Run synchronously
+	      (when vc-command-messages
+	        (let ((inhibit-message vc-inhibit-message))
+		  (message "Running in foreground: %s" full-command)))
+	      (let ((buffer-undo-list t))
+	        (setq status (apply #'process-file
+                                    command nil t nil squeezed)))
+	      (when (and (not (eq t okstatus))
+		         (or (not (integerp status))
+			     (and okstatus (< okstatus status))))
+                (unless (eq ?\s (aref (buffer-name (current-buffer)) 0))
+                  (pop-to-buffer (current-buffer))
+                  (goto-char (point-min))
+                  (shrink-window-if-larger-than-buffer))
+	        (error "Failed (%s): %s"
+		       (if (integerp status)
+                           (format "status %d" status)
+                         status)
+		       full-command))
+	      (when vc-command-messages
+	        (let ((inhibit-message vc-inhibit-message))
+		  (message "Done (status=%d): %s"
+                           status full-command)))))
+	  (vc-run-delayed
+	    (run-hook-with-args 'vc-post-command-functions
+			        command file-or-list flags))
+	  status)))))
 
 (defun vc-do-async-command (buffer root command &rest args)
   "Run COMMAND asynchronously with ARGS, displaying the result.
@@ -416,27 +429,30 @@ vc-do-async-command
 of a buffer, which is created.
 ROOT should be the directory in which the command should be run.
 Display the buffer in some window, but don't select it."
-  (letrec ((dir default-directory)
-	   (inhibit-read-only t)
-           (fun (lambda (command _ args)
-                  (remove-hook 'vc-pre-command-functions fun)
-                  (goto-char (point-max))
-                  (unless (eq (point) (point-min))
-	            (insert "\f\n"))
-                  (setq new-window-start (point))
-                  (insert "Running \"" command)
-                  (dolist (arg args)
-	            (insert " " arg))
-                  (insert "\"...\n")))
-	   (window nil) (new-window-start nil))
+  (let ((dir default-directory)
+	(inhibit-read-only t)
+	window new-window-start)
     (setq buffer (get-buffer-create buffer))
     (if (get-buffer-process buffer)
 	(error "Another VC action on %s is running" root))
     (with-current-buffer buffer
       (setq default-directory root)
-      (add-hook 'vc-pre-command-functions fun)
-      ;; Run in the original working directory.
-      (let ((default-directory dir))
+      (let* (;; Run in the original working directory.
+             (default-directory dir)
+             (orig-fun vc-filter-command-function)
+             (vc-filter-command-function
+              (lambda (&rest args)
+                (cl-destructuring-bind (&whole args cmd _ &rest flags)
+                    (apply orig-fun args)
+                  (goto-char (point-max))
+                  (unless (eq (point) (point-min))
+	            (insert "\f\n"))
+                  (setq new-window-start (point))
+                  (insert "Running \"" cmd)
+                  (dolist (flag flags)
+	            (insert " " flag))
+                  (insert "\"...\n")
+                  args))))
 	(apply #'vc-do-command t 'async command nil args)))
     (setq window (display-buffer buffer))
     (if window
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 3816d323e6..e7acfb0122 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1094,20 +1094,21 @@ vc-want-edit-command-p
 (defun vc-git--pushpull (command prompt extra-args)
   "Run COMMAND (a string; either push or pull) on the current Git branch.
 If PROMPT is non-nil, prompt for the Git command to run."
+  (require 'vc-dispatcher)
   (let* ((root (vc-git-root default-directory))
 	 (buffer (format "*vc-git : %s*" (expand-file-name root)))
          (git-program vc-git-program)
          ;; TODO if pushing, prompt if no default push location - cf bzr.
-         (vc-want-edit-command-p prompt))
-    (require 'vc-dispatcher)
-    (when vc-want-edit-command-p
-      (with-current-buffer (get-buffer-create buffer)
-        (add-hook 'vc-pre-command-functions
-                  (lambda (&rest args)
-                    (setq git-program (car args)
-                          command (caaddr args)
-                          extra-args (cdaddr args)))
-                  nil t)))
+         (vc-filter-command-function
+          (if prompt
+              (lambda (&rest args)
+                (cl-destructuring-bind (&whole args git _ &rest flags)
+                    (apply #'vc-user-edit-command args)
+                  (setq git-program git
+                        command (car flags)
+                        extra-args (cdr flags))
+                  args))
+            vc-filter-command-function)))
     (apply #'vc-do-async-command
            buffer root git-program command extra-args)
     (with-current-buffer buffer
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index a45e0e0c52..c7e120ee7d 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -2764,7 +2764,9 @@ vc-print-branch-log
     (error "No branch specified"))
   (let* ((backend (vc-responsible-backend default-directory))
          (rootdir (vc-call-backend backend 'root default-directory))
-         (vc-want-edit-command-p arg))
+         (vc-filter-command-function (if arg
+                                         #'vc-user-edit-command
+                                       vc-filter-command-function)))
     (vc-print-log-internal backend
                            (list rootdir) branch t
                            (when (> vc-log-show-limit 0) vc-log-show-limit))))
-- 
2.30.2

Re: master 101f3cf5b9: Add support for user edits to VC command arguments

[Stefan Monnier]

>>> Previously it was inserted without going via the hook.
>> But why did you change the code so it goes via the hook?
> So that it uses the user-updated value of the command,

Ah, that makes sense.

> From b0fcba8791a1702a41df0d4f12bb47d151eec5b9 Mon Sep 17 00:00:00 2001
> From: Sean Whitton <spwhitton@spwhitton.name>
> Date: Fri, 23 Sep 2022 10:43:31 -0700
> Subject: [PATCH 1/2] vc-git--pushpull: Restore handling of vc-git-program
>
> * lisp/vc/vc-git.el (vc-git--pushpull): Restore handling of
> vc-git-program before recent change: respect a buffer-local value of
> vc-git-program, and don't ignore user edits to the git program name
> when PROMPT.

LGTM.

> From 594ff0b4bdad1f756614556f3487b98ad632c617 Mon Sep 17 00:00:00 2001
> From: Sean Whitton <spwhitton@spwhitton.name>
> Date: Fri, 23 Sep 2022 10:09:34 -0700
> Subject: [PATCH 2/2] Rework user edits to VC commands to use a more functional
>  style
>
> * lisp/vc/vc-dispatcher.el (vc-pre-command-functions): Delete.
> (vc-filter-command-function): New variable.
> (vc-user-edit-command): Factor out of vc-do-command.
> (vc-do-command, vc-do-async-command):
> * lisp/vc/vc-git.el (vc-git--pushpull):
> * lisp/vc/vc.el (vc-print-branch-log): Use vc-filter-command-function
> in place of vc-pre-command-functions and vc-pre-command-functions.

IIUC you could also remove the `vc-want-edit-command-p` variable, right?

> +(defvar vc-filter-command-function (lambda (&rest args) args)
> +  "Function called to transform VC commands before execution.
> +The function is called inside the buffer in which the command
> +will be run and is passed the COMMAND, FILE-OR-LIST and FLAGS
> +arguments to `vc-do-command'.  It should return a list with the
> +same structure containing new values for these arguments.")

That makes it sound like it receives 3 args (which sounds fine to me).

> +(defun vc-user-edit-command (command file-or-list &rest flags)

But these aren't 3 args, it's "2 or more".
So the doc and the code don't match.

BTW, `apply` and `&rest` are noticeably less efficient than `funcall`
and normal args.  Efficiency admittedly doesn't matter much here, but
I'd recommend we don't needlessly use apply/&rest (i.e. I recommend we
fix the code to match the doc than the other way around).


        Stefan

Re: master 101f3cf5b9: Add support for user edits to VC command arguments

[Sean Whitton]

Hello,

On Fri 23 Sep 2022 at 03:32PM -04, Stefan Monnier wrote:

>>>> Previously it was inserted without going via the hook.
>>> But why did you change the code so it goes via the hook?
>> So that it uses the user-updated value of the command,
>
> Ah, that makes sense.
>
>> From b0fcba8791a1702a41df0d4f12bb47d151eec5b9 Mon Sep 17 00:00:00 2001
>> From: Sean Whitton <spwhitton@spwhitton.name>
>> Date: Fri, 23 Sep 2022 10:43:31 -0700
>> Subject: [PATCH 1/2] vc-git--pushpull: Restore handling of vc-git-program
>>
>> * lisp/vc/vc-git.el (vc-git--pushpull): Restore handling of
>> vc-git-program before recent change: respect a buffer-local value of
>> vc-git-program, and don't ignore user edits to the git program name
>> when PROMPT.
>
> LGTM.

Thanks.

>> From 594ff0b4bdad1f756614556f3487b98ad632c617 Mon Sep 17 00:00:00 2001
>> From: Sean Whitton <spwhitton@spwhitton.name>
>> Date: Fri, 23 Sep 2022 10:09:34 -0700
>> Subject: [PATCH 2/2] Rework user edits to VC commands to use a more functional
>>  style
>>
>> * lisp/vc/vc-dispatcher.el (vc-pre-command-functions): Delete.
>> (vc-filter-command-function): New variable.
>> (vc-user-edit-command): Factor out of vc-do-command.
>> (vc-do-command, vc-do-async-command):
>> * lisp/vc/vc-git.el (vc-git--pushpull):
>> * lisp/vc/vc.el (vc-print-branch-log): Use vc-filter-command-function
>> in place of vc-pre-command-functions and vc-pre-command-functions.
>
> IIUC you could also remove the `vc-want-edit-command-p` variable, right?

Yeah the changelog is wrong, thanks.

>> +(defvar vc-filter-command-function (lambda (&rest args) args)
>> +  "Function called to transform VC commands before execution.
>> +The function is called inside the buffer in which the command
>> +will be run and is passed the COMMAND, FILE-OR-LIST and FLAGS
>> +arguments to `vc-do-command'.  It should return a list with the
>> +same structure containing new values for these arguments.")
>
> That makes it sound like it receives 3 args (which sounds fine to me).
>
>> +(defun vc-user-edit-command (command file-or-list &rest flags)
>
> But these aren't 3 args, it's "2 or more".
> So the doc and the code don't match.
>
> BTW, `apply` and `&rest` are noticeably less efficient than `funcall`
> and normal args.  Efficiency admittedly doesn't matter much here, but
> I'd recommend we don't needlessly use apply/&rest (i.e. I recommend we
> fix the code to match the doc than the other way around).

I was thinking that having a lambda list consonant with vc-do-command
and vc-do-async-command was more important here.  Fits with how it acts
a filter on their lambda lists.

-- 
Sean Whitton