all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* Re: master 101f3cf5b9: Add support for user edits to VC command arguments
       [not found] ` <20220921193303.1B687C11D81@vcs2.savannah.gnu.org>
@ 2022-09-22  9:25   ` Robert Pluim
  2022-09-22 13:24     ` Stefan Monnier
  2022-09-22 16:06     ` Sean Whitton
  2022-09-22 18:45   ` Stefan Monnier
  1 sibling, 2 replies; 16+ messages in thread
From: Robert Pluim @ 2022-09-22  9:25 UTC (permalink / raw)
  To: emacs-devel; +Cc: Sean Whitton

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



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

* Re: master 101f3cf5b9: Add support for user edits to VC command arguments
  2022-09-22  9:25   ` master 101f3cf5b9: Add support for user edits to VC command arguments Robert Pluim
@ 2022-09-22 13:24     ` Stefan Monnier
  2022-09-22 16:08       ` Sean Whitton
  2022-09-22 16:06     ` Sean Whitton
  1 sibling, 1 reply; 16+ messages in thread
From: Stefan Monnier @ 2022-09-22 13:24 UTC (permalink / raw)
  To: Robert Pluim; +Cc: emacs-devel, Sean Whitton

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




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

* Re: master 101f3cf5b9: Add support for user edits to VC command arguments
  2022-09-22  9:25   ` master 101f3cf5b9: Add support for user edits to VC command arguments Robert Pluim
  2022-09-22 13:24     ` Stefan Monnier
@ 2022-09-22 16:06     ` Sean Whitton
  2022-09-23  8:03       ` Robert Pluim
  1 sibling, 1 reply; 16+ messages in thread
From: Sean Whitton @ 2022-09-22 16:06 UTC (permalink / raw)
  To: Robert Pluim; +Cc: emacs-devel

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



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

* Re: master 101f3cf5b9: Add support for user edits to VC command arguments
  2022-09-22 13:24     ` Stefan Monnier
@ 2022-09-22 16:08       ` Sean Whitton
  2022-09-22 16:55         ` Stefan Monnier
  2022-09-22 22:21         ` [External] : " Drew Adams
  0 siblings, 2 replies; 16+ messages in thread
From: Sean Whitton @ 2022-09-22 16:08 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Robert Pluim, emacs-devel

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



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

* Re: master 101f3cf5b9: Add support for user edits to VC command arguments
  2022-09-22 16:08       ` Sean Whitton
@ 2022-09-22 16:55         ` Stefan Monnier
  2022-09-22 21:18           ` Sean Whitton
  2022-09-22 22:21         ` [External] : " Drew Adams
  1 sibling, 1 reply; 16+ messages in thread
From: Stefan Monnier @ 2022-09-22 16:55 UTC (permalink / raw)
  To: Sean Whitton; +Cc: Robert Pluim, emacs-devel

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




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

* Re: master 101f3cf5b9: Add support for user edits to VC command arguments
       [not found] ` <20220921193303.1B687C11D81@vcs2.savannah.gnu.org>
  2022-09-22  9:25   ` master 101f3cf5b9: Add support for user edits to VC command arguments Robert Pluim
@ 2022-09-22 18:45   ` Stefan Monnier
  2022-09-22 21:38     ` Sean Whitton
  1 sibling, 1 reply; 16+ messages in thread
From: Stefan Monnier @ 2022-09-22 18:45 UTC (permalink / raw)
  To: emacs-devel; +Cc: Sean Whitton

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




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

* Re: master 101f3cf5b9: Add support for user edits to VC command arguments
  2022-09-22 16:55         ` Stefan Monnier
@ 2022-09-22 21:18           ` Sean Whitton
  2022-09-23  6:39             ` Juri Linkov
  0 siblings, 1 reply; 16+ messages in thread
From: Sean Whitton @ 2022-09-22 21:18 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Robert Pluim, emacs-devel

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



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

* Re: master 101f3cf5b9: Add support for user edits to VC command arguments
  2022-09-22 18:45   ` Stefan Monnier
@ 2022-09-22 21:38     ` Sean Whitton
  2022-09-22 22:41       ` Stefan Monnier
  0 siblings, 1 reply; 16+ messages in thread
From: Sean Whitton @ 2022-09-22 21:38 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

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



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

* RE: [External] : Re: master 101f3cf5b9: Add support for user edits to VC command arguments
  2022-09-22 16:08       ` Sean Whitton
  2022-09-22 16:55         ` Stefan Monnier
@ 2022-09-22 22:21         ` Drew Adams
  1 sibling, 0 replies; 16+ messages in thread
From: Drew Adams @ 2022-09-22 22:21 UTC (permalink / raw)
  To: Sean Whitton, Stefan Monnier; +Cc: Robert Pluim, emacs-devel@gnu.org

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



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

* Re: master 101f3cf5b9: Add support for user edits to VC command arguments
  2022-09-22 21:38     ` Sean Whitton
@ 2022-09-22 22:41       ` Stefan Monnier
  2022-09-23 17:56         ` Sean Whitton
  0 siblings, 1 reply; 16+ messages in thread
From: Stefan Monnier @ 2022-09-22 22:41 UTC (permalink / raw)
  To: Sean Whitton; +Cc: emacs-devel

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




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

* Re: master 101f3cf5b9: Add support for user edits to VC command arguments
  2022-09-22 21:18           ` Sean Whitton
@ 2022-09-23  6:39             ` Juri Linkov
  0 siblings, 0 replies; 16+ messages in thread
From: Juri Linkov @ 2022-09-23  6:39 UTC (permalink / raw)
  To: Sean Whitton; +Cc: Stefan Monnier, Robert Pluim, emacs-devel

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



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

* Re: master 101f3cf5b9: Add support for user edits to VC command arguments
  2022-09-22 16:06     ` Sean Whitton
@ 2022-09-23  8:03       ` Robert Pluim
  2022-09-23 16:15         ` Sean Whitton
  0 siblings, 1 reply; 16+ messages in thread
From: Robert Pluim @ 2022-09-23  8:03 UTC (permalink / raw)
  To: Sean Whitton; +Cc: emacs-devel

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



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

* Re: master 101f3cf5b9: Add support for user edits to VC command arguments
  2022-09-23  8:03       ` Robert Pluim
@ 2022-09-23 16:15         ` Sean Whitton
  0 siblings, 0 replies; 16+ messages in thread
From: Sean Whitton @ 2022-09-23 16:15 UTC (permalink / raw)
  To: Robert Pluim; +Cc: emacs-devel

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



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

* Re: master 101f3cf5b9: Add support for user edits to VC command arguments
  2022-09-22 22:41       ` Stefan Monnier
@ 2022-09-23 17:56         ` Sean Whitton
  2022-09-23 19:32           ` Stefan Monnier
  0 siblings, 1 reply; 16+ messages in thread
From: Sean Whitton @ 2022-09-23 17:56 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

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


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

* Re: master 101f3cf5b9: Add support for user edits to VC command arguments
  2022-09-23 17:56         ` Sean Whitton
@ 2022-09-23 19:32           ` Stefan Monnier
  2022-09-23 21:51             ` Sean Whitton
  0 siblings, 1 reply; 16+ messages in thread
From: Stefan Monnier @ 2022-09-23 19:32 UTC (permalink / raw)
  To: Sean Whitton; +Cc: emacs-devel

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




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

* Re: master 101f3cf5b9: Add support for user edits to VC command arguments
  2022-09-23 19:32           ` Stefan Monnier
@ 2022-09-23 21:51             ` Sean Whitton
  0 siblings, 0 replies; 16+ messages in thread
From: Sean Whitton @ 2022-09-23 21:51 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

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



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

end of thread, other threads:[~2022-09-23 21:51 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <166378878197.3568.7077863090259744101@vcs2.savannah.gnu.org>
     [not found] ` <20220921193303.1B687C11D81@vcs2.savannah.gnu.org>
2022-09-22  9:25   ` master 101f3cf5b9: Add support for user edits to VC command arguments Robert Pluim
2022-09-22 13:24     ` Stefan Monnier
2022-09-22 16:08       ` Sean Whitton
2022-09-22 16:55         ` Stefan Monnier
2022-09-22 21:18           ` Sean Whitton
2022-09-23  6:39             ` Juri Linkov
2022-09-22 22:21         ` [External] : " Drew Adams
2022-09-22 16:06     ` Sean Whitton
2022-09-23  8:03       ` Robert Pluim
2022-09-23 16:15         ` Sean Whitton
2022-09-22 18:45   ` Stefan Monnier
2022-09-22 21:38     ` Sean Whitton
2022-09-22 22:41       ` Stefan Monnier
2022-09-23 17:56         ` Sean Whitton
2022-09-23 19:32           ` Stefan Monnier
2022-09-23 21:51             ` Sean Whitton

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.