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