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.