GNU bug report logs - #60147
30.0.50; vc-prepare-patch: Add numbered patch file names

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 60147 in the body.
You can then email your comments to 60147 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

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

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; vc-prepare-patch: Add numbered patch file names
Date: Sat, 17 Dec 2022 05:40:58 +0000

[Message part 1 (text/plain, inline)]

X-debbugs-cc: philipk <at> posteo.net

Hello,

If you receive a message generated by vc-prepare-patch and then you save all
the attachments to a directory, you lose any indication of the order in which
the patches are meant to be applied.

git-format-patch(1) numbers the patch files it saves.  Thus, with most mail
clients, when the recipient saves all the attachments to a local directory,
they'll be ordered.  I think it would be useful for vc-prepare-patch to do
something similar.  Please find a patch attached.

--
Sean Whitton

[0001-vc-prepare-patch-Number-the-attached-patches.patch (text/x-diff, attachment)]

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

From: Philip Kaludercic <philipk <at> posteo.net>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 60147 <at> debbugs.gnu.org
Subject: Re: bug#60147: 30.0.50; vc-prepare-patch: Add numbered patch file
 names
Date: Sat, 17 Dec 2022 09:33:20 +0000

Sean Whitton <spwhitton <at> spwhitton.name> writes:

> X-debbugs-cc: philipk <at> posteo.net
>
> Hello,
>
> If you receive a message generated by vc-prepare-patch and then you save all
> the attachments to a directory, you lose any indication of the order in which
> the patches are meant to be applied.
>
> git-format-patch(1) numbers the patch files it saves.  Thus, with most mail
> clients, when the recipient saves all the attachments to a local directory,
> they'll be ordered.  I think it would be useful for vc-prepare-patch to do
> something similar.  Please find a patch attached.

This sounds good, find my comments below.

> --
> Sean Whitton
>
> From 226e689b1183a34f3b611185201416e4f3205304 Mon Sep 17 00:00:00 2001
> From: Sean Whitton <spwhitton <at> spwhitton.name>
> Date: Fri, 16 Dec 2022 22:34:52 -0700
> Subject: [PATCH] vc-prepare-patch: Number the attached patches
>
> * lisp/gnus/mml.el (mml-attach-buffer): New FILENAME argument.
> * lisp/vc/vc.el (vc-prepare-patch): When vc-prepare-patches-separately
> is nil, generate file names for the attached patches.  The file names
> begin with numbers indicating the ordering of the patches.
> ---
>  lisp/gnus/mml.el | 13 ++++++++-----
>  lisp/vc/vc.el    | 26 +++++++++++++++++++++-----
>  2 files changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/lisp/gnus/mml.el b/lisp/gnus/mml.el
> index ebd0adf2e25..dc86fe6db96 100644
> --- a/lisp/gnus/mml.el
> +++ b/lisp/gnus/mml.el
> @@ -1484,10 +1484,12 @@ mml-dnd-attach-file
>  	  (setq disposition (mml-minibuffer-read-disposition type nil file)))
>  	(mml-attach-file file type description disposition)))))
>  
> -(defun mml-attach-buffer (buffer &optional type description disposition)
> +(defun mml-attach-buffer (buffer &optional type description disposition filename)
>    "Attach a buffer to the outgoing MIME message.
>  BUFFER is the name of the buffer to attach.  See
> -`mml-attach-file' for details of operation."
> +`mml-attach-file' regarding TYPE, DESCRIPTION and DISPOSITION.
> +FILENAME is a suggested file name for the attachment should a
> +recipient wish to save a copy separate from the message."
>    (interactive
>     (let* ((buffer (read-buffer "Attach buffer: "))
>  	  (type (mml-minibuffer-read-type buffer "text/plain"))
> @@ -1497,9 +1499,10 @@ mml-attach-buffer
>    ;; If in the message header, attach at the end and leave point unchanged.
>    (let ((head (unless (message-in-body-p) (point))))
>      (if head (goto-char (point-max)))
> -    (mml-insert-empty-tag 'part 'type type 'buffer buffer
> -			  'disposition disposition
> -			  'description description)
> +    (apply #'mml-insert-empty-tag
> +           'part 'type type 'buffer buffer
> +	   'disposition disposition 'description description
> +           (and filename `(filename ,filename)))
>      ;; When using Mail mode, make sure it does the mime encoding
>      ;; when you send the message.
>      (or (eq mail-user-agent 'message-user-agent)
> diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
> index 690c907c77e..ed10909fee8 100644
> --- a/lisp/vc/vc.el
> +++ b/lisp/vc/vc.el
> @@ -3466,11 +3466,27 @@ vc-prepare-patch
>          (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")))
> +          (let ((i 0))
> +            (dolist (patch patches)
> +              (let* ((patch-subject (plist-get patch :subject))
> +                     (stripped-subject
> +                      (replace-regexp-in-string
> +                       "^\\[.*PATCH.*\\]\\s-*" "" patch-subject))

I think using a \` instead of ^ would be more robust.

> +                     (filename
> +                      (concat
> +                       (string-trim
> +                        (replace-regexp-in-string
> +                         "\\W" "-" (if (length> stripped-subject 50)
> +                                       (substring stripped-subject 0 50)
> +                                     stripped-subject))

Is limiting the file names to ~50 characters a Git thing?

> +                        "-+" "-+")
> +                       ".patch")))

Perhaps this should be pulled out into a separate function.

> +                (mml-attach-buffer
> +                 (buffer-name (plist-get patch :buffer))
> +                 "text/x-patch"
> +                 patch-subject
> +                 "attachment"
> +                 (format "%04d-%s" (cl-incf i) filename))))))

Is the new additional argument really necessary, or couldn't we just
rename the generated buffer?  We could specify that the buffer must be
fresh/renameable.

>          (open-line 2)))))
>  
>  (defun vc-default-responsible-p (_backend _file)

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

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 60147 <at> debbugs.gnu.org
Subject: Re: bug#60147: 30.0.50; vc-prepare-patch: Add numbered patch file
 names
Date: Sat, 17 Dec 2022 17:33:30 -0700

Hello,

On Sat 17 Dec 2022 at 09:33AM GMT, Philip Kaludercic wrote:

>> +                     (filename
>> +                      (concat
>> +                       (string-trim
>> +                        (replace-regexp-in-string
>> +                         "\\W" "-" (if (length> stripped-subject 50)
>> +                                       (substring stripped-subject 0 50)
>> +                                     stripped-subject))
>
> Is limiting the file names to ~50 characters a Git thing?

Git does it, yes, and I thought it seemed like a good idea in general.

>> +                (mml-attach-buffer
>> +                 (buffer-name (plist-get patch :buffer))
>> +                 "text/x-patch"
>> +                 patch-subject
>> +                 "attachment"
>> +                 (format "%04d-%s" (cl-incf i) filename))))))
>
> Is the new additional argument really necessary, or couldn't we just
> rename the generated buffer?  We could specify that the buffer must be
> fresh/renameable.

The description and the filename for an attachment are not the same
thing -- I don't believe MUAs will save the files with the correct name
unless there is the filename= field.  And I think it's a useful general
addition to mml-attach-buffer.

-- 
Sean Whitton

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

From: Philip Kaludercic <philipk <at> posteo.net>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 60147 <at> debbugs.gnu.org
Subject: Re: bug#60147: 30.0.50; vc-prepare-patch: Add numbered patch file
 names
Date: Sun, 18 Dec 2022 10:45:17 +0000

Sean Whitton <spwhitton <at> spwhitton.name> writes:

> Hello,
>
> On Sat 17 Dec 2022 at 09:33AM GMT, Philip Kaludercic wrote:
>
>>> +                     (filename
>>> +                      (concat
>>> +                       (string-trim
>>> +                        (replace-regexp-in-string
>>> +                         "\\W" "-" (if (length> stripped-subject 50)
>>> +                                       (substring stripped-subject 0 50)
>>> +                                     stripped-subject))
>>
>> Is limiting the file names to ~50 characters a Git thing?
>
> Git does it, yes, and I thought it seemed like a good idea in general.

OK, then it should be a good convention.  And if the file names are
numbered, then there shouldn't be any conflicts when downloading
patches.

>>> +                (mml-attach-buffer
>>> +                 (buffer-name (plist-get patch :buffer))
>>> +                 "text/x-patch"
>>> +                 patch-subject
>>> +                 "attachment"
>>> +                 (format "%04d-%s" (cl-incf i) filename))))))
>>
>> Is the new additional argument really necessary, or couldn't we just
>> rename the generated buffer?  We could specify that the buffer must be
>> fresh/renameable.
>
> The description and the filename for an attachment are not the same
> thing -- I don't believe MUAs will save the files with the correct name
> unless there is the filename= field.  And I think it's a useful general
> addition to mml-attach-buffer.

I see, in that case this should be fine.  Can you apply the patch?

Message #19 received at 60147-done <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Philip Kaludercic <philipk <at> posteo.net>
Cc: 60147-done <at> debbugs.gnu.org
Subject: Re: bug#60147: 30.0.50; vc-prepare-patch: Add numbered patch file
 names
Date: Mon, 19 Dec 2022 17:17:56 -0700

Hello,

Pushed.  Thank you for looking.  We may later want to refine the
vc--subject-to-file-name subroutine to replace other characters too.

-- 
Sean Whitton

Previous Next

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