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