Re: master 59df0a7bd9: Add a VC command to prepare patches

Re: master 59df0a7bd9: Add a VC command to prepare patches

[Michael Albinus]

Philip Kaludercic <philipk@posteo.net> writes:

Hi Philip,

> --- a/etc/NEWS
> +++ b/etc/NEWS
> +---
> +*** The function 'vc-read-revision' accepts a new MULTIPLE argument.
> +If non-nil, multiple revisions can be queried.  This is done using
> +'completing-read-multiple'.
> +
> +---
> +*** New function 'vc-read-multiple-revisions'
> +This function invokes 'vc-read-revision' with a non-nil value for
> +MULTIPLE.

Both functions haven't a docstring. Now that we mention them in NEWS,
they should get it, shouldn't they?

Best regards, Michael.

Re: master 59df0a7bd9: Add a VC command to prepare patches

[Philip Kaludercic]

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

Michael Albinus <michael.albinus@gmx.de> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
> Hi Philip,
>
>> --- a/etc/NEWS
>> +++ b/etc/NEWS
>> +---
>> +*** The function 'vc-read-revision' accepts a new MULTIPLE argument.
>> +If non-nil, multiple revisions can be queried.  This is done using
>> +'completing-read-multiple'.
>> +
>> +---
>> +*** New function 'vc-read-multiple-revisions'
>> +This function invokes 'vc-read-revision' with a non-nil value for
>> +MULTIPLE.
>
> Both functions haven't a docstring. Now that we mention them in NEWS,
> they should get it, shouldn't they?

How do you like the following:


[-- Attachment #2: Type: text/plain, Size: 1338 bytes --]

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 14b149310c..2867539aa3 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1926,6 +1926,13 @@ vc-revision-history
   "History for `vc-read-revision'.")
 
 (defun vc-read-revision (prompt &optional files backend default initial-input multiple)
+  "Query the user for a revision using PROMPT.
+All subsequent arguments are optional.  FILES may specify a file
+set to restrict the revisions to.  BACKEND is a VC backend as
+listed in `vc-handled-backends'.  DEFAULT and INITIAL-INPUT are
+handled as defined by `completing-read'.  If MULTIPLE is non-nil,
+the user may be prompted for multiple revisions.  If possible
+this means that `completing-read-multiple' will be used."
   (cond
    ((null files)
     (let ((vc-fileset (vc-deduce-fileset t))) ;FIXME: why t?  --Stef
@@ -1947,6 +1954,10 @@ vc-read-revision
           answer)))))
 
 (defun vc-read-multiple-revisions (prompt &optional files backend default initial-input)
+  "Query the user for multiple revisions.
+This is equivalent to invoking `vc-read-revision' with t for
+MULTIPLE.  The arguments PROMPT, FILES, BACKEND, DEFAULT and
+INITIAL-INPUT are passed on to `vc-read-revision' directly."
   (vc-read-revision prompt files backend default initial-input t))
 
 (defun vc-diff-build-argument-list-internal (&optional fileset)

[-- Attachment #3: Type: text/plain, Size: 27 bytes --]



> Best regards, Michael.

Re: master 59df0a7bd9: Add a VC command to prepare patches

[Michael Albinus]

Philip Kaludercic <philipk@posteo.net> writes:

Hi Philip,

>> Both functions haven't a docstring. Now that we mention them in NEWS,
>> they should get it, shouldn't they?
>
> How do you like the following:

I don't know too much about vc.el implementation, so it doesn't matter
how I do like :-)

> diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
> index 14b149310c..2867539aa3 100644
> --- a/lisp/vc/vc.el
> +++ b/lisp/vc/vc.el
> @@ -1926,6 +1926,13 @@ vc-revision-history
>    "History for `vc-read-revision'.")
>
>  (defun vc-read-revision (prompt &optional files backend default initial-input multiple)
> +  "Query the user for a revision using PROMPT.
> +All subsequent arguments are optional.  FILES may specify a file
> +set to restrict the revisions to.  BACKEND is a VC backend as
> +listed in `vc-handled-backends'.  DEFAULT and INITIAL-INPUT are
> +handled as defined by `completing-read'.  If MULTIPLE is non-nil,
> +the user may be prompted for multiple revisions.  If possible
> +this means that `completing-read-multiple' will be used."
>    (cond
>     ((null files)
>      (let ((vc-fileset (vc-deduce-fileset t))) ;FIXME: why t?  --Stef
> @@ -1947,6 +1954,10 @@ vc-read-revision
>            answer)))))
>
>  (defun vc-read-multiple-revisions (prompt &optional files backend default initial-input)
> +  "Query the user for multiple revisions.
> +This is equivalent to invoking `vc-read-revision' with t for
> +MULTIPLE.  The arguments PROMPT, FILES, BACKEND, DEFAULT and
> +INITIAL-INPUT are passed on to `vc-read-revision' directly."
>    (vc-read-revision prompt files backend default initial-input t))
>
>  (defun vc-diff-build-argument-list-internal (&optional fileset)

With my limited scope, it looks OK.

Best regards, Michael.

Re: master 59df0a7bd9: Add a VC command to prepare patches

[Sean Whitton]

Hello,

On Sat 08 Oct 2022 at 05:52AM -04, Philip Kaludercic wrote:

> +@vindex vc-prepare-patches-separately
> +Depending on the value of the user option
> +@code{vc-prepare-patches-separately}, @code{vc-prepare-patch} will
> +generate one or more messages.  The default value @code{t} means
> +prepare and display a message for each revision, one after another.  A
> +value of @code{nil} means to generate a single message with all
> +patches attached in the body.
> +
> +@vindex vc-default-patch-addressee
> +If you expect to contribute patches on a regular basis, you can set
> +the user option @code{vc-default-patch-addressee} to the address(es)
> +you wish to use.  This will be used as the default value when invoking
> +@code{vc-prepare-patch}.  Project maintainers may consider setting
> +this as a directory local variable (@pxref{Directory Variables}).
> +

Please excuse me if I missed discussion in a bug, but wouldn't it make
sense to set *both* of these in .dir-locals.el for emacs.git?

-- 
Sean Whitton

Re: master 59df0a7bd9: Add a VC command to prepare patches

[Philip Kaludercic]

Sean Whitton <spwhitton@spwhitton.name> writes:

> Hello,
>
> On Sat 08 Oct 2022 at 05:52AM -04, Philip Kaludercic wrote:
>
>> +@vindex vc-prepare-patches-separately
>> +Depending on the value of the user option
>> +@code{vc-prepare-patches-separately}, @code{vc-prepare-patch} will
>> +generate one or more messages.  The default value @code{t} means
>> +prepare and display a message for each revision, one after another.  A
>> +value of @code{nil} means to generate a single message with all
>> +patches attached in the body.
>> +
>> +@vindex vc-default-patch-addressee
>> +If you expect to contribute patches on a regular basis, you can set
>> +the user option @code{vc-default-patch-addressee} to the address(es)
>> +you wish to use.  This will be used as the default value when invoking
>> +@code{vc-prepare-patch}.  Project maintainers may consider setting
>> +this as a directory local variable (@pxref{Directory Variables}).
>> +
>
> Please excuse me if I missed discussion in a bug, but wouldn't it make
> sense to set *both* of these in .dir-locals.el for emacs.git?

Yes, but I would wait a bit to avoid the same issues as we had when
`diff-add-log-use-relative-names' was set.  Anyone who had pulled the
new commits with the change but hadn't rebuilt Emacs to know that these
are safe values was prompted the "are you sure these variables are safe"
queries.  If we wait for a week or two then I would guess that most
people following development will have pulled and rebuilt at least once
by then.

Re: master 59df0a7bd9: Add a VC command to prepare patches

[Philip Kaludercic]

Sean Whitton <spwhitton@spwhitton.name> writes:

> Hello,
>
> On Sat 08 Oct 2022 at 05:52AM -04, Philip Kaludercic wrote:
>
>> +@vindex vc-prepare-patches-separately
>> +Depending on the value of the user option
>> +@code{vc-prepare-patches-separately}, @code{vc-prepare-patch} will
>> +generate one or more messages.  The default value @code{t} means
>> +prepare and display a message for each revision, one after another.  A
>> +value of @code{nil} means to generate a single message with all
>> +patches attached in the body.
>> +
>> +@vindex vc-default-patch-addressee
>> +If you expect to contribute patches on a regular basis, you can set
>> +the user option @code{vc-default-patch-addressee} to the address(es)
>> +you wish to use.  This will be used as the default value when invoking
>> +@code{vc-prepare-patch}.  Project maintainers may consider setting
>> +this as a directory local variable (@pxref{Directory Variables}).
>> +
>
> Please excuse me if I missed discussion in a bug, but wouldn't it make
> sense to set *both* of these in .dir-locals.el for emacs.git?

Also, a nice feature to add in the future would be if
`vc-default-patch-addressee' could be computed by inferring maintainers
for the files changes (sort of what the Linux kernel also does with the
get_maintainer.pl script).  I don't know of any generic mechanism, but
maybe one could come up with some kind of a standard for the GNU project
and try to promote it for others as well?

Re: master 59df0a7bd9: Add a VC command to prepare patches

[Sean Whitton]

Hello,

On Sat 08 Oct 2022 at 05:29PM GMT, Philip Kaludercic wrote:

> Yes, but I would wait a bit to avoid the same issues as we had when
> `diff-add-log-use-relative-names' was set.  Anyone who had pulled the
> new commits with the change but hadn't rebuilt Emacs to know that these
> are safe values was prompted the "are you sure these variables are safe"
> queries.  If we wait for a week or two then I would guess that most
> people following development will have pulled and rebuilt at least once
> by then.

Cool.  Perhaps the docstring for vc-prepare-patches-separately should
also mention that it can be set locally, then?

(Btw, just filed a feature request, but forgot to X-debbugs-cc you.)

Thanks for this cool new feature.

-- 
Sean Whitton

Re: master 59df0a7bd9: Add a VC command to prepare patches

[Philip Kaludercic]

Sean Whitton <spwhitton@spwhitton.name> writes:

> Hello,
>
> On Sat 08 Oct 2022 at 05:29PM GMT, Philip Kaludercic wrote:
>
>> Yes, but I would wait a bit to avoid the same issues as we had when
>> `diff-add-log-use-relative-names' was set.  Anyone who had pulled the
>> new commits with the change but hadn't rebuilt Emacs to know that these
>> are safe values was prompted the "are you sure these variables are safe"
>> queries.  If we wait for a week or two then I would guess that most
>> people following development will have pulled and rebuilt at least once
>> by then.
>
> Cool.  Perhaps the docstring for vc-prepare-patches-separately should
> also mention that it can be set locally, then?

I don't think it is necessary, that information is automatically
inserted into the help buffer:

        vc-prepare-patches-separately is a variable defined in ‘vc.el’.

        Its value is t

        Non-nil means that ‘vc-prepare-patch’ creates a single message.
        A single message is created by attaching all patches to the body
        of a single message.  If nil, each patch will be sent out in a
        separate message, which will be prepared sequentially.

-->       This variable is safe as a file local variable if its value
          satisfies the predicate ‘booleanp’.
          This variable was introduced, or its default value was changed, in
          version 29.1 of Emacs.
          You can customize this variable.

        [back]

> (Btw, just filed a feature request, but forgot to X-debbugs-cc you.)

bug#58383 I assume?

> Thanks for this cool new feature.

I am glad it is of use.

Re: master 59df0a7bd9: Add a VC command to prepare patches

[Robert Pluim]

>>>>> On Sat,  8 Oct 2022 05:52:12 -0400 (EDT), Philip Kaludercic <philipk@posteo.net> said:
    Philip> +;;;###autoload
    Philip> +(defun vc-prepare-patch (addressee subject revisions)
    Philip> +  "Compose an Email sending patches for REVISIONS to ADDRESSEE.
    Philip> +If `vc-prepare-patches-separately' is non-nil, SUBJECT will be used
                                                   ^^^

Beware the dangers of last-minute changes in default values :) I fixed
it.

One surprising thing that isnʼt called out explicitly is that if you
do `vc-prepare-patch' for multiple revisions, it prepares the first
patch, waits for you to do something with it (like send it), and then
moves on to the next. I guess I was expecting it to prepare *all* of
them (that way I could just save them all, and look in my drafts
folder afterwards, and send them all at once). Maybe 'git
format-patch' has spoiled me.

Robert
-- 

Re: master 59df0a7bd9: Add a VC command to prepare patches

[Philip Kaludercic]

Robert Pluim <rpluim@gmail.com> writes:

>>>>>> On Sat,  8 Oct 2022 05:52:12 -0400 (EDT), Philip Kaludercic <philipk@posteo.net> said:
>     Philip> +;;;###autoload
>     Philip> +(defun vc-prepare-patch (addressee subject revisions)
>     Philip> +  "Compose an Email sending patches for REVISIONS to ADDRESSEE.
>     Philip> +If `vc-prepare-patches-separately' is non-nil, SUBJECT will be used
>                                                    ^^^
>
> Beware the dangers of last-minute changes in default values :) I fixed
> it.
>
> One surprising thing that isnʼt called out explicitly is that if you
> do `vc-prepare-patch' for multiple revisions, it prepares the first
> patch, waits for you to do something with it (like send it), and then
> moves on to the next. I guess I was expecting it to prepare *all* of
> them (that way I could just save them all, and look in my drafts
> folder afterwards, and send them all at once). Maybe 'git
> format-patch' has spoiled me.

It would be possible to add either another option or a third value to
have all messages automatically send out.

> Robert

Re: master 59df0a7bd9: Add a VC command to prepare patches

[Robert Pluim]

>>>>> On Mon, 10 Oct 2022 11:22:33 +0000, Philip Kaludercic <philipk@posteo.net> said:
    >> One surprising thing that isnʼt called out explicitly is that if you
    >> do `vc-prepare-patch' for multiple revisions, it prepares the first
    >> patch, waits for you to do something with it (like send it), and then
    >> moves on to the next. I guess I was expecting it to prepare *all* of
    >> them (that way I could just save them all, and look in my drafts
    >> folder afterwards, and send them all at once). Maybe 'git
    >> format-patch' has spoiled me.

    Philip> It would be possible to add either another option or a third value to
    Philip> have all messages automatically send out.

I was thinking more along the lines of an option to prepare all the
mail messages and *not* send them out.

Robert
-- 

Re: master 59df0a7bd9: Add a VC command to prepare patches

[Sean Whitton]

Hello,

On Sun 09 Oct 2022 at 12:30PM GMT, Philip Kaludercic wrote:

> I don't think it is necessary, that information is automatically
> inserted into the help buffer:
>
>         vc-prepare-patches-separately is a variable defined in ‘vc.el’.
>
>         Its value is t
>
>         Non-nil means that ‘vc-prepare-patch’ creates a single message.
>         A single message is created by attaching all patches to the body
>         of a single message.  If nil, each patch will be sent out in a
>         separate message, which will be prepared sequentially.
>
> -->       This variable is safe as a file local variable if its value
>           satisfies the predicate ‘booleanp’.
>           This variable was introduced, or its default value was changed, in
>           version 29.1 of Emacs.
>           You can customize this variable.
>
>         [back]

Good point.

>> (Btw, just filed a feature request, but forgot to X-debbugs-cc you.)
>
> bug#58383 I assume?

Indeed.

-- 
Sean Whitton

Re: master 59df0a7bd9: Add a VC command to prepare patches

[Philip Kaludercic]

Philip Kaludercic <philipk@posteo.net> writes:

> Sean Whitton <spwhitton@spwhitton.name> writes:
>
>> Hello,
>>
>> On Sat 08 Oct 2022 at 05:52AM -04, Philip Kaludercic wrote:
>>
>>> +@vindex vc-prepare-patches-separately
>>> +Depending on the value of the user option
>>> +@code{vc-prepare-patches-separately}, @code{vc-prepare-patch} will
>>> +generate one or more messages.  The default value @code{t} means
>>> +prepare and display a message for each revision, one after another.  A
>>> +value of @code{nil} means to generate a single message with all
>>> +patches attached in the body.
>>> +
>>> +@vindex vc-default-patch-addressee
>>> +If you expect to contribute patches on a regular basis, you can set
>>> +the user option @code{vc-default-patch-addressee} to the address(es)
>>> +you wish to use.  This will be used as the default value when invoking
>>> +@code{vc-prepare-patch}.  Project maintainers may consider setting
>>> +this as a directory local variable (@pxref{Directory Variables}).
>>> +
>>
>> Please excuse me if I missed discussion in a bug, but wouldn't it make
>> sense to set *both* of these in .dir-locals.el for emacs.git?
>
> Yes, but I would wait a bit to avoid the same issues as we had when
> `diff-add-log-use-relative-names' was set.  Anyone who had pulled the
> new commits with the change but hadn't rebuilt Emacs to know that these
> are safe values was prompted the "are you sure these variables are safe"
> queries.  If we wait for a week or two then I would guess that most
> people following development will have pulled and rebuilt at least once
> by then.

I guess enough time has gone by and the variable could be added to
.dir-locals.el?

Re: master 59df0a7bd9: Add a VC command to prepare patches

[Sean Whitton]

Hello,

On Tue 15 Nov 2022 at 07:18PM GMT, Philip Kaludercic wrote:

> I guess enough time has gone by and the variable could be added to
> .dir-locals.el?

I'd go for it!

-- 
Sean Whitton

Re: master 59df0a7bd9: Add a VC command to prepare patches

[Philip Kaludercic]

Sean Whitton <spwhitton@spwhitton.name> writes:

> Hello,
>
> On Tue 15 Nov 2022 at 07:18PM GMT, Philip Kaludercic wrote:
>
>> I guess enough time has gone by and the variable could be added to
>> .dir-locals.el?
>
> I'd go for it!

Done.