[PATCH] add new options to git format-patch: --cover-subject and --cover-blurb

[PATCH] add new options to git format-patch: --cover-subject and --cover-blurb

[Larry D'Anna]

This is useful because if you're preparing a patch series with a cover letter
you can easily put together one line to format and email the whole thing to
yourself.  You check to make sure everything is right, and then just change the
recipient address and run it again.

git send-email --to my@mydomain.org  master..HEAD --cover-letter \
    --cover-subject "this is my patch series" --cover-blurb "$(cat blurb.txt)"

check the results in my inbox

git send-email --to git@vger.kernel.org  master..HEAD --cover-letter \
    --cover-subject "this is my patch series" --cover-blurb "$(cat blurb.txt)"

Signed-off-by: Larry D'Anna <larry@elder-gods.org>
---
 Documentation/git-format-patch.txt |    8 ++++++++
 builtin-log.c                      |   15 +++++++++++++--
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 9674f9d..522c56f 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -176,6 +176,14 @@ will want to ensure that threading is disabled for `git send-email`.
 	containing the shortlog and the overall diffstat.  You can
 	fill in a description in the file before sending it out.
 
+--cover-subject=<subject>
+	Instead of using *** SUBJECT HERE ***, specify the subject line of the
+	cover letter.
+
+--cover-blurb=<blurb>
+	Instead of using *** BLURB HERE ***, specify a blurb for the body of the
+	cover letter.
+
 --suffix=.<sfx>::
 	Instead of using `.patch` as the suffix for generated
 	filenames, use specified suffix.  A common alternative is
diff --git a/builtin-log.c b/builtin-log.c
index 8d16832..e7ae37e 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -452,6 +452,8 @@ int cmd_log(int argc, const char **argv, const char *prefix)
 
 /* format-patch */
 
+static const char *cover_subject = "*** SUBJECT HERE ***";
+static const char *cover_blurb = "*** BLURB HERE ***";
 static const char *fmt_patch_suffix = ".patch";
 static int numbered = 0;
 static int auto_number = 1;
@@ -647,7 +649,6 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 {
 	const char *committer;
 	const char *subject_start = NULL;
-	const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n";
 	const char *msg;
 	const char *extra_headers = rev->extra_headers;
 	struct shortlog log;
@@ -695,12 +696,15 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 		if (has_non_ascii(list[i]->buffer))
 			need_8bit_cte = 1;
 
-	msg = body;
 	pp_user_info(NULL, CMIT_FMT_EMAIL, &sb, committer, DATE_RFC2822,
 		     encoding);
+
+	msg = cover_subject;
 	pp_title_line(CMIT_FMT_EMAIL, &msg, &sb, subject_start, extra_headers,
 		      encoding, need_8bit_cte);
+	msg = cover_blurb;
 	pp_remainder(CMIT_FMT_EMAIL, &msg, &sb, 0);
+
 	printf("%s\n", sb.buf);
 
 	strbuf_release(&sb);
@@ -913,6 +917,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			    "print patches to standard out"),
 		OPT_BOOLEAN(0, "cover-letter", &cover_letter,
 			    "generate a cover letter"),
+		OPT_STRING(0, "cover-subject", &cover_subject, "subject",
+				   "use <subject> in the subject line of the cover letter"),
+		OPT_STRING(0, "cover-blurb", &cover_blurb, "blurb",
+				   "use <blurb> in the body of the cover letter"),
 		OPT_BOOLEAN(0, "numbered-files", &numbered_files,
 			    "use simple number sequence for output file names"),
 		OPT_STRING(0, "suffix", &fmt_patch_suffix, "sfx",
@@ -1048,6 +1056,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF)
 		die("--check does not make sense");
 
+	if (strchr(cover_subject, '\n'))
+		die("--cover-subject can not contain newlines");
+
 	if (!use_patch_format &&
 		(!rev.diffopt.output_format ||
 		 rev.diffopt.output_format == DIFF_FORMAT_PATCH))
-- 
1.7.0.rc1.33.g07cf0f.dirty

Re: [PATCH] add new options to git format-patch: --cover-subject and --cover-blurb

[Wesley J. Landaker]

On Friday 05 February 2010 14:39:33 Larry D'Anna wrote:
> This is useful because if you're preparing a patch series with a cover
>  letter you can easily put together one line to format and email the
>  whole thing to yourself.  You check to make sure everything is right,
>  and then just change the recipient address and run it again.
> 
> git send-email --to my@mydomain.org  master..HEAD --cover-letter \
>     --cover-subject "this is my patch series" --cover-blurb "$(cat
>  blurb.txt)"

One (minor?) issue is that the cover blub would be limited to the maximum 
allowed length of the command-line arguments set by the shell or OS. Since 
you are just catting a file, maybe "--cover-blub-file" would be better?

Just a thought.

Re: [PATCH] add new options to git format-patch: --cover-subject and --cover-blurb

[Junio C Hamano]

"Wesley J. Landaker" <wjl@icecavern.net> writes:

> On Friday 05 February 2010 14:39:33 Larry D'Anna wrote:
>> This is useful because if you're preparing a patch series with a cover
>>  letter you can easily put together one line to format and email the
>>  whole thing to yourself.  You check to make sure everything is right,
>>  and then just change the recipient address and run it again.
>> 
>> git send-email --to my@mydomain.org  master..HEAD --cover-letter \
>>     --cover-subject "this is my patch series" --cover-blurb "$(cat
>>  blurb.txt)"
>
> One (minor?) issue is that the cover blub would be limited to the maximum 
> allowed length of the command-line arguments set by the shell or OS. Since 
> you are just catting a file, maybe "--cover-blub-file" would be better?
>
> Just a thought.

The placeholder in particular and the cover letter itself in general are
meant to be edited.  I do not see much point in forcing people to edit yet
another file and have them specify with an cover-blurb option.

Not very interested.

Re: [PATCH] add new options to git format-patch: --cover-subject and --cover-blurb

[Wesley J. Landaker]

On Friday 05 February 2010 15:33:23 Junio C Hamano wrote:
> The placeholder in particular and the cover letter itself in general are
> meant to be edited.  I do not see much point in forcing people to edit
>  yet another file and have them specify with an cover-blurb option.
> 
> Not very interested.

The original use-case is also pretty close to just doing the following:

$ git format-patch master..HEAD --cover-letter 
$ vi 0000-cover-letter.patch
$ git send-email --to my@mydomain.org *.patch
$ git send-email --to git@vger.kernel.org *.patch

Isn't that just as easy as the proposed --cover-* options?

Re: [PATCH] add new options to git format-patch: --cover-subject and --cover-blurb

[Larry D'Anna]

* Wesley J. Landaker (wjl@icecavern.net) [100205 17:53]:
> On Friday 05 February 2010 15:33:23 Junio C Hamano wrote:
> > The placeholder in particular and the cover letter itself in general are
> > meant to be edited.  I do not see much point in forcing people to edit
> >  yet another file and have them specify with an cover-blurb option.
> > 
> > Not very interested.
> 
> The original use-case is also pretty close to just doing the following:
> 
> $ git format-patch master..HEAD --cover-letter 
> $ vi 0000-cover-letter.patch
> $ git send-email --to my@mydomain.org *.patch
> $ git send-email --to git@vger.kernel.org *.patch
> 
> Isn't that just as easy as the proposed --cover-* options?

Except when you decide you need to modify it after sending it to yourself.  

       --larry

Re: [PATCH] add new options to git format-patch: --cover-subject and --cover-blurb

[Larry D'Anna]

* Junio C Hamano (gitster@pobox.com) [100205 17:33]:

> The placeholder in particular and the cover letter itself in general are
> meant to be edited.  I do not see much point in forcing people to edit yet
> another file and have them specify with an cover-blurb option.
> 
> Not very interested.

Yes, they're meant to be edited, but if you look at the steps required to submit
a series with cover letter, it's clear it could be a bit streamlined:

1) make your branch

2) git format-patch --cover-letter

3) edit the cover letter

3) review the series, and realize you need to fix something, fix it.

4) git format-patch --cover-letter again

5) edit the cover letter, *again*.  hopefully you didn't overwrite the old one.

6) git send-email --to myself

7) one last look over it in my inbox

8) git send-email --to the list

The whole thing is a lot less annoying and error-prone if you can have
git-send-email call git-format-patch.  

Besides, you're not forcing anyone to edit an extra file.  If you leave out
--cover-subject or --cover-blurb it just behaves in exactly the same way it
always did.


        --larry

Re: [PATCH] add new options to git format-patch: --cover-subject and --cover-blurb

[Junio C Hamano]

Larry D'Anna <larry@elder-gods.org> writes:

> 1) make your branch
>
> 2) git format-patch --cover-letter
>
> 3) edit the cover letter
>
> 3) review the series, and realize you need to fix something, fix it.

Hmph, this begs a natural question: why didn't you review and realize that
in step (1)?

> 4) git format-patch --cover-letter again
>
> 5) edit the cover letter, *again*.  hopefully you didn't overwrite the
> old one.

This step I can understand and am very sympathetic to the cause, even
though I may not be convinced that the patch under discussion is the best
solution to the issue.

What argument are you giving to the "-o" option?  If your series changed
(e.g. inserted or deleted a commit in the middle, retitled, etc.), and
your output is going to the same directory, you would end up with files
with duplicate serial numbers and you would need to purge the old one
before your next invocation of send-email.  For this reason, people
quickly learn to either give a different -o location (so that they can
compare two versions), or to purge the old contents before running
format-patch.  If the latter, it would be sufficient to save the old
0000-cover before removing them, and if the former, the old cover is
already there.  You can cut and paste from there while editing the new
one.

The thing I found suboptimal in your approach is that most often the cover
letter is written to explain what the overall goal of the series is and
how each patch relates to each other to achieve that goal.  In order to
effectively do so, the overview format-patch leaves in 0000-cover template
file helps a lot (actually that is half the reason why it shows the
overview---the other is for the recipients).

Your approach forces the user to write the blurb part in a separate file
on blank sheet of paper _before_ running format-patch, iow, without the
help of that series overview, if they want to take advantage of your "I
don't want to lose what I wrote already" feature.  To put it another way,
people who use --cover-blurb would write suboptimal (or maybe useless)
blurb text exactly because they don't look at the series overview while
they write it---the option encourages a bad cover letter to be sent to
reviewers.

I am hoping we can do better than that.

It might be sufficient for format-patch to notice a 0000-cover file that
is already there, read the subject and blurb part and carry that forward,
instead of unconditionally writing "*** SUBJECT HERE ***" and stuff.  That
way, the user does not have to prepare a separate file before running
format-patch.

By scanning from the bottom of the existing 0000-cover file, skipping
diffstat part (easy to spot with regexp) and then skip backwards a block
of text whose lines are one of:

 (1) two space indented---that's one-line-per-commit;

 (2) empty line---separator; or

 (3) unindented line that ends with '(' number ')' ':'---the author.

The remainder would be the BLURB.  And you know it is much easier to find
where the Subject: is ;-)

Re: [PATCH] add new options to git format-patch: --cover-subject and --cover-blurb

[Jonathan Nieder]

Junio C Hamano wrote:
> Larry D'Anna <larry@elder-gods.org> writes:
 
>> 1) make your branch
>>
>> 2) git format-patch --cover-letter
>>
>> 3) edit the cover letter
>>
>> 3) review the series, and realize you need to fix something, fix it.
>
> Hmph, this begs a natural question: why didn't you review and realize that
> in step (1)?

One answer: writing a cover letter forces one to reflect a little.
Perhaps that is why the cover letter and review share step 3. ;-)

> It might be sufficient for format-patch to notice a 0000-cover file that
> is already there, read the subject and blurb part and carry that forward,
> instead of unconditionally writing "*** SUBJECT HERE ***" and stuff.  That
> way, the user does not have to prepare a separate file before running
> format-patch.

FWIW I think this sounds sane and would be happy to see this feature.

Jonathan

> By scanning from the bottom of the existing 0000-cover file, skipping
> diffstat part (easy to spot with regexp) and then skip backwards a block
> of text whose lines are one of:
> 
>  (1) two space indented---that's one-line-per-commit;
> 
>  (2) empty line---separator; or
> 
>  (3) unindented line that ends with '(' number ')' ':'---the author.
> 
> The remainder would be the BLURB.  And you know it is much easier to find
> where the Subject: is ;-)