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

[Philip Kaludercic <philipk@posteo.net>]

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?