bug#57400: 29.0.50; Support sending patches from VC directly

bug#57400: 29.0.50; Support sending patches from VC directly

[Antoine Kalmbach]

See the discussion here

https://mail.gnu.org/archive/html/emacs-devel/2022-08/msg01059.html

The idea would be to support sending patches using Emacs mail user agent
capabilities directly from VC projects. This would depend on the chosen
VC backend and whether it has support for email-based workflows in the
first place.

The reference implementation would be for Git. A command such as
`vc-prepare-patch`, which would prompt for a Git revision range and then
generate a set of .patch files. These then would be opened in the mail
user agent configured in Emacs for sending.


In GNU Emacs 29.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.34, cairo version 1.17.6) of 2022-08-23 built on thanatos
Repository revision: 67a15ce1564ce35ece24a19f00e03a36e0575746
Repository branch: master
System Description: Fedora Linux 36 (Workstation Edition)

Configured using:
 'configure --with-x-toolkit=motif --with-json --with-native-compilation
 --with-pgtk --with-imagemagick --with-mailutils'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ
IMAGEMAGICK JPEG JSON LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 MODULES
NATIVE_COMP NOTIFY INOTIFY PDUMPER PGTK PNG RSVG SECCOMP SOUND SQLITE3
THREADS TIFF TOOLKIT_SCROLL_BARS XIM GTK3 ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8

Major mode: notmuch-hello

Minor modes in effect:
  eros-mode: t
  electric-pair-mode: t
  global-corfu-mode: t
  corfu-mode: t
  marginalia-mode: t
  vertico-mode: t
  recentf-mode: t
  savehist-mode: t
  display-time-mode: t
  global-hl-line-mode: t
  global-auto-revert-mode: t
  winner-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tab-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  column-number-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
/home/ane/.emacs.d/elpa/transient-20220806.2224/transient hides /usr/local/share/emacs/29.0.50/lisp/transient

Features:
(shadow emacsbug rfc2104 secrets dbus xml network-stream nsm mailalias
textsec uni-scripts idna-mapping ucs-normalize uni-confusable
textsec-check eudc-capf eudc cus-edit cus-start cus-load eudc-vars sort
company-oddmuse company-keywords company-etags etags fileloop generator
company-gtags company-dabbrev-code company-dabbrev company-files
company-clang company-capf company-cmake company-semantic
company-template company-bbdb company notmuch notmuch-tree notmuch-jump
notmuch-hello notmuch-show notmuch-print notmuch-crypto notmuch-mua
notmuch-message notmuch-draft notmuch-maildir-fcc notmuch-address
notmuch-company notmuch-parser format-spec notmuch-wash coolj goto-addr
icalendar diary-lib diary-loaddefs cal-menu calendar cal-loaddefs
notmuch-tag crm notmuch-lib notmuch-compat mm-view mml-smime smime
gnutls dig mule-util mail-extr flyspell ispell smex ido eros checkdoc
lisp-mnt noutline outline rainbow-mode color rainbow-delimiters paredit
eldoc-box eglot array xref flymake-proc flymake thingatpt compile comint
ansi-color project imenu jsonrpc ert pp ewoc debug backtrace advice
find-func vc-hg vc-git diff-mode vc-bzr vc-src vc-sccs vc-svn vc-cvs
vc-rcs log-view pcvs-util vc vc-dispatcher bug-reference elec-pair corfu
marginalia vertico recentf tree-widget wid-edit cc-mode cc-fonts
cc-guess cc-menus cc-cmds cc-styles cc-align cc-engine cc-vars cc-defs
hydra lv smtpmail message sendmail yank-media puny dired dired-loaddefs
rfc822 mml mml-sec epa derived epg rfc6068 epg-config gnus-util
text-property-search time-date mm-decode mm-bodies mm-encode mail-parse
rfc2231 rfc2047 rfc2045 mm-util ietf-drums mail-prsvr mailabbrev
mail-utils gmm-utils mailheader diminish modus-operandi-theme
modus-themes use-package-diminish edmacro kmacro use-package-bind-key
bind-key easy-mmode use-package-ensure use-package-core savehist comp
comp-cstr warnings icons rx cl-extra pcase finder-inf
ace-window-autoloads avy-autoloads clj-refactor-autoloads
cider-autoloads clojure-mode-extra-font-locking-autoloads
clojure-mode-autoloads company-autoloads consult-autoloads
corfu-doc-autoloads corfu-autoloads debbugs-autoloads diff-hl-autoloads
diminish-autoloads edit-indirect-autoloads eglot-autoloads
eldoc-box-autoloads eros-autoloads exec-path-from-shell-autoloads
expand-region-autoloads flycheck-autoloads geiser-guile-autoloads
geiser-impl help-fns radix-tree help-mode geiser-custom geiser-base
geiser-autoloads gif-screencast-autoloads go-mode-autoloads
google-c-style-autoloads graphviz-dot-mode-autoloads hl-todo-autoloads
hydra-autoloads inflections-autoloads keycast-autoloads
kind-icon-autoloads lv-autoloads magit-autoloads git-commit-autoloads
magit-section-autoloads marginalia-autoloads markdown-mode-autoloads
meson-mode-autoloads modus-themes-autoloads multiple-cursors-autoloads
neotree-autoloads ninja-mode-autoloads notmuch-autoloads
paredit-autoloads parseedn-autoloads parseclj-autoloads
pinentry-autoloads pkg-info-autoloads epl-autoloads
plantuml-mode-autoloads dash-autoloads queue-autoloads
rainbow-delimiters-autoloads rainbow-mode-autoloads rust-mode-autoloads
scss-mode-autoloads sesman-autoloads sly-asdf-autoloads popup-autoloads
sly-macrostep-autoloads macrostep-autoloads
sly-repl-ansi-color-autoloads sly-autoloads smex-autoloads
spinner-autoloads ssh-config-mode-autoloads svg-lib-autoloads
transient-autoloads tree-sitter-langs-autoloads tree-sitter-autoloads
tsc-autoloads typescript-mode-autoloads use-package-autoloads
bind-key-autoloads vertico-autoloads web-mode-autoloads wgrep-autoloads
with-editor-autoloads info compat-autoloads yaml-mode-autoloads
yasnippet-snippets-autoloads yasnippet-autoloads zig-mode-autoloads
package browse-url url url-proxy url-privacy url-expand url-methods
url-history url-cookie generate-lisp-file url-domsuf url-util mailcap
url-handlers url-parse auth-source cl-seq eieio eieio-core cl-macs
password-cache json subr-x map byte-opt gv bytecomp byte-compile cconv
url-vars time hl-line autorevert filenotify cl-loaddefs cl-lib winner
ring rmc iso-transl tooltip eldoc paren electric uniquify ediff-hook
vc-hooks lisp-float-type elisp-mode mwheel term/pgtk-win pgtk-win
term/common-win pgtk-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs faces cus-face macroexp files window
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget keymap hashtable-print-readable backquote threads dbusbind
inotify dynamic-setting system-font-setting font-render-setting cairo
gtk pgtk multi-tty make-network-process native-compile emacs)

Memory information:
((conses 16 815238 428939)
 (symbols 48 32388 106)
 (strings 32 177583 63256)
 (string-bytes 1 5448488)
 (vectors 16 78687)
 (vector-slots 8 1356913 2880582)
 (floats 8 317 2125)
 (intervals 56 1817 7744)
 (buffers 1000 18))

-- 
Antoine Kalmbach

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Antoine Kalmbach <ane@iki.fi> writes:

>
>
> See the discussion here
>
> https://mail.gnu.org/archive/html/emacs-devel/2022-08/msg01059.html
>
> The idea would be to support sending patches using Emacs mail user agent
> capabilities directly from VC projects. This would depend on the chosen
> VC backend and whether it has support for email-based workflows in the
> first place.
>
> The reference implementation would be for Git. A command such as
> `vc-prepare-patch`, which would prompt for a Git revision range and then
> generate a set of .patch files. These then would be opened in the mail
> user agent configured in Emacs for sending.

Have you started implementing anything yet?

bug#57400: 29.0.50; Support sending patches from VC directly

[Antoine Kalmbach]

Philip Kaludercic <philipk@posteo.net> writes:

> Have you started implementing anything yet?

No, not yet.  Hadn't you also thought about implementing this, if I
recall correctly?

-- 
Antoine Kalmbach

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Antoine Kalmbach <ane@iki.fi> writes:

>
> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Have you started implementing anything yet?
>
> No, not yet.  Hadn't you also thought about implementing this, if I
> recall correctly?

Yes, and I began implementing a different approach (as mentioned on the
emacs-devel thread), which I have since abandoned.  If you haven't
written anything yet, and don't insist on it, I could propose to start
sketching out your suggestions.

bug#57400: 29.0.50; Support sending patches from VC directly

[Antoine Kalmbach]

Philip Kaludercic <philipk@posteo.net> writes:

> Yes, and I began implementing a different approach (as mentioned on the
> emacs-devel thread), which I have since abandoned.  If you haven't
> written anything yet, and don't insist on it, I could propose to start
> sketching out your suggestions.

Sure!  I was thinking we could start from a very basic command, call it
`vc-prepare-patch` as per your suggestion. Since VC uses generics, we
can dispatch to backend-specific implementations, something like this,
with Git:

  1. `M-x vc-prepare-patch`
  2. Dispatch to `vc-git-prepare-patch`
  3. Git wants a revision range, so interactively prompt for that
     (e.g. `HEAD^`, `abcd1234..ghjk5678`, or `-1`)
  4. `call-process` to `git format-patch $REV`, and so forth, get the
     list of files.
  5. Loop each file in `message-mode`. `C-c C-c` sends and goes to the
     next patch, `C-c C-k` cancels the whole thing.

Once the file opens in message-mode, most likely we need to strip the
magic From <sha> <date> header from the beginning of the mail. Then we ensure
don't do any nasty whitespace removal or wrapping.

Most likely, depending on the backend, we should not require any
parameters besides the "set of changes". For instance, in Git you can
configure `git-format-patch` in the git configuration for several
attributes, like --to=, --annotate, --prefix, etc.

I don't remember how Mercurial works with this. Probably similar. It
should generate mbox entries as well, I think. 

-- 
Antoine Kalmbach

bug#57400: 29.0.50; Support sending patches from VC directly

[Eli Zaretskii]

> Cc: 57400@debbugs.gnu.org
> From: Antoine Kalmbach <ane@iki.fi>
> Date: Fri, 26 Aug 2022 13:45:51 +0300
> 
>   1. `M-x vc-prepare-patch`
>   2. Dispatch to `vc-git-prepare-patch`
>   3. Git wants a revision range, so interactively prompt for that
>      (e.g. `HEAD^`, `abcd1234..ghjk5678`, or `-1`)

Please allow the user to specify the range of commits in a log-like
display, e.g. by having mark and point around them.  It makes no sense
to force users to type revisions in the Git syntax, and come up with
the SHA1 codes on top of that.  VC is supposed to be a convenient UI,
not just a dumb front-end to the VCS.

>   4. `call-process` to `git format-patch $REV`, and so forth, get the
>      list of files.

By "list of files" do you mean the list of patch files?

>   5. Loop each file in `message-mode`. `C-c C-c` sends and goes to the
>      next patch, `C-c C-k` cancels the whole thing.

Please don't hard-code message-mode.  Please honor the user setting of
mail-user-agent instead.

Also, I'm not sure why we'd need to send each patch file separately.
Why not add them one by one as attachments to the same email message?

> Most likely, depending on the backend, we should not require any
> parameters besides the "set of changes".

If you allow to specify that via the region, all your problems are
solved, and the only one that remains is how to express that in the
backend-specific syntax.

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>>   5. Loop each file in `message-mode`. `C-c C-c` sends and goes to the
>>      next patch, `C-c C-k` cancels the whole thing.
>
> Please don't hard-code message-mode.  Please honor the user setting of
> mail-user-agent instead.

Is there a generic way to handle any mail-user-agent?  E.g. if you want
to add attachments, is there any better way that just doing a case
distinction on known user agent implementations?

> Also, I'm not sure why we'd need to send each patch file separately.
> Why not add them one by one as attachments to the same email message?

This wouldn't work if we are sending patches to a mailing list that
assumes patches are sent out by git send-email, and that the messages
can be filtered through git am.

bug#57400: 29.0.50; Support sending patches from VC directly

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: Antoine Kalmbach <ane@iki.fi>,  57400@debbugs.gnu.org
> Date: Fri, 26 Aug 2022 11:26:29 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >>   5. Loop each file in `message-mode`. `C-c C-c` sends and goes to the
> >>      next patch, `C-c C-k` cancels the whole thing.
> >
> > Please don't hard-code message-mode.  Please honor the user setting of
> > mail-user-agent instead.
> 
> Is there a generic way to handle any mail-user-agent?

For some value of "generic way to handle", yes.  For example,
compose-mail does that.

> E.g. if you want to add attachments, is there any better way that
> just doing a case distinction on known user agent implementations?

Not sure about attachments, but we could either add another property
to mail-user-agent, like we do with composefunc property, or dispatch
on the agent itself.

> > Also, I'm not sure why we'd need to send each patch file separately.
> > Why not add them one by one as attachments to the same email message?
> 
> This wouldn't work if we are sending patches to a mailing list that
> assumes patches are sent out by git send-email, and that the messages
> can be filtered through git am.

"git am" handles attachments without any problems, I do it all the
time.

But I don't object to having optional behaviors here.  My point is
that we should allow sending all the patches together, as that is the
preferred/usual practice in Emacs development.

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>
>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: Antoine Kalmbach <ane@iki.fi>,  57400@debbugs.gnu.org
>> Date: Fri, 26 Aug 2022 11:26:29 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >>   5. Loop each file in `message-mode`. `C-c C-c` sends and goes to the
>> >>      next patch, `C-c C-k` cancels the whole thing.
>> >
>> > Please don't hard-code message-mode.  Please honor the user setting of
>> > mail-user-agent instead.
>> 
>> Is there a generic way to handle any mail-user-agent?
>
> For some value of "generic way to handle", yes.  For example,
> compose-mail does that.
>
>> E.g. if you want to add attachments, is there any better way that
>> just doing a case distinction on known user agent implementations?
>
> Not sure about attachments, but we could either add another property
> to mail-user-agent, like we do with composefunc property, or dispatch
> on the agent itself.

That sounds like a good approach.

>> > Also, I'm not sure why we'd need to send each patch file separately.
>> > Why not add them one by one as attachments to the same email message?
>> 
>> This wouldn't work if we are sending patches to a mailing list that
>> assumes patches are sent out by git send-email, and that the messages
>> can be filtered through git am.
>
> "git am" handles attachments without any problems, I do it all the
> time.

Only if the MUA can recognise the patch and pipe it into a git am
process.  But if we are trying to re-create the behaviour of "git
send-email" (as I think is necessary if we want the feature to be of use
outside of Emacs circles, such as sending a patch to the Linux Kernel
Mailing List), then we need to consider people using clients like Mutt
or Aerc (https://aerc-mail.org/) that just pipe the entire message
through "git am".

> But I don't object to having optional behaviors here.  My point is
> that we should allow sending all the patches together, as that is the
> preferred/usual practice in Emacs development.

Of course, the idea that was proposed on emacs-devel was to have this
behaviour be controlled by a (file-local) variable that could be set on
a per-project basis.

bug#57400: 29.0.50; Support sending patches from VC directly

[Antoine Kalmbach]

Eli Zaretskii <eliz@gnu.org> writes:

>> > Also, I'm not sure why we'd need to send each patch file separately.
>> > Why not add them one by one as attachments to the same email message?
>> 
>> This wouldn't work if we are sending patches to a mailing list that
>> assumes patches are sent out by git send-email, and that the messages
>> can be filtered through git am.
>
> "git am" handles attachments without any problems, I do it all the
> time.
>

But you have to save the attached .patch files and then run git am on
those.  Typically, without attachments, you'd run `git am` on the
message (mbox) directly.  Each mail is a patch in that sense, and a
separate cover letter is sent first when there's some introductory words
that need to be said.

> But I don't object to having optional behaviors here.  My point is
> that we should allow sending all the patches together, as that is the
> preferred/usual practice in Emacs development.

Yes, but users would have to apply the customization to get the Emacs
way of working, I think.  

-- 
Antoine Kalmbach

bug#57400: 29.0.50; Support sending patches from VC directly

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: ane@iki.fi,  57400@debbugs.gnu.org
> Date: Fri, 26 Aug 2022 12:05:07 +0000
> 
> >> > Also, I'm not sure why we'd need to send each patch file separately.
> >> > Why not add them one by one as attachments to the same email message?
> >> 
> >> This wouldn't work if we are sending patches to a mailing list that
> >> assumes patches are sent out by git send-email, and that the messages
> >> can be filtered through git am.
> >
> > "git am" handles attachments without any problems, I do it all the
> > time.
> 
> Only if the MUA can recognise the patch and pipe it into a git am
> process.

What do you mean by "MUA can recognize" here? which Emacs MUA
recognizes Git-formatted patches and applies them?

What I do is invoke "M-|" and send the region to "git am".  That
requires myself to recognize the patches, not the MUA I use.

> But if we are trying to re-create the behaviour of "git
> send-email" (as I think is necessary if we want the feature to be of use
> outside of Emacs circles, such as sending a patch to the Linux Kernel
> Mailing List), then we need to consider people using clients like Mutt
> or Aerc (https://aerc-mail.org/) that just pipe the entire message
> through "git am".

Do you intend to provide a VC front-end to applying the patch-set, as
part of this job?  Because if not, what happens on the receiving end
is out of the scope of the feature we are discussing.

> > But I don't object to having optional behaviors here.  My point is
> > that we should allow sending all the patches together, as that is the
> > preferred/usual practice in Emacs development.
> 
> Of course, the idea that was proposed on emacs-devel was to have this
> behaviour be controlled by a (file-local) variable that could be set on
> a per-project basis.

We should have a user option that doesn't require project.el
(project.el can override it, of course).  There should be no
requirement to use project.el to send patches from VC.

bug#57400: 29.0.50; Support sending patches from VC directly

[Eli Zaretskii]

> From: Antoine Kalmbach <ane@iki.fi>
> Cc: 57400@debbugs.gnu.org
> Date: Fri, 26 Aug 2022 15:08:01 +0300
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> > Also, I'm not sure why we'd need to send each patch file separately.
> >> > Why not add them one by one as attachments to the same email message?
> >> 
> >> This wouldn't work if we are sending patches to a mailing list that
> >> assumes patches are sent out by git send-email, and that the messages
> >> can be filtered through git am.
> >
> > "git am" handles attachments without any problems, I do it all the
> > time.
> 
> But you have to save the attached .patch files and then run git am on
> those.

No, I just use M-| from Emacs.

> > But I don't object to having optional behaviors here.  My point is
> > that we should allow sending all the patches together, as that is the
> > preferred/usual practice in Emacs development.
> 
> Yes, but users would have to apply the customization to get the Emacs
> way of working, I think.  

You are now arguing about the default value of that option?  Let's
delay that until the feature is done and the option is available,
okay?

bug#57400: 29.0.50; Support sending patches from VC directly

[Antoine Kalmbach]

Eli Zaretskii <eliz@gnu.org> writes:

>> Only if the MUA can recognise the patch and pipe it into a git am
>> process.
>
> What do you mean by "MUA can recognize" here? which Emacs MUA
> recognizes Git-formatted patches and applies them?

I don't think any does.  Insofar as Mutt and Aerc are concerned, all they
provide is functionality for syntax highlighting the diff and then a
command for piping the message.  Emacs can do, and does, both of
those. (At least Gnus highlights patch blocks.)

> What I do is invoke "M-|" and send the region to "git am".  That
> requires myself to recognize the patches, not the MUA I use.

If the patch is attached, you open the patch, mark it, and then M-| git
am, right? The standard Git approach is to just pipe the whole message,
expecting the patch to be in the email directly.

Or even with attachments, can you actually mark the whole email buffer
and pipe that? 

> Do you intend to provide a VC front-end to applying the patch-set, as
> part of this job?  Because if not, what happens on the receiving end
> is out of the scope of the feature we are discussing.
>

Having a complementary `vc-apply-patch` wouldn't be a bad idea, but
I think we should do the sending part first. 

>
> We should have a user option that doesn't require project.el
> (project.el can override it, of course).  There should be no
> requirement to use project.el to send patches from VC.

I think Philip means directory-local variables, not project.el.

-- 
Antoine Kalmbach

bug#57400: 29.0.50; Support sending patches from VC directly

[Eli Zaretskii]

> From: Antoine Kalmbach <ane@iki.fi>
> Cc: 57400@debbugs.gnu.org
> Date: Fri, 26 Aug 2022 16:10:58 +0300
> 
> > What I do is invoke "M-|" and send the region to "git am".  That
> > requires myself to recognize the patches, not the MUA I use.
> 
> If the patch is attached, you open the patch, mark it, and then M-| git
> am, right?

Yes (modulo "cd .." to the right directory).

> The standard Git approach is to just pipe the whole message,
> expecting the patch to be in the email directly.

The patch attachments can all be shown inline, since they are text,
and then you can pipe the entire message, yes.

> Or even with attachments, can you actually mark the whole email buffer
> and pipe that? 

I think I once did that, and it worked.  Not sure.

But if you intend to provide/improve what happens on the receiving end
as well, then Emacs could filter the irrelevant parts from what it
sends down the "git am" pipe.

> > Do you intend to provide a VC front-end to applying the patch-set, as
> > part of this job?  Because if not, what happens on the receiving end
> > is out of the scope of the feature we are discussing.
> 
> Having a complementary `vc-apply-patch` wouldn't be a bad idea, but
> I think we should do the sending part first. 

Fine by me.

> > We should have a user option that doesn't require project.el
> > (project.el can override it, of course).  There should be no
> > requirement to use project.el to send patches from VC.
> 
> I think Philip means directory-local variables, not project.el.

Once we have an option, it can be included in directory-local
variables, right?

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>
>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: ane@iki.fi,  57400@debbugs.gnu.org
>> Date: Fri, 26 Aug 2022 12:05:07 +0000
>> 
>> >> > Also, I'm not sure why we'd need to send each patch file separately.
>> >> > Why not add them one by one as attachments to the same email message?
>> >> 
>> >> This wouldn't work if we are sending patches to a mailing list that
>> >> assumes patches are sent out by git send-email, and that the messages
>> >> can be filtered through git am.
>> >
>> > "git am" handles attachments without any problems, I do it all the
>> > time.
>> 
>> Only if the MUA can recognise the patch and pipe it into a git am
>> process.
>
> What do you mean by "MUA can recognize" here? which Emacs MUA
> recognizes Git-formatted patches and applies them?

The MUA recognizes the patch as a an attachment.  E.g. in Gnus the patch
is highlighted and "|" is bound to a command that pipes the contents of
the attachment through a command.

> What I do is invoke "M-|" and send the region to "git am".  That
> requires myself to recognize the patches, not the MUA I use.

I hadn't considered that, but again, if we are thinking about preparing
messages that are sent out to other developers using other MUAs, then I
don't know if this kind of functionality is available.

>> But if we are trying to re-create the behaviour of "git
>> send-email" (as I think is necessary if we want the feature to be of use
>> outside of Emacs circles, such as sending a patch to the Linux Kernel
>> Mailing List), then we need to consider people using clients like Mutt
>> or Aerc (https://aerc-mail.org/) that just pipe the entire message
>> through "git am".
>
> Do you intend to provide a VC front-end to applying the patch-set, as
> part of this job?  Because if not, what happens on the receiving end
> is out of the scope of the feature we are discussing.

No, this is just about sending patches, but if the patches sent out are
of no use to the developers receiving them, then the feature is not as
useful as it could be.

>> > But I don't object to having optional behaviors here.  My point is
>> > that we should allow sending all the patches together, as that is the
>> > preferred/usual practice in Emacs development.
>> 
>> Of course, the idea that was proposed on emacs-devel was to have this
>> behaviour be controlled by a (file-local) variable that could be set on
>> a per-project basis.
>
> We should have a user option that doesn't require project.el
> (project.el can override it, of course).  There should be no
> requirement to use project.el to send patches from VC.

There should be no need for project.el, this could just be set in dir
.dir-locals.el file in emacs.git.

But I don't think there is an actual issue here, the plan has been all
along to provide both kinds of patches (git am-style, attached) to be
sent out.

bug#57400: 29.0.50; Support sending patches from VC directly

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: ane@iki.fi,  57400@debbugs.gnu.org
> Date: Fri, 26 Aug 2022 13:29:22 +0000
> 
> >> Only if the MUA can recognise the patch and pipe it into a git am
> >> process.
> >
> > What do you mean by "MUA can recognize" here? which Emacs MUA
> > recognizes Git-formatted patches and applies them?
> 
> The MUA recognizes the patch as a an attachment.  E.g. in Gnus the patch
> is highlighted and "|" is bound to a command that pipes the contents of
> the attachment through a command.
> 
> > What I do is invoke "M-|" and send the region to "git am".  That
> > requires myself to recognize the patches, not the MUA I use.
> 
> I hadn't considered that, but again, if we are thinking about preparing
> messages that are sent out to other developers using other MUAs, then I
> don't know if this kind of functionality is available.

I fail to see how other MUAs used by other developers are relevant
here.  We are talking about sending patches, not receiving them.
Sending them as attachments is a known, and frequently required or
preferred, technique.  How the patches are handled on the receiving
end is a separate problem, but it already has its solution, because
people send patches as attachments all over the place.

> >> But if we are trying to re-create the behaviour of "git
> >> send-email" (as I think is necessary if we want the feature to be of use
> >> outside of Emacs circles, such as sending a patch to the Linux Kernel
> >> Mailing List), then we need to consider people using clients like Mutt
> >> or Aerc (https://aerc-mail.org/) that just pipe the entire message
> >> through "git am".
> >
> > Do you intend to provide a VC front-end to applying the patch-set, as
> > part of this job?  Because if not, what happens on the receiving end
> > is out of the scope of the feature we are discussing.
> 
> No, this is just about sending patches, but if the patches sent out are
> of no use to the developers receiving them, then the feature is not as
> useful as it could be.

They cannot be "of no use", because these practices are already in
use, regardless of whether Emacs does or doesn't support them.

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>
>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: ane@iki.fi,  57400@debbugs.gnu.org
>> Date: Fri, 26 Aug 2022 13:29:22 +0000
>> 
>> >> Only if the MUA can recognise the patch and pipe it into a git am
>> >> process.
>> >
>> > What do you mean by "MUA can recognize" here? which Emacs MUA
>> > recognizes Git-formatted patches and applies them?
>> 
>> The MUA recognizes the patch as a an attachment.  E.g. in Gnus the patch
>> is highlighted and "|" is bound to a command that pipes the contents of
>> the attachment through a command.
>> 
>> > What I do is invoke "M-|" and send the region to "git am".  That
>> > requires myself to recognize the patches, not the MUA I use.
>> 
>> I hadn't considered that, but again, if we are thinking about preparing
>> messages that are sent out to other developers using other MUAs, then I
>> don't know if this kind of functionality is available.
>
> I fail to see how other MUAs used by other developers are relevant
> here.  We are talking about sending patches, not receiving them.
> Sending them as attachments is a known, and frequently required or
> preferred, technique.  How the patches are handled on the receiving
> end is a separate problem, but it already has its solution, because
> people send patches as attachments all over the place.

But there are plenty of projects that will only accept patches
formatted and sent by "git send-email".  If you send them an attachment
with a patch, they might complain and ask you to send it back again
using "git send-email".

Once again, I don't think we disagree on anything substantial here.  You
have already said that it is fine to implement support for both, and
everything beyond that should not matter for the proposal.

bug#57400: 29.0.50; Support sending patches from VC directly

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: ane@iki.fi,  57400@debbugs.gnu.org
> Date: Sat, 27 Aug 2022 08:21:36 +0000
> 
> But there are plenty of projects that will only accept patches
> formatted and sent by "git send-email".  If you send them an attachment
> with a patch, they might complain and ask you to send it back again
> using "git send-email".
> 
> Once again, I don't think we disagree on anything substantial here.  You
> have already said that it is fine to implement support for both, and
> everything beyond that should not matter for the proposal.

So why are we still arguing?

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>
>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: ane@iki.fi,  57400@debbugs.gnu.org
>> Date: Sat, 27 Aug 2022 08:21:36 +0000
>> 
>> But there are plenty of projects that will only accept patches
>> formatted and sent by "git send-email".  If you send them an attachment
>> with a patch, they might complain and ask you to send it back again
>> using "git send-email".
>> 
>> Once again, I don't think we disagree on anything substantial here.  You
>> have already said that it is fine to implement support for both, and
>> everything beyond that should not matter for the proposal.
>
> So why are we still arguing?

I don't know.

bug#57400: 29.0.50; Support sending patches from VC directly

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

    > 1. `M-x vc-prepare-patch`
    > 2. Dispatch to `vc-git-prepare-patch`

That would happen if the file uses git,
Please implement a default alternative that works generically
for other VC systems.

    > 3. Git wants a revision range, so interactively prompt for that
    >    (e.g. `HEAD^`, `abcd1234..ghjk5678`, or `-1`)
    > 4. `call-process` to `git format-patch $REV`, and so forth, get the
    >    list of files.
    > 5. Loop each file in `message-mode`. `C-c C-c` sends and goes to the
    >    next patch, `C-c C-k` cancels the whole thing.

`message-mode' is one of Emacs's packages for composing mail.  Please
use `compose-mail' so that you use whichever one the user has selected.

I think you can insert the patch text into the composition buffer
after `compose-mail' returns, and it will work with either of the two
usual mail composition packages.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

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

Antoine Kalmbach <ane@iki.fi> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Yes, and I began implementing a different approach (as mentioned on the
>> emacs-devel thread), which I have since abandoned.  If you haven't
>> written anything yet, and don't insist on it, I could propose to start
>> sketching out your suggestions.
>
> Sure!  I was thinking we could start from a very basic command, call it
> `vc-prepare-patch` as per your suggestion. Since VC uses generics, we
> can dispatch to backend-specific implementations, something like this,
> with Git:
>
>   1. `M-x vc-prepare-patch`
>   2. Dispatch to `vc-git-prepare-patch`
>   3. Git wants a revision range, so interactively prompt for that
>      (e.g. `HEAD^`, `abcd1234..ghjk5678`, or `-1`)
>   4. `call-process` to `git format-patch $REV`, and so forth, get the
>      list of files.
>   5. Loop each file in `message-mode`. `C-c C-c` sends and goes to the
>      next patch, `C-c C-k` cancels the whole thing.

Sorry for the delay, here is a first approximation of this idea:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-a-command-to-send-patches.patch --]
[-- Type: text/x-patch, Size: 6852 bytes --]

From a350b7cbd1b61925c687b501c6251a8ef4fb5549 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Mon, 3 Oct 2022 20:54:38 +0200
Subject: [PATCH] Add a command to send patches

* lisp/vc/vc-git.el (vc-git-prepare-patch):  Add Git implementation.
* lisp/vc/vc.el (vc-read-revision):  Add a MULTIPLE argument.
 (vc-read-multiple-revisions):  Add an auxiliary function that always
 calls 'vc-read-revision' with a non-nil value for MULTIPLE.
(vc-compose-patches-inline):  Add user option.
(message-goto-body):  Declare function.
(message--name-table):  Declare function.
(vc-compose-patch):  Add command.  (bug#57400)
---
 lisp/vc/vc-git.el | 27 ++++++++++++++++++
 lisp/vc/vc.el     | 70 ++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 93 insertions(+), 4 deletions(-)

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index f5ac43f536..439e82b12e 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1705,6 +1705,33 @@ vc-git-extra-status-menu
 (defun vc-git-root (file)
   (vc-find-root file ".git"))
 
+(defun vc-git-prepare-patch (rev)
+  (with-temp-buffer
+    (call-process vc-git-program nil t nil "format-patch"
+                  "--no-numbered" "--stdout"
+                  ;; From gitrevisions(7): ^<n> means the <n>th parent
+                  ;; (i.e.  <rev>^ is equivalent to <rev>^1). As a
+                  ;; special rule, <rev>^0 means the commit itself and
+                  ;; is used when <rev> is the object name of a tag
+                  ;; object that refers to a commit object.
+                  (concat rev "^0"))
+    (let (filename subject body)
+      ;; Save the patch in a temporary file if required
+      (when (bound-and-true-p vc-compose-patches-inline)
+        (setq filename (make-temp-file "vc-git-prepare-patch"))
+        (write-region nil nil filename)) ;FIXME: Clean up
+      ;; Extract the subject line
+      (goto-char (point-min))
+      (search-forward-regexp "^Subject: \\(.+\\)")
+      (setq subject (match-string 1))
+      ;; Extract the patch body
+      (search-forward-regexp "\n\n")
+      (setq body (buffer-substring (point) (point-max)))
+      ;; Return the extracted data
+      (list :filename filename
+            :subject subject
+            :body body))))
+
 ;; grep-compute-defaults autoloads grep.
 (declare-function grep-read-regexp "grep" ())
 (declare-function grep-read-files "grep" (regexp))
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 24300e014a..e990f51a91 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -574,6 +574,14 @@
 ;;   containing FILE-OR-DIR.  The optional REMOTE-NAME specifies the
 ;;   remote (in Git parlance) whose URL is to be returned.  It has
 ;;   only a meaning for distributed VCS and is ignored otherwise.
+;;
+;; - prepare-patch (rev)
+;;
+;;   Prepare a patch and return a property list with the keys
+;;   `:subject' indicating the patch message as a string, `:body'
+;;   containing the contents of the patch as a string (excluding the
+;;   header) and `:filename' pointing to a file where the patch has
+;;   been stored.
 
 ;;; Changes from the pre-25.1 API:
 ;;
@@ -1910,7 +1918,7 @@ vc-diff-internal
 (defvar vc-revision-history nil
   "History for `vc-read-revision'.")
 
-(defun vc-read-revision (prompt &optional files backend default initial-input)
+(defun vc-read-revision (prompt &optional files backend default initial-input multiple)
   (cond
    ((null files)
     (let ((vc-fileset (vc-deduce-fileset t))) ;FIXME: why t?  --Stef
@@ -1920,9 +1928,16 @@ vc-read-revision
   (let ((completion-table
          (vc-call-backend backend 'revision-completion-table files)))
     (if completion-table
-        (completing-read prompt completion-table
-                         nil nil initial-input 'vc-revision-history default)
-      (read-string prompt initial-input nil default))))
+        (funcall
+         (if multiple #'completing-read-multiple #'completing-read)
+         prompt completion-table nil nil initial-input 'vc-revision-history default)
+      (let ((answer (read-string prompt initial-input nil default)))
+        (if multiple
+            (split-string answer "[ \t]*,[ \t]*")
+          answer)))))
+
+(defun vc-read-multiple-revisions (prompt &optional files backend default initial-input)
+  (vc-read-revision prompt files backend default initial-input t))
 
 (defun vc-diff-build-argument-list-internal (&optional fileset)
   "Build argument list for calling internal diff functions."
@@ -3243,6 +3258,53 @@ vc-update-change-log
   (vc-call-backend (vc-responsible-backend default-directory)
                    'update-changelog args))
 
+(defcustom vc-compose-patches-inline nil
+  "Non-nil means that `vc-compose-patch' creates a single message."
+  :type 'boolean
+  :safe #'booleanp)
+
+(declare-function message-goto-body "message" (&optional interactive))
+(declare-function message--name-table "message" (orig-string))
+
+;;;###autoload
+(defun vc-compose-patch (addressee subject revisions)
+  "Compose a message sending REVISIONS to ADDRESSEE with SUBJECT."
+  (interactive (save-current-buffer
+                 (require 'message)
+                 (vc-ensure-vc-buffer)
+                 (let ((revs (vc-read-multiple-revisions "Revisions: ")) to)
+                   (while (null (setq to (completing-read-multiple
+                                          "Whom to send: "
+                                          (message--name-table ""))))
+                     (message "At least one addressee required.")
+                     (sit-for 1))
+                   (list (string-join to ", ")
+                         (read-string "Subject: " "[PATCH] " nil nil t)
+                         revs))))
+  (require 'message)
+  (save-current-buffer
+    (vc-ensure-vc-buffer)
+    (let ((patches (mapcar (lambda (rev)
+                             (vc-call-backend
+                              (vc-responsible-backend default-directory)
+                              'prepare-patch rev))
+                           revisions)))
+      (if (not vc-compose-patches-inline)
+          (dolist (patch patches)
+            (compose-mail addressee (plist-get patch :subject)
+                          nil nil nil nil
+                          `((exit-recursive-edit)))
+            (message-goto-body)
+            (save-excursion             ;don't jump to the end
+              (insert (plist-get patch :body)))
+            (recursive-edit))
+        (compose-mail addressee subject)
+        (message-goto-body)
+        (dolist (patch patches)
+          (mml-attach-file (plist-get patch :filename) "text/x-patch"))
+        (message-goto-body)
+        (open-line 2)))))
+
 (defun vc-default-responsible-p (_backend _file)
   "Indicate whether BACKEND is responsible for FILE.
 The default is to return nil always."
-- 
2.37.3


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


The documentation is not complete yet, and I haven't tested it yet
extensively, but it fundamentally does the right thing.  I am not too
sure about the generic interface and the plist that `prepare-patch'
returns.  There might be a better way to do this, generating less
garbage, but I'm stuck between trying to be VC-generic and MUA-generic
which is unsurprisingly tricky.

> Once the file opens in message-mode, most likely we need to strip the
> magic From <sha> <date> header from the beginning of the mail. Then we ensure
> don't do any nasty whitespace removal or wrapping.
>
> Most likely, depending on the backend, we should not require any
> parameters besides the "set of changes". For instance, in Git you can
> configure `git-format-patch` in the git configuration for several
> attributes, like --to=, --annotate, --prefix, etc.
>
> I don't remember how Mercurial works with this. Probably similar. It
> should generate mbox entries as well, I think. 

bug#57400: 29.0.50; Support sending patches from VC directly

[Lars Ingebrigtsen]

Philip Kaludercic <philipk@posteo.net> writes:

> Sorry for the delay, here is a first approximation of this idea:

Looks good, but one question:

> +(defun vc-git-prepare-patch (rev)

Do many other VCs have commands to prepare patches like this?  If not,
would then most of the others have a generic vc-*-prepare-patch function
that just basically calls `vc-diff'?

bug#57400: 29.0.50; Support sending patches from VC directly

[Eli Zaretskii]

> Cc: 57400@debbugs.gnu.org, Antoine Kalmbach <ane@iki.fi>
> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Mon, 03 Oct 2022 21:06:57 +0200
> 
> Do many other VCs have commands to prepare patches like this?

Hg and bzr do have such commands.  Others should indeed run vc-diff, I
think.

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 57400@debbugs.gnu.org, Antoine Kalmbach <ane@iki.fi>
>> From: Lars Ingebrigtsen <larsi@gnus.org>
>> Date: Mon, 03 Oct 2022 21:06:57 +0200
>> 
>> Do many other VCs have commands to prepare patches like this?
>
> Hg and bzr do have such commands.  

Yes, and I would plan to implement it for these as well (even though I
have never used bzr).

>                                    Others should indeed run vc-diff, I
> think.

That seem possible, but for that to work I will need a generic way to
detect the predecessor of a commit and extract a commit message.
Currently, I am not sure if this can be done.

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

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

Philip Kaludercic <philipk@posteo.net> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Cc: 57400@debbugs.gnu.org, Antoine Kalmbach <ane@iki.fi>
>>> From: Lars Ingebrigtsen <larsi@gnus.org>
>>> Date: Mon, 03 Oct 2022 21:06:57 +0200
>>> 
>>> Do many other VCs have commands to prepare patches like this?
>>
>> Hg and bzr do have such commands.  
>
> Yes, and I would plan to implement it for these as well (even though I
> have never used bzr).
>
>>                                    Others should indeed run vc-diff, I
>> think.
>
> That seem possible, but for that to work I will need a generic way to
> detect the predecessor of a commit and extract a commit message.
> Currently, I am not sure if this can be done.

I had forgotten about `previous-revision', so that was easy and I
decided to fall back to a generic subject as that can be easily revised:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-a-command-to-send-patches.patch --]
[-- Type: text/x-patch, Size: 7701 bytes --]

From cfb24a0bd74d4af4e4ed86f5a0029cb7f2cf1aed Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Mon, 3 Oct 2022 20:54:38 +0200
Subject: [PATCH] Add a command to send patches

* lisp/vc/vc-git.el (vc-git-prepare-patch):  Add Git implementation.
* lisp/vc/vc.el (vc-read-revision):  Add a MULTIPLE argument.
 (vc-read-multiple-revisions):  Add an auxiliary function that always
 calls 'vc-read-revision' with a non-nil value for MULTIPLE.
(vc-compose-patches-inline):  Add user option.
(message-goto-body):  Declare function.
(message--name-table):  Declare function.
(vc-default-prepare-patch): Add a default implementation.
(vc-compose-patch):  Add command.  (bug#57400)
---
 lisp/vc/vc-git.el | 27 ++++++++++++++
 lisp/vc/vc.el     | 90 ++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 113 insertions(+), 4 deletions(-)

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index f5ac43f536..439e82b12e 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1705,6 +1705,33 @@ vc-git-extra-status-menu
 (defun vc-git-root (file)
   (vc-find-root file ".git"))
 
+(defun vc-git-prepare-patch (rev)
+  (with-temp-buffer
+    (call-process vc-git-program nil t nil "format-patch"
+                  "--no-numbered" "--stdout"
+                  ;; From gitrevisions(7): ^<n> means the <n>th parent
+                  ;; (i.e.  <rev>^ is equivalent to <rev>^1). As a
+                  ;; special rule, <rev>^0 means the commit itself and
+                  ;; is used when <rev> is the object name of a tag
+                  ;; object that refers to a commit object.
+                  (concat rev "^0"))
+    (let (filename subject body)
+      ;; Save the patch in a temporary file if required
+      (when (bound-and-true-p vc-compose-patches-inline)
+        (setq filename (make-temp-file "vc-git-prepare-patch"))
+        (write-region nil nil filename)) ;FIXME: Clean up
+      ;; Extract the subject line
+      (goto-char (point-min))
+      (search-forward-regexp "^Subject: \\(.+\\)")
+      (setq subject (match-string 1))
+      ;; Extract the patch body
+      (search-forward-regexp "\n\n")
+      (setq body (buffer-substring (point) (point-max)))
+      ;; Return the extracted data
+      (list :filename filename
+            :subject subject
+            :body body))))
+
 ;; grep-compute-defaults autoloads grep.
 (declare-function grep-read-regexp "grep" ())
 (declare-function grep-read-files "grep" (regexp))
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 24300e014a..70f1f75d00 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -574,6 +574,14 @@
 ;;   containing FILE-OR-DIR.  The optional REMOTE-NAME specifies the
 ;;   remote (in Git parlance) whose URL is to be returned.  It has
 ;;   only a meaning for distributed VCS and is ignored otherwise.
+;;
+;; - prepare-patch (rev)
+;;
+;;   Prepare a patch and return a property list with the keys
+;;   `:subject' indicating the patch message as a string, `:body'
+;;   containing the contents of the patch as a string (excluding the
+;;   header) and `:filename' pointing to a file where the patch has
+;;   been stored.
 
 ;;; Changes from the pre-25.1 API:
 ;;
@@ -1910,7 +1918,7 @@ vc-diff-internal
 (defvar vc-revision-history nil
   "History for `vc-read-revision'.")
 
-(defun vc-read-revision (prompt &optional files backend default initial-input)
+(defun vc-read-revision (prompt &optional files backend default initial-input multiple)
   (cond
    ((null files)
     (let ((vc-fileset (vc-deduce-fileset t))) ;FIXME: why t?  --Stef
@@ -1920,9 +1928,16 @@ vc-read-revision
   (let ((completion-table
          (vc-call-backend backend 'revision-completion-table files)))
     (if completion-table
-        (completing-read prompt completion-table
-                         nil nil initial-input 'vc-revision-history default)
-      (read-string prompt initial-input nil default))))
+        (funcall
+         (if multiple #'completing-read-multiple #'completing-read)
+         prompt completion-table nil nil initial-input 'vc-revision-history default)
+      (let ((answer (read-string prompt initial-input nil default)))
+        (if multiple
+            (split-string answer "[ \t]*,[ \t]*")
+          answer)))))
+
+(defun vc-read-multiple-revisions (prompt &optional files backend default initial-input)
+  (vc-read-revision prompt files backend default initial-input t))
 
 (defun vc-diff-build-argument-list-internal (&optional fileset)
   "Build argument list for calling internal diff functions."
@@ -3243,6 +3258,73 @@ vc-update-change-log
   (vc-call-backend (vc-responsible-backend default-directory)
                    'update-changelog args))
 
+(defcustom vc-compose-patches-inline nil
+  "Non-nil means that `vc-compose-patch' creates a single message."
+  :type 'boolean
+  :safe #'booleanp)
+
+(declare-function message-goto-body "message" (&optional interactive))
+(declare-function message--name-table "message" (orig-string))
+
+(defun vc-default-prepare-patch (rev)
+  (let ((backend (vc-backend buffer-file-name))
+        filename)
+    (with-temp-buffer
+      (vc-diff-internal
+       nil (list backend) rev
+       (vc-call-backend backend 'previous-revision
+                        buffer-file-name rev)
+       nil t)
+      (when vc-compose-patches-inline
+        (setq filename (make-temp-file "vc-default-prepare-patch"))
+        (write-region nil nil filename))
+      (list :filename filename :body (buffer-string)))))
+
+;;;###autoload
+(defun vc-compose-patch (addressee subject revisions)
+  "Compose a message sending REVISIONS to ADDRESSEE with SUBJECT."
+  (interactive (save-current-buffer
+                 (require 'message)
+                 (vc-ensure-vc-buffer)
+                 (let ((revs (vc-read-multiple-revisions "Revisions: ")) to)
+                   (while (null (setq to (completing-read-multiple
+                                          "Whom to send: "
+                                          (message--name-table ""))))
+                     (message "At least one addressee required.")
+                     (sit-for 1))
+                   (list (string-join to ", ")
+                         (read-string "Subject: " "[PATCH] " nil nil t)
+                         revs))))
+  (require 'message)
+  (save-current-buffer
+    (vc-ensure-vc-buffer)
+    (let ((patches (mapcar (lambda (rev)
+                             (vc-call-backend
+                              (vc-responsible-backend default-directory)
+                              'prepare-patch rev))
+                           revisions)))
+      (if (not vc-compose-patches-inline)
+          (dolist (patch patches)
+            (compose-mail addressee
+                          (or (plist-get patch :subject)
+                              (concat
+                               "Patch for " ;guess
+                               (file-name-nondirectory
+                                (directory-file-name
+                                 (vc-root-dir)))))
+                          nil nil nil nil
+                          `((exit-recursive-edit)))
+            (message-goto-body)
+            (save-excursion             ;don't jump to the end
+              (insert (plist-get patch :body)))
+            (recursive-edit))
+        (compose-mail addressee subject)
+        (message-goto-body)
+        (dolist (patch patches)
+          (mml-attach-file (plist-get patch :filename) "text/x-patch"))
+        (message-goto-body)
+        (open-line 2)))))
+
 (defun vc-default-responsible-p (_backend _file)
   "Indicate whether BACKEND is responsible for FILE.
 The default is to return nil always."
-- 
2.37.3

bug#57400: 29.0.50; Support sending patches from VC directly

[Lars Ingebrigtsen]

Philip Kaludercic <philipk@posteo.net> writes:

> I had forgotten about `previous-revision', so that was easy and I
> decided to fall back to a generic subject as that can be easily revised:

I haven't tested it, but it looks good to me, so feel free to push when
you feel it's ready (after adding documentation and a NEWS item).

bug#57400: 29.0.50; Support sending patches from VC directly

[Eli Zaretskii]

> Cc: 57400@debbugs.gnu.org, Antoine Kalmbach <ane@iki.fi>
> From: Philip Kaludercic <philipk@posteo.net>
> Date: Mon, 03 Oct 2022 21:55:35 +0000
> 
> >>                                    Others should indeed run vc-diff, I
> >> think.
> >
> > That seem possible, but for that to work I will need a generic way to
> > detect the predecessor of a commit and extract a commit message.
> > Currently, I am not sure if this can be done.
> 
> I had forgotten about `previous-revision', so that was easy and I
> decided to fall back to a generic subject as that can be easily revised:

Thanks.  A few comments below.

> +(defun vc-git-prepare-patch (rev)
> +  (with-temp-buffer
> +    (call-process vc-git-program nil t nil "format-patch"
> +                  "--no-numbered" "--stdout"
> +                  ;; From gitrevisions(7): ^<n> means the <n>th parent
> +                  ;; (i.e.  <rev>^ is equivalent to <rev>^1). As a
> +                  ;; special rule, <rev>^0 means the commit itself and
> +                  ;; is used when <rev> is the object name of a tag
> +                  ;; object that refers to a commit object.
> +                  (concat rev "^0"))

This needs to set up decoding of the stuff Git outputs, because it is
usually in UTF-8 (but could be in some other encoding), and the
defaults could be different, depending on the locale.  See
vc-git-log-output-coding-system and its use elsewhere.

Or maybe you should just use vc-git--call here, which handles that
already, and uses process-file instead of call-process, so this will
work via Tramp: an additional benefit.

> +    (let (filename subject body)
> +      ;; Save the patch in a temporary file if required
> +      (when (bound-and-true-p vc-compose-patches-inline)
> +        (setq filename (make-temp-file "vc-git-prepare-patch"))
> +        (write-region nil nil filename)) ;FIXME: Clean up

By "clean up" did you mean delete the temporary file?

> +      ;; Return the extracted data
> +      (list :filename filename
> +            :subject subject
> +            :body body))))

I think this should include :encoding ENCODING, to provide the
temporary file's encoding to the caller.  The encoding should be the
same as buffer-file-coding-system in the buffer which you write above.

> -(defun vc-read-revision (prompt &optional files backend default initial-input)
> +(defun vc-read-revision (prompt &optional files backend default initial-input multiple)

This API change warrants a NEWS entry, I think.

>    (cond
>     ((null files)
>      (let ((vc-fileset (vc-deduce-fileset t))) ;FIXME: why t?  --Stef
> @@ -1920,9 +1928,16 @@ vc-read-revision
>    (let ((completion-table
>           (vc-call-backend backend 'revision-completion-table files)))
>      (if completion-table
> -        (completing-read prompt completion-table
> -                         nil nil initial-input 'vc-revision-history default)
> -      (read-string prompt initial-input nil default))))
> +        (funcall
> +         (if multiple #'completing-read-multiple #'completing-read)
> +         prompt completion-table nil nil initial-input 'vc-revision-history default)
> +      (let ((answer (read-string prompt initial-input nil default)))
> +        (if multiple
> +            (split-string answer "[ \t]*,[ \t]*")
> +          answer)))))
> +
> +(defun vc-read-multiple-revisions (prompt &optional files backend default initial-input)
> +  (vc-read-revision prompt files backend default initial-input t))
>  
>  (defun vc-diff-build-argument-list-internal (&optional fileset)
>    "Build argument list for calling internal diff functions."
> @@ -3243,6 +3258,73 @@ vc-update-change-log
>    (vc-call-backend (vc-responsible-backend default-directory)
>                     'update-changelog args))
>  
> +(defcustom vc-compose-patches-inline nil
> +  "Non-nil means that `vc-compose-patch' creates a single message."

This is rather cryptic, IMO, and will benefit from more detailed
description, after the initial line.  Also, I suggest a slight
rewording of the above sentence:

  If non-nil, `vc-compose-patch' will create a single email message.

> +  :type 'boolean
> +  :safe #'booleanp)

The :version tag is missing.

> +(declare-function message-goto-body "message" (&optional interactive))
> +(declare-function message--name-table "message" (orig-string))

Can we not force the use of message.el?  Can we use mail-user-agent
instead, and use its properties to find out the compose function the
user has set up, like "C-x m" does?  This would probably eliminate
quite a few problems with user email setup, which might not support
message.el.

> +(defun vc-compose-patch (addressee subject revisions)
> +  "Compose a message sending REVISIONS to ADDRESSEE with SUBJECT."

  "Compose an email message to send REVISIONS to ADDRESSEE with SUBJECT."

The "email" part is important, because we cannot assume the reader of
the doc string is necessarily aware of the context.

> +  (interactive (save-current-buffer
> +                 (require 'message)
> +                 (vc-ensure-vc-buffer)
> +                 (let ((revs (vc-read-multiple-revisions "Revisions: ")) to)
> +                   (while (null (setq to (completing-read-multiple
> +                                          "Whom to send: "

Instead "Whom to send:" I suggest just "Addressee:" or maybe even
"Send patches to:"

> +            (compose-mail addressee
> +                          (or (plist-get patch :subject)
> +                              (concat
> +                               "Patch for " ;guess
> +                               (file-name-nondirectory
> +                                (directory-file-name
> +                                 (vc-root-dir)))))
> +                          nil nil nil nil
> +                          `((exit-recursive-edit)))
> +            (message-goto-body)

compose-mail doesn't necessarily invoke message.el functions, so
message-goto-body is not necessarily appropriate here.

bug#57400: 29.0.50; Support sending patches from VC directly

[Eli Zaretskii]

> Cc: 57400@debbugs.gnu.org, Antoine Kalmbach <ane@iki.fi>
> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Tue, 04 Oct 2022 01:32:15 +0200
> 
> Philip Kaludercic <philipk@posteo.net> writes:
> 
> > I had forgotten about `previous-revision', so that was easy and I
> > decided to fall back to a generic subject as that can be easily revised:
> 
> I haven't tested it, but it looks good to me, so feel free to push when
> you feel it's ready (after adding documentation and a NEWS item).

Please allow some time for patch review and comments, there's no
reason to rush ahead with such a significant new feature.  Ideally,
Dmitry should chime in.

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 57400@debbugs.gnu.org, Antoine Kalmbach <ane@iki.fi>
>> From: Philip Kaludercic <philipk@posteo.net>
>> Date: Mon, 03 Oct 2022 21:55:35 +0000
>> 
>> >>                                    Others should indeed run vc-diff, I
>> >> think.
>> >
>> > That seem possible, but for that to work I will need a generic way to
>> > detect the predecessor of a commit and extract a commit message.
>> > Currently, I am not sure if this can be done.
>> 
>> I had forgotten about `previous-revision', so that was easy and I
>> decided to fall back to a generic subject as that can be easily revised:
>
> Thanks.  A few comments below.
>
>> +(defun vc-git-prepare-patch (rev)
>> +  (with-temp-buffer
>> +    (call-process vc-git-program nil t nil "format-patch"
>> +                  "--no-numbered" "--stdout"
>> +                  ;; From gitrevisions(7): ^<n> means the <n>th parent
>> +                  ;; (i.e.  <rev>^ is equivalent to <rev>^1). As a
>> +                  ;; special rule, <rev>^0 means the commit itself and
>> +                  ;; is used when <rev> is the object name of a tag
>> +                  ;; object that refers to a commit object.
>> +                  (concat rev "^0"))
>
> This needs to set up decoding of the stuff Git outputs, because it is
> usually in UTF-8 (but could be in some other encoding), and the
> defaults could be different, depending on the locale.  See
> vc-git-log-output-coding-system and its use elsewhere.
>
> Or maybe you should just use vc-git--call here, which handles that
> already, and uses process-file instead of call-process, so this will
> work via Tramp: an additional benefit.

I will look into this.

>> +    (let (filename subject body)
>> +      ;; Save the patch in a temporary file if required
>> +      (when (bound-and-true-p vc-compose-patches-inline)
>> +        (setq filename (make-temp-file "vc-git-prepare-patch"))
>> +        (write-region nil nil filename)) ;FIXME: Clean up
>
> By "clean up" did you mean delete the temporary file?

Yes.  Another idea was to create buffers and attach those, but I still
haven't decided which is preferable.

>> +      ;; Return the extracted data
>> +      (list :filename filename
>> +            :subject subject
>> +            :body body))))
>
> I think this should include :encoding ENCODING, to provide the
> temporary file's encoding to the caller.  The encoding should be the
> same as buffer-file-coding-system in the buffer which you write above.

OK.

>> -(defun vc-read-revision (prompt &optional files backend default initial-input)
>> +(defun vc-read-revision (prompt &optional files backend default initial-input multiple)
>
> This API change warrants a NEWS entry, I think.

More NEWS entries that just this one will be necessary.

>>    (cond
>>     ((null files)
>>      (let ((vc-fileset (vc-deduce-fileset t))) ;FIXME: why t?  --Stef
>> @@ -1920,9 +1928,16 @@ vc-read-revision
>>    (let ((completion-table
>>           (vc-call-backend backend 'revision-completion-table files)))
>>      (if completion-table
>> -        (completing-read prompt completion-table
>> -                         nil nil initial-input 'vc-revision-history default)
>> -      (read-string prompt initial-input nil default))))
>> +        (funcall
>> +         (if multiple #'completing-read-multiple #'completing-read)
>> +         prompt completion-table nil nil initial-input 'vc-revision-history default)
>> +      (let ((answer (read-string prompt initial-input nil default)))
>> +        (if multiple
>> +            (split-string answer "[ \t]*,[ \t]*")
>> +          answer)))))
>> +
>> +(defun vc-read-multiple-revisions (prompt &optional files backend default initial-input)
>> +  (vc-read-revision prompt files backend default initial-input t))
>>  
>>  (defun vc-diff-build-argument-list-internal (&optional fileset)
>>    "Build argument list for calling internal diff functions."
>> @@ -3243,6 +3258,73 @@ vc-update-change-log
>>    (vc-call-backend (vc-responsible-backend default-directory)
>>                     'update-changelog args))
>>  
>> +(defcustom vc-compose-patches-inline nil
>> +  "Non-nil means that `vc-compose-patch' creates a single message."
>
> This is rather cryptic, IMO, and will benefit from more detailed
> description, after the initial line.  Also, I suggest a slight
> rewording of the above sentence:
>
>   If non-nil, `vc-compose-patch' will create a single email message.

You are right, the docstring was just a temporary fill-in so that I
could specify the following user option attributes.  It has to be
expanded anyway.

>> +  :type 'boolean
>> +  :safe #'booleanp)
>
> The :version tag is missing.

Thanks for the reminder.

>> +(declare-function message-goto-body "message" (&optional interactive))
>> +(declare-function message--name-table "message" (orig-string))
>
> Can we not force the use of message.el?  Can we use mail-user-agent
> instead, and use its properties to find out the compose function the
> user has set up, like "C-x m" does?  This would probably eliminate
> quite a few problems with user email setup, which might not support
> message.el.

I'd like to avoid that if possible, but I don't see how.

>> +(defun vc-compose-patch (addressee subject revisions)
>> +  "Compose a message sending REVISIONS to ADDRESSEE with SUBJECT."
>
>   "Compose an email message to send REVISIONS to ADDRESSEE with SUBJECT."
>
> The "email" part is important, because we cannot assume the reader of
> the doc string is necessarily aware of the context.

Good point.

>> +  (interactive (save-current-buffer
>> +                 (require 'message)
>> +                 (vc-ensure-vc-buffer)
>> +                 (let ((revs (vc-read-multiple-revisions "Revisions: ")) to)
>> +                   (while (null (setq to (completing-read-multiple
>> +                                          "Whom to send: "
>
> Instead "Whom to send:" I suggest just "Addressee:" or maybe even
> "Send patches to:"

You are right, that sounds better.

>> +            (compose-mail addressee
>> +                          (or (plist-get patch :subject)
>> +                              (concat
>> +                               "Patch for " ;guess
>> +                               (file-name-nondirectory
>> +                                (directory-file-name
>> +                                 (vc-root-dir)))))
>> +                          nil nil nil nil
>> +                          `((exit-recursive-edit)))
>> +            (message-goto-body)
>
> compose-mail doesn't necessarily invoke message.el functions, so
> message-goto-body is not necessarily appropriate here.

Oh, I thought it was because `submit-emacs-patch' did the same.  Again,
do you have any suggestions what else could be done to keep this
generic?

bug#57400: 29.0.50; Support sending patches from VC directly

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: larsi@gnus.org,  57400@debbugs.gnu.org,  ane@iki.fi
> Date: Tue, 04 Oct 2022 07:10:15 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> +            (compose-mail addressee
> >> +                          (or (plist-get patch :subject)
> >> +                              (concat
> >> +                               "Patch for " ;guess
> >> +                               (file-name-nondirectory
> >> +                                (directory-file-name
> >> +                                 (vc-root-dir)))))
> >> +                          nil nil nil nil
> >> +                          `((exit-recursive-edit)))
> >> +            (message-goto-body)
> >
> > compose-mail doesn't necessarily invoke message.el functions, so
> > message-goto-body is not necessarily appropriate here.
> 
> Oh, I thought it was because `submit-emacs-patch' did the same.  Again,
> do you have any suggestions what else could be done to keep this
> generic?

Can you ask more specific questions?  Are you looking for a generic
way of doing what message-goto-body does?  If so, would
rfc822-goto-eoh do the job?

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: larsi@gnus.org,  57400@debbugs.gnu.org,  ane@iki.fi
>> Date: Tue, 04 Oct 2022 07:10:15 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> +            (compose-mail addressee
>> >> +                          (or (plist-get patch :subject)
>> >> +                              (concat
>> >> +                               "Patch for " ;guess
>> >> +                               (file-name-nondirectory
>> >> +                                (directory-file-name
>> >> +                                 (vc-root-dir)))))
>> >> +                          nil nil nil nil
>> >> +                          `((exit-recursive-edit)))
>> >> +            (message-goto-body)
>> >
>> > compose-mail doesn't necessarily invoke message.el functions, so
>> > message-goto-body is not necessarily appropriate here.
>> 
>> Oh, I thought it was because `submit-emacs-patch' did the same.  Again,
>> do you have any suggestions what else could be done to keep this
>> generic?
>
> Can you ask more specific questions?  Are you looking for a generic
> way of doing what message-goto-body does?  

Kind of, I would like to have some function that would place the point
at the beginning of the message body, no matter what MUA is used.  From
what I see, the implicit assumption always is that a message is composed
in a single buffer where the headers are written out at the beginning of
the buffer, then there is some kind of to detect the end of the headers,
followed by the body.  But what if a MUA wants to use a separate buffer
for the headers and the body, placing them in two separate windows?
What if the headers aren't shown at all?  If I want to handle the
situation generically, it seems like I would have to take all the design
decisions into consideration.

>                                            If so, would
> rfc822-goto-eoh do the job?

It doesn't seem to do the same, as in message-mode, it jumps to the
beginning of the "--text follows this line--" line, not to the line
after it.

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Philip Kaludercic <philipk@posteo.net> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Philip Kaludercic <philipk@posteo.net>
>>> Cc: larsi@gnus.org,  57400@debbugs.gnu.org,  ane@iki.fi
>>> Date: Tue, 04 Oct 2022 07:10:15 +0000
>>> 
>>> Eli Zaretskii <eliz@gnu.org> writes:
>>> 
>>> >> +            (compose-mail addressee
>>> >> +                          (or (plist-get patch :subject)
>>> >> +                              (concat
>>> >> +                               "Patch for " ;guess
>>> >> +                               (file-name-nondirectory
>>> >> +                                (directory-file-name
>>> >> +                                 (vc-root-dir)))))
>>> >> +                          nil nil nil nil
>>> >> +                          `((exit-recursive-edit)))
>>> >> +            (message-goto-body)
>>> >
>>> > compose-mail doesn't necessarily invoke message.el functions, so
>>> > message-goto-body is not necessarily appropriate here.
>>> 
>>> Oh, I thought it was because `submit-emacs-patch' did the same.  Again,
>>> do you have any suggestions what else could be done to keep this
>>> generic?
>>
>> Can you ask more specific questions?  Are you looking for a generic
>> way of doing what message-goto-body does?  
>
> Kind of, I would like to have some function that would place the point
> at the beginning of the message body, no matter what MUA is used.  From
> what I see, the implicit assumption always is that a message is composed
> in a single buffer where the headers are written out at the beginning of
> the buffer, then there is some kind of to detect the end of the headers,
> followed by the body.  But what if a MUA wants to use a separate buffer
> for the headers and the body, placing them in two separate windows?
> What if the headers aren't shown at all?  If I want to handle the
> situation generically, it seems like I would have to take all the design
> decisions into consideration.

Another issue is how attachments are made:  mml-attach-* is part of Gnus
and inserts literal text that some MUAs interpret, but is this something
that all MUAs have to do?

>>                                            If so, would
>> rfc822-goto-eoh do the job?
>
> It doesn't seem to do the same, as in message-mode, it jumps to the
> beginning of the "--text follows this line--" line, not to the line
> after it.

bug#57400: 29.0.50; Support sending patches from VC directly

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: larsi@gnus.org,  57400@debbugs.gnu.org,  ane@iki.fi
> Date: Tue, 04 Oct 2022 10:40:23 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Can you ask more specific questions?  Are you looking for a generic
> > way of doing what message-goto-body does?  
> 
> Kind of, I would like to have some function that would place the point
> at the beginning of the message body, no matter what MUA is used.  From
> what I see, the implicit assumption always is that a message is composed
> in a single buffer where the headers are written out at the beginning of
> the buffer, then there is some kind of to detect the end of the headers,
> followed by the body.  But what if a MUA wants to use a separate buffer
> for the headers and the body, placing them in two separate windows?
> What if the headers aren't shown at all?  If I want to handle the
> situation generically, it seems like I would have to take all the design
> decisions into consideration.

Do you happen to know about such MUAs?  mail-user-agent currently
supports just 4 MUAs: do any of them work like above?

We could bite the bullet and make this operation a property of
mail-user-agent, like 'composefunc' we already have (see the doc
string of define-mail-user-agent for the full list).  But if
rfc822-goto-eoh can do the job, maybe it's "good enough"?

> >                                            If so, would
> > rfc822-goto-eoh do the job?
> 
> It doesn't seem to do the same, as in message-mode, it jumps to the
> beginning of the "--text follows this line--" line, not to the line
> after it.

Well, moving one line after that, if this is the line you get to with
that function, is easy.  Don't all MUAs have that line?

bug#57400: 29.0.50; Support sending patches from VC directly

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: larsi@gnus.org,  57400@debbugs.gnu.org,  ane@iki.fi
> Date: Tue, 04 Oct 2022 10:42:19 +0000
> 
> Another issue is how attachments are made:  mml-attach-* is part of Gnus
> and inserts literal text that some MUAs interpret, but is this something
> that all MUAs have to do?

How about mail-add-attachment?

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

(not sure if my last message got send out, so I'll rewrite it)

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: larsi@gnus.org,  57400@debbugs.gnu.org,  ane@iki.fi
>> Date: Tue, 04 Oct 2022 10:40:23 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Can you ask more specific questions?  Are you looking for a generic
>> > way of doing what message-goto-body does?  
>> 
>> Kind of, I would like to have some function that would place the point
>> at the beginning of the message body, no matter what MUA is used.  From
>> what I see, the implicit assumption always is that a message is composed
>> in a single buffer where the headers are written out at the beginning of
>> the buffer, then there is some kind of to detect the end of the headers,
>> followed by the body.  But what if a MUA wants to use a separate buffer
>> for the headers and the body, placing them in two separate windows?
>> What if the headers aren't shown at all?  If I want to handle the
>> situation generically, it seems like I would have to take all the design
>> decisions into consideration.
>
> Do you happen to know about such MUAs?  mail-user-agent currently
> supports just 4 MUAs: do any of them work like above?

Not to my knowledge, but my concern was that it might be possible for
something like that to exist.  I'll ask around to find out.

> We could bite the bullet and make this operation a property of
> mail-user-agent, like 'composefunc' we already have (see the doc
> string of define-mail-user-agent for the full list).  But if
> rfc822-goto-eoh can do the job, maybe it's "good enough"?

If you think so, then I'll follow that.

>> >                                            If so, would
>> > rfc822-goto-eoh do the job?
>> 
>> It doesn't seem to do the same, as in message-mode, it jumps to the
>> beginning of the "--text follows this line--" line, not to the line
>> after it.
>
> Well, moving one line after that, if this is the line you get to with
> that function, is easy.  Don't all MUAs have that line?

Perhaps, but again not by necessity, just by (historical) chance.

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 57400@debbugs.gnu.org, Antoine Kalmbach <ane@iki.fi>
>> From: Lars Ingebrigtsen <larsi@gnus.org>
>> Date: Mon, 03 Oct 2022 21:06:57 +0200
>> 
>> Do many other VCs have commands to prepare patches like this?
>
> Hg and bzr do have such commands.
         ^

As I mentioned before, I have never used Bazaar before so I downloaded a
repository to test out how patches are created and the output I get when
picking out some revision is as follows:

--8<---------------cut here---------------start------------->8---
rhea$ brz send --revision 6438 -o -
Using saved parent location "http://bazaar.launchpad.net/~bzr-pqm/bzr/bzr.dev/" to determine what changes to submit.
Bundling 0 revisions.
# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: pqm@pqm.ubuntu.com-20120116142737-incpbjom3tqo3hdb
# target_branch: http://bazaar.launchpad.net/~bzr-pqm/bzr/bzr.dev/
# testament_sha1: 0363416e18dfbea56a231638540b5f10be10c81c
# timestamp: 2022-10-04 21:13:18 +0200
# base_revision_id: pqm@pqm.ubuntu.com-20120116142737-incpbjom3tqo3hdb
# 
# Begin patch
# Begin bundle
IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWdjgvvgAABJfgAAQQGFxUBIAAACv794QIABkRTaajamQyGjTaRiFGgBMBBkwQhDmaVnH5r9hMFQyJ7EUzThiw4Ixc/mQVpexbPS29yLLTxaFbWvXcN2zcydOQxpD652acQC4g4Z96jI5BipgKAAiM5Zz45Kd/4u5IpwoSGxwX3wA
--8<---------------cut here---------------end--------------->8---

Which doesn't look like a patch to me.  My understanding from[0] is that
this is a "merge directive" indicating what change has to be made.  But
according to [1] a merge directive

> [...] provides many things needed for requesting merges:

> - A machine-readable description of the merge to perform
> - An optional patch that is a preview of the changes requested
> - An optional bundle of revision data, so that the changes can be applied directly from the merge directive, without retrieving data from a branch.

so it might just be that my command is wrong?

[0] http://doc.bazaar.canonical.com/latest/en/user-guide/sending_changes.html#creating-a-merge-directive
[1] http://doc.bazaar.canonical.com/latest/en/user-reference/send-help.html

bug#57400: 29.0.50; Support sending patches from VC directly

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>,  57400@debbugs.gnu.org,  ane@iki.fi
> Date: Tue, 04 Oct 2022 19:19:45 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Hg and bzr do have such commands.
>          ^
> 
> As I mentioned before, I have never used Bazaar before so I downloaded a
> repository to test out how patches are created and the output I get when
> picking out some revision is as follows:
> 
> --8<---------------cut here---------------start------------->8---
> rhea$ brz send --revision 6438 -o -
> Using saved parent location "http://bazaar.launchpad.net/~bzr-pqm/bzr/bzr.dev/" to determine what changes to submit.
> Bundling 0 revisions.
> # Bazaar merge directive format 2 (Bazaar 0.90)
> # revision_id: pqm@pqm.ubuntu.com-20120116142737-incpbjom3tqo3hdb
> # target_branch: http://bazaar.launchpad.net/~bzr-pqm/bzr/bzr.dev/
> # testament_sha1: 0363416e18dfbea56a231638540b5f10be10c81c
> # timestamp: 2022-10-04 21:13:18 +0200
> # base_revision_id: pqm@pqm.ubuntu.com-20120116142737-incpbjom3tqo3hdb
> # 
> # Begin patch
> # Begin bundle
> IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWdjgvvgAABJfgAAQQGFxUBIAAACv794QIABkRTaajamQyGjTaRiFGgBMBBkwQhDmaVnH5r9hMFQyJ7EUzThiw4Ixc/mQVpexbPS29yLLTxaFbWvXcN2zcydOQxpD652acQC4g4Z96jI5BipgKAAiM5Zz45Kd/4u5IpwoSGxwX3wA
> --8<---------------cut here---------------end--------------->8---
> 
> Which doesn't look like a patch to me.  My understanding from[0] is that
> this is a "merge directive" indicating what change has to be made.  But
> according to [1] a merge directive
> 
> > [...] provides many things needed for requesting merges:
> 
> > - A machine-readable description of the merge to perform
> > - An optional patch that is a preview of the changes requested
> > - An optional bundle of revision data, so that the changes can be applied directly from the merge directive, without retrieving data from a branch.
> 
> so it might just be that my command is wrong?

Yes, try this instead:

  brz send --revision 6437..6438 -o -

(Repeat after me: Bazaar is not Git, Bazaar is not Git, Bazaar is...)

bug#57400: 29.0.50; Support sending patches from VC directly

[Antoine Kalmbach]

Philip Kaludercic <philipk@posteo.net> writes:

> Sorry for the delay, here is a first approximation of this idea:

Great news!

> +(defun vc-compose-patch (addressee subject revisions)
> +  "Compose a message sending REVISIONS to ADDRESSEE with SUBJECT."
> +  (interactive (save-current-buffer

Overall, this looks great.  If I understand this right, this doesn't
support entering revision ranges (abcd1234..ghjk568, HEAD~2, etc), but
you have to input each absolute revision?  I think that's actually fine,
because it composes with the proposed vc-log feature of marking commits
there and then applying vc-compose-patch on the commits.  Trying to
convert those into VCS specific revision ranges is probably asking for trouble.

-- 
Antoine Kalmbach

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

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


Here is an updated version of the patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-a-VC-command-to-prepare-patches.patch --]
[-- Type: text/x-patch, Size: 14762 bytes --]

From a384a0b74e8037c642ac49dbad9489fd51f6c8cd Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Mon, 3 Oct 2022 20:54:38 +0200
Subject: [PATCH] Add a VC command to prepare patches

* doc/emacs/vc1-xtra.texi (Miscellaneous VC):  Add new node.
(Editing VC Commands):  Document new feature.
* etc/NEWS:  Mention 'vc-prepare-patch'.
* lisp/vc/log-view.el: Autoload 'log-view-get-marked'.
* lisp/vc/vc-git.el (vc-git-prepare-patch):  Add Git implementation.
* lisp/vc/vc-hg.el (vc-git-prepare-patch):  Add Mercurial implementation.
* lisp/vc/vc-bzr.el (vc-git-prepare-patch):  Add Bazaar implementation.
* lisp/vc/vc.el (vc-read-revision):  Add a MULTIPLE argument.
(vc-read-multiple-revisions):  Add an auxiliary function that always
calls 'vc-read-revision' with a non-nil value for MULTIPLE.
(vc-prepare-patches-inline):  Add user option.
(message-goto-body):  Declare function.
(message--name-table):  Declare function.
(vc-default-prepare-patch): Add a default implementation.
(vc-prepare-patch):  Add command.  (bug#57400)
---
 doc/emacs/vc1-xtra.texi |  26 +++++++++
 etc/NEWS                |  15 +++++
 lisp/vc/log-view.el     |   1 +
 lisp/vc/vc-bzr.el       |  14 +++++
 lisp/vc/vc-git.el       |  24 ++++++++
 lisp/vc/vc-hg.el        |  12 ++++
 lisp/vc/vc.el           | 121 ++++++++++++++++++++++++++++++++++++++--
 7 files changed, 209 insertions(+), 4 deletions(-)

diff --git a/doc/emacs/vc1-xtra.texi b/doc/emacs/vc1-xtra.texi
index 05d2144380..523e4611f0 100644
--- a/doc/emacs/vc1-xtra.texi
+++ b/doc/emacs/vc1-xtra.texi
@@ -16,6 +16,7 @@ Miscellaneous VC
 * Revision Tags::       Symbolic names for revisions.
 * Version Headers::     Inserting version control headers into working files.
 * Editing VC Commands:: Editing the VC shell commands that Emacs will run.
+* Preparing Patches::   Preparing and Composing patches from within VC
 @end menu
 
 @node Change Logs and VC
@@ -282,6 +283,31 @@ Editing VC Commands
 additional branches to the end of the @samp{git log} command that VC
 is about to run.
 
+@node Preparing Patches
+@subsubsection Preparing Patches
+
+@findex vc-prepare-patch
+When collaborating on projects it is common to send patches via Email,
+to share changes.  If you wish to do this using VC, you can use the
+@code{vc-prepare-patch} command.  This will prompt you which revisions
+you wish to share and who the addressee is.  The command will then use
+your @abbr{MUA, Mail User Agent} for you to review and send out.
+
+@vindex vc-prepare-patches-inline
+Depending on configuration of the user option
+@code{vc-prepare-patches-inline}, @code{vc-prepare-patch} will either
+generate a single or multiple messages.  A @code{nil} value (the default)
+will prepare and display a message for each revision, one after
+another.  A non-@code{nil} value will have all patches attached to the
+body of a single message.
+
+@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 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}).
+
 @node Customizing VC
 @subsection Customizing VC
 
diff --git a/etc/NEWS b/etc/NEWS
index 0fa24c577c..78c607f0e9 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1846,6 +1846,14 @@ Git commands display summary lines.  See the two new user options
 It is used to style the line that separates the 'log-edit' headers
 from the 'log-edit' summary.
 
++++
+*** New 'vc-prepare-patch' command.
+Patches for any version control system can be prepared using VC.  The
+command will query what commits to send and will compose messages for
+your mail user agent.  The behaviour of 'vc-prepare-patch' can be
+modified by the user options 'vc-prepare-patches-inline' and
+'vc-default-patch-addressee'.
+
 ** Message
 
 ---
@@ -2043,6 +2051,13 @@ The new ':doc-spec-function' element can be used to compute the
 ':doc-spec' element when the user asks for info on that particular
 mode (instead of at load time).
 
+** Subr-x
+
+---
+*** New macro 'with-funcall-substitutions'.
+The macro can be used to generically substitute function symbols in
+expressions.
+
 ** Ansi-color
 
 ---
diff --git a/lisp/vc/log-view.el b/lisp/vc/log-view.el
index 415b1564ed..7a710386fe 100644
--- a/lisp/vc/log-view.el
+++ b/lisp/vc/log-view.el
@@ -359,6 +359,7 @@ log-view-toggle-mark-entry
 	    (overlay-put ov 'log-view-self ov)
 	    (overlay-put ov 'log-view-marked (nth 1 entry))))))))
 
+;;;###autoload
 (defun log-view-get-marked ()
   "Return the list of tags for the marked log entries."
   (save-excursion
diff --git a/lisp/vc/vc-bzr.el b/lisp/vc/vc-bzr.el
index bce7996712..6f77f99555 100644
--- a/lisp/vc/vc-bzr.el
+++ b/lisp/vc/vc-bzr.el
@@ -1324,6 +1324,20 @@ vc-bzr-repository-url
           (match-string 1)
         (error "Cannot determine Bzr repository URL")))))
 
+(defun vc-bzr-prepare-patch (rev)
+  (with-current-buffer (generate-new-buffer " *vc-bzr-prepare-patch*")
+    (vc-bzr-command
+     "send" t 0 '()
+     "--revision" (concat (vc-bzr-previous-revision nil rev) ".." rev)
+     "--output" "-")
+    (let (subject)
+      ;; Extract the subject line
+      (goto-char (point-min))
+      (search-forward-regexp "^[^#].*")
+      (setq subject (match-string 0))
+      ;; Return the extracted data
+      (list :subject subject :buffer (current-buffer)))))
+
 (provide 'vc-bzr)
 
 ;;; vc-bzr.el ends here
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 4a2a42ad2a..f9dae8b9ea 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -95,6 +95,7 @@
 ;; - find-file-hook ()                             OK
 ;; - conflicted-files                              OK
 ;; - repository-url (file-or-dir)                  OK
+;; - prepare-patch (rev)                           OK
 
 ;;; Code:
 
@@ -1742,6 +1743,29 @@ vc-git-extra-status-menu
 (defun vc-git-root (file)
   (vc-find-root file ".git"))
 
+(defun vc-git-prepare-patch (rev)
+  (with-current-buffer (generate-new-buffer " *vc-git-prepare-patch*")
+    (vc-git-command
+     t 0 '()  "format-patch"
+     "--no-numbered" "--stdout"
+     ;; From gitrevisions(7): ^<n> means the <n>th parent
+     ;; (i.e.  <rev>^ is equivalent to <rev>^1). As a
+     ;; special rule, <rev>^0 means the commit itself and
+     ;; is used when <rev> is the object name of a tag
+     ;; object that refers to a commit object.
+     (concat rev "^.." rev))
+    (let (subject)
+      ;; Extract the subject line
+      (goto-char (point-min))
+      (search-forward-regexp "^Subject: \\(.+\\)")
+      (setq subject (match-string 1))
+      ;; Jump to the beginning for the patch
+      (search-forward-regexp "\n\n")
+      ;; Return the extracted data
+      (list :subject subject
+            :buffer (current-buffer)
+            :body-start (point)))))
+
 ;; grep-compute-defaults autoloads grep.
 (declare-function grep-read-regexp "grep" ())
 (declare-function grep-read-files "grep" (regexp))
diff --git a/lisp/vc/vc-hg.el b/lisp/vc/vc-hg.el
index eed9592378..2eebe2d543 100644
--- a/lisp/vc/vc-hg.el
+++ b/lisp/vc/vc-hg.el
@@ -80,6 +80,7 @@
 ;; - delete-file (file)                        TEST IT
 ;; - rename-file (old new)                     OK
 ;; - find-file-hook ()                         added for bug#10709
+;; - prepare-patch (rev)                       OK
 
 ;; 2) Implement Stefan Monnier's advice:
 ;; vc-hg-registered and vc-hg-state
@@ -1507,6 +1508,17 @@ vc-hg-merge-branch
     (with-current-buffer buffer (vc-run-delayed (vc-compilation-mode 'hg)))
     (vc-set-async-update buffer)))
 
+(defun vc-hg-prepare-patch (rev)
+  (with-current-buffer (generate-new-buffer " *vc-hg-prepare-patch*")
+    (vc-hg-command t 0 '() "export" "--rev" rev)
+    (let (subject)
+      ;; Extract the subject line
+      (goto-char (point-min))
+      (search-forward-regexp "^[^#].*")
+      (setq subject (match-string 0))
+      ;; Return the extracted data
+      (list :subject subject :buffer (current-buffer)))))
+
 ;;; Internal functions
 
 (defun vc-hg-command (buffer okstatus file-or-list &rest flags)
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index f0f5ee504c..f0bd1023ac 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -574,6 +574,14 @@
 ;;   containing FILE-OR-DIR.  The optional REMOTE-NAME specifies the
 ;;   remote (in Git parlance) whose URL is to be returned.  It has
 ;;   only a meaning for distributed VCS and is ignored otherwise.
+;;
+;; - prepare-patch (rev)
+;;
+;;   Prepare a patch and return a property list with the keys
+;;   `:subject' indicating the patch message as a string, `:body'
+;;   containing the contents of the patch as a string (excluding the
+;;   header) and `:filename' pointing to a file where the patch has
+;;   been stored.
 
 ;;; Changes from the pre-25.1 API:
 ;;
@@ -1910,7 +1918,7 @@ vc-diff-internal
 (defvar vc-revision-history nil
   "History for `vc-read-revision'.")
 
-(defun vc-read-revision (prompt &optional files backend default initial-input)
+(defun vc-read-revision (prompt &optional files backend default initial-input multiple)
   (cond
    ((null files)
     (let ((vc-fileset (vc-deduce-fileset t))) ;FIXME: why t?  --Stef
@@ -1923,9 +1931,16 @@ vc-read-revision
          (completion-table
           (vc-call-backend backend 'revision-completion-table files)))
     (if completion-table
-        (completing-read prompt completion-table
-                         nil nil initial-input 'vc-revision-history default)
-      (read-string prompt initial-input nil default))))
+        (funcall
+         (if multiple #'completing-read-multiple #'completing-read)
+         prompt completion-table nil nil initial-input 'vc-revision-history default)
+      (let ((answer (read-string prompt initial-input nil default)))
+        (if multiple
+            (split-string answer "[ \t]*,[ \t]*")
+          answer)))))
+
+(defun vc-read-multiple-revisions (prompt &optional files backend default initial-input)
+  (vc-read-revision prompt files backend default initial-input t))
 
 (defun vc-diff-build-argument-list-internal (&optional fileset)
   "Build argument list for calling internal diff functions."
@@ -3237,6 +3252,104 @@ vc-update-change-log
   (vc-call-backend (vc-responsible-backend default-directory)
                    'update-changelog args))
 
+(defcustom vc-prepare-patches-inline nil
+  "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."
+  :type 'boolean
+  :safe #'booleanp
+  :version "29.1")
+
+(defcustom vc-default-patch-addressee nil
+  "Default addressee for `vc-prepare-patch'.
+If nil, no default will be used.  This option may be set locally."
+  :type '(choice (const :tag "No default" nil) string)
+  :safe #'stringp
+  :version "29.1")
+
+(declare-function message--name-table "message" (orig-string))
+(declare-function mml-attach-buffer "mml"
+                  (buffer &optional type description disposition))
+(declare-function log-view-get-marked "log-view" ())
+
+(defun vc-default-prepare-patch (rev)
+  (let ((backend (vc-backend buffer-file-name)))
+    (with-current-buffer (generate-new-buffer " *vc-default-prepare-patch*")
+      (vc-diff-internal
+       nil (list backend) rev
+       (vc-call-backend backend 'previous-revision
+                        buffer-file-name rev)
+       nil t)
+      (list :subject (concat "Patch for "
+                             (file-name-nondirectory
+                              (directory-file-name
+                               (vc-root-dir))))
+            :buffer (current-buffer)))))
+
+;;;###autoload
+(defun vc-prepare-patch (addressee subject revisions)
+  "Compose an Email sending patches for REVISIONS to ADDRESSEE.
+If `vc-prepare-patches-inline' is non-nil, SUBJECT will be used
+as the default subject for the message.  Otherwise a separate
+message will be composed for each revision.
+
+When invoked interactively in a Log View buffer with marked
+revisions, these revisions will be used."
+  (interactive
+   (let ((revs (or (log-view-get-marked)
+                   (vc-read-multiple-revisions "Revisions: ")))
+         to)
+     (require 'message)
+     (while (null (setq to (completing-read-multiple
+                            (format-prompt
+                             "Addressee"
+                             vc-default-patch-addressee)
+                            (message--name-table "")
+                            nil nil nil nil
+                            vc-default-patch-addressee)))
+       (message "At least one addressee required.")
+       (sit-for blink-matching-delay))
+     (list (string-join to ", ")
+           (and vc-prepare-patches-inline
+                (read-string "Subject: " "[PATCH] " nil nil t))
+           revs)))
+  (save-current-buffer
+    (vc-ensure-vc-buffer)
+    (let ((patches (mapcar (lambda (rev)
+                             (vc-call-backend
+                              (vc-responsible-backend default-directory)
+                              'prepare-patch rev))
+                           revisions)))
+      (if (not vc-prepare-patches-inline)
+          (dolist (patch patches)
+            (compose-mail addressee
+                          (plist-get patch :subject)
+                          nil nil nil nil
+                          `((kill-buffer ,(plist-get patch :buffer))
+                            (exit-recursive-edit)))
+            (rfc822-goto-eoh) (forward-line)
+            (save-excursion             ;don't jump to the end
+              (insert-buffer-substring
+               (plist-get patch :buffer)
+               (plist-get patch :body-start)
+               (plist-get patch :body-end)))
+            (recursive-edit))
+        (compose-mail addressee subject nil nil nil nil
+                      (mapcar
+                       (lambda (p)
+                         (list #'kill-buffer (plist-get p :buffer)))
+                       patches))
+        (rfc822-goto-eoh)
+        (forward-line)
+        (save-excursion
+          (dolist (patch patches)
+            (mml-attach-buffer (buffer-name (plist-get patch :buffer))
+                               "text/x-patch"
+                               (plist-get patch :subject)
+                               "attachment")))
+        (open-line 2)))))
+
 (defun vc-default-responsible-p (_backend _file)
   "Indicate whether BACKEND is responsible for FILE.
 The default is to return nil always."
-- 
2.37.3


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


It includes

- some documentation for the Emacs manual and etc/NEWS,

- a revised "prepare-patch" interface that uses buffers instead of
  temporary files (I hope this improves the encoding issue),

- use log-view commits if anything is marked,

- removed hard-coded `message-goto-body' calls,

- added bzr and hg support,

- A new user option 'vc-default-patch-addressee'.

Antoine Kalmbach <ane@iki.fi> writes:

>> +(defun vc-compose-patch (addressee subject revisions)
>> +  "Compose a message sending REVISIONS to ADDRESSEE with SUBJECT."
>> +  (interactive (save-current-buffer
>
> Overall, this looks great.  If I understand this right, this doesn't
> support entering revision ranges (abcd1234..ghjk568, HEAD~2, etc), but
> you have to input each absolute revision?  I think that's actually fine,
> because it composes with the proposed vc-log feature of marking commits
> there and then applying vc-compose-patch on the commits.  Trying to
> convert those into VCS specific revision ranges is probably asking for trouble.

Yes, though the previous version of that patch didn't do this, and
instead just passed the commit directly to "git format-patch", which
isn't what the documentation claims the command does.

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Philip Kaludercic <philipk@posteo.net> writes:

> Here is an updated version of the patch:

Another question is if `vc-prepare-patch' should be bound to a key or
not.  From what I see "C-x v p" appears to be free, which might be a
possible candidate.

bug#57400: 29.0.50; Support sending patches from VC directly

[Eli Zaretskii]

> Cc: 57400@debbugs.gnu.org
> From: Philip Kaludercic <philipk@posteo.net>
> Date: Wed, 05 Oct 2022 17:34:22 +0000
> 
> Here is an updated version of the patch:

Thanks.

> +@findex vc-prepare-patch
> +When collaborating on projects it is common to send patches via Email,

I think "email" should not be capitalized.

> ++++
> +*** New 'vc-prepare-patch' command.

"New command 'vc-prepare-patch'."

> +** Subr-x
> +
> +---
> +*** New macro 'with-funcall-substitutions'.
> +The macro can be used to generically substitute function symbols in
> +expressions.

This looks unrelated?

bug#57400: 29.0.50; Support sending patches from VC directly

[Lars Ingebrigtsen]

Philip Kaludercic <philipk@posteo.net> writes:

> Another question is if `vc-prepare-patch' should be bound to a key or
> not.  From what I see "C-x v p" appears to be free, which might be a
> possible candidate.

There's some potential for confusion with pull/push, I guess, but I
can't think of any better key (that's free).

Er...  `C-x v S'?  (For send.)  Hm...  no, `S' is taken in vc-dir
buffers, and it'd be nice to have the same key in vc-dir-mode.  Oh, `p'
is taken in `vc-dir-mode', too.

*scratches head*

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 57400@debbugs.gnu.org
>> From: Philip Kaludercic <philipk@posteo.net>
>> Date: Wed, 05 Oct 2022 17:34:22 +0000
>> 
>> Here is an updated version of the patch:
>
> Thanks.
>
>> +@findex vc-prepare-patch
>> +When collaborating on projects it is common to send patches via Email,
>
> I think "email" should not be capitalized.

Done too.

>> ++++
>> +*** New 'vc-prepare-patch' command.
>
> "New command 'vc-prepare-patch'."

Also done.

>> +** Subr-x
>> +
>> +---
>> +*** New macro 'with-funcall-substitutions'.
>> +The macro can be used to generically substitute function symbols in
>> +expressions.
>
> This looks unrelated?

You are right, this is from a different patch.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-a-VC-command-to-prepare-patches.patch --]
[-- Type: text/x-patch, Size: 14437 bytes --]

From 5a79e575ab37a1fa60d9428e5d05e7bf95ad570a Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Mon, 3 Oct 2022 20:54:38 +0200
Subject: [PATCH] Add a VC command to prepare patches

* doc/emacs/vc1-xtra.texi (Miscellaneous VC):  Add new node.
(Editing VC Commands):  Document new feature.
* etc/NEWS:  Mention 'vc-prepare-patch'.
* lisp/vc/log-view.el: Autoload 'log-view-get-marked'.
* lisp/vc/vc-git.el (vc-git-prepare-patch):  Add Git implementation.
* lisp/vc/vc-hg.el (vc-git-prepare-patch):  Add Mercurial implementation.
* lisp/vc/vc-bzr.el (vc-git-prepare-patch):  Add Bazaar implementation.
* lisp/vc/vc.el (vc-read-revision):  Add a MULTIPLE argument.
(vc-read-multiple-revisions):  Add an auxiliary function that always
calls 'vc-read-revision' with a non-nil value for MULTIPLE.
(vc-prepare-patches-inline):  Add user option.
(message-goto-body):  Declare function.
(message--name-table):  Declare function.
(vc-default-prepare-patch): Add a default implementation.
(vc-prepare-patch):  Add command.  (bug#57400)
---
 doc/emacs/vc1-xtra.texi |  26 +++++++++
 etc/NEWS                |   8 +++
 lisp/vc/log-view.el     |   1 +
 lisp/vc/vc-bzr.el       |  14 +++++
 lisp/vc/vc-git.el       |  24 ++++++++
 lisp/vc/vc-hg.el        |  12 ++++
 lisp/vc/vc.el           | 122 ++++++++++++++++++++++++++++++++++++++--
 7 files changed, 203 insertions(+), 4 deletions(-)

diff --git a/doc/emacs/vc1-xtra.texi b/doc/emacs/vc1-xtra.texi
index 05d2144380..b27dd6644f 100644
--- a/doc/emacs/vc1-xtra.texi
+++ b/doc/emacs/vc1-xtra.texi
@@ -16,6 +16,7 @@ Miscellaneous VC
 * Revision Tags::       Symbolic names for revisions.
 * Version Headers::     Inserting version control headers into working files.
 * Editing VC Commands:: Editing the VC shell commands that Emacs will run.
+* Preparing Patches::   Preparing and Composing patches from within VC
 @end menu
 
 @node Change Logs and VC
@@ -282,6 +283,31 @@ Editing VC Commands
 additional branches to the end of the @samp{git log} command that VC
 is about to run.
 
+@node Preparing Patches
+@subsubsection Preparing Patches
+
+@findex vc-prepare-patch
+When collaborating on projects it is common to send patches via email,
+to share changes.  If you wish to do this using VC, you can use the
+@code{vc-prepare-patch} command.  This will prompt you which revisions
+you wish to share and who the addressee is.  The command will then use
+your @abbr{MUA, Mail User Agent} for you to review and send out.
+
+@vindex vc-prepare-patches-inline
+Depending on configuration of the user option
+@code{vc-prepare-patches-inline}, @code{vc-prepare-patch} will either
+generate a single or multiple messages.  A @code{nil} value (the default)
+will prepare and display a message for each revision, one after
+another.  A non-@code{nil} value will have all patches attached to the
+body of a single message.
+
+@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 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}).
+
 @node Customizing VC
 @subsection Customizing VC
 
diff --git a/etc/NEWS b/etc/NEWS
index 0fa24c577c..9cf1930c39 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1846,6 +1846,14 @@ Git commands display summary lines.  See the two new user options
 It is used to style the line that separates the 'log-edit' headers
 from the 'log-edit' summary.
 
++++
+*** New command 'vc-prepare-patch'.
+Patches for any version control system can be prepared using VC.  The
+command will query what commits to send and will compose messages for
+your mail user agent.  The behaviour of 'vc-prepare-patch' can be
+modified by the user options 'vc-prepare-patches-inline' and
+'vc-default-patch-addressee'.
+
 ** Message
 
 ---
diff --git a/lisp/vc/log-view.el b/lisp/vc/log-view.el
index 415b1564ed..7a710386fe 100644
--- a/lisp/vc/log-view.el
+++ b/lisp/vc/log-view.el
@@ -359,6 +359,7 @@ log-view-toggle-mark-entry
 	    (overlay-put ov 'log-view-self ov)
 	    (overlay-put ov 'log-view-marked (nth 1 entry))))))))
 
+;;;###autoload
 (defun log-view-get-marked ()
   "Return the list of tags for the marked log entries."
   (save-excursion
diff --git a/lisp/vc/vc-bzr.el b/lisp/vc/vc-bzr.el
index bce7996712..6f77f99555 100644
--- a/lisp/vc/vc-bzr.el
+++ b/lisp/vc/vc-bzr.el
@@ -1324,6 +1324,20 @@ vc-bzr-repository-url
           (match-string 1)
         (error "Cannot determine Bzr repository URL")))))
 
+(defun vc-bzr-prepare-patch (rev)
+  (with-current-buffer (generate-new-buffer " *vc-bzr-prepare-patch*")
+    (vc-bzr-command
+     "send" t 0 '()
+     "--revision" (concat (vc-bzr-previous-revision nil rev) ".." rev)
+     "--output" "-")
+    (let (subject)
+      ;; Extract the subject line
+      (goto-char (point-min))
+      (search-forward-regexp "^[^#].*")
+      (setq subject (match-string 0))
+      ;; Return the extracted data
+      (list :subject subject :buffer (current-buffer)))))
+
 (provide 'vc-bzr)
 
 ;;; vc-bzr.el ends here
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 4a2a42ad2a..f9dae8b9ea 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -95,6 +95,7 @@
 ;; - find-file-hook ()                             OK
 ;; - conflicted-files                              OK
 ;; - repository-url (file-or-dir)                  OK
+;; - prepare-patch (rev)                           OK
 
 ;;; Code:
 
@@ -1742,6 +1743,29 @@ vc-git-extra-status-menu
 (defun vc-git-root (file)
   (vc-find-root file ".git"))
 
+(defun vc-git-prepare-patch (rev)
+  (with-current-buffer (generate-new-buffer " *vc-git-prepare-patch*")
+    (vc-git-command
+     t 0 '()  "format-patch"
+     "--no-numbered" "--stdout"
+     ;; From gitrevisions(7): ^<n> means the <n>th parent
+     ;; (i.e.  <rev>^ is equivalent to <rev>^1). As a
+     ;; special rule, <rev>^0 means the commit itself and
+     ;; is used when <rev> is the object name of a tag
+     ;; object that refers to a commit object.
+     (concat rev "^.." rev))
+    (let (subject)
+      ;; Extract the subject line
+      (goto-char (point-min))
+      (search-forward-regexp "^Subject: \\(.+\\)")
+      (setq subject (match-string 1))
+      ;; Jump to the beginning for the patch
+      (search-forward-regexp "\n\n")
+      ;; Return the extracted data
+      (list :subject subject
+            :buffer (current-buffer)
+            :body-start (point)))))
+
 ;; grep-compute-defaults autoloads grep.
 (declare-function grep-read-regexp "grep" ())
 (declare-function grep-read-files "grep" (regexp))
diff --git a/lisp/vc/vc-hg.el b/lisp/vc/vc-hg.el
index eed9592378..2eebe2d543 100644
--- a/lisp/vc/vc-hg.el
+++ b/lisp/vc/vc-hg.el
@@ -80,6 +80,7 @@
 ;; - delete-file (file)                        TEST IT
 ;; - rename-file (old new)                     OK
 ;; - find-file-hook ()                         added for bug#10709
+;; - prepare-patch (rev)                       OK
 
 ;; 2) Implement Stefan Monnier's advice:
 ;; vc-hg-registered and vc-hg-state
@@ -1507,6 +1508,17 @@ vc-hg-merge-branch
     (with-current-buffer buffer (vc-run-delayed (vc-compilation-mode 'hg)))
     (vc-set-async-update buffer)))
 
+(defun vc-hg-prepare-patch (rev)
+  (with-current-buffer (generate-new-buffer " *vc-hg-prepare-patch*")
+    (vc-hg-command t 0 '() "export" "--rev" rev)
+    (let (subject)
+      ;; Extract the subject line
+      (goto-char (point-min))
+      (search-forward-regexp "^[^#].*")
+      (setq subject (match-string 0))
+      ;; Return the extracted data
+      (list :subject subject :buffer (current-buffer)))))
+
 ;;; Internal functions
 
 (defun vc-hg-command (buffer okstatus file-or-list &rest flags)
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index f0f5ee504c..130dee531f 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -574,6 +574,14 @@
 ;;   containing FILE-OR-DIR.  The optional REMOTE-NAME specifies the
 ;;   remote (in Git parlance) whose URL is to be returned.  It has
 ;;   only a meaning for distributed VCS and is ignored otherwise.
+;;
+;; - prepare-patch (rev)
+;;
+;;   Prepare a patch and return a property list with the keys
+;;   `:subject' indicating the patch message as a string, `:body'
+;;   containing the contents of the patch as a string (excluding the
+;;   header) and `:filename' pointing to a file where the patch has
+;;   been stored.
 
 ;;; Changes from the pre-25.1 API:
 ;;
@@ -1910,7 +1918,7 @@ vc-diff-internal
 (defvar vc-revision-history nil
   "History for `vc-read-revision'.")
 
-(defun vc-read-revision (prompt &optional files backend default initial-input)
+(defun vc-read-revision (prompt &optional files backend default initial-input multiple)
   (cond
    ((null files)
     (let ((vc-fileset (vc-deduce-fileset t))) ;FIXME: why t?  --Stef
@@ -1923,9 +1931,16 @@ vc-read-revision
          (completion-table
           (vc-call-backend backend 'revision-completion-table files)))
     (if completion-table
-        (completing-read prompt completion-table
-                         nil nil initial-input 'vc-revision-history default)
-      (read-string prompt initial-input nil default))))
+        (funcall
+         (if multiple #'completing-read-multiple #'completing-read)
+         prompt completion-table nil nil initial-input 'vc-revision-history default)
+      (let ((answer (read-string prompt initial-input nil default)))
+        (if multiple
+            (split-string answer "[ \t]*,[ \t]*")
+          answer)))))
+
+(defun vc-read-multiple-revisions (prompt &optional files backend default initial-input)
+  (vc-read-revision prompt files backend default initial-input t))
 
 (defun vc-diff-build-argument-list-internal (&optional fileset)
   "Build argument list for calling internal diff functions."
@@ -3237,6 +3252,105 @@ vc-update-change-log
   (vc-call-backend (vc-responsible-backend default-directory)
                    'update-changelog args))
 
+(defcustom vc-prepare-patches-inline nil
+  "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."
+  :type 'boolean
+  :safe #'booleanp
+  :version "29.1")
+
+(defcustom vc-default-patch-addressee nil
+  "Default addressee for `vc-prepare-patch'.
+If nil, no default will be used.  This option may be set locally."
+  :type '(choice (const :tag "No default" nil)
+                 (string :tag "Addressee"))
+  :safe #'stringp
+  :version "29.1")
+
+(declare-function message--name-table "message" (orig-string))
+(declare-function mml-attach-buffer "mml"
+                  (buffer &optional type description disposition))
+(declare-function log-view-get-marked "log-view" ())
+
+(defun vc-default-prepare-patch (rev)
+  (let ((backend (vc-backend buffer-file-name)))
+    (with-current-buffer (generate-new-buffer " *vc-default-prepare-patch*")
+      (vc-diff-internal
+       nil (list backend) rev
+       (vc-call-backend backend 'previous-revision
+                        buffer-file-name rev)
+       nil t)
+      (list :subject (concat "Patch for "
+                             (file-name-nondirectory
+                              (directory-file-name
+                               (vc-root-dir))))
+            :buffer (current-buffer)))))
+
+;;;###autoload
+(defun vc-prepare-patch (addressee subject revisions)
+  "Compose an Email sending patches for REVISIONS to ADDRESSEE.
+If `vc-prepare-patches-inline' is non-nil, SUBJECT will be used
+as the default subject for the message.  Otherwise a separate
+message will be composed for each revision.
+
+When invoked interactively in a Log View buffer with marked
+revisions, these revisions will be used."
+  (interactive
+   (let ((revs (or (log-view-get-marked)
+                   (vc-read-multiple-revisions "Revisions: ")))
+         to)
+     (require 'message)
+     (while (null (setq to (completing-read-multiple
+                            (format-prompt
+                             "Addressee"
+                             vc-default-patch-addressee)
+                            (message--name-table "")
+                            nil nil nil nil
+                            vc-default-patch-addressee)))
+       (message "At least one addressee required.")
+       (sit-for blink-matching-delay))
+     (list (string-join to ", ")
+           (and vc-prepare-patches-inline
+                (read-string "Subject: " "[PATCH] " nil nil t))
+           revs)))
+  (save-current-buffer
+    (vc-ensure-vc-buffer)
+    (let ((patches (mapcar (lambda (rev)
+                             (vc-call-backend
+                              (vc-responsible-backend default-directory)
+                              'prepare-patch rev))
+                           revisions)))
+      (if (not vc-prepare-patches-inline)
+          (dolist (patch patches)
+            (compose-mail addressee
+                          (plist-get patch :subject)
+                          nil nil nil nil
+                          `((kill-buffer ,(plist-get patch :buffer))
+                            (exit-recursive-edit)))
+            (rfc822-goto-eoh) (forward-line)
+            (save-excursion             ;don't jump to the end
+              (insert-buffer-substring
+               (plist-get patch :buffer)
+               (plist-get patch :body-start)
+               (plist-get patch :body-end)))
+            (recursive-edit))
+        (compose-mail addressee subject nil nil nil nil
+                      (mapcar
+                       (lambda (p)
+                         (list #'kill-buffer (plist-get p :buffer)))
+                       patches))
+        (rfc822-goto-eoh)
+        (forward-line)
+        (save-excursion
+          (dolist (patch patches)
+            (mml-attach-buffer (buffer-name (plist-get patch :buffer))
+                               "text/x-patch"
+                               (plist-get patch :subject)
+                               "attachment")))
+        (open-line 2)))))
+
 (defun vc-default-responsible-p (_backend _file)
   "Indicate whether BACKEND is responsible for FILE.
 The default is to return nil always."
-- 
2.37.3

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Another question is if `vc-prepare-patch' should be bound to a key or
>> not.  From what I see "C-x v p" appears to be free, which might be a
>> possible candidate.
>
> There's some potential for confusion with pull/push, I guess, but I
> can't think of any better key (that's free).
>
> Er...  `C-x v S'?  (For send.)  Hm...  no, `S' is taken in vc-dir
> buffers, and it'd be nice to have the same key in vc-dir-mode.  Oh, `p'
> is taken in `vc-dir-mode', too.
>
> *scratches head*

There is no inherent reason why it has to be bound.  I just wanted to
bring it up.

bug#57400: 29.0.50; Support sending patches from VC directly

[Lars Ingebrigtsen]

Philip Kaludercic <philipk@posteo.net> writes:

> There is no inherent reason why it has to be bound.  I just wanted to
> bring it up.

Sure.  But I think it would be nice if it had a binding -- I think this
is very useful functionality.

bug#57400: 29.0.50; Support sending patches from VC directly

[Juri Linkov]

>> Another question is if `vc-prepare-patch' should be bound to a key or
>> not.  From what I see "C-x v p" appears to be free, which might be a
>> possible candidate.
>
> There's some potential for confusion with pull/push, I guess, but I
> can't think of any better key (that's free).
>
> Er...  `C-x v S'?  (For send.)  Hm...  no, `S' is taken in vc-dir
> buffers, and it'd be nice to have the same key in vc-dir-mode.  Oh, `p'
> is taken in `vc-dir-mode', too.

Currently 'C-x v =' (vc-diff) can be easily mistyped as 'C-x v +' (vc-pull)
often with an adverse effect because '=' and '+' are on the same key.
It would be nice to bring both 'vc-pull' and 'vc-push' under the same
prefix key 'C-x v p'.  And to add vc-prepare-patch under the same prefix.

'p' and 'S' in vc-dir are specific to the vc-dir buffer.
So 'C-x v S' could be bound to the command vc-log-search
that has no key binding.

bug#57400: 29.0.50; Support sending patches from VC directly

[Robert Pluim]

>>>>> On Wed, 05 Oct 2022 21:08:36 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:

    Lars> Philip Kaludercic <philipk@posteo.net> writes:
    >> There is no inherent reason why it has to be bound.  I just wanted to
    >> bring it up.

    Lars> Sure.  But I think it would be nice if it had a binding -- I think this
    Lars> is very useful functionality.

If we once again steal from Magit, it has patch commands on 'W',
presumably for 'W'rite (and 'git am' is on 'w')

Robert
-- 

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Robert Pluim <rpluim@gmail.com> writes:

>>>>>> On Wed, 05 Oct 2022 21:08:36 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:
>
>     Lars> Philip Kaludercic <philipk@posteo.net> writes:
>     >> There is no inherent reason why it has to be bound.  I just wanted to
>     >> bring it up.
>
>     Lars> Sure.  But I think it would be nice if it had a binding -- I think this
>     Lars> is very useful functionality.
>
> If we once again steal from Magit, it has patch commands on 'W',
> presumably for 'W'rite (and 'git am' is on 'w')

Personally I have never been fond of that binding, and have invoked it a
few times by mistake assuming that w would add something like a commit
tag to the kill ring.

> Robert

bug#57400: 29.0.50; Support sending patches from VC directly

[Robert Pluim]

>>>>> On Thu, 06 Oct 2022 08:35:51 +0000, Philip Kaludercic <philipk@posteo.net> said:

    Philip> Robert Pluim <rpluim@gmail.com> writes:
    >>>>>>> On Wed, 05 Oct 2022 21:08:36 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:
    >> 
    Lars> Philip Kaludercic <philipk@posteo.net> writes:
    >> >> There is no inherent reason why it has to be bound.  I just wanted to
    >> >> bring it up.
    >> 
    Lars> Sure.  But I think it would be nice if it had a binding -- I think this
    Lars> is very useful functionality.
    >> 
    >> If we once again steal from Magit, it has patch commands on 'W',
    >> presumably for 'W'rite (and 'git am' is on 'w')

    Philip> Personally I have never been fond of that binding, and have invoked it a
    Philip> few times by mistake assuming that w would add something like a commit
    Philip> tag to the kill ring.

Thatʼs on 'C-w'

Robert
-- 

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Robert Pluim <rpluim@gmail.com> writes:

>>>>>> On Thu, 06 Oct 2022 08:35:51 +0000, Philip Kaludercic <philipk@posteo.net> said:
>
>     Philip> Robert Pluim <rpluim@gmail.com> writes:
>     >>>>>>> On Wed, 05 Oct 2022 21:08:36 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:
>     >> 
>     Lars> Philip Kaludercic <philipk@posteo.net> writes:
>     >> >> There is no inherent reason why it has to be bound.  I just wanted to
>     >> >> bring it up.
>     >> 
>     Lars> Sure.  But I think it would be nice if it had a binding -- I think this
>     Lars> is very useful functionality.
>     >> 
>     >> If we once again steal from Magit, it has patch commands on 'W',
>     >> presumably for 'W'rite (and 'git am' is on 'w')
>
>     Philip> Personally I have never been fond of that binding, and have invoked it a
>     Philip> few times by mistake assuming that w would add something like a commit
>     Philip> tag to the kill ring.
>
> Thatʼs on 'C-w'

OK, but I would have never guessed that, especially since I bind C-w to
a custom command that runs `backwards-kill-word' when the region is not
active.

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Philip Kaludercic <philipk@posteo.net> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Cc: 57400@debbugs.gnu.org
>>> From: Philip Kaludercic <philipk@posteo.net>
>>> Date: Wed, 05 Oct 2022 17:34:22 +0000
>>> 
>>> Here is an updated version of the patch:
>>
>> Thanks.
>>
>>> +@findex vc-prepare-patch
>>> +When collaborating on projects it is common to send patches via Email,
>>
>> I think "email" should not be capitalized.
>
> Done too.
>
>>> ++++
>>> +*** New 'vc-prepare-patch' command.
>>
>> "New command 'vc-prepare-patch'."
>
> Also done.

Any comments on the updated patch?  Should I push it?  The additional
keybindings can be resolved later.

bug#57400: 29.0.50; Support sending patches from VC directly

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: ane@iki.fi,  57400@debbugs.gnu.org
> Date: Thu, 06 Oct 2022 09:14:13 +0000
> 
> Any comments on the updated patch?  Should I push it?  The additional
> keybindings can be resolved later.

I'd like Dmitry to review and comment, if he has time.

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Philip Kaludercic <philipk@posteo.net> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Another question is if `vc-prepare-patch' should be bound to a key or
>>> not.  From what I see "C-x v p" appears to be free, which might be a
>>> possible candidate.
>>
>> There's some potential for confusion with pull/push, I guess, but I
>> can't think of any better key (that's free).
>>
>> Er...  `C-x v S'?  (For send.)  Hm...  no, `S' is taken in vc-dir
>> buffers, and it'd be nice to have the same key in vc-dir-mode.  Oh, `p'
>> is taken in `vc-dir-mode', too.
>>
>> *scratches head*
>
> There is no inherent reason why it has to be bound.  I just wanted to
> bring it up.

Another reason against "C-x v p" is that "p" is bound to
`vc-dir-previous-line' in vc-dir.

bug#57400: 29.0.50; Support sending patches from VC directly

[Robert Pluim]

>>>>> On Wed, 05 Oct 2022 17:34:22 +0000, Philip Kaludercic <philipk@posteo.net> said:

    Philip> +@code{vc-prepare-patch} command.  This will prompt you which revisions
    Philip> +you wish to share and who the addressee is.  The command will then use
    Philip> +your @abbr{MUA, Mail User Agent} for you to review and send out.
    Philip> +

How about

--begin--
This will prompt you for the revisions you wish to share, and which
destination email address(es) to use.  The command will then prepare
those revisions using your @abbr{MUA, Mail User Agent} for you to
review and send.
--end--

The semantics is 'one-or-more addresses', right?

    Philip> +@vindex vc-prepare-patches-inline
    Philip> +Depending on configuration of the user option

"Depending on the value of the user option"

    Philip> +@code{vc-prepare-patches-inline}, @code{vc-prepare-patch} will either
    Philip> +generate a single or multiple messages.  A @code{nil} value (the default)
    Philip> +will prepare and display a message for each revision, one after
    Philip> +another.  A non-@code{nil} value will have all patches attached to the
    Philip> +body of a single message.
    Philip> +

--begin--
@code{vc-prepare-patches-inline}, @code{vc-prepare-patch} will
generate one or more messages.  The default value @code{nil} means
prepare and display a message for each revision, one after another.  A
non-@code{nil} value means to generate a single message with all
patches attached in the body.
--end--

    Philip> +@vindex vc-default-patch-addressee
    Philip> +If you expect to contribute patches on a regular basis, you can set
    Philip> +the user option @code{vc-default-patch-addressee} to the address you
    Philip> +wish to use.  This will be used as the default value when invoking
    Philip> +@code{vc-prepare-patch}.  Project maintainers may consider setting
    Philip> +this as a directory local variable (@pxref{Directory Variables}).
    Philip> +

This can contain multiple addresses, I think, in which case it should
say so.

 
    Philip> +** Subr-x
    Philip> +
    Philip> +---
    Philip> +*** New macro 'with-funcall-substitutions'.
    Philip> +The macro can be used to generically substitute function symbols in
    Philip> +expressions.
    Philip> +
    Philip>  ** Ansi-color

That sounds interesting, but I donʼt see it in the patch :-)
 
    Philip> +(defcustom vc-prepare-patches-inline nil
    Philip> +  "Non-nil means that `vc-prepare-patch' creates a single
    Philip> message.

"Whether `vc-prepare-patch' attaches all revision in a single message."

Iʼm not sure this should have the suffix '-inline', because you can
have inline attachments and attached attachments, but itʼs not a big
deal.

I also wonder about the default. Creating 100 mail buffers by accident
is harder to recover from than a single one with 100 attachments, but
I guess experience will inform us.

    Philip> +A single message is created by attaching all patches to the body
    Philip> +of a single message.  If nil, each patch will be sent out in a
    Philip> +separate message, which will be prepared sequentially."
    Philip> +  :type 'boolean
    Philip> +  :safe #'booleanp
    Philip> +  :version "29.1")
    Philip> +

(I didnʼt check, can this do the [PATCH n/m] stuff with the
subject that 'git format-patch' can do?)

    Philip> +(defcustom vc-default-patch-addressee nil
    Philip> +  "Default addressee for `vc-prepare-patch'.
    Philip> +If nil, no default will be used.  This option may be set locally."
    Philip> +  :type '(choice (const :tag "No default" nil) string)
    Philip> +  :safe #'stringp
    Philip> +  :version "29.1")
    Philip> +

Again, I think this can be multiple addresses. Either as a string
with commas or as a list of strings perhaps? 

    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-inline' is non-nil, SUBJECT will be used
    Philip> +as the default subject for the message.  Otherwise a separate
    Philip> +message will be composed for each revision.
    Philip> +

? What does `vc-prepare-patches-inline' have to do with the SUBJECT?

    Philip> It includes

    Philip> - some documentation for the Emacs manual and etc/NEWS,

    Philip> - a revised "prepare-patch" interface that uses buffers instead of
    Philip>   temporary files (I hope this improves the encoding issue),

If itʼs all buffers now then I think you need to update this comment:

+;;
+;; - prepare-patch (rev)
+;;
+;;   Prepare a patch and return a property list with the keys
+;;   `:subject' indicating the patch message as a string, `:body'
+;;   containing the contents of the patch as a string (excluding the
+;;   header) and `:filename' pointing to a file where the patch has
+;;   been stored.

I have no firm opinion on if there should be a default binding nor
what it should be 😺

Thanks for this, it will be useful

Robert
-- 

bug#57400: 29.0.50; Support sending patches from VC directly

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> Currently 'C-x v =' (vc-diff) can be easily mistyped as 'C-x v +' (vc-pull)
> often with an adverse effect because '=' and '+' are on the same key.
> It would be nice to bring both 'vc-pull' and 'vc-push' under the same
> prefix key 'C-x v p'.  And to add vc-prepare-patch under the same prefix.

Personally, I've never hit `C-x v +' when I meant to hit `C-x v =' --
because the latter is much easier to type.  (On a US keyboard, I'm sure
it varies.)

But grouping these commands more logically under `C-x v p' does sound
attractive.  That would also give us a place to put pull-and-then-push.
But:

> 'p' and 'S' in vc-dir are specific to the vc-dir buffer.
> So 'C-x v S' could be bound to the command vc-log-search
> that has no key binding.

If we want `C-x v p' as a prefix, then I think `p' should also be a
prefix in vc-dir.  But it's taken for a command that I think people
probably use all the time, so I don't think it's feasible to take it.

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

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

Robert Pluim <rpluim@gmail.com> writes:

>>>>>> On Wed, 05 Oct 2022 17:34:22 +0000, Philip Kaludercic
> <philipk@posteo.net> said:
>
>     Philip> +@code{vc-prepare-patch} command.  This will prompt you
>     Philip> which revisions
>     Philip> +you wish to share and who the addressee is.  The command
>     Philip> will then use
>     Philip> +your @abbr{MUA, Mail User Agent} for you to review and send out.
>     Philip> +
>
> How about
>
> --begin--
> This will prompt you for the revisions you wish to share, and which
> destination email address(es) to use.  The command will then prepare
> those revisions using your @abbr{MUA, Mail User Agent} for you to
> review and send.
> --end--

I like it.

> The semantics is 'one-or-more addresses', right?

Yes, right.

>     Philip> +@vindex vc-prepare-patches-inline
>     Philip> +Depending on configuration of the user option
>
> "Depending on the value of the user option"

Sounds better.


>     Philip> +@code{vc-prepare-patches-inline}, @code{vc-prepare-patch}
>     Philip> will either
>     Philip> +generate a single or multiple messages.  A @code{nil}
>     Philip> value (the default)
>     Philip> +will prepare and display a message for each revision, one after
>     Philip> +another.  A non-@code{nil} value will have all patches
>     Philip> attached to the
>     Philip> +body of a single message.
>     Philip> +
>
> --begin--
> @code{vc-prepare-patches-inline}, @code{vc-prepare-patch} will
> generate one or more messages.  The default value @code{nil} means
> prepare and display a message for each revision, one after another.  A
> non-@code{nil} value means to generate a single message with all
> patches attached in the body.
> --end--

Sounds a lot better.

>     Philip> +@vindex vc-default-patch-addressee
>     Philip> +If you expect to contribute patches on a regular basis, you can set
>     Philip> +the user option @code{vc-default-patch-addressee} to the address you
>     Philip> +wish to use.  This will be used as the default value when invoking
>     Philip> +@code{vc-prepare-patch}.  Project maintainers may consider setting
>     Philip> +this as a directory local variable (@pxref{Directory Variables}).
>     Philip> +
>
> This can contain multiple addresses, I think, in which case it should
> say so.

Would replacing "address" with "address(es)" suffice?

>     Philip> +** Subr-x
>     Philip> +
>     Philip> +---
>     Philip> +*** New macro 'with-funcall-substitutions'.
>     Philip> +The macro can be used to generically substitute function symbols in
>     Philip> +expressions.
>     Philip> +
>     Philip>  ** Ansi-color
>
> That sounds interesting, but I donʼt see it in the patch :-)

As mentioned in another response, this is from a different patch I hope
to submit soon.  I just had it lying around in etc/NEWS.

>     Philip> +(defcustom vc-prepare-patches-inline nil
>     Philip> +  "Non-nil means that `vc-prepare-patch' creates a single
>     Philip> message.
>
> "Whether `vc-prepare-patch' attaches all revision in a single message."
>
> Iʼm not sure this should have the suffix '-inline', because you can
> have inline attachments and attached attachments, but itʼs not a big
> deal.

If you have a better name, there is no better time to change it than now.

> I also wonder about the default. Creating 100 mail buffers by accident
> is harder to recover from than a single one with 100 attachments, but
> I guess experience will inform us.

The only case where this might happen by accident is when someone
invokes `vc-prepare-patch' in a log-edit buffer where all (or at least a
lot) of revisions have been marked.  In that case, one could add a
"safely check" and make sure that the user actually wants to proceed.

>     Philip> +A single message is created by attaching all patches to the body
>     Philip> +of a single message.  If nil, each patch will be sent out in a
>     Philip> +separate message, which will be prepared sequentially."
>     Philip> +  :type 'boolean
>     Philip> +  :safe #'booleanp
>     Philip> +  :version "29.1")
>     Philip> +
>
> (I didnʼt check, can this do the [PATCH n/m] stuff with the
> subject that 'git format-patch' can do?)

Yes, as the Git backend just copies the subject name that
git-format-patch generates.

>     Philip> +(defcustom vc-default-patch-addressee nil
>     Philip> +  "Default addressee for `vc-prepare-patch'.
>     Philip> +If nil, no default will be used.  This option may be set locally."
>     Philip> +  :type '(choice (const :tag "No default" nil) string)
>     Philip> +  :safe #'stringp
>     Philip> +  :version "29.1")
>     Philip> +
>
> Again, I think this can be multiple addresses. Either as a string
> with commas or as a list of strings perhaps? 

As this is just the default value for `read-multiple-choice' a list with
commae should do.  That being said, how common is it to have multiple
people you consistently want to send a patch to?  Usually you'd have a
central mailing list or something like that, I'd assume.

>     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-inline' is non-nil, SUBJECT will be used
>     Philip> +as the default subject for the message.  Otherwise a separate
>     Philip> +message will be composed for each revision.
>     Philip> +
>
> ? What does `vc-prepare-patches-inline' have to do with the SUBJECT?

Because the subject for an "inline patch" is extracted from the commit
message.

>     Philip> It includes
>
>     Philip> - some documentation for the Emacs manual and etc/NEWS,
>
>     Philip> - a revised "prepare-patch" interface that uses buffers instead of
>     Philip>   temporary files (I hope this improves the encoding issue),
>
> If itʼs all buffers now then I think you need to update this comment:
>
> +;;
> +;; - prepare-patch (rev)
> +;;
> +;;   Prepare a patch and return a property list with the keys
> +;;   `:subject' indicating the patch message as a string, `:body'
> +;;   containing the contents of the patch as a string (excluding the
> +;;   header) and `:filename' pointing to a file where the patch has
> +;;   been stored.

You are right, thanks for the reminder!

> I have no firm opinion on if there should be a default binding nor
> what it should be 😺
>
> Thanks for this, it will be useful

I'm glad to hear that.  Here is the updated patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-a-VC-command-to-prepare-patches.patch --]
[-- Type: text/x-patch, Size: 14924 bytes --]

From 9e802cb4fd5657e7536bbff54fef1e8cfe7eafca Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Mon, 3 Oct 2022 20:54:38 +0200
Subject: [PATCH] Add a VC command to prepare patches

* doc/emacs/vc1-xtra.texi (Miscellaneous VC):  Add new node.
(Editing VC Commands):  Document new feature.
* etc/NEWS:  Mention 'vc-prepare-patch'.
* lisp/vc/log-view.el: Autoload 'log-view-get-marked'.
* lisp/vc/vc-git.el (vc-git-prepare-patch):  Add Git implementation.
* lisp/vc/vc-hg.el (vc-git-prepare-patch):  Add Mercurial implementation.
* lisp/vc/vc-bzr.el (vc-git-prepare-patch):  Add Bazaar implementation.
* lisp/vc/vc.el (vc-read-revision):  Add a MULTIPLE argument.
(vc-read-multiple-revisions):  Add an auxiliary function that always
calls 'vc-read-revision' with a non-nil value for MULTIPLE.
(vc-prepare-patches-inline):  Add user option.
(message-goto-body):  Declare function.
(message--name-table):  Declare function.
(vc-default-prepare-patch): Add a default implementation.
(vc-prepare-patch):  Add command.  (Bug#57400)
---
 doc/emacs/vc1-xtra.texi |  27 +++++++++
 etc/NEWS                |  18 ++++++
 lisp/vc/log-view.el     |   1 +
 lisp/vc/vc-bzr.el       |  14 +++++
 lisp/vc/vc-git.el       |  24 ++++++++
 lisp/vc/vc-hg.el        |  12 ++++
 lisp/vc/vc.el           | 124 ++++++++++++++++++++++++++++++++++++++--
 7 files changed, 216 insertions(+), 4 deletions(-)

diff --git a/doc/emacs/vc1-xtra.texi b/doc/emacs/vc1-xtra.texi
index 05d2144380..00f5e5aac6 100644
--- a/doc/emacs/vc1-xtra.texi
+++ b/doc/emacs/vc1-xtra.texi
@@ -16,6 +16,7 @@ Miscellaneous VC
 * Revision Tags::       Symbolic names for revisions.
 * Version Headers::     Inserting version control headers into working files.
 * Editing VC Commands:: Editing the VC shell commands that Emacs will run.
+* Preparing Patches::   Preparing and Composing patches from within VC
 @end menu
 
 @node Change Logs and VC
@@ -282,6 +283,32 @@ Editing VC Commands
 additional branches to the end of the @samp{git log} command that VC
 is about to run.
 
+@node Preparing Patches
+@subsubsection Preparing Patches
+
+@findex vc-prepare-patch
+When collaborating on projects it is common to send patches via email,
+to share changes.  If you wish to do this using VC, you can use the
+@code{vc-prepare-patch} command.  This will prompt you for the
+revisions you wish to share, and which destination email address(es)
+to use.  The command will then prepare those revisions using your
+@abbr{MUA, Mail User Agent} for you to review and send.
+
+@vindex vc-prepare-patches-inline
+Depending on the value of the user option
+@code{vc-prepare-patches-inline}, @code{vc-prepare-patch} will
+generate one or more messages.  The default value @code{nil} means
+prepare and display a message for each revision, one after another.  A
+non-@code{nil} value 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}).
+
 @node Customizing VC
 @subsection Customizing VC
 
diff --git a/etc/NEWS b/etc/NEWS
index 536c7aa319..58a0745a1b 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1865,6 +1865,24 @@ Git commands display summary lines.  See the two new user options
 It is used to style the line that separates the 'log-edit' headers
 from the 'log-edit' summary.
 
+---
+*** 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.
+
++++
+*** New command 'vc-prepare-patch'.
+Patches for any version control system can be prepared using VC.  The
+command will query what commits to send and will compose messages for
+your mail user agent.  The behaviour of 'vc-prepare-patch' can be
+modified by the user options 'vc-prepare-patches-inline' and
+'vc-default-patch-addressee'.
+
 ** Message
 
 ---
diff --git a/lisp/vc/log-view.el b/lisp/vc/log-view.el
index 415b1564ed..7a710386fe 100644
--- a/lisp/vc/log-view.el
+++ b/lisp/vc/log-view.el
@@ -359,6 +359,7 @@ log-view-toggle-mark-entry
 	    (overlay-put ov 'log-view-self ov)
 	    (overlay-put ov 'log-view-marked (nth 1 entry))))))))
 
+;;;###autoload
 (defun log-view-get-marked ()
   "Return the list of tags for the marked log entries."
   (save-excursion
diff --git a/lisp/vc/vc-bzr.el b/lisp/vc/vc-bzr.el
index bce7996712..6f77f99555 100644
--- a/lisp/vc/vc-bzr.el
+++ b/lisp/vc/vc-bzr.el
@@ -1324,6 +1324,20 @@ vc-bzr-repository-url
           (match-string 1)
         (error "Cannot determine Bzr repository URL")))))
 
+(defun vc-bzr-prepare-patch (rev)
+  (with-current-buffer (generate-new-buffer " *vc-bzr-prepare-patch*")
+    (vc-bzr-command
+     "send" t 0 '()
+     "--revision" (concat (vc-bzr-previous-revision nil rev) ".." rev)
+     "--output" "-")
+    (let (subject)
+      ;; Extract the subject line
+      (goto-char (point-min))
+      (search-forward-regexp "^[^#].*")
+      (setq subject (match-string 0))
+      ;; Return the extracted data
+      (list :subject subject :buffer (current-buffer)))))
+
 (provide 'vc-bzr)
 
 ;;; vc-bzr.el ends here
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 4a2a42ad2a..f9dae8b9ea 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -95,6 +95,7 @@
 ;; - find-file-hook ()                             OK
 ;; - conflicted-files                              OK
 ;; - repository-url (file-or-dir)                  OK
+;; - prepare-patch (rev)                           OK
 
 ;;; Code:
 
@@ -1742,6 +1743,29 @@ vc-git-extra-status-menu
 (defun vc-git-root (file)
   (vc-find-root file ".git"))
 
+(defun vc-git-prepare-patch (rev)
+  (with-current-buffer (generate-new-buffer " *vc-git-prepare-patch*")
+    (vc-git-command
+     t 0 '()  "format-patch"
+     "--no-numbered" "--stdout"
+     ;; From gitrevisions(7): ^<n> means the <n>th parent
+     ;; (i.e.  <rev>^ is equivalent to <rev>^1). As a
+     ;; special rule, <rev>^0 means the commit itself and
+     ;; is used when <rev> is the object name of a tag
+     ;; object that refers to a commit object.
+     (concat rev "^.." rev))
+    (let (subject)
+      ;; Extract the subject line
+      (goto-char (point-min))
+      (search-forward-regexp "^Subject: \\(.+\\)")
+      (setq subject (match-string 1))
+      ;; Jump to the beginning for the patch
+      (search-forward-regexp "\n\n")
+      ;; Return the extracted data
+      (list :subject subject
+            :buffer (current-buffer)
+            :body-start (point)))))
+
 ;; grep-compute-defaults autoloads grep.
 (declare-function grep-read-regexp "grep" ())
 (declare-function grep-read-files "grep" (regexp))
diff --git a/lisp/vc/vc-hg.el b/lisp/vc/vc-hg.el
index eed9592378..2eebe2d543 100644
--- a/lisp/vc/vc-hg.el
+++ b/lisp/vc/vc-hg.el
@@ -80,6 +80,7 @@
 ;; - delete-file (file)                        TEST IT
 ;; - rename-file (old new)                     OK
 ;; - find-file-hook ()                         added for bug#10709
+;; - prepare-patch (rev)                       OK
 
 ;; 2) Implement Stefan Monnier's advice:
 ;; vc-hg-registered and vc-hg-state
@@ -1507,6 +1508,17 @@ vc-hg-merge-branch
     (with-current-buffer buffer (vc-run-delayed (vc-compilation-mode 'hg)))
     (vc-set-async-update buffer)))
 
+(defun vc-hg-prepare-patch (rev)
+  (with-current-buffer (generate-new-buffer " *vc-hg-prepare-patch*")
+    (vc-hg-command t 0 '() "export" "--rev" rev)
+    (let (subject)
+      ;; Extract the subject line
+      (goto-char (point-min))
+      (search-forward-regexp "^[^#].*")
+      (setq subject (match-string 0))
+      ;; Return the extracted data
+      (list :subject subject :buffer (current-buffer)))))
+
 ;;; Internal functions
 
 (defun vc-hg-command (buffer okstatus file-or-list &rest flags)
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 787dd51d07..52ea2b83cf 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -574,6 +574,16 @@
 ;;   containing FILE-OR-DIR.  The optional REMOTE-NAME specifies the
 ;;   remote (in Git parlance) whose URL is to be returned.  It has
 ;;   only a meaning for distributed VCS and is ignored otherwise.
+;;
+;; - prepare-patch (rev)
+;;
+;;   Prepare a patch and return a property list with the keys
+;;   `:subject' indicating the patch message as a string, `:buffer'
+;;   with a buffer object that contains the entire patch message and
+;;   `:body-start' and `:body-end' demarcating what part of said
+;;   buffer should be inserted into an inline patch.  If the two last
+;;   properties are omitted, `point-min' and `point-max' will
+;;   respectively be used instead.
 
 ;;; Changes from the pre-25.1 API:
 ;;
@@ -1910,7 +1920,7 @@ vc-diff-internal
 (defvar vc-revision-history nil
   "History for `vc-read-revision'.")
 
-(defun vc-read-revision (prompt &optional files backend default initial-input)
+(defun vc-read-revision (prompt &optional files backend default initial-input multiple)
   (cond
    ((null files)
     (let ((vc-fileset (vc-deduce-fileset t))) ;FIXME: why t?  --Stef
@@ -1923,9 +1933,16 @@ vc-read-revision
          (completion-table
           (vc-call-backend backend 'revision-completion-table files)))
     (if completion-table
-        (completing-read prompt completion-table
-                         nil nil initial-input 'vc-revision-history default)
-      (read-string prompt initial-input nil default))))
+        (funcall
+         (if multiple #'completing-read-multiple #'completing-read)
+         prompt completion-table nil nil initial-input 'vc-revision-history default)
+      (let ((answer (read-string prompt initial-input nil default)))
+        (if multiple
+            (split-string answer "[ \t]*,[ \t]*")
+          answer)))))
+
+(defun vc-read-multiple-revisions (prompt &optional files backend default initial-input)
+  (vc-read-revision prompt files backend default initial-input t))
 
 (defun vc-diff-build-argument-list-internal (&optional fileset)
   "Build argument list for calling internal diff functions."
@@ -3264,6 +3281,105 @@ vc-edit-next-command
           (lambda (&rest args)
             (apply #'vc-user-edit-command (apply old args))))))
 
+(defcustom vc-prepare-patches-inline nil
+  "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."
+  :type 'boolean
+  :safe #'booleanp
+  :version "29.1")
+
+(defcustom vc-default-patch-addressee nil
+  "Default addressee for `vc-prepare-patch'.
+If nil, no default will be used.  This option may be set locally."
+  :type '(choice (const :tag "No default" nil)
+                 (string :tag "Addressee"))
+  :safe #'stringp
+  :version "29.1")
+
+(declare-function message--name-table "message" (orig-string))
+(declare-function mml-attach-buffer "mml"
+                  (buffer &optional type description disposition))
+(declare-function log-view-get-marked "log-view" ())
+
+(defun vc-default-prepare-patch (rev)
+  (let ((backend (vc-backend buffer-file-name)))
+    (with-current-buffer (generate-new-buffer " *vc-default-prepare-patch*")
+      (vc-diff-internal
+       nil (list backend) rev
+       (vc-call-backend backend 'previous-revision
+                        buffer-file-name rev)
+       nil t)
+      (list :subject (concat "Patch for "
+                             (file-name-nondirectory
+                              (directory-file-name
+                               (vc-root-dir))))
+            :buffer (current-buffer)))))
+
+;;;###autoload
+(defun vc-prepare-patch (addressee subject revisions)
+  "Compose an Email sending patches for REVISIONS to ADDRESSEE.
+If `vc-prepare-patches-inline' is non-nil, SUBJECT will be used
+as the default subject for the message.  Otherwise a separate
+message will be composed for each revision.
+
+When invoked interactively in a Log View buffer with marked
+revisions, these revisions will be used."
+  (interactive
+   (let ((revs (or (log-view-get-marked)
+                   (vc-read-multiple-revisions "Revisions: ")))
+         to)
+     (require 'message)
+     (while (null (setq to (completing-read-multiple
+                            (format-prompt
+                             "Addressee"
+                             vc-default-patch-addressee)
+                            (message--name-table "")
+                            nil nil nil nil
+                            vc-default-patch-addressee)))
+       (message "At least one addressee required.")
+       (sit-for blink-matching-delay))
+     (list (string-join to ", ")
+           (and vc-prepare-patches-inline
+                (read-string "Subject: " "[PATCH] " nil nil t))
+           revs)))
+  (save-current-buffer
+    (vc-ensure-vc-buffer)
+    (let ((patches (mapcar (lambda (rev)
+                             (vc-call-backend
+                              (vc-responsible-backend default-directory)
+                              'prepare-patch rev))
+                           revisions)))
+      (if (not vc-prepare-patches-inline)
+          (dolist (patch patches)
+            (compose-mail addressee
+                          (plist-get patch :subject)
+                          nil nil nil nil
+                          `((kill-buffer ,(plist-get patch :buffer))
+                            (exit-recursive-edit)))
+            (rfc822-goto-eoh) (forward-line)
+            (save-excursion             ;don't jump to the end
+              (insert-buffer-substring
+               (plist-get patch :buffer)
+               (plist-get patch :body-start)
+               (plist-get patch :body-end)))
+            (recursive-edit))
+        (compose-mail addressee subject nil nil nil nil
+                      (mapcar
+                       (lambda (p)
+                         (list #'kill-buffer (plist-get p :buffer)))
+                       patches))
+        (rfc822-goto-eoh)
+        (forward-line)
+        (save-excursion
+          (dolist (patch patches)
+            (mml-attach-buffer (buffer-name (plist-get patch :buffer))
+                               "text/x-patch"
+                               (plist-get patch :subject)
+                               "attachment")))
+        (open-line 2)))))
+
 (defun vc-default-responsible-p (_backend _file)
   "Indicate whether BACKEND is responsible for FILE.
 The default is to return nil always."
-- 
2.37.3

bug#57400: 29.0.50; Support sending patches from VC directly

[Robert Pluim]

>>>>> On Thu, 06 Oct 2022 12:38:25 +0000, Philip Kaludercic <philipk@posteo.net> said:
    Philip> +(defcustom vc-prepare-patches-inline nil
    Philip> +  "Non-nil means that `vc-prepare-patch' creates a single
    Philip> message.
    >> 
    >> "Whether `vc-prepare-patch' attaches all revision in a single message."
    >> 
    >> Iʼm not sure this should have the suffix '-inline', because you can
    >> have inline attachments and attached attachments, but itʼs not a big
    >> deal.

    Philip> If you have a better name, there is no better time to change it than now.

`vc-prepare-patch-attach'? `vc-prepare-patch-attach-patches'? Itʼs all
a bit of a mouthful to type, and it doesnʼt feel like much of an
improvement over what you have.

    >> I also wonder about the default. Creating 100 mail buffers by accident
    >> is harder to recover from than a single one with 100 attachments, but
    >> I guess experience will inform us.

    Philip> The only case where this might happen by accident is when someone
    Philip> invokes `vc-prepare-patch' in a log-edit buffer where all (or at least a
    Philip> lot) of revisions have been marked.  In that case, one could add a
    Philip> "safely check" and make sure that the user actually wants to proceed.

That sounds sufficiently hard to achieve by accident that we
should leave it alone for now.

    Philip> +A single message is created by attaching all patches to the body
    Philip> +of a single message.  If nil, each patch will be sent out in a
    Philip> +separate message, which will be prepared sequentially."
    Philip> +  :type 'boolean
    Philip> +  :safe #'booleanp
    Philip> +  :version "29.1")
    Philip> +
    >> 
    >> (I didnʼt check, can this do the [PATCH n/m] stuff with the
    >> subject that 'git format-patch' can do?)

    Philip> Yes, as the Git backend just copies the subject name that
    Philip> git-format-patch generates.

Perfect

    Philip> As this is just the default value for `read-multiple-choice' a list with
    Philip> commae should do.  That being said, how common is it to have multiple
    Philip> people you consistently want to send a patch to?  Usually you'd have a
    Philip> central mailing list or something like that, I'd assume.

Right, and itʼs a string, so it caters for multiple addresses.

    >> ? What does `vc-prepare-patches-inline' have to do with the SUBJECT?

    Philip> Because the subject for an "inline patch" is extracted from the commit
    Philip> message.

Perhaps mention that in the docstring?

Anyway, I think Iʼve picked enough nits for this patch.

Robert
-- 

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Robert Pluim <rpluim@gmail.com> writes:

>>>>>> On Thu, 06 Oct 2022 12:38:25 +0000, Philip Kaludercic
> <philipk@posteo.net> said:
>     Philip> +(defcustom vc-prepare-patches-inline nil
>     Philip> +  "Non-nil means that `vc-prepare-patch' creates a single
>     Philip> message.
>     >> 
>     >> "Whether `vc-prepare-patch' attaches all revision in a single message."
>     >> 
>     >> Iʼm not sure this should have the suffix '-inline', because you can
>     >> have inline attachments and attached attachments, but itʼs not a big
>     >> deal.
>
>     Philip> If you have a better name, there is no better time to
>     Philip> change it than now.
>
> `vc-prepare-patch-attach'? `vc-prepare-patch-attach-patches'? Itʼs all
> a bit of a mouthful to type, and it doesnʼt feel like much of an
> improvement over what you have.

Maybe `vc-prepare-patches-separately' and set the value to t by defaut?

>     >> I also wonder about the default. Creating 100 mail buffers by accident
>     >> is harder to recover from than a single one with 100 attachments, but
>     >> I guess experience will inform us.
>
>     Philip> The only case where this might happen by accident is when someone
>     Philip> invokes `vc-prepare-patch' in a log-edit buffer where all
>     Philip> (or at least a
>     Philip> lot) of revisions have been marked.  In that case, one could add a
>     Philip> "safely check" and make sure that the user actually wants to proceed.
>
> That sounds sufficiently hard to achieve by accident that we
> should leave it alone for now.

Ok.

>     Philip> +A single message is created by attaching all patches to the body
>     Philip> +of a single message.  If nil, each patch will be sent out in a
>     Philip> +separate message, which will be prepared sequentially."
>     Philip> +  :type 'boolean
>     Philip> +  :safe #'booleanp
>     Philip> +  :version "29.1")
>     Philip> +
>     >> 
>     >> (I didnʼt check, can this do the [PATCH n/m] stuff with the
>     >> subject that 'git format-patch' can do?)
>
>     Philip> Yes, as the Git backend just copies the subject name that
>     Philip> git-format-patch generates.
>
> Perfect
>
>     Philip> As this is just the default value for
>     Philip> `read-multiple-choice' a list with
>     Philip> commae should do.  That being said, how common is it to have multiple
>     Philip> people you consistently want to send a patch to?  Usually
>     Philip> you'd have a
>     Philip> central mailing list or something like that, I'd assume.
>
> Right, and itʼs a string, so it caters for multiple addresses.

I am confused, so everything in fine?

>     >> ? What does `vc-prepare-patches-inline' have to do with the SUBJECT?
>
>     Philip> Because the subject for an "inline patch" is extracted
>     Philip> from the commit
>     Philip> message.
>
> Perhaps mention that in the docstring?

That should be doable.

> Anyway, I think Iʼve picked enough nits for this patch.

If there is anything more to nit, please pick.

> Robert

bug#57400: 29.0.50; Support sending patches from VC directly

[Robert Pluim]

>>>>> On Thu, 06 Oct 2022 14:37:59 +0000, Philip Kaludercic <philipk@posteo.net> said:

    Philip> Maybe `vc-prepare-patches-separately' and set the value to t by defaut?

Yes, that sounds good

    >> Right, and itʼs a string, so it caters for multiple addresses.

    Philip> I am confused, so everything in fine?

Yes, itʼs fine

    Philip> If there is anything more to nit, please pick.

At some point you just have to push the changes, and deal with the
fallout :-)

Robert
-- 

bug#57400: 29.0.50; Support sending patches from VC directly

[Eli Zaretskii]

> Cc: 57400@debbugs.gnu.org, Antoine Kalmbach <ane@iki.fi>
> From: Philip Kaludercic <philipk@posteo.net>
> Date: Thu, 06 Oct 2022 14:37:59 +0000
> 
> Maybe `vc-prepare-patches-separately' and set the value to t by defaut?

Why t by default?  Isn't it better to have all the patch series in the
same message?

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 57400@debbugs.gnu.org, Antoine Kalmbach <ane@iki.fi>
>> From: Philip Kaludercic <philipk@posteo.net>
>> Date: Thu, 06 Oct 2022 14:37:59 +0000
>> 
>> Maybe `vc-prepare-patches-separately' and set the value to t by defaut?
>
> Why t by default?  Isn't it better to have all the patch series in the
> same message?

I believe we have discussed this already in a tangent to this bug
report, but if the option is to be useful as a "git send-email"
replacement, it should conform to that style by default.  I believe more
mailing-list based projects insist on that style, that there are
projects that accept attachments.

And as this is a user option, anyone can easily change it, so I think
that the default choice is reasonable.

bug#57400: 29.0.50; Support sending patches from VC directly

[Dmitry Gutov]

Hi Philip,

On 06.10.2022 15:38, Philip Kaludercic wrote:
> I'm glad to hear that.  Here is the updated patch:

This version resolves the main questions I had as well.

Here's one more, though: the way I normally used 'git format-patch' is I 
pass just one ref, and that's usually the name of the branch to start 
the history at (the <since> argument from the manual). So I never had to 
"type revisions in the Git syntax" for this to work, something Eli was 
worried about.

Should this new command support this usage as well?

The range of revisions could be fetched by passing the base revision as 
LIMIT to the 'print-log' action (like vc-log-mergebase does), but how 
the updated calling convention for vc-prepare-patch will look is not 
obvious to me.

bug#57400: 29.0.50; Support sending patches from VC directly

[Dmitry Gutov]

On 06.10.2022 12:19, Eli Zaretskii wrote:
>> From: Philip Kaludercic<philipk@posteo.net>
>> Cc:ane@iki.fi,57400@debbugs.gnu.org
>> Date: Thu, 06 Oct 2022 09:14:13 +0000
>>
>> Any comments on the updated patch?  Should I push it?  The additional
>> keybindings can be resolved later.
> I'd like Dmitry to review and comment, if he has time.

Thanks for the ping, fortunately I noticed this message.

Either the volume of messages on the mailing lists picked up, or I'm 
finding less free time these days, but the numbers of unread messages in 
my Debbugs and Emacs-Diffs folders is now in the thousands, and when I'm 
paging quickly through them, there are good odds of me missing some 
mentions.

I'll try to set up tags through message filters, and see how it goes.

This brings me back to the older discussion about forges and their email 
notifications. And being able to subscribe to specific subjects only. 
And also, when one is not subscribed to a thread (or perhaps to any 
threads as all), a @mention will be sure to attract their attention 
because it doesn't drown in an existing thread of messages that one 
might or might not be interested in reading.

bug#57400: 29.0.50; Support sending patches from VC directly

[Eli Zaretskii]

> Date: Fri, 7 Oct 2022 01:22:36 +0300
> Cc: ane@iki.fi, 57400@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> Either the volume of messages on the mailing lists picked up, or I'm 
> finding less free time these days

The former is definitely true, unfortunately.  I hope it's just a
transient thing, and will subside soon enough...

bug#57400: 29.0.50; Support sending patches from VC directly

[Juri Linkov]

>> If you have a better name, there is no better time to change it than now.
>
> `vc-prepare-patch-attach'? `vc-prepare-patch-attach-patches'? Itʼs all
> a bit of a mouthful to type, and it doesnʼt feel like much of an
> improvement over what you have.

We could base the name of the existing commands.  There is the command
'submit-emacs-patch', so the corresponding VC command could be
'vc-submit-patch'.  Also like the existing command 'git-send-email'
the name could be 'vc-send-patch'.

Also please note that the most convenient way to select revisions
to submit is to show the log buffer, and use 'm' to mark revisions
(log-view-toggle-mark-entry).  Then 'log-view-get-marked' returns
all marked revisions.

I suggest to push the current version of the patch, then such marking
could be added later.

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Hi Philip,
>
> On 06.10.2022 15:38, Philip Kaludercic wrote:
>> I'm glad to hear that.  Here is the updated patch:
>
> This version resolves the main questions I had as well.
>
> Here's one more, though: the way I normally used 'git format-patch' is
> I pass just one ref, and that's usually the name of the branch to
> start the history at (the <since> argument from the manual). So I
> never had to "type revisions in the Git syntax" for this to work,
> something Eli was worried about.

It might be tricky to do this in a VC-generic way, but what I can think
of would be if the command is invoked with a prefix argument, then
instead of querying for specific revisions, we query for one only, then
use 'next-revision' to check if it is a predecessor of the current
revision.  If so, all the commits in-between are used.

The main issue I see here is that 'next-revision' requires a file
argument.  What should that be?

> Should this new command support this usage as well?
>
> The range of revisions could be fetched by passing the base revision
> as LIMIT to the 'print-log' action (like vc-log-mergebase does), but
> how the updated calling convention for vc-prepare-patch will look is
> not obvious to me.

Even if we do this, the value of the argument "files" still remains an
open question.

It is for this reason that I prefer the current approach, especially
when combined with the ability to select commits in a log. 

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Juri Linkov <juri@linkov.net> writes:

>>> If you have a better name, there is no better time to change it than now.
>>
>> `vc-prepare-patch-attach'? `vc-prepare-patch-attach-patches'? Itʼs all
>> a bit of a mouthful to type, and it doesnʼt feel like much of an
>> improvement over what you have.
>
> We could base the name of the existing commands.  There is the command
> 'submit-emacs-patch', so the corresponding VC command could be
> 'vc-submit-patch'.  Also like the existing command 'git-send-email'
> the name could be 'vc-send-patch'.

So should I rename the command?  I'm not committed to any name.

> Also please note that the most convenient way to select revisions
> to submit is to show the log buffer, and use 'm' to mark revisions
> (log-view-toggle-mark-entry).  Then 'log-view-get-marked' returns
> all marked revisions.

This is supported, but currently not in this order.  Do you think an
option should be added that would use recursive editing to prompt the
user?

> I suggest to push the current version of the patch, then such marking
> could be added later.

OK, I'll do so later today.

bug#57400: 29.0.50; Support sending patches from VC directly

[Dmitry Gutov]

On 07.10.2022 09:36, Eli Zaretskii wrote:
>> Date: Fri, 7 Oct 2022 01:22:36 +0300
>> Cc:ane@iki.fi,57400@debbugs.gnu.org
>> From: Dmitry Gutov<dgutov@yandex.ru>
>>
>> Either the volume of messages on the mailing lists picked up, or I'm
>> finding less free time these days
> The former is definitely true, unfortunately.  I hope it's just a
> transient thing, and will subside soon enough...

It's probably a good thing (more activity might mean more improvements 
and better features), but it does put a strain on everybody following 
everything.

bug#57400: 29.0.50; Support sending patches from VC directly

[Dmitry Gutov]

On 07.10.2022 11:03, Philip Kaludercic wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
> 
>> Hi Philip,
>>
>> On 06.10.2022 15:38, Philip Kaludercic wrote:
>>> I'm glad to hear that.  Here is the updated patch:
>>
>> This version resolves the main questions I had as well.
>>
>> Here's one more, though: the way I normally used 'git format-patch' is
>> I pass just one ref, and that's usually the name of the branch to
>> start the history at (the <since> argument from the manual). So I
>> never had to "type revisions in the Git syntax" for this to work,
>> something Eli was worried about.
> 
> It might be tricky to do this in a VC-generic way, but what I can think
> of would be if the command is invoked with a prefix argument, then

prefix argument? Ok. I would imagine this to be the "default" usage 
scenario, though.

> instead of querying for specific revisions, we query for one only, then
> use 'next-revision' to check if it is a predecessor of the current
> revision.  If so, all the commits in-between are used.

<since> is not necessarily a predecessor: 'git format-patch master' 
works even when master has some more extra commits since the "merge 
base" commit. 'master' will point to the commit that's not present in 
the current branch.

But vc-log-mergebase is fine with such situation. It calls 
(vc-call-backend backend 'mergebase rev1 rev2) to find the most recent 
common revision, and start after it.

> The main issue I see here is that 'next-revision' requires a file
> argument.  What should that be?

The 'mergebase' and 'print-log' actions don't seem to require it.

>> Should this new command support this usage as well?
>>
>> The range of revisions could be fetched by passing the base revision
>> as LIMIT to the 'print-log' action (like vc-log-mergebase does), but
>> how the updated calling convention for vc-prepare-patch will look is
>> not obvious to me.
> 
> Even if we do this, the value of the argument "files" still remains an
> open question.

vc-log-mergebase passes (list rootdir) as FILES to 'print-log'.

> It is for this reason that I prefer the current approach, especially
> when combined with the ability to select commits in a log.

I'm definitely not going to insist: I'm not the target audience of this 
feature.

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Dmitry Gutov <dgutov@yandex.ru> writes:

>
> On 07.10.2022 11:03, Philip Kaludercic wrote:
>> Dmitry Gutov <dgutov@yandex.ru> writes:
>> 
>>> Hi Philip,
>>>
>>> On 06.10.2022 15:38, Philip Kaludercic wrote:
>>>> I'm glad to hear that.  Here is the updated patch:
>>>
>>> This version resolves the main questions I had as well.
>>>
>>> Here's one more, though: the way I normally used 'git format-patch' is
>>> I pass just one ref, and that's usually the name of the branch to
>>> start the history at (the <since> argument from the manual). So I
>>> never had to "type revisions in the Git syntax" for this to work,
>>> something Eli was worried about.
>> It might be tricky to do this in a VC-generic way, but what I can
>> think
>> of would be if the command is invoked with a prefix argument, then
>
> prefix argument? Ok. I would imagine this to be the "default" usage
> scenario, though.

It could be the default too, I guess it depends on how well the feature
works.  I certainly know from my own works flow that I don't always want
to submit all the commits on a branch, but just one or two, so having
the option would be appreciated.

>> instead of querying for specific revisions, we query for one only, then
>> use 'next-revision' to check if it is a predecessor of the current
>> revision.  If so, all the commits in-between are used.
>
> <since> is not necessarily a predecessor: 'git format-patch master'
> works even when master has some more extra commits since the "merge
> base" commit. 'master' will point to the commit that's not present in
> the current branch.
>
> But vc-log-mergebase is fine with such situation. It calls
> (vc-call-backend backend 'mergebase rev1 rev2) to find the most recent
> common revision, and start after it.
>
>> The main issue I see here is that 'next-revision' requires a file
>> argument.  What should that be?
>
> The 'mergebase' and 'print-log' actions don't seem to require it.

I will take a look at those.

>>> Should this new command support this usage as well?
>>>
>>> The range of revisions could be fetched by passing the base revision
>>> as LIMIT to the 'print-log' action (like vc-log-mergebase does), but
>>> how the updated calling convention for vc-prepare-patch will look is
>>> not obvious to me.
>> Even if we do this, the value of the argument "files" still remains
>> an
>> open question.
>
> vc-log-mergebase passes (list rootdir) as FILES to 'print-log'.

That appears to only work if the backend defines a root directory.

>> It is for this reason that I prefer the current approach, especially
>> when combined with the ability to select commits in a log.
>
> I'm definitely not going to insist: I'm not the target audience of
> this feature.

I could imagine falling back onto the manual selection of revisions when
the root directory is not defined, but I fear that would be too
inconsistent and might lead to unintended/unexpected behaviour when
switching between VCSs.

bug#57400: 29.0.50; Support sending patches from VC directly

[Dmitry Gutov]

On 07.10.2022 18:30, Philip Kaludercic wrote:
>>> It is for this reason that I prefer the current approach, especially
>>> when combined with the ability to select commits in a log.
>> I'm definitely not going to insist: I'm not the target audience of
>> this feature.
> I could imagine falling back onto the manual selection of revisions when
> the root directory is not defined, but I fear that would be too
> inconsistent and might lead to unintended/unexpected behaviour when
> switching between VCSs.

IMHO we could just refuse to support the backends which don't have the 
notion of root directory, in this feature.

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Dmitry Gutov <dgutov@yandex.ru> writes:

>
> On 07.10.2022 18:30, Philip Kaludercic wrote:
>>>> It is for this reason that I prefer the current approach, especially
>>>> when combined with the ability to select commits in a log.
>>> I'm definitely not going to insist: I'm not the target audience of
>>> this feature.
>> I could imagine falling back onto the manual selection of revisions when
>> the root directory is not defined, but I fear that would be too
>> inconsistent and might lead to unintended/unexpected behaviour when
>> switching between VCSs.
>
> IMHO we could just refuse to support the backends which don't have the
> notion of root directory, in this feature.

I wouldn't be a fan, one such backend is CVS that still has people using
it, eg. the BSD people, who also use mailing lists for development.

bug#57400: 29.0.50; Support sending patches from VC directly

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > Currently 'C-x v =' (vc-diff) can be easily mistyped as 'C-x v +' (vc-pull)
  > often with an adverse effect because '=' and '+' are on the same key.
  > It would be nice to bring both 'vc-pull' and 'vc-push' under the same
  > prefix key 'C-x v p'.  And to add vc-prepare-patch under the same prefix.

Making C-x v p a prefix character  implies 4-char key sequences.
That is rather inconvenient.  Can we find another way?

Also, the similarity will invite confusion just like the similarity
between + and =.

Can we do anything to decide automatically whether to push or pull?


-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Philip Kaludercic <philipk@posteo.net> writes:

>> I suggest to push the current version of the patch, then such marking
>> could be added later.
>
> OK, I'll do so later today.

I have now pushed the patch.  Thanks to all the people who participated
and gave their input.

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Dmitry Gutov <dgutov@yandex.ru> writes:

>> The main issue I see here is that 'next-revision' requires a file
>> argument.  What should that be?
>
> The 'mergebase' and 'print-log' actions don't seem to require it.

I am not sure if this might help, but it seems that (vc-deduce-fileset
t) could give some useful information?

bug#57400: 29.0.50; Support sending patches from VC directly

[German Pacenza]

Philip Kaludercic <philipk@posteo.net> writes:

vc-prepare-patches-separately info manual and docstring contradict each
other:

Info:
"The default value ‘t’ means prepare and display a
message for each revision, one after another.  A value of ‘nil’ means to
generate a single message with all patches attached in the body."

Docstring:
"Non-nil means that vc-prepare-patch creates a single message."

German Pacenza

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

German Pacenza <germanp82@hotmail.com> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
> vc-prepare-patches-separately info manual and docstring contradict each
> other:
>
> Info:
> "The default value ‘t’ means prepare and display a
> message for each revision, one after another.  A value of ‘nil’ means to
> generate a single message with all patches attached in the body."
>
> Docstring:
> "Non-nil means that vc-prepare-patch creates a single message."

The docstring was not updated, I'll fix that.

> German Pacenza

Thank you for noticing.

bug#57400: 29.0.50; Support sending patches from VC directly

[Dmitry Gutov]

On 08.10.2022 15:11, Philip Kaludercic wrote:
> Dmitry Gutov<dgutov@yandex.ru>  writes:
> 
>>> The main issue I see here is that 'next-revision' requires a file
>>> argument.  What should that be?
>> The 'mergebase' and 'print-log' actions don't seem to require it.
> I am not sure if this might help, but it seems that (vc-deduce-fileset
> t) could give some useful information?

I suppose it could, if we wanted to support limiting the generated 
patches to a subset of files. That might be not very obvious behavior, 
though. I don't know.

BTW, does vc-print-root-log really not work with CVS? Perhaps it will be 
a good idea to define the 'root' action there (which will recursively 
check the parent directory and see whether it still contains the "CVS" 
dir). As long as CVS knows how to recursively diff the directory tree, I 
suppose. And show history for it whole.

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 08.10.2022 15:11, Philip Kaludercic wrote:
>> Dmitry Gutov<dgutov@yandex.ru>  writes:
>> 
>>>> The main issue I see here is that 'next-revision' requires a file
>>>> argument.  What should that be?
>>> The 'mergebase' and 'print-log' actions don't seem to require it.
>> I am not sure if this might help, but it seems that (vc-deduce-fileset
>> t) could give some useful information?
>
> I suppose it could, if we wanted to support limiting the generated
> patches to a subset of files. That might be not very obvious behavior,
> though. I don't know.
>
> BTW, does vc-print-root-log really not work with CVS? Perhaps it will
> be a good idea to define the 'root' action there (which will
> recursively check the parent directory and see whether it still
> contains the "CVS" dir). As long as CVS knows how to recursively diff
> the directory tree, I suppose. And show history for it whole.

I checked out the Emacs www repository and tried it out.  This was the
error I got:

Debugger entered--Lisp error: (vc-not-supported root CVS)
  signal(vc-not-supported (root CVS))
  vc-call-backend(CVS root "~/Source/emacs-www/")
  vc-print-root-log(2000)
  funcall-interactively(vc-print-root-log 2000)
  call-interactively(vc-print-root-log nil nil)
  command-execute(vc-print-root-log)

bug#57400: 29.0.50; Support sending patches from VC directly

[Dmitry Gutov]

On 08.10.2022 16:42, Philip Kaludercic wrote:
>> Perhaps it will
>> be a good idea to define the 'root' action there (which will
>> recursively check the parent directory and see whether it still
>> contains the "CVS" dir). As long as CVS knows how to recursively diff
>> the directory tree, I suppose. And show history for it whole.
> I checked out the Emacs www repository and tried it out.  This was the
> error I got:
> 
> Debugger entered--Lisp error: (vc-not-supported root CVS)
>    signal(vc-not-supported (root CVS))
>    vc-call-backend(CVS root "~/Source/emacs-www/")
>    vc-print-root-log(2000)
>    funcall-interactively(vc-print-root-log 2000)
>    call-interactively(vc-print-root-log nil nil)
>    command-execute(vc-print-root-log)

Right, I did too. So I described above how 'vc-cvs-root' could be 
implemented. Aside from minor concerns about performance with Tramp, my 
question is whether vc-root-diff and/or vc-print-root-log will be able 
to work without individual files specified (when FILES is (list root)).

vc-cvs-diff starts with looking for individual file backups, but maybe 
the "slow" fallback path will handle the directory just fine.

bug#57400: 29.0.50; Support sending patches from VC directly

[Juri Linkov]

>> We could base the name of the existing commands.  There is the command
>> 'submit-emacs-patch', so the corresponding VC command could be
>> 'vc-submit-patch'.  Also like the existing command 'git-send-email'
>> the name could be 'vc-send-patch'.
>
> So should I rename the command?  I'm not committed to any name.

The current name is fine.

>> Also please note that the most convenient way to select revisions
>> to submit is to show the log buffer, and use 'm' to mark revisions
>> (log-view-toggle-mark-entry).  Then 'log-view-get-marked' returns
>> all marked revisions.
>
> This is supported,

Thanks.  Since it's the most convenient UI, shouldn't this be
documented in the Info?  IOW, what about copying this from the docstring
to the manual:

  When invoked interactively in a Log View buffer with marked
  revisions, these revisions will be used.

Also the use of "," to separate revisions needs to be documented
in the docstrings and the manual.  Maybe also mention the separator
in the prompt that reads multiple revisions.

> but currently not in this order.  Do you think an option should be
> added that would use recursive editing to prompt the user?

I see that currently the chronological order of revisions is used from the log.
This looks like the right order.  Prompting with the default value that is set
to these revisions from the log would be also nice to do, so users
will be able to change the order manually by moving revisions between ",".

Additional question: shouldn't the behavior of vc-prepare-patches-separately=nil
be equivalent to vc-prepare-patches-separately=t when only one revision is given?
Both cases create just one mail.  So when vc-prepare-patches-separately is nil,
could it extract the subject from the single revision?  Or maybe
vc-prepare-patches-separately should support a new customization value for this?

bug#57400: 29.0.50; Support sending patches from VC directly

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > IMHO we could just refuse to support the backends which don't have the 
  > notion of root directory, in this feature.

Surely we can find a kludge which will usually be somewhat helpful for
those backends, even if it requires a little human help.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Juri Linkov <juri@linkov.net> writes:

>>> Also please note that the most convenient way to select revisions
>>> to submit is to show the log buffer, and use 'm' to mark revisions
>>> (log-view-toggle-mark-entry).  Then 'log-view-get-marked' returns
>>> all marked revisions.
>>
>> This is supported,
>
> Thanks.  Since it's the most convenient UI, shouldn't this be
> documented in the Info?  IOW, what about copying this from the docstring
> to the manual:
>
>   When invoked interactively in a Log View buffer with marked
>   revisions, these revisions will be used.

I can add that.

> Also the use of "," to separate revisions needs to be documented
> in the docstrings and the manual.  Maybe also mention the separator
> in the prompt that reads multiple revisions.

Actually it is whatever was specified by `crm-separator', right?  But
yes, that can be mentioned too.

>> but currently not in this order.  Do you think an option should be
>> added that would use recursive editing to prompt the user?
>
> I see that currently the chronological order of revisions is used from
> the log.  This looks like the right order.  Prompting with the default
> value that is set to these revisions from the log would be also nice
> to do, so users will be able to change the order manually by moving
> revisions between ",".

Again, regarding `crm-separator' I don't know if this is a value that
people change or not (perhaps it should be changed to a defconst).  But
setting that aside, that sounds like a good idea.

I can also imagine that the initial input outside of a log buffer ought
to just be the last commit.

> Additional question: shouldn't the behavior of
> vc-prepare-patches-separately=nil be equivalent to
> vc-prepare-patches-separately=t when only one revision is given?  Both
> cases create just one mail.  So when vc-prepare-patches-separately is
> nil, could it extract the subject from the single revision?  Or maybe
> vc-prepare-patches-separately should support a new customization value
> for this?

Yes, but the formatting is different.  In the one case you attach a
patch to a regular message, while in the other case the message is
prepared in such a way that (at least when using Git) the entire message
is a valid patch, where we can use "git am".

bug#57400: 29.0.50; Support sending patches from VC directly

[Filipp Gunbin]

On 05/10/2022 22:57 +0300, Juri Linkov wrote:

>>> Another question is if `vc-prepare-patch' should be bound to a key or
>>> not.  From what I see "C-x v p" appears to be free, which might be a
>>> possible candidate.
>>
>> There's some potential for confusion with pull/push, I guess, but I
>> can't think of any better key (that's free).
>>
>> Er...  `C-x v S'?  (For send.)  Hm...  no, `S' is taken in vc-dir
>> buffers, and it'd be nice to have the same key in vc-dir-mode.  Oh, `p'
>> is taken in `vc-dir-mode', too.
>
> Currently 'C-x v =' (vc-diff) can be easily mistyped as 'C-x v +' (vc-pull)
> often with an adverse effect because '=' and '+' are on the same key.
> It would be nice to bring both 'vc-pull' and 'vc-push' under the same
> prefix key 'C-x v p'.  And to add vc-prepare-patch under the same
> prefix.

I find there's some similarity between vc-log-outgoing (`C-x v O') and
vc-prepare-patch.  So maybe `C-x v o'?

> 'p' and 'S' in vc-dir are specific to the vc-dir buffer.

> So 'C-x v S' could be bound to the command vc-log-search
> that has no key binding.

It would be great to put this command on some key binding.

Filipp

bug#57400: 29.0.50; Support sending patches from VC directly

[Juri Linkov]

>> Currently 'C-x v =' (vc-diff) can be easily mistyped as 'C-x v +' (vc-pull)
>> often with an adverse effect because '=' and '+' are on the same key.
>> It would be nice to bring both 'vc-pull' and 'vc-push' under the same
>> prefix key 'C-x v p'.  And to add vc-prepare-patch under the same
>> prefix.
>
> I find there's some similarity between vc-log-outgoing (`C-x v O') and
> vc-prepare-patch.  So maybe `C-x v o'?

Not only vc-log-outgoing, but also another useful command to show
the log to be able to mark commits in it for preparing a patch,
is `C-x v M L' (vc-log-mergebase) that shows the log of all changes
in the branch.  So I'd rather turn `C-x v l' into a prefix key.

>> 'p' and 'S' in vc-dir are specific to the vc-dir buffer.
>
>> So 'C-x v S' could be bound to the command vc-log-search
>> that has no key binding.
>
> It would be great to put this command on some key binding.

With `C-x v l' as a prefix key it could be `C-x v l s'.

bug#57400: 29.0.50; Support sending patches from VC directly

[Juri Linkov]

> Again, regarding `crm-separator' I don't know if this is a value that
> people change or not (perhaps it should be changed to a defconst).

At least, it's not defcustom, so it's not intended for user customization.

> I can also imagine that the initial input outside of a log buffer ought
> to just be the last commit.

Depends on whether this is one of the most popular workflows.

>> Additional question: shouldn't the behavior of
>> vc-prepare-patches-separately=nil be equivalent to
>> vc-prepare-patches-separately=t when only one revision is given?  Both
>> cases create just one mail.  So when vc-prepare-patches-separately is
>> nil, could it extract the subject from the single revision?  Or maybe
>> vc-prepare-patches-separately should support a new customization value
>> for this?
>
> Yes, but the formatting is different.  In the one case you attach a
> patch to a regular message, while in the other case the message is
> prepared in such a way that (at least when using Git) the entire message
> is a valid patch, where we can use "git am".

Hmm, I tried both variants, and still not sure which is better
for the 1-patch case.  However, what I definitely suggest to do is
to get rid of recursive-edit, that also Robert noticed on emacs-devel.
recursive-edit is too fragile for modal editing, because such
commands as keyboard-escape-quit can easily break it.  Without
recursive-edit you can just create all mail buffers at once.
Then after sending one mail, its buffer gets buried, and the
next mail buffer will be shown instead, etc.

bug#57400: 29.0.50; Support sending patches from VC directly

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

Can someone please send me a self-contained description of this new
feature?

Which VC backends does it support?

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#57400: 29.0.50; Support sending patches from VC directly

[Lars Ingebrigtsen]

Filipp Gunbin <fgunbin@fastmail.fm> writes:

> I find there's some similarity between vc-log-outgoing (`C-x v O') and
> vc-prepare-patch.  So maybe `C-x v o'?

It sounds logical to me, but unfortunately `o' is already taken in
vc-dir-mode.  😐

bug#57400: 29.0.50; Support sending patches from VC directly

[Lars Ingebrigtsen]

Juri Linkov <juri@linkov.net> writes:

> Not only vc-log-outgoing, but also another useful command to show
> the log to be able to mark commits in it for preparing a patch,
> is `C-x v M L' (vc-log-mergebase) that shows the log of all changes
> in the branch.  So I'd rather turn `C-x v l' into a prefix key.

I think `C-x v l' is one of the more commonly used commands, so changing
it would probably be annoying.

bug#57400: 29.0.50; Support sending patches from VC directly

[Eli Zaretskii]

> Cc: philipk@posteo.net, 57400@debbugs.gnu.org, rpluim@gmail.com, ane@iki.fi
> From: Richard Stallman <rms@gnu.org>
> Date: Mon, 10 Oct 2022 18:01:21 -0400
> 
> Can someone please send me a self-contained description of this new
> feature?

It's in Git, including documentation which describes the feature.  You
can find the changeset by searching "git log" output for the bug
number, 57400.  I attach below the docs parts of the changeset and the
command itself.

> Which VC backends does it support?

All of them, AFAIU.

diff --git a/doc/emacs/vc1-xtra.texi b/doc/emacs/vc1-xtra.texi
index 05d2144..66d3f51 100644
--- a/doc/emacs/vc1-xtra.texi
+++ b/doc/emacs/vc1-xtra.texi
@@ -16,6 +16,7 @@ Miscellaneous VC
 * Revision Tags::       Symbolic names for revisions.
 * Version Headers::     Inserting version control headers into working files.
 * Editing VC Commands:: Editing the VC shell commands that Emacs will run.
+* Preparing Patches::   Preparing and Composing patches from within VC
 @end menu
 
 @node Change Logs and VC
@@ -282,6 +283,32 @@ Editing VC Commands
 additional branches to the end of the @samp{git log} command that VC
 is about to run.
 
+@node Preparing Patches
+@subsubsection Preparing Patches
+
+@findex vc-prepare-patch
+When collaborating on projects it is common to send patches via email,
+to share changes.  If you wish to do this using VC, you can use the
+@code{vc-prepare-patch} command.  This will prompt you for the
+revisions you wish to share, and which destination email address(es)
+to use.  The command will then prepare those revisions using your
+@abbr{MUA, Mail User Agent} for you to review and send.
+
+@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}).
+
 @node Customizing VC
 @subsection Customizing VC
 
diff --git a/etc/NEWS b/etc/NEWS
index f674423..ca85705 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1866,6 +1866,24 @@ Git commands display summary lines.  See the two new user options
 It is used to style the line that separates the 'log-edit' headers
 from the 'log-edit' summary.
 
+---
+*** 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.
+
++++
+*** New command 'vc-prepare-patch'.
+Patches for any version control system can be prepared using VC.  The
+command will query what commits to send and will compose messages for
+your mail user agent.  The behaviour of 'vc-prepare-patch' can be
+modified by the user options 'vc-prepare-patches-separately' and
+'vc-default-patch-addressee'.
+
 ** Message
 
 ---
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 787dd51..72189cf 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -574,6 +574,16 @@
 ;;   containing FILE-OR-DIR.  The optional REMOTE-NAME specifies the
 ;;   remote (in Git parlance) whose URL is to be returned.  It has
 ;;   only a meaning for distributed VCS and is ignored otherwise.
+;;
+;; - prepare-patch (rev)
+;;
+;;   Prepare a patch and return a property list with the keys
+;;   `:subject' indicating the patch message as a string, `:buffer'
+;;   with a buffer object that contains the entire patch message and
+;;   `:body-start' and `:body-end' demarcating what part of said
+;;   buffer should be inserted into an inline patch.  If the two last
+;;   properties are omitted, `point-min' and `point-max' will
+;;   respectively be used instead.
 
 
+(defcustom vc-prepare-patches-separately 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."
+  :type 'boolean
+  :safe #'booleanp
+  :version "29.1")
+
+(defcustom vc-default-patch-addressee nil
+  "Default addressee for `vc-prepare-patch'.
+If nil, no default will be used.  This option may be set locally."
+  :type '(choice (const :tag "No default" nil)
+                 (string :tag "Addressee"))
+  :safe #'stringp
+  :version "29.1")
+
+;;;###autoload
+(defun vc-prepare-patch (addressee subject revisions)
+  "Compose an Email sending patches for REVISIONS to ADDRESSEE.
+If `vc-prepare-patches-separately' is non-nil, SUBJECT will be used
+as the default subject for the message.  Otherwise a separate
+message will be composed for each revision.
+
+When invoked interactively in a Log View buffer with marked
+revisions, these revisions will be used."
+  (interactive
+   (let ((revs (or (log-view-get-marked)
+                   (vc-read-multiple-revisions "Revisions: ")))
+         to)
+     (require 'message)
+     (while (null (setq to (completing-read-multiple
+                            (format-prompt
+                             "Addressee"
+                             vc-default-patch-addressee)
+                            (message--name-table "")
+                            nil nil nil nil
+                            vc-default-patch-addressee)))
+       (message "At least one addressee required.")
+       (sit-for blink-matching-delay))
+     (list (string-join to ", ")
+           (and (not vc-prepare-patches-separately)
+                (read-string "Subject: " "[PATCH] " nil nil t))
+           revs)))
+  (save-current-buffer
+    (vc-ensure-vc-buffer)
+    (let ((patches (mapcar (lambda (rev)
+                             (vc-call-backend
+                              (vc-responsible-backend default-directory)
+                              'prepare-patch rev))
+                           revisions)))
+      (if vc-prepare-patches-separately
+          (dolist (patch patches)
+            (compose-mail addressee
+                          (plist-get patch :subject)
+                          nil nil nil nil
+                          `((kill-buffer ,(plist-get patch :buffer))
+                            (exit-recursive-edit)))
+            (rfc822-goto-eoh) (forward-line)
+            (save-excursion             ;don't jump to the end
+              (insert-buffer-substring
+               (plist-get patch :buffer)
+               (plist-get patch :body-start)
+               (plist-get patch :body-end)))
+            (recursive-edit))
+        (compose-mail addressee subject nil nil nil nil
+                      (mapcar
+                       (lambda (p)
+                         (list #'kill-buffer (plist-get p :buffer)))
+                       patches))
+        (rfc822-goto-eoh)
+        (forward-line)
+        (save-excursion
+          (dolist (patch patches)
+            (mml-attach-buffer (buffer-name (plist-get patch :buffer))
+                               "text/x-patch"
+                               (plist-get patch :subject)
+                               "attachment")))
+        (open-line 2)))))
+
 (defun vc-default-responsible-p (_backend _file)
   "Indicate whether BACKEND is responsible for FILE.
 The default is to return nil always."

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Juri Linkov <juri@linkov.net> writes:

>> Again, regarding `crm-separator' I don't know if this is a value that
>> people change or not (perhaps it should be changed to a defconst).
>
> At least, it's not defcustom, so it's not intended for user customization.

Sure, but could a completion frontend decide to change it?

>> I can also imagine that the initial input outside of a log buffer ought
>> to just be the last commit.
>
> Depends on whether this is one of the most popular workflows.

Seems popular to me, you make a change and want to submit it. The issue
I have noticed is that vc doesn't allow for an easy way to detect the
latest revision.  The best I can do is the last revision for the file
associated with the current buffer.

>>> Additional question: shouldn't the behavior of
>>> vc-prepare-patches-separately=nil be equivalent to
>>> vc-prepare-patches-separately=t when only one revision is given?  Both
>>> cases create just one mail.  So when vc-prepare-patches-separately is
>>> nil, could it extract the subject from the single revision?  Or maybe
>>> vc-prepare-patches-separately should support a new customization value
>>> for this?
>>
>> Yes, but the formatting is different.  In the one case you attach a
>> patch to a regular message, while in the other case the message is
>> prepared in such a way that (at least when using Git) the entire message
>> is a valid patch, where we can use "git am".
>
> Hmm, I tried both variants, and still not sure which is better
> for the 1-patch case.  However, what I definitely suggest to do is
> to get rid of recursive-edit, that also Robert noticed on emacs-devel.
> recursive-edit is too fragile for modal editing, because such
> commands as keyboard-escape-quit can easily break it.  Without
> recursive-edit you can just create all mail buffers at once.
> Then after sending one mail, its buffer gets buried, and the
> next mail buffer will be shown instead, etc.

I think you are right, I'll do that and push the changes.

bug#57400: 29.0.50; Support sending patches from VC directly

[Robert Pluim]

>>>>> On Tue, 11 Oct 2022 12:44:26 +0000, Philip Kaludercic <philipk@posteo.net> said:

    Philip> Juri Linkov <juri@linkov.net> writes:
    >>> Again, regarding `crm-separator' I don't know if this is a value that
    >>> people change or not (perhaps it should be changed to a defconst).
    >> 
    >> At least, it's not defcustom, so it's not intended for user customization.

    Philip> Sure, but could a completion frontend decide to change it?

Iʼm not aware of anything that changes it permanently. org let-binds it in
a few places, but thatʼs perfectly ok. <time passes> So does magit,
forge, helm: but again, thatʼs all harmless.

Robert
-- 

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

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

Juri Linkov <juri@linkov.net> writes:

>> Again, regarding `crm-separator' I don't know if this is a value that
>> people change or not (perhaps it should be changed to a defconst).
>
> At least, it's not defcustom, so it's not intended for user customization.
>
>> I can also imagine that the initial input outside of a log buffer ought
>> to just be the last commit.
>
> Depends on whether this is one of the most popular workflows.
>
>>> Additional question: shouldn't the behavior of
>>> vc-prepare-patches-separately=nil be equivalent to
>>> vc-prepare-patches-separately=t when only one revision is given?  Both
>>> cases create just one mail.  So when vc-prepare-patches-separately is
>>> nil, could it extract the subject from the single revision?  Or maybe
>>> vc-prepare-patches-separately should support a new customization value
>>> for this?
>>
>> Yes, but the formatting is different.  In the one case you attach a
>> patch to a regular message, while in the other case the message is
>> prepared in such a way that (at least when using Git) the entire message
>> is a valid patch, where we can use "git am".
>
> Hmm, I tried both variants, and still not sure which is better
> for the 1-patch case.  However, what I definitely suggest to do is
> to get rid of recursive-edit, that also Robert noticed on emacs-devel.
> recursive-edit is too fragile for modal editing, because such
> commands as keyboard-escape-quit can easily break it.  Without
> recursive-edit you can just create all mail buffers at once.
> Then after sending one mail, its buffer gets buried, and the
> next mail buffer will be shown instead, etc.

I have done all of the above and am prepared to push the changes, but
before I do I was wondering if anyone would have any objections to the
following changes to vc-git.el:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-vc-vc-git.el-vc-git-rev-parse-Abbreviate-commit.patch --]
[-- Type: text/x-patch, Size: 1005 bytes --]

From 7d3a07bc6fdfae579681d8e71685d94b7bfa9ad3 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Tue, 11 Oct 2022 21:11:20 +0200
Subject: [PATCH 1/2] * lisp/vc/vc-git.el (vc-git--rev-parse): Abbreviate
 commits

---
 lisp/vc/vc-git.el | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index ea06ccaf87..3822bb8574 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1677,8 +1677,10 @@ vc-git-previous-revision
 (defun vc-git--rev-parse (rev)
   (with-temp-buffer
     (and
-     (vc-git--out-ok "rev-parse" rev)
-     (buffer-substring-no-properties (point-min) (+ (point-min) 40)))))
+     (vc-git--out-ok "rev-parse" "--short" rev)
+     (string-trim-right
+      (buffer-substring-no-properties (point-min) (min (+ (point-min) 40)
+                                                       (point-max)))))))
 
 (defun vc-git-next-revision (file rev)
   "Git-specific version of `vc-next-revision'."
-- 
2.37.3


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Return-symbolic-commit-for-the-working-revision-if-p.patch --]
[-- Type: text/x-patch, Size: 937 bytes --]

From 8bb9fbfdb39d1fd17430f24e9572bf4dde830758 Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Tue, 11 Oct 2022 21:14:48 +0200
Subject: [PATCH 2/2] Return symbolic commit for the working revision if
 possible

* lisp/vc/vc-git.el (vc-git-working-revision): Attempt to use
'vc-git-symbolic-commit'.
---
 lisp/vc/vc-git.el | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 3822bb8574..d17c02852c 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -373,8 +373,9 @@ vc-git-state
 
 (defun vc-git-working-revision (_file)
   "Git-specific version of `vc-working-revision'."
-  (let (process-file-side-effects)
-    (vc-git--rev-parse "HEAD")))
+  (let* ((process-file-side-effects nil)
+         (commit (vc-git--rev-parse "HEAD")))
+    (or (vc-git-symbolic-commit commit) commit)))
 
 (defun vc-git--symbolic-ref (file)
   (or
-- 
2.37.3

bug#57400: 29.0.50; Support sending patches from VC directly

[Juri Linkov]

>>> I can also imagine that the initial input outside of a log buffer ought
>>> to just be the last commit.
>>
>> Depends on whether this is one of the most popular workflows.
>
> Seems popular to me, you make a change and want to submit it. The issue
> I have noticed is that vc doesn't allow for an easy way to detect the
> latest revision.  The best I can do is the last revision for the file
> associated with the current buffer.

The last revision currently is supported only by git and hg backends
in vc-git-log-edit-toggle-amend and vc-hg-log-edit-toggle-amend.

> I have done all of the above and am prepared to push the changes, but
> before I do I was wondering if anyone would have any objections to the
> following changes to vc-git.el:

Indeed, abbreviated commits are more usable.  But please wait a little
for more comments.

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Juri Linkov <juri@linkov.net> writes:

>>>> I can also imagine that the initial input outside of a log buffer ought
>>>> to just be the last commit.
>>>
>>> Depends on whether this is one of the most popular workflows.
>>
>> Seems popular to me, you make a change and want to submit it. The issue
>> I have noticed is that vc doesn't allow for an easy way to detect the
>> latest revision.  The best I can do is the last revision for the file
>> associated with the current buffer.
>
> The last revision currently is supported only by git and hg backends
> in vc-git-log-edit-toggle-amend and vc-hg-log-edit-toggle-amend.

If it is not supported, then the initial input will be empty.

>> I have done all of the above and am prepared to push the changes, but
>> before I do I was wondering if anyone would have any objections to the
>> following changes to vc-git.el:
>
> Indeed, abbreviated commits are more usable.  But please wait a little
> for more comments.

Of course, that was the plan.

bug#57400: 29.0.50; Support sending patches from VC directly

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

Thanks.  It looks good to me.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#57400: 29.0.50; Support sending patches from VC directly

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > The last revision currently is supported only by git and hg backends
  > in vc-git-log-edit-toggle-amend and vc-hg-log-edit-toggle-amend.

1. I think this can be supported in CVS.  How about doing that?

2. Isn't it natural to be able to specify a previous version
to compare the current workfile against?  C-x v = supports that.

Why not compare the work file
-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#57400: 29.0.50; Support sending patches from VC directly

[Juri Linkov]

>   > The last revision currently is supported only by git and hg backends
>   > in vc-git-log-edit-toggle-amend and vc-hg-log-edit-toggle-amend.
>
> 1. I think this can be supported in CVS.  How about doing that?

It seems this is already supported in vc-cvs-working-revision,
but currently I can't confirm this.

> 2. Isn't it natural to be able to specify a previous version
> to compare the current workfile against?  C-x v = supports that.
>
> Why not compare the work file

In case of vc-git-diff, it just runs `git diff-index` to compare
to the working tree.  However, 'C-u C-x v =' raises more questions:

1. When used on one file, for the default value of the old revision,
it converts the name "HEAD" of the current branch to its unabbreviated
hash number.  So the question is why not leave it alone
and display the default value "HEAD" as is?

2. When used on many files, there is no default value.
But why not use the same "HEAD" even in case of multiple files?

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Juri Linkov <juri@linkov.net> writes:

>>>> I can also imagine that the initial input outside of a log buffer ought
>>>> to just be the last commit.
>>>
>>> Depends on whether this is one of the most popular workflows.
>>
>> Seems popular to me, you make a change and want to submit it. The issue
>> I have noticed is that vc doesn't allow for an easy way to detect the
>> latest revision.  The best I can do is the last revision for the file
>> associated with the current buffer.
>
> The last revision currently is supported only by git and hg backends
> in vc-git-log-edit-toggle-amend and vc-hg-log-edit-toggle-amend.
>
>> I have done all of the above and am prepared to push the changes, but
>> before I do I was wondering if anyone would have any objections to the
>> following changes to vc-git.el:
>
> Indeed, abbreviated commits are more usable.  But please wait a little
> for more comments.

Also note that this change would also only affect working-revision and
previous-revision.  If necessary or preferred, I could also modify the
changes to only affect working-revision.

bug#57400: 29.0.50; Support sending patches from VC directly

[Juri Linkov]

>>> I have done all of the above and am prepared to push the changes, but
>>> before I do I was wondering if anyone would have any objections to the
>>> following changes to vc-git.el:
>>
>> Indeed, abbreviated commits are more usable.  But please wait a little
>> for more comments.
>
> Also note that this change would also only affect working-revision and
> previous-revision.  If necessary or preferred, I could also modify the
> changes to only affect working-revision.

Nicer names would be better everywhere, including previous-revision.
But: In my recent mail to RMS I raised questions about 'C-u C-x v ='.
And when I tried your patch then the default value for "HEAD"
often is very strange.  For example, try to type 'C-u C-x v ='
on the file lisp/paren.el.  Here is what it shows:

  Older revision [remotes/origin/scratch/eglot2emacs~488]: 

I wonder where comes "eglot2emacs" from when the current branch is master?

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Juri Linkov <juri@linkov.net> writes:

>>>> I have done all of the above and am prepared to push the changes, but
>>>> before I do I was wondering if anyone would have any objections to the
>>>> following changes to vc-git.el:
>>>
>>> Indeed, abbreviated commits are more usable.  But please wait a little
>>> for more comments.
>>
>> Also note that this change would also only affect working-revision and
>> previous-revision.  If necessary or preferred, I could also modify the
>> changes to only affect working-revision.
>
> Nicer names would be better everywhere, including previous-revision.
> But: In my recent mail to RMS I raised questions about 'C-u C-x v ='.
> And when I tried your patch then the default value for "HEAD"
> often is very strange.  For example, try to type 'C-u C-x v ='
> on the file lisp/paren.el.  Here is what it shows:
>
>   Older revision [remotes/origin/scratch/eglot2emacs~488]: 
>
> I wonder where comes "eglot2emacs" from when the current branch is master?

Hmm, it looks like Git tried to find a symbolic reference at any cost,
so it found some branch found a relative commit.  I'll try and find out
if there is a way to avoid this, and only use symbolic references if
there is no positional offset.

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

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

Philip Kaludercic <philipk@posteo.net> writes:

> Juri Linkov <juri@linkov.net> writes:
>
>>>>> I have done all of the above and am prepared to push the changes, but
>>>>> before I do I was wondering if anyone would have any objections to the
>>>>> following changes to vc-git.el:
>>>>
>>>> Indeed, abbreviated commits are more usable.  But please wait a little
>>>> for more comments.
>>>
>>> Also note that this change would also only affect working-revision and
>>> previous-revision.  If necessary or preferred, I could also modify the
>>> changes to only affect working-revision.
>>
>> Nicer names would be better everywhere, including previous-revision.
>> But: In my recent mail to RMS I raised questions about 'C-u C-x v ='.
>> And when I tried your patch then the default value for "HEAD"
>> often is very strange.  For example, try to type 'C-u C-x v ='
>> on the file lisp/paren.el.  Here is what it shows:
>>
>>   Older revision [remotes/origin/scratch/eglot2emacs~488]: 
>>
>> I wonder where comes "eglot2emacs" from when the current branch is master?
>
> Hmm, it looks like Git tried to find a symbolic reference at any cost,
> so it found some branch found a relative commit.  I'll try and find out
> if there is a way to avoid this, and only use symbolic references if
> there is no positional offset.

This patch should prevent these kinds things from happening:


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

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 5d564f3c94..47c9082368 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -2030,14 +2030,17 @@ vc-git--run-command-string
                     (setq ok nil))))))
     (and ok str)))
 
-(defun vc-git-symbolic-commit (commit)
+(defun vc-git-symbolic-commit (commit &optional force)
   "Translate COMMIT string into symbolic form.
-Returns nil if not possible."
+Returns nil if not possible.  If the optional argument FORCE is
+non-nil, revisions containing positional
+arguments (e.g. \"master~8\") will also be accepted."
   (and commit
        (let ((name (with-temp-buffer
                      (and
                       (vc-git--out-ok "name-rev" "--name-only" commit)
                       (goto-char (point-min))
+                      (or force (not (looking-at "^.*[~^].*$" t)))
                       (= (forward-line 2) 1)
                       (bolp)
                       (buffer-substring-no-properties (point-min)

bug#57400: 29.0.50; Support sending patches from VC directly

[Eli Zaretskii]

> Cc: Robert Pluim <rpluim@gmail.com>, 57400@debbugs.gnu.org,
>  Antoine Kalmbach <ane@iki.fi>
> From: Philip Kaludercic <philipk@posteo.net>
> Date: Thu, 13 Oct 2022 20:25:03 +0000
> 
> -(defun vc-git-symbolic-commit (commit)
> +(defun vc-git-symbolic-commit (commit &optional force)
>    "Translate COMMIT string into symbolic form.
> -Returns nil if not possible."
> +Returns nil if not possible.

And if possible?  I guess the first sentence lacks something it should
say about that?  (Also, we use "Return", not "Returns", to be
consistent with "Translate".)

>                               If the optional argument FORCE is
> +non-nil, revisions containing positional
> +arguments (e.g. \"master~8\") will also be accepted."

It is not a good idea to use "also" when you didn't explain before
that what this does without FORCE.  (Maybe this should be part of the
explanation of what you mean by "not possible" above?)

Also, there's a passive tense here...

And finally, you use "positional arguments", but "man gitrevisions"
doesn't use this term at all.  So maybe we should use a more accepted
terminology, or at least provide more than just one example to explain
that via the examples?

Thanks.

bug#57400: 29.0.50; Support sending patches from VC directly

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > In case of vc-git-diff, it just runs `git diff-index` to compare
  > to the working tree.  However, 'C-u C-x v =' raises more questions:

  > 1. When used on one file, for the default value of the old revision,
  > it converts the name "HEAD" of the current branch to its unabbreviated
  > hash number.  So the question is why not leave it alone
  > and display the default value "HEAD" as is?

  > 2. When used on many files, there is no default value.
  > But why not use the same "HEAD" even in case of multiple files?

Are you proposing to change C-u C-x v =?  Or asking what this
new command should do?

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: Robert Pluim <rpluim@gmail.com>, 57400@debbugs.gnu.org,
>>  Antoine Kalmbach <ane@iki.fi>
>> From: Philip Kaludercic <philipk@posteo.net>
>> Date: Thu, 13 Oct 2022 20:25:03 +0000
>> 
>> -(defun vc-git-symbolic-commit (commit)
>> +(defun vc-git-symbolic-commit (commit &optional force)
>>    "Translate COMMIT string into symbolic form.
>> -Returns nil if not possible."
>> +Returns nil if not possible.
>
> And if possible?  I guess the first sentence lacks something it should
> say about that?  (Also, we use "Return", not "Returns", to be
> consistent with "Translate".)
>
>>                               If the optional argument FORCE is
>> +non-nil, revisions containing positional
>> +arguments (e.g. \"master~8\") will also be accepted."
>
> It is not a good idea to use "also" when you didn't explain before
> that what this does without FORCE.  (Maybe this should be part of the
> explanation of what you mean by "not possible" above?)
>
> Also, there's a passive tense here...

(This is getting embarrassing...)

> And finally, you use "positional arguments", but "man gitrevisions"
> doesn't use this term at all.  So maybe we should use a more accepted
> terminology, or at least provide more than just one example to explain
> that via the examples?
>
> Thanks.

I've rewritten the entire thing, how does this sound:

  "Translate COMMIT string into symbolic form.
A symbolic form is any revision that is not expressed in using
SHA-1 object name.  If the optional argument FORCE is non-nil,
this might include revision specifications like \"master~8\" (the
8th parent of the commit that \"master\" is currently pointing
to).  If it is not possible to determine a symbolic commit, the
function returns nil."

bug#57400: 29.0.50; Support sending patches from VC directly

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: juri@linkov.net,  rpluim@gmail.com,  57400@debbugs.gnu.org,  ane@iki.fi
> Date: Thu, 13 Oct 2022 22:05:52 +0000
> 
> > Also, there's a passive tense here...
> 
> (This is getting embarrassing...)

No need, it takes time to develop sensitivity to this stuff, and all
of us, including myself, make these mistakes from time to time.

> I've rewritten the entire thing, how does this sound:
> 
>   "Translate COMMIT string into symbolic form.
> A symbolic form is any revision that is not expressed in using
> SHA-1 object name.  If the optional argument FORCE is non-nil,
> this might include revision specifications like \"master~8\" (the
> 8th parent of the commit that \"master\" is currently pointing
> to).  If it is not possible to determine a symbolic commit, the
> function returns nil."

This is much better, but still leaves some obvious questions
unanswered.

>   Translate COMMIT string into symbolic form.

What is a "COMMIT string"?  I guess you mean the commit ID, and the
"string" part is just to indicate that its Lisp data type is a string?
If so, we usually say something like

  Translate Git revision descriptor of COMMIT, a string, to a symbolic form.

>                     If the optional argument FORCE is non-nil,
> this might include revision specifications like \"master~8\" (the
> 8th parent of the commit that \"master\" is currently pointing
> to).

This begs the question: what kind of COMMIT strings are acceptable if
FORCE is nil or omitted?  If we only accept SHA-1 hashes then, this
should perhaps be mentioned in the first sentence.  But from reading
the (unhelpful) man page of "git name-rev" (which leads down the
rabbit hole to "git rev-parse"), it is my understanding that this will
accept _any_ revision descriptor in any form.  So now I wonder why
accepting something like "master~8" needs a special knob: it's just
one of the forms supported by "git name-rev", isn't it?  So maybe you
don't even need the additional argument and don't have to document it?

But if there _are_ valid reasons not to accept the likes of
"master~8", they should be in the doc string.  For example:

  By default, COMMIT strings of the form "master~8" are rejected,
  because <describe the reason here>, but if FORCE is non-nil, they
  are allowed.

If "master~8" (or in general SOMETHING~N) is not the only form that is
rejected, the description should include the other forms as well,
perhaps as examples.

(And note that the text I proposed above fixed yet another
inconsistency: COMMIT is first described as a "revision" and "string"
and then as "revision specification"; a good doc string should use the
same consistent terminology throughout.)

>       If it is not possible to determine a symbolic commit, the
> function returns nil.

Again, for consistency, it is better to say

  If it is not possible to determine the symbolic form of COMMIT, the
  function returns nil.

because the first sentence talked about converting COMMIT into a
symbolic form.

Thanks, and keep up the good work.

bug#57400: 29.0.50; Support sending patches from VC directly

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > > Also, there's a passive tense here...
  > > 
  > > (This is getting embarrassing...)

There is no need to feel embarrassed.  Using the passive
voice when it doesn't help is an imperfection, not sn error.
And there are exceptions where it is an improvement.

As you learn to get sensitive to use of the passive voice,
you'll notice the opportunities to avoid it, and your habits
of writing will improve.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: juri@linkov.net,  rpluim@gmail.com,  57400@debbugs.gnu.org,  ane@iki.fi
>> Date: Thu, 13 Oct 2022 22:05:52 +0000
>> 
>> > Also, there's a passive tense here...
>> 
>> (This is getting embarrassing...)
>
> No need, it takes time to develop sensitivity to this stuff, and all
> of us, including myself, make these mistakes from time to time.

I appreciate hearing this.

>> I've rewritten the entire thing, how does this sound:
>> 
>>   "Translate COMMIT string into symbolic form.
>> A symbolic form is any revision that is not expressed in using
>> SHA-1 object name.  If the optional argument FORCE is non-nil,
>> this might include revision specifications like \"master~8\" (the
>> 8th parent of the commit that \"master\" is currently pointing
>> to).  If it is not possible to determine a symbolic commit, the
>> function returns nil."
>
> This is much better, but still leaves some obvious questions
> unanswered.
>
>>   Translate COMMIT string into symbolic form.
>
> What is a "COMMIT string"?  I guess you mean the commit ID, and the
> "string" part is just to indicate that its Lisp data type is a string?
> If so, we usually say something like
>
>   Translate Git revision descriptor of COMMIT, a string, to a symbolic form.

Is this perhaps not too long?  Would "Translate revision string COMMIT
to a symbolic form." be sufficient, especially as this is actually just
an internal function that wasn't marked as such (the Git history
indicates that this function, with this documentation string has been
around since the initial revision of the file in 2007)?

>>                     If the optional argument FORCE is non-nil,
>> this might include revision specifications like \"master~8\" (the
>> 8th parent of the commit that \"master\" is currently pointing
>> to).
>
> This begs the question: what kind of COMMIT strings are acceptable if
> FORCE is nil or omitted?  If we only accept SHA-1 hashes then, this
> should perhaps be mentioned in the first sentence.  But from reading
> the (unhelpful) man page of "git name-rev" (which leads down the
> rabbit hole to "git rev-parse"), it is my understanding that this will
> accept _any_ revision descriptor in any form.  

Yes, right.  And in absence of any restriction "COMMIT" should be
understood to be a SHA-1 reference or a symbolic reference, right?

>                                                So now I wonder why
> accepting something like "master~8" needs a special knob: it's just
> one of the forms supported by "git name-rev", isn't it?  So maybe you
> don't even need the additional argument and don't have to document it?

That is an open debate, the function is currently only used in vc-git.el
and is never invoked with the optional argument.  I've only added it
because it might be that it could be useful at some point in the future.

> But if there _are_ valid reasons not to accept the likes of
> "master~8", they should be in the doc string.  For example:
>
>   By default, COMMIT strings of the form "master~8" are rejected,
>   because <describe the reason here>, but if FORCE is non-nil, they
>   are allowed.

I guess it is difficult to come up with a "valid reason", the motivation
is that I wanted to have some way to ensure that
`vc-git-working-revision' only returns a symbolic form iff a branch or
tag is pointing to the working revision.  If you think it is preferable,
I could also invert the argument and make it into something like
"no-relative" or even pull the check into `vc-git-working-revision'.

> If "master~8" (or in general SOMETHING~N) is not the only form that is
> rejected, the description should include the other forms as well,
> perhaps as examples.
>
> (And note that the text I proposed above fixed yet another
> inconsistency: COMMIT is first described as a "revision" and "string"
> and then as "revision specification"; a good doc string should use the
> same consistent terminology throughout.)

Good point.

>>       If it is not possible to determine a symbolic commit, the
>> function returns nil.
>
> Again, for consistency, it is better to say
>
>   If it is not possible to determine the symbolic form of COMMIT, the
>   function returns nil.
>
> because the first sentence talked about converting COMMIT into a
> symbolic form.

Done.

> Thanks, and keep up the good work.

Thank you for your effort and detail.

bug#57400: 29.0.50; Support sending patches from VC directly

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: juri@linkov.net,  rpluim@gmail.com,  57400@debbugs.gnu.org,  ane@iki.fi
> Date: Fri, 14 Oct 2022 21:47:43 +0000
> 
> >   Translate Git revision descriptor of COMMIT, a string, to a symbolic form.
> 
> Is this perhaps not too long?  Would "Translate revision string COMMIT
> to a symbolic form." be sufficient, especially as this is actually just
> an internal function that wasn't marked as such (the Git history
> indicates that this function, with this documentation string has been
> around since the initial revision of the file in 2007)?

I disregarded the fact that this is an internal function, because the
issues are general.  But yes, we could make this shorter, once we
agree that the full text should be something like I suggested.  For
example:

  Translate revision string of COMMIT to a symbolic form.

Note that I said "string of COMMIT", because COMMIT is not a string,
it is an entity which the string describes.

> >>                     If the optional argument FORCE is non-nil,
> >> this might include revision specifications like \"master~8\" (the
> >> 8th parent of the commit that \"master\" is currently pointing
> >> to).
> >
> > This begs the question: what kind of COMMIT strings are acceptable if
> > FORCE is nil or omitted?  If we only accept SHA-1 hashes then, this
> > should perhaps be mentioned in the first sentence.  But from reading
> > the (unhelpful) man page of "git name-rev" (which leads down the
> > rabbit hole to "git rev-parse"), it is my understanding that this will
> > accept _any_ revision descriptor in any form.  
> 
> Yes, right.  And in absence of any restriction "COMMIT" should be
> understood to be a SHA-1 reference or a symbolic reference, right?

Hmm... now I'm confused.  I'd think COMMIT could be _any_ reference to
a revision, not just SHA-1?  Because "git name-rev" accepts them all,
no?

> >                                                So now I wonder why
> > accepting something like "master~8" needs a special knob: it's just
> > one of the forms supported by "git name-rev", isn't it?  So maybe you
> > don't even need the additional argument and don't have to document it?
> 
> That is an open debate, the function is currently only used in vc-git.el
> and is never invoked with the optional argument.  I've only added it
> because it might be that it could be useful at some point in the future.
> 
> > But if there _are_ valid reasons not to accept the likes of
> > "master~8", they should be in the doc string.  For example:
> >
> >   By default, COMMIT strings of the form "master~8" are rejected,
> >   because <describe the reason here>, but if FORCE is non-nil, they
> >   are allowed.
> 
> I guess it is difficult to come up with a "valid reason", the motivation
> is that I wanted to have some way to ensure that
> `vc-git-working-revision' only returns a symbolic form iff a branch or
> tag is pointing to the working revision.  If you think it is preferable,
> I could also invert the argument and make it into something like
> "no-relative" or even pull the check into `vc-git-working-revision'.

I'm asking why not accept everything that "git name-rev" accepts, and
remove the need for the additional FORCE argument.  (But this is not a
documentation issue anymore ;-)

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: juri@linkov.net,  rpluim@gmail.com,  57400@debbugs.gnu.org,  ane@iki.fi
>> Date: Fri, 14 Oct 2022 21:47:43 +0000
>> 
>> >   Translate Git revision descriptor of COMMIT, a string, to a symbolic form.
>> 
>> Is this perhaps not too long?  Would "Translate revision string COMMIT
>> to a symbolic form." be sufficient, especially as this is actually just
>> an internal function that wasn't marked as such (the Git history
>> indicates that this function, with this documentation string has been
>> around since the initial revision of the file in 2007)?
>
> I disregarded the fact that this is an internal function, because the
> issues are general.  But yes, we could make this shorter, once we
> agree that the full text should be something like I suggested.  For
> example:
>
>   Translate revision string of COMMIT to a symbolic form.
>
> Note that I said "string of COMMIT", because COMMIT is not a string,
> it is an entity which the string describes.

Right, makes sense.

>> >>                     If the optional argument FORCE is non-nil,
>> >> this might include revision specifications like \"master~8\" (the
>> >> 8th parent of the commit that \"master\" is currently pointing
>> >> to).
>> >
>> > This begs the question: what kind of COMMIT strings are acceptable if
>> > FORCE is nil or omitted?  If we only accept SHA-1 hashes then, this
>> > should perhaps be mentioned in the first sentence.  But from reading
>> > the (unhelpful) man page of "git name-rev" (which leads down the
>> > rabbit hole to "git rev-parse"), it is my understanding that this will
>> > accept _any_ revision descriptor in any form.  
>> 
>> Yes, right.  And in absence of any restriction "COMMIT" should be
>> understood to be a SHA-1 reference or a symbolic reference, right?
>
> Hmm... now I'm confused.  I'd think COMMIT could be _any_ reference to
> a revision, not just SHA-1?  Because "git name-rev" accepts them all,
> no?

Yes?  Maybe I am also confused, but my understanding is that a "commit
reference" (e.g. the contents of COMMIT) is either a symbolic reference
of a SHA-1 reference (non-symbolic).

>> >                                                So now I wonder why
>> > accepting something like "master~8" needs a special knob: it's just
>> > one of the forms supported by "git name-rev", isn't it?  So maybe you
>> > don't even need the additional argument and don't have to document it?
>> 
>> That is an open debate, the function is currently only used in vc-git.el
>> and is never invoked with the optional argument.  I've only added it
>> because it might be that it could be useful at some point in the future.
>> 
>> > But if there _are_ valid reasons not to accept the likes of
>> > "master~8", they should be in the doc string.  For example:
>> >
>> >   By default, COMMIT strings of the form "master~8" are rejected,
>> >   because <describe the reason here>, but if FORCE is non-nil, they
>> >   are allowed.
>> 
>> I guess it is difficult to come up with a "valid reason", the motivation
>> is that I wanted to have some way to ensure that
>> `vc-git-working-revision' only returns a symbolic form iff a branch or
>> tag is pointing to the working revision.  If you think it is preferable,
>> I could also invert the argument and make it into something like
>> "no-relative" or even pull the check into `vc-git-working-revision'.
>
> I'm asking why not accept everything that "git name-rev" accepts, and
> remove the need for the additional FORCE argument.  (But this is not a
> documentation issue anymore ;-)

The function does accept everything that "git name-rev" accepts, after
all it just passes commit to the subcommand.  What FORCE does is
restrict what "git name-rev" response is accepted to be returned by
`vc-git-symbolic-commit'.

bug#57400: 29.0.50; Support sending patches from VC directly

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: juri@linkov.net,  rpluim@gmail.com,  57400@debbugs.gnu.org,  ane@iki.fi
> Date: Sat, 15 Oct 2022 11:44:58 +0000
> 
> The function does accept everything that "git name-rev" accepts, after
> all it just passes commit to the subcommand.  What FORCE does is
> restrict what "git name-rev" response is accepted to be returned by
> `vc-git-symbolic-commit'.

Ah, so this is what I was missing...

Then how about

   If the optional argument FORCE is non-nil, the returned value is
   allowed to include revision specifications like \"master~8\"
   (the 8th parent of the commit currently pointed to by the master
   branch), otherwise such revision specifications are rejected, and
   the function returns nil.

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: juri@linkov.net,  rpluim@gmail.com,  57400@debbugs.gnu.org,  ane@iki.fi
>> Date: Sat, 15 Oct 2022 11:44:58 +0000
>> 
>> The function does accept everything that "git name-rev" accepts, after
>> all it just passes commit to the subcommand.  What FORCE does is
>> restrict what "git name-rev" response is accepted to be returned by
>> `vc-git-symbolic-commit'.
>
> Ah, so this is what I was missing...
>
> Then how about
>
>    If the optional argument FORCE is non-nil, the returned value is
>    allowed to include revision specifications like \"master~8\"
>    (the 8th parent of the commit currently pointed to by the master
>    branch), otherwise such revision specifications are rejected, and
>    the function returns nil.

Sounds good to me.

bug#57400: 29.0.50; Support sending patches from VC directly

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: juri@linkov.net,  rpluim@gmail.com,  57400@debbugs.gnu.org,  ane@iki.fi
> Date: Sat, 15 Oct 2022 15:15:02 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >    If the optional argument FORCE is non-nil, the returned value is
> >    allowed to include revision specifications like \"master~8\"
> >    (the 8th parent of the commit currently pointed to by the master
> >    branch), otherwise such revision specifications are rejected, and
> >    the function returns nil.
> 
> Sounds good to me.

Great!

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Cc: juri@linkov.net,  rpluim@gmail.com,  57400@debbugs.gnu.org,  ane@iki.fi
>> Date: Sat, 15 Oct 2022 15:15:02 +0000
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >    If the optional argument FORCE is non-nil, the returned value is
>> >    allowed to include revision specifications like \"master~8\"
>> >    (the 8th parent of the commit currently pointed to by the master
>> >    branch), otherwise such revision specifications are rejected, and
>> >    the function returns nil.
>> 
>> Sounds good to me.
>
> Great!

OK, I have pushed the changes.

bug#57400: 29.0.50; Support sending patches from VC directly

[Juri Linkov]

>> Hmm, I tried both variants, and still not sure which is better
>> for the 1-patch case.  However, what I definitely suggest to do is
>> to get rid of recursive-edit, that also Robert noticed on emacs-devel.
>> recursive-edit is too fragile for modal editing, because such
>> commands as keyboard-escape-quit can easily break it.  Without
>> recursive-edit you can just create all mail buffers at once.
>> Then after sending one mail, its buffer gets buried, and the
>> next mail buffer will be shown instead, etc.
>
> I think you are right, I'll do that and push the changes.

Thanks.  I noticed there is still dangling exit-recursive-edit.

bug#57400: 29.0.50; Support sending patches from VC directly

[Juri Linkov]

>   > In case of vc-git-diff, it just runs `git diff-index` to compare
>   > to the working tree.  However, 'C-u C-x v =' raises more questions:
>
>   > 1. When used on one file, for the default value of the old revision,
>   > it converts the name "HEAD" of the current branch to its unabbreviated
>   > hash number.  So the question is why not leave it alone
>   > and display the default value "HEAD" as is?
>
>   > 2. When used on many files, there is no default value.
>   > But why not use the same "HEAD" even in case of multiple files?
>
> Are you proposing to change C-u C-x v =?  Or asking what this
> new command should do?

This is related to what Philip is currently doing to use
symbolic names instead of hash numbers as default values
for vc commands like C-u C-x v =, so I hope these questions
will be addressed now.

bug#57400: 29.0.50; Support sending patches from VC directly

[Philip Kaludercic]

Juri Linkov <juri@linkov.net> writes:

>>> Hmm, I tried both variants, and still not sure which is better
>>> for the 1-patch case.  However, what I definitely suggest to do is
>>> to get rid of recursive-edit, that also Robert noticed on emacs-devel.
>>> recursive-edit is too fragile for modal editing, because such
>>> commands as keyboard-escape-quit can easily break it.  Without
>>> recursive-edit you can just create all mail buffers at once.
>>> Then after sending one mail, its buffer gets buried, and the
>>> next mail buffer will be shown instead, etc.
>>
>> I think you are right, I'll do that and push the changes.
>
> Thanks.  I noticed there is still dangling exit-recursive-edit.

Whoops, you are right.  I've fixed it.