bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Sean Whitton]

Hello,

How about a prefix argument to vc-prepare-patch to invert one's usual
setting for vc-prepare-patches-separately?  Most people who contribute
to more than one project regularly will want to use both.

On the other hand, having a numeric prefix argument mean "send patches
correspoding to the top N revisions of the current branch" would be very
convenient.  Perhaps these two could be combined by using a negative
number to mean also invert?

-- 
Sean Whitton

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Philip Kaludercic]

Sean Whitton <spwhitton@spwhitton.name> writes:

> Hello,
>
> How about a prefix argument to vc-prepare-patch to invert one's usual
> setting for vc-prepare-patches-separately?  Most people who contribute
> to more than one project regularly will want to use both.

How would this be preferable to setting `vc-prepare-patches-separately'
as a directory local variable?  That way you don't have to remember to
use a prefix argument whenever invoking `vc-prepare-patch'.

> On the other hand, having a numeric prefix argument mean "send patches
> correspoding to the top N revisions of the current branch" would be very
> convenient.  Perhaps these two could be combined by using a negative
> number to mean also invert?

This is the usual problem with numeric prefix arguments.  You don't get
that much expressivity with just an integer.

That is why I would hesitate to assign any particular interpretation to
prefix arguments, before considering and weighing the options.

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Philip Kaludercic]

Philip Kaludercic <philipk@posteo.net> writes:

> Sean Whitton <spwhitton@spwhitton.name> writes:
>
>> Hello,
>>
>> How about a prefix argument to vc-prepare-patch to invert one's usual
>> setting for vc-prepare-patches-separately?  Most people who contribute
>> to more than one project regularly will want to use both.
>
> How would this be preferable to setting `vc-prepare-patches-separately'
> as a directory local variable?  That way you don't have to remember to
> use a prefix argument whenever invoking `vc-prepare-patch'.
>
>> On the other hand, having a numeric prefix argument mean "send patches
>> correspoding to the top N revisions of the current branch" would be very
>> convenient.  Perhaps these two could be combined by using a negative
>> number to mean also invert?
>
> This is the usual problem with numeric prefix arguments.  You don't get
> that much expressivity with just an integer.
>
> That is why I would hesitate to assign any particular interpretation to
> prefix arguments, before considering and weighing the options.

Ping?

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Sean Whitton]

Hello Philip,

Not sure what input is wanted from me.  Happy to think through anything
in particular you were waiting to hear from me about?

-- 
Sean Whitton

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Philip Kaludercic]

Sean Whitton <spwhitton@spwhitton.name> writes:

> Hello Philip,
>
> Not sure what input is wanted from me.  Happy to think through anything
> in particular you were waiting to hear from me about?

I was wondering if you had any comments on my last message:

Sean Whitton <spwhitton@spwhitton.name> writes:

> Hello,
>
> How about a prefix argument to vc-prepare-patch to invert one's usual
> setting for vc-prepare-patches-separately?  Most people who contribute
> to more than one project regularly will want to use both.

How would this be preferable to setting `vc-prepare-patches-separately'
as a directory local variable?  That way you don't have to remember to
use a prefix argument whenever invoking `vc-prepare-patch'.

> On the other hand, having a numeric prefix argument mean "send patches
> correspoding to the top N revisions of the current branch" would be
> very
> convenient.  Perhaps these two could be combined by using a negative
> number to mean also invert?

This is the usual problem with numeric prefix arguments.  You don't get
that much expressivity with just an integer.

That is why I would hesitate to assign any particular interpretation to
prefix arguments, before considering and weighing the options.

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Sean Whitton]

Hello,

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

> Sean Whitton <spwhitton@spwhitton.name> writes:
>
>> Hello,
>>
>> How about a prefix argument to vc-prepare-patch to invert one's usual
>> setting for vc-prepare-patches-separately?  Most people who contribute
>> to more than one project regularly will want to use both.
>
> How would this be preferable to setting `vc-prepare-patches-separately'
> as a directory local variable?  That way you don't have to remember to
> use a prefix argument whenever invoking `vc-prepare-patch'.

Makes sense.

>> On the other hand, having a numeric prefix argument mean "send patches
>> correspoding to the top N revisions of the current branch" would be very
>> convenient.  Perhaps these two could be combined by using a negative
>> number to mean also invert?
>
> This is the usual problem with numeric prefix arguments.  You don't get
> that much expressivity with just an integer.
>
> That is why I would hesitate to assign any particular interpretation to
> prefix arguments, before considering and weighing the options.

Well, any other options in mind?  Varying the -N argument to
git-format-patch/git-send-email is what I find myself using the most.

-- 
Sean Whitton

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Philip Kaludercic]

Sean Whitton <spwhitton@spwhitton.name> writes:

>>> On the other hand, having a numeric prefix argument mean "send patches
>>> correspoding to the top N revisions of the current branch" would be very
>>> convenient.  Perhaps these two could be combined by using a negative
>>> number to mean also invert?
>>
>> This is the usual problem with numeric prefix arguments.  You don't get
>> that much expressivity with just an integer.
>>
>> That is why I would hesitate to assign any particular interpretation to
>> prefix arguments, before considering and weighing the options.
>
> Well, any other options in mind?  Varying the -N argument to
> git-format-patch/git-send-email is what I find myself using the most.

The issue is finding a way for this to be expressed VC-generically.

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Sean Whitton]

Hello,

On Tue 08 Nov 2022 at 08:31PM GMT, Philip Kaludercic wrote:

> Sean Whitton <spwhitton@spwhitton.name> writes:
>
>>>> On the other hand, having a numeric prefix argument mean "send patches
>>>> correspoding to the top N revisions of the current branch" would be very
>>>> convenient.  Perhaps these two could be combined by using a negative
>>>> number to mean also invert?
>>>
>>> This is the usual problem with numeric prefix arguments.  You don't get
>>> that much expressivity with just an integer.
>>>
>>> That is why I would hesitate to assign any particular interpretation to
>>> prefix arguments, before considering and weighing the options.
>>
>> Well, any other options in mind?  Varying the -N argument to
>> git-format-patch/git-send-email is what I find myself using the most.
>
> The issue is finding a way for this to be expressed VC-generically.

s/expressed/implemented/, right?

-- 
Sean Whitton

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Philip Kaludercic]

Sean Whitton <spwhitton@spwhitton.name> writes:

> Hello,
>
> On Tue 08 Nov 2022 at 08:31PM GMT, Philip Kaludercic wrote:
>
>> Sean Whitton <spwhitton@spwhitton.name> writes:
>>
>>>>> On the other hand, having a numeric prefix argument mean "send patches
>>>>> correspoding to the top N revisions of the current branch" would be very
>>>>> convenient.  Perhaps these two could be combined by using a negative
>>>>> number to mean also invert?
>>>>
>>>> This is the usual problem with numeric prefix arguments.  You don't get
>>>> that much expressivity with just an integer.
>>>>
>>>> That is why I would hesitate to assign any particular interpretation to
>>>> prefix arguments, before considering and weighing the options.
>>>
>>> Well, any other options in mind?  Varying the -N argument to
>>> git-format-patch/git-send-email is what I find myself using the most.
>>
>> The issue is finding a way for this to be expressed VC-generically.
>
> s/expressed/implemented/, right?

I meant expressed, but I don't remember what I meant ^^

So let's say implement.  What we could do is just call
`previous-revision' for the backend N times, but with what file?

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Sean Whitton]

Hello,

On Wed 09 Nov 2022 at 08:28AM GMT, Philip Kaludercic wrote:

> Sean Whitton <spwhitton@spwhitton.name> writes:
>
>> Hello,
>>
>> On Tue 08 Nov 2022 at 08:31PM GMT, Philip Kaludercic wrote:
>>
>>> Sean Whitton <spwhitton@spwhitton.name> writes:
>>>
>>>>>> On the other hand, having a numeric prefix argument mean "send patches
>>>>>> correspoding to the top N revisions of the current branch" would be very
>>>>>> convenient.  Perhaps these two could be combined by using a negative
>>>>>> number to mean also invert?
>>>>>
>>>>> This is the usual problem with numeric prefix arguments.  You don't get
>>>>> that much expressivity with just an integer.
>>>>>
>>>>> That is why I would hesitate to assign any particular interpretation to
>>>>> prefix arguments, before considering and weighing the options.
>>>>
>>>> Well, any other options in mind?  Varying the -N argument to
>>>> git-format-patch/git-send-email is what I find myself using the most.
>>>
>>> The issue is finding a way for this to be expressed VC-generically.
>>
>> s/expressed/implemented/, right?
>
> I meant expressed, but I don't remember what I meant ^^
>
> So let's say implement.  What we could do is just call
> `previous-revision' for the backend N times, but with what file?

I don't know, but the meaning of "the past N checked-in revisions of the
whole tree" is what we need to agree upon, then it's just coding.

-- 
Sean Whitton

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Philip Kaludercic]

Sean Whitton <spwhitton@spwhitton.name> writes:

> Hello,
>
> On Wed 09 Nov 2022 at 08:28AM GMT, Philip Kaludercic wrote:
>
>> Sean Whitton <spwhitton@spwhitton.name> writes:
>>
>>> Hello,
>>>
>>> On Tue 08 Nov 2022 at 08:31PM GMT, Philip Kaludercic wrote:
>>>
>>>> Sean Whitton <spwhitton@spwhitton.name> writes:
>>>>
>>>>>>> On the other hand, having a numeric prefix argument mean "send patches
>>>>>>> correspoding to the top N revisions of the current branch" would be very
>>>>>>> convenient.  Perhaps these two could be combined by using a negative
>>>>>>> number to mean also invert?
>>>>>>
>>>>>> This is the usual problem with numeric prefix arguments.  You don't get
>>>>>> that much expressivity with just an integer.
>>>>>>
>>>>>> That is why I would hesitate to assign any particular interpretation to
>>>>>> prefix arguments, before considering and weighing the options.
>>>>>
>>>>> Well, any other options in mind?  Varying the -N argument to
>>>>> git-format-patch/git-send-email is what I find myself using the most.
>>>>
>>>> The issue is finding a way for this to be expressed VC-generically.
>>>
>>> s/expressed/implemented/, right?
>>
>> I meant expressed, but I don't remember what I meant ^^
>>
>> So let's say implement.  What we could do is just call
>> `previous-revision' for the backend N times, but with what file?
>
> I don't know, but the meaning of "the past N checked-in revisions of the
> whole tree" is what we need to agree upon, then it's just coding.

The critical edge-case here is what happens when branches are merged.
Do you pick a random branch or collect all the patches.  Or do you raise
an error, but then how do you detect that vc-generically. 

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Sean Whitton]

Hello,

On Wed 09 Nov 2022 at 05:46PM GMT, Philip Kaludercic wrote:

> The critical edge-case here is what happens when branches are merged.
> Do you pick a random branch or collect all the patches.  Or do you raise
> an error, but then how do you detect that vc-generically.

Typically you wouldn't want to format patches across a merge, so I would
suggest raising an error.

-- 
Sean Whitton

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Philip Kaludercic]

Sean Whitton <spwhitton@spwhitton.name> writes:

> Hello,
>
> On Wed 09 Nov 2022 at 05:46PM GMT, Philip Kaludercic wrote:
>
>> The critical edge-case here is what happens when branches are merged.
>> Do you pick a random branch or collect all the patches.  Or do you raise
>> an error, but then how do you detect that vc-generically.
>
> Typically you wouldn't want to format patches across a merge, so I would
> suggest raising an error.

And this is something I don't think can be /expressed/ using vc, because
while I can collect a number of revisions using `previous-revision',
there is no general way to verify if a commit is a merge commit.

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Sean Whitton]

Hello,

On Thu 10 Nov 2022 at 08:14PM GMT, Philip Kaludercic wrote:

> Sean Whitton <spwhitton@spwhitton.name> writes:
>
>> Hello,
>>
>> On Wed 09 Nov 2022 at 05:46PM GMT, Philip Kaludercic wrote:
>>
>>> The critical edge-case here is what happens when branches are merged.
>>> Do you pick a random branch or collect all the patches.  Or do you raise
>>> an error, but then how do you detect that vc-generically.
>>
>> Typically you wouldn't want to format patches across a merge, so I would
>> suggest raising an error.
>
> And this is something I don't think can be /expressed/ using vc, because
> while I can collect a number of revisions using `previous-revision',
> there is no general way to verify if a commit is a merge commit.

Can we do that part on a VCS-by-VCS basis?  Default to just calling
previous-revision and hoping for the best, but giving vc-git.el a chance
to raise an error.

-- 
Sean Whitton

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Philip Kaludercic]

Sean Whitton <spwhitton@spwhitton.name> writes:

>>> Typically you wouldn't want to format patches across a merge, so I would
>>> suggest raising an error.
>>
>> And this is something I don't think can be /expressed/ using vc, because
>> while I can collect a number of revisions using `previous-revision',
>> there is no general way to verify if a commit is a merge commit.
>
> Can we do that part on a VCS-by-VCS basis?  Default to just calling
> previous-revision and hoping for the best, but giving vc-git.el a chance
> to raise an error.

I guess that would be possible, though it will probably require a new
VC method :/  The new `prepare-patch' takes a revision, so it doesn't
make sense to pass it a number and have it return multiple patches.

Perhaps it will be easier/cleaner to just accept that avoiding merge
revisions is the users responsibility.

But just to have mentioned it:  Do you know that you can mark revisions
in log-view and then vc-prepare-patches will use these as the default
input when prompting for revisions?

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Sean Whitton]

Hello,

On Fri 11 Nov 2022 at 06:32AM GMT, Philip Kaludercic wrote:

> Sean Whitton <spwhitton@spwhitton.name> writes:
>
>>>> Typically you wouldn't want to format patches across a merge, so I would
>>>> suggest raising an error.
>>>
>>> And this is something I don't think can be /expressed/ using vc, because
>>> while I can collect a number of revisions using `previous-revision',
>>> there is no general way to verify if a commit is a merge commit.
>>
>> Can we do that part on a VCS-by-VCS basis?  Default to just calling
>> previous-revision and hoping for the best, but giving vc-git.el a chance
>> to raise an error.
>
> I guess that would be possible, though it will probably require a new
> VC method :/  The new `prepare-patch' takes a revision, so it doesn't
> make sense to pass it a number and have it return multiple patches.
>
> Perhaps it will be easier/cleaner to just accept that avoiding merge
> revisions is the users responsibility.

Sounds reasonable -- it can always be enhanced later in a way that's
backwards-compatible.

> But just to have mentioned it: Do you know that you can mark revisions
> in log-view and then vc-prepare-patches will use these as the default
> input when prompting for revisions?

Yeah, but marking in those buffers is way more awkward than marking in,
e.g., dired, and I usually know how many commits I want to send without
looking at the log.

-- 
Sean Whitton

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Philip Kaludercic]

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

Sean Whitton <spwhitton@spwhitton.name> writes:

> Hello,
>
> On Fri 11 Nov 2022 at 06:32AM GMT, Philip Kaludercic wrote:
>
>> Sean Whitton <spwhitton@spwhitton.name> writes:
>>
>>>>> Typically you wouldn't want to format patches across a merge, so I would
>>>>> suggest raising an error.
>>>>
>>>> And this is something I don't think can be /expressed/ using vc, because
>>>> while I can collect a number of revisions using `previous-revision',
>>>> there is no general way to verify if a commit is a merge commit.
>>>
>>> Can we do that part on a VCS-by-VCS basis?  Default to just calling
>>> previous-revision and hoping for the best, but giving vc-git.el a chance
>>> to raise an error.
>>
>> I guess that would be possible, though it will probably require a new
>> VC method :/  The new `prepare-patch' takes a revision, so it doesn't
>> make sense to pass it a number and have it return multiple patches.
>>
>> Perhaps it will be easier/cleaner to just accept that avoiding merge
>> revisions is the users responsibility.
>
> Sounds reasonable -- it can always be enhanced later in a way that's
> backwards-compatible.
>
>> But just to have mentioned it: Do you know that you can mark revisions
>> in log-view and then vc-prepare-patches will use these as the default
>> input when prompting for revisions?
>
> Yeah, but marking in those buffers is way more awkward than marking in,
> e.g., dired, and I usually know how many commits I want to send without
> looking at the log.

How does the following look like:


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

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 513fbb23fe..0b8a8d83e3 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -3391,14 +3391,24 @@ vc-prepare-patch
 as the default subject for the message (and it will be prompted
 for when called interactively).  Otherwise a separate message
 will be composed for each revision, with SUBJECT derived from the
-invidividual commits.
-
-When invoked interactively in a Log View buffer with marked
-revisions, those revisions will be used."
+invidividual commits.  When invoked with a numerical prefix
+argument, the last N revisions will be used.  When invoked
+interactively in a Log View buffer with marked revisions, those
+revisions will be used."
   (interactive
    (let ((revs (vc-read-multiple-revisions
                 "Revisions: " nil nil nil
-                (or (and-let* ((revs (log-view-get-marked)))
+                (or (and-let* ((arg current-prefix-arg)
+                               (fs (vc-deduce-fileset t)))
+                      (cl-loop with file = (caadr fs)
+                               repeat (prefix-numeric-value arg)
+                               for rev = (vc-working-revision file)
+                               then (vc-call-backend
+                                     (car fs) 'previous-revision
+                                     file rev)
+                               when rev collect it into revs
+                               finally return (mapconcat #'identity revs ",")))
+                    (and-let* ((revs (log-view-get-marked)))
                       (mapconcat #'identity revs ","))
                     (and-let* ((file (buffer-file-name)))
                       (vc-working-revision file)))))

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Philip Kaludercic]

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

Philip Kaludercic <philipk@posteo.net> writes:

> How does the following look like:

I have prepared a proper patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Have-vc-prepare-patch-handle-prefix-arguments.patch --]
[-- Type: text/x-diff, Size: 4768 bytes --]

From 724b85acd34e4e5dd6e8cb521eb03eda08ff105a Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Sun, 13 Nov 2022 17:05:20 +0100
Subject: [PATCH] Have 'vc-prepare-patch' handle prefix arguments.

* lisp/emacs-lisp/package-vc.el (package-vc-prepare-patch): Use
'vc-prepare-patch-prompt-revisions'.
* lisp/vc/vc.el (vc-prepare-patch-prompt-revisions): Extract common
function and handle prefix arguments.
(vc-prepare-patch): Pull logic out to
'vc-prepare-patch-prompt-revisions'.
---
 lisp/emacs-lisp/package-vc.el | 14 ++++++------
 lisp/vc/vc.el                 | 40 +++++++++++++++++++++++++----------
 2 files changed, 36 insertions(+), 18 deletions(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 664629d156..37ef35edad 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -733,17 +733,17 @@ package-vc-rebuild
 ;;;###autoload
 (defun package-vc-prepare-patch (pkg subject revisions)
   "Send patch for REVISIONS to maintainer of the package PKG using SUBJECT.
-SUBJECT and REVISIONS are passed on to `vc-prepare-patch', which see.
-PKG must be a package description.
-Interactively, prompt for PKG, SUBJECT, and REVISIONS.  However,
-if the current buffer has marked commit log entries, REVISIONS
-are the tags of the marked entries, see `log-view-get-marked'."
+SUBJECT and REVISIONS are passed on to `vc-prepare-patch', which
+see.  PKG must be a package description.  Interactively, prompt
+for PKG, SUBJECT, and REVISIONS.  When invoked with a numerical
+prefix argument, the last N revisions will be used.  When invoked
+interactively in a Log View buffer with marked revisions, those
+revisions will be used."
   (interactive
    (list (package-vc--read-package-desc "Package to prepare a patch for: " t)
          (and (not vc-prepare-patches-separately)
               (read-string "Subject: " "[PATCH] " nil nil t))
-         (or (log-view-get-marked)
-             (vc-read-multiple-revisions "Revisions: "))))
+         (vc-prepare-patch-prompt-revisions)))
   (vc-prepare-patch (package-maintainers pkg t)
                     subject revisions))
 
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 513fbb23fe..f49f5d3307 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -3384,6 +3384,30 @@ vc-default-prepare-patch
                                (vc-root-dir))))
             :buffer (current-buffer)))))
 
+(defun vc-prepare-patch-prompt-revisions ()
+  "Prompt the user for a list revisions.
+Prepare a default value, depending on the current context.  With
+a numerical prefix argument, use the last N revisions as the
+default value.  If the current buffer is a log-view buffer, use
+the marked commits.  Otherwise fall back to the working revision
+of the current file."
+  (vc-read-multiple-revisions
+   "Revisions: " nil nil nil
+   (or (and-let* ((arg current-prefix-arg)
+                  (fs (vc-deduce-fileset t)))
+         (cl-loop with file = (caadr fs)
+                  repeat (prefix-numeric-value arg)
+                  for rev = (vc-working-revision file)
+                  then (vc-call-backend
+                        (car fs) 'previous-revision
+                        file rev)
+                  when rev collect it into revs
+                  finally return (mapconcat #'identity revs ",")))
+       (and-let* ((revs (log-view-get-marked)))
+         (mapconcat #'identity revs ","))
+       (and-let* ((file (buffer-file-name)))
+         (vc-working-revision file)))))
+
 ;;;###autoload
 (defun vc-prepare-patch (addressee subject revisions)
   "Compose an Email sending patches for REVISIONS to ADDRESSEE.
@@ -3391,18 +3415,12 @@ vc-prepare-patch
 as the default subject for the message (and it will be prompted
 for when called interactively).  Otherwise a separate message
 will be composed for each revision, with SUBJECT derived from the
-invidividual commits.
-
-When invoked interactively in a Log View buffer with marked
-revisions, those revisions will be used."
+invidividual commits.  When invoked with a numerical prefix
+argument, the last N revisions will be used.  When invoked
+interactively in a Log View buffer with marked revisions, those
+revisions will be used."
   (interactive
-   (let ((revs (vc-read-multiple-revisions
-                "Revisions: " nil nil nil
-                (or (and-let* ((revs (log-view-get-marked)))
-                      (mapconcat #'identity revs ","))
-                    (and-let* ((file (buffer-file-name)))
-                      (vc-working-revision file)))))
-         to)
+   (let ((revs (vc-prepare-patch-prompt-revisions)) to)
      (require 'message)
      (while (null (setq to (completing-read-multiple
                             (format-prompt
-- 
2.35.1

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Eli Zaretskii]

> Cc: 58383@debbugs.gnu.org
> From: Philip Kaludercic <philipk@posteo.net>
> Date: Sun, 13 Nov 2022 13:56:38 +0000
> 
> --- a/lisp/vc/vc.el
> +++ b/lisp/vc/vc.el
> @@ -3391,14 +3391,24 @@ vc-prepare-patch
>  as the default subject for the message (and it will be prompted
>  for when called interactively).  Otherwise a separate message
>  will be composed for each revision, with SUBJECT derived from the
> -invidividual commits.
> -
> -When invoked interactively in a Log View buffer with marked
> -revisions, those revisions will be used."
> +invidividual commits.  When invoked with a numerical prefix
> +argument, the last N revisions will be used.  When invoked
> +interactively in a Log View buffer with marked revisions, those
> +revisions will be used."

Can we take this opportunity to get rid of the passive voice and
clarify the doc string, please?

Thanks.

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Philip Kaludercic]

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

Eli Zaretskii <eliz@gnu.org> writes:

> Can we take this opportunity to get rid of the passive voice and
> clarify the doc string, please?

Done:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Have-vc-prepare-patch-handle-prefix-arguments.patch --]
[-- Type: text/x-diff, Size: 5000 bytes --]

From 454c8c6ebb69c3cd3f671f7700ded14826492ce1 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Sun, 13 Nov 2022 17:05:20 +0100
Subject: [PATCH] Have 'vc-prepare-patch' handle prefix arguments.

* lisp/emacs-lisp/package-vc.el (package-vc-prepare-patch): Use
'vc-prepare-patch-prompt-revisions'.
* lisp/vc/vc.el (vc-prepare-patch-prompt-revisions): Extract common
function and handle prefix arguments.
(vc-prepare-patch): Pull logic out to
'vc-prepare-patch-prompt-revisions'.
---
 lisp/emacs-lisp/package-vc.el | 14 +++++------
 lisp/vc/vc.el                 | 47 ++++++++++++++++++++++++-----------
 2 files changed, 39 insertions(+), 22 deletions(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 664629d156..37ef35edad 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -733,17 +733,17 @@ package-vc-rebuild
 ;;;###autoload
 (defun package-vc-prepare-patch (pkg subject revisions)
   "Send patch for REVISIONS to maintainer of the package PKG using SUBJECT.
-SUBJECT and REVISIONS are passed on to `vc-prepare-patch', which see.
-PKG must be a package description.
-Interactively, prompt for PKG, SUBJECT, and REVISIONS.  However,
-if the current buffer has marked commit log entries, REVISIONS
-are the tags of the marked entries, see `log-view-get-marked'."
+SUBJECT and REVISIONS are passed on to `vc-prepare-patch', which
+see.  PKG must be a package description.  Interactively, prompt
+for PKG, SUBJECT, and REVISIONS.  When invoked with a numerical
+prefix argument, the last N revisions will be used.  When invoked
+interactively in a Log View buffer with marked revisions, those
+revisions will be used."
   (interactive
    (list (package-vc--read-package-desc "Package to prepare a patch for: " t)
          (and (not vc-prepare-patches-separately)
               (read-string "Subject: " "[PATCH] " nil nil t))
-         (or (log-view-get-marked)
-             (vc-read-multiple-revisions "Revisions: "))))
+         (vc-prepare-patch-prompt-revisions)))
   (vc-prepare-patch (package-maintainers pkg t)
                     subject revisions))
 
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 513fbb23fe..f71783db97 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -3384,25 +3384,42 @@ vc-default-prepare-patch
                                (vc-root-dir))))
             :buffer (current-buffer)))))
 
+(defun vc-prepare-patch-prompt-revisions ()
+  "Prompt the user for a list revisions.
+Prepare a default value, depending on the current context.  With
+a numerical prefix argument, use the last N revisions as the
+default value.  If the current buffer is a log-view buffer, use
+the marked commits.  Otherwise fall back to the working revision
+of the current file."
+  (vc-read-multiple-revisions
+   "Revisions: " nil nil nil
+   (or (and-let* ((arg current-prefix-arg)
+                  (fs (vc-deduce-fileset t)))
+         (cl-loop with file = (caadr fs)
+                  repeat (prefix-numeric-value arg)
+                  for rev = (vc-working-revision file)
+                  then (vc-call-backend
+                        (car fs) 'previous-revision
+                        file rev)
+                  when rev collect it into revs
+                  finally return (mapconcat #'identity revs ",")))
+       (and-let* ((revs (log-view-get-marked)))
+         (mapconcat #'identity revs ","))
+       (and-let* ((file (buffer-file-name)))
+         (vc-working-revision file)))))
+
 ;;;###autoload
 (defun vc-prepare-patch (addressee subject revisions)
   "Compose an Email sending patches for REVISIONS to ADDRESSEE.
-If `vc-prepare-patches-separately' is nil, SUBJECT will be used
-as the default subject for the message (and it will be prompted
-for when called interactively).  Otherwise a separate message
-will be composed for each revision, with SUBJECT derived from the
-invidividual commits.
-
-When invoked interactively in a Log View buffer with marked
-revisions, those revisions will be used."
+If `vc-prepare-patches-separately' is nil, use SUBJECT as the
+default subject for the message, or prompt a subject when invoked
+interactively.  Otherwise compose a separate message for each
+revision, with SUBJECT derived from each revision subject.  When
+invoked with a numerical prefix argument, use the last N
+revisions.  When invoked interactively in a Log View buffer with
+marked revisions, use those these."
   (interactive
-   (let ((revs (vc-read-multiple-revisions
-                "Revisions: " nil nil nil
-                (or (and-let* ((revs (log-view-get-marked)))
-                      (mapconcat #'identity revs ","))
-                    (and-let* ((file (buffer-file-name)))
-                      (vc-working-revision file)))))
-         to)
+   (let ((revs (vc-prepare-patch-prompt-revisions)) to)
      (require 'message)
      (while (null (setq to (completing-read-multiple
                             (format-prompt
-- 
2.35.1

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: spwhitton@spwhitton.name,  58383@debbugs.gnu.org
> Date: Sun, 13 Nov 2022 16:45:26 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Can we take this opportunity to get rid of the passive voice and
> > clarify the doc string, please?
> 
> Done:

Thanks, and the same with package-vc-prepare-patch, please?

Btw, this kind of reformatting:

> -SUBJECT and REVISIONS are passed on to `vc-prepare-patch', which see.
> -PKG must be a package description.
> -Interactively, prompt for PKG, SUBJECT, and REVISIONS.  However,
> -if the current buffer has marked commit log entries, REVISIONS
> -are the tags of the marked entries, see `log-view-get-marked'."
> +SUBJECT and REVISIONS are passed on to `vc-prepare-patch', which
> +see.  PKG must be a package description.  Interactively, prompt
> +for PKG, SUBJECT, and REVISIONS.  When invoked with a numerical
> +prefix argument, the last N revisions will be used.  When invoked
> +interactively in a Log View buffer with marked revisions, those
> +revisions will be used."

is IMO for the worse: when the sentence starting with "Interactively"
begins a new line, it stands out, which helps users find the most
relevant parts faster.

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Philip Kaludercic]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: spwhitton@spwhitton.name,  58383@debbugs.gnu.org
>> Date: Sun, 13 Nov 2022 16:45:26 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Can we take this opportunity to get rid of the passive voice and
>> > clarify the doc string, please?
>> 
>> Done:
>
> Thanks, and the same with package-vc-prepare-patch, please?
>
> Btw, this kind of reformatting:
>
>> -SUBJECT and REVISIONS are passed on to `vc-prepare-patch', which see.
>> -PKG must be a package description.
>> -Interactively, prompt for PKG, SUBJECT, and REVISIONS.  However,
>> -if the current buffer has marked commit log entries, REVISIONS
>> -are the tags of the marked entries, see `log-view-get-marked'."
>> +SUBJECT and REVISIONS are passed on to `vc-prepare-patch', which
>> +see.  PKG must be a package description.  Interactively, prompt
>> +for PKG, SUBJECT, and REVISIONS.  When invoked with a numerical
>> +prefix argument, the last N revisions will be used.  When invoked
>> +interactively in a Log View buffer with marked revisions, those
>> +revisions will be used."
>
> is IMO for the worse: when the sentence starting with "Interactively"
> begins a new line, it stands out, which helps users find the most
> relevant parts faster.

Addressed both issues here:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Have-vc-prepare-patch-handle-prefix-arguments.patch --]
[-- Type: text/x-diff, Size: 5195 bytes --]

From aeadd1555c4d75f63b0c115737d9f465534af687 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Sun, 13 Nov 2022 17:05:20 +0100
Subject: [PATCH] Have 'vc-prepare-patch' handle prefix arguments.

* lisp/emacs-lisp/package-vc.el (package-vc-prepare-patch): Use
'vc-prepare-patch-prompt-revisions'.
* lisp/vc/vc.el (vc-prepare-patch-prompt-revisions): Extract common
function and handle prefix arguments.
(vc-prepare-patch): Pull logic out to
'vc-prepare-patch-prompt-revisions'.
---
 lisp/emacs-lisp/package-vc.el | 18 ++++++-------
 lisp/vc/vc.el                 | 48 ++++++++++++++++++++++++-----------
 2 files changed, 42 insertions(+), 24 deletions(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 664629d156..156e7576f6 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -731,20 +731,20 @@ package-vc-rebuild
   (package-vc--unpack-1 pkg-desc (package-desc-dir pkg-desc)))
 
 ;;;###autoload
-(defun package-vc-prepare-patch (pkg subject revisions)
+(defun package-vc-prepare-patch (pkg-desc subject revisions)
   "Send patch for REVISIONS to maintainer of the package PKG using SUBJECT.
-SUBJECT and REVISIONS are passed on to `vc-prepare-patch', which see.
-PKG must be a package description.
-Interactively, prompt for PKG, SUBJECT, and REVISIONS.  However,
-if the current buffer has marked commit log entries, REVISIONS
-are the tags of the marked entries, see `log-view-get-marked'."
+The function uses `vc-prepare-patch', passing SUBJECT and
+REVISIONS directly.  PKG-DESC must be a package description.
+Interactively, prompt for PKG-DESC, SUBJECT, and REVISIONS.  When
+invoked with a numerical prefix argument, use the last N
+revisions.  When invoked interactively in a Log View buffer with
+marked revisions, use those."
   (interactive
    (list (package-vc--read-package-desc "Package to prepare a patch for: " t)
          (and (not vc-prepare-patches-separately)
               (read-string "Subject: " "[PATCH] " nil nil t))
-         (or (log-view-get-marked)
-             (vc-read-multiple-revisions "Revisions: "))))
-  (vc-prepare-patch (package-maintainers pkg t)
+         (vc-prepare-patch-prompt-revisions)))
+  (vc-prepare-patch (package-maintainers pkg-desc t)
                     subject revisions))
 
 (provide 'package-vc)
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 513fbb23fe..2314672bb8 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -3384,25 +3384,43 @@ vc-default-prepare-patch
                                (vc-root-dir))))
             :buffer (current-buffer)))))
 
+(defun vc-prepare-patch-prompt-revisions ()
+  "Prompt the user for a list revisions.
+Prepare a default value, depending on the current context.  With
+a numerical prefix argument, use the last N revisions as the
+default value.  If the current buffer is a log-view buffer, use
+the marked commits.  Otherwise fall back to the working revision
+of the current file."
+  (vc-read-multiple-revisions
+   "Revisions: " nil nil nil
+   (or (and-let* ((arg current-prefix-arg)
+                  (fs (vc-deduce-fileset t)))
+         (cl-loop with file = (caadr fs)
+                  repeat (prefix-numeric-value arg)
+                  for rev = (vc-working-revision file)
+                  then (vc-call-backend
+                        (car fs) 'previous-revision
+                        file rev)
+                  when rev collect it into revs
+                  finally return (mapconcat #'identity revs ",")))
+       (and-let* ((revs (log-view-get-marked)))
+         (mapconcat #'identity revs ","))
+       (and-let* ((file (buffer-file-name)))
+         (vc-working-revision file)))))
+
 ;;;###autoload
 (defun vc-prepare-patch (addressee subject revisions)
   "Compose an Email sending patches for REVISIONS to ADDRESSEE.
-If `vc-prepare-patches-separately' is nil, SUBJECT will be used
-as the default subject for the message (and it will be prompted
-for when called interactively).  Otherwise a separate message
-will be composed for each revision, with SUBJECT derived from the
-invidividual commits.
-
-When invoked interactively in a Log View buffer with marked
-revisions, those revisions will be used."
+If `vc-prepare-patches-separately' is nil, use SUBJECT as the
+default subject for the message, or prompt a subject when invoked
+interactively.  Otherwise compose a separate message for each
+revision, with SUBJECT derived from each revision subject.
+When invoked with a numerical prefix argument, use the last N
+revisions.
+When invoked interactively in a Log View buffer with
+marked revisions, use those these."
   (interactive
-   (let ((revs (vc-read-multiple-revisions
-                "Revisions: " nil nil nil
-                (or (and-let* ((revs (log-view-get-marked)))
-                      (mapconcat #'identity revs ","))
-                    (and-let* ((file (buffer-file-name)))
-                      (vc-working-revision file)))))
-         to)
+   (let ((revs (vc-prepare-patch-prompt-revisions)) to)
      (require 'message)
      (while (null (setq to (completing-read-multiple
                             (format-prompt
-- 
2.35.1


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


That being said, the patch now relies on changes made in
scratch/package-vc-fixes.  I can "backport" it onto master, or push it
in a few days when the issues related to that branch are resolved.

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: spwhitton@spwhitton.name,  58383@debbugs.gnu.org
> Date: Sun, 13 Nov 2022 18:17:43 +0000
> 
> Addressed both issues here:

Thanks!

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Sean Whitton]

Hello,

Just based on reviewing the docstrings, lgtm.  Thank you for your
interest in my idea.

-- 
Sean Whitton

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Philip Kaludercic]

Sean Whitton <spwhitton@spwhitton.name> writes:

> Hello,
>
> Just based on reviewing the docstrings, lgtm.  Thank you for your
> interest in my idea.

Great, I'll close the issue then.  The patch will be pushed along with
some package-vc related changes in the next new days.

bug#58383: 29.0.50; Make it easier to invert vc-prepare-patches-separately

[Eli Zaretskii]

> Cc: 58383@debbugs.gnu.org
> From: Sean Whitton <spwhitton@spwhitton.name>
> Date: Thu, 10 Nov 2022 17:07:03 -0700
> 
> >> On Wed 09 Nov 2022 at 05:46PM GMT, Philip Kaludercic wrote:
> >>
> >>> The critical edge-case here is what happens when branches are merged.
> >>> Do you pick a random branch or collect all the patches.  Or do you raise
> >>> an error, but then how do you detect that vc-generically.
> >>
> >> Typically you wouldn't want to format patches across a merge, so I would
> >> suggest raising an error.
> >
> > And this is something I don't think can be /expressed/ using vc, because
> > while I can collect a number of revisions using `previous-revision',
> > there is no general way to verify if a commit is a merge commit.
> 
> Can we do that part on a VCS-by-VCS basis?  Default to just calling
> previous-revision and hoping for the best, but giving vc-git.el a chance
> to raise an error.

Please don't forget that Git's notion and implementation of
merge-commits is (AFAIR) quite unique.  I'm not sure there's another
VCS which handles merge-commits like Git does.

So when you build your notion of what merge-commit is and how to deal
with it in the context of this issue, I think it is best to study what
the other VCSes do, before you base the design on what Git does.