GNU bug report logs - #57400
29.0.50; Support sending patches from VC directly

Previous Next

Full log

Message #89 received at 57400 <at> debbugs.gnu.org (full text, mbox):

From: Philip Kaludercic <philipk <at> posteo.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: larsi <at> gnus.org, 57400 <at> debbugs.gnu.org, ane <at> iki.fi
Subject: Re: bug#57400: 29.0.50; Support sending patches from VC directly
Date: Tue, 04 Oct 2022 07:10:15 +0000

Eli Zaretskii <eliz <at> gnu.org> writes:

>> Cc: 57400 <at> debbugs.gnu.org, Antoine Kalmbach <ane <at> iki.fi>
>> From: Philip Kaludercic <philipk <at> 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?

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.