[RFC PATCH] format-patch: Add config option format.coverbodytext to change the cover letter body

[RFC PATCH] format-patch: Add config option format.coverbodytext to change the cover letter body

[Stefan Beller]

When sending out patch series one of the last things doing is writing
the cover letter. The cover letter would be a good place to remind
people to check the todo list for sending patches. As people have
different levels of confidence and sloppiness this todo list may be
lengthier for some people compared to others. To make this possible
this adds a way to put your personal todo list for sending patches in
the cover letter, so you'll see it every time you intend to send patches.

This intentionally doesn't let you configure the subject line of the cover letter
as send email will stop you if you want to send out the coverletter with untouched
subject line (*** SUBJECT HERE***).

Not-signed-off-by: Stefan Beller <sbeller@google.com> as it's RFC.

---
 builtin/log.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 734aab3..84da54d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -34,6 +34,7 @@ static int default_show_root = 1;
 static int decoration_style;
 static int decoration_given;
 static int use_mailmap_config;
+static const char *fmt_patch_body_text = "*** BLURB HERE ***";
 static const char *fmt_patch_subject_prefix = "PATCH";
 static const char *fmt_pretty;
 
@@ -374,6 +375,8 @@ static int git_log_config(const char *var, const char *value, void *cb)
 		return git_config_string(&fmt_pretty, var, value);
 	if (!strcmp(var, "format.subjectprefix"))
 		return git_config_string(&fmt_patch_subject_prefix, var, value);
+	if (!strcmp(var, "format.coverbodytext"))
+		return git_config_string(&fmt_patch_body_text, var, value);
 	if (!strcmp(var, "log.abbrevcommit")) {
 		default_abbrev_commit = git_config_bool(var, value);
 		return 0;
@@ -904,8 +907,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 			      int quiet)
 {
 	const char *committer;
-	const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n";
-	const char *msg;
+	const char *subject = "*** SUBJECT HERE ***\n\n";
+	struct strbuf msg = STRBUF_INIT;
 	struct shortlog log;
 	struct strbuf sb = STRBUF_INIT;
 	int i;
@@ -937,16 +940,18 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	if (!branch_name)
 		branch_name = find_branch_name(rev);
 
-	msg = body;
+	strbuf_addstr(&msg, subject);
+	strbuf_addstr(&msg, fmt_patch_body_text);
 	pp.fmt = CMIT_FMT_EMAIL;
 	pp.date_mode = DATE_RFC2822;
 	pp_user_info(&pp, NULL, &sb, committer, encoding);
-	pp_title_line(&pp, &msg, &sb, encoding, need_8bit_cte);
-	pp_remainder(&pp, &msg, &sb, 0);
+	pp_title_line(&pp, &msg.buf, &sb, encoding, need_8bit_cte);
+	pp_remainder(&pp, &msg.buf, &sb, 0);
 	add_branch_description(&sb, branch_name);
 	printf("%s\n", sb.buf);
 
 	strbuf_release(&sb);
+	strbuf_release(&msg);
 
 	shortlog_init(&log);
 	log.wrap_lines = 1;
-- 
2.2.1.62.g3f15098

Re: [RFC PATCH] format-patch: Add config option format.coverbodytext to change the cover letter body

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> When sending out patch series one of the last things doing is writing
> the cover letter. The cover letter would be a good place to remind
> people to check the todo list for sending patches.

I do not quite understand.  Wouldn't a check-list be useful _before_
you start series of things (I am assuming that you meant a list like
1. run spell check; 2. run checkpatch; 3. run full test suite;
4. format the docs for HTML and manpage)?  Time to write cover
letter (or running format-patch in general) is way too late for
many of these things.

There may be a check-list that is still useful after commits to be
sent are perfect and ready to be formatted.  "Describe change since
the last round after three-dash line." would be one of them
("Sign-off the patch" is not---without one, the commits would not
have been perfect yet).  But for such a check-list, wouldn't we want
remainder not only on the cover but on each individual patch?

Perhaps --add-header="x-reminder: what changed since the last?"
would be sufficient for your purpose instead?

Re: [RFC PATCH] format-patch: Add config option format.coverbodytext to change the cover letter body

[Stefan Beller]

On Tue, Jan 6, 2015 at 2:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> When sending out patch series one of the last things doing is writing
>> the cover letter. The cover letter would be a good place to remind
>> people to check the todo list for sending patches.
>
> I do not quite understand.  Wouldn't a check-list be useful _before_
> you start series of things (I am assuming that you meant a list like
> 1. run spell check; 2. run checkpatch; 3. run full test suite;
> 4. format the docs for HTML and manpage)?  Time to write cover
> letter (or running format-patch in general) is way too late for
> many of these things.

Actually I do mean these things.
As I have all changes to be sent in branches, each commit having all the
notes, sending them to the mailing list is becoming less of a hassle.
All I need to do is

    git format-patch --cover-letter --notes -subject-prefix=PATCHvX
origin/master.. &&
    $editor 0000-cover-letter.patch &&
    git send-email 00* --to=git@vger.kernel.org --cc=...

There is *no* postprocessing of other *.patch files apart from the cover letter
involved any more in my workflow. If something needs to change I need to
change it directly in the commit with

    git rebase -i origin/master
     ... and edit/reword the specific patch

By choosing this way of working I am able to send out any branch for review
in a heartbeat. I think the advantage of this approach is to have git
support for
most of the time, i.e. I can use gitk for reviewing my patches
including notes as
well as inspecting the filesystem. Also I try to force myself into
reviewing each
patch twice (with time in between) before sending it out for public review.

The disadvantage is that I need to have the high quality in a branch before
sending it out for review. But I personally find it easier to deal
with git branches
than with patch files of different versions. Branches do not forget
anything once
I edited it in which turns that point into an advantage for me.

>
> There may be a check-list that is still useful after commits to be
> sent are perfect and ready to be formatted.

> "Describe change since
> the last round after three-dash line." would be one of them
> ("Sign-off the patch" is not---without one, the commits would not
> have been perfect yet).

But how do I know if a patch is perfect?
By checking all points of my checklist for each patch. But during the
iterations of reviews, patches come and go, so I need to have
a checklist after I think each patch is perfect on its own.
So what about:
    * Are the patches in the correct order? (Implement the feature
      before advertising it)
    * Do some of the patches still make sense in the context of this
      series? (Sometimes the focus of a series changes quite a bit
      after input from reviewers, so some patches may be dropped)


> But for such a check-list, wouldn't we want
> remainder not only on the cover but on each individual patch?

>
> Perhaps --add-header="x-reminder: what changed since the last?"
> would be sufficient for your purpose instead?

I am not quite sure if that is my problem any more. Say I have the
following check list:
* Do I follow the coding style?
  * indentation by tabs and spaces
  * no superfluous braces

* The code itself
  * Does it embed into the current logic flow?
  * memory leaks?
  * Does it compile and test (git rebase --exec=make --exec="make test") ?

* Is a patch small enough?
  * Does it do one thing? (move code or add code, not both!)

* Do I have a proper commit message?
  * spelling and grammar
  * Does the commit message (still) describe the changes of the patch?

* After doing changes, wait at least 12 hours for second self-review

* sending out:
    git format-patch --cover-letter --notes --subject-prefix=PATCHvX

Most of it is on a per-patch basis, but it is easier to check for the whole
branch/series in one go after some time when you found some mental
distance to the code you wrote yourself. And for me this is usually
the next day, when I review it again and ask myself: Do I send out or not?

As you can see there may be quite some discussion on what you want
to put into that check list, hence it should be configurable. We could of
course think about pre-populating the check list for new comers.

Thanks,
Stefan

Re: [RFC PATCH] format-patch: Add config option format.coverbodytext to change the cover letter body

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The disadvantage is that I need to have the high quality in a branch before
> sending it out for review. But I personally find it easier to deal
> with git branches
> than with patch files of different versions. Branches do not forget
> anything once
> I edited it in which turns that point into an advantage for me.
> ...
> But how do I know if a patch is perfect?

You said it yourself above, didn't you?  While perfecting your
branch and while perfecting reroll of your branch.

>> Perhaps --add-header="x-reminder: what changed since the last?"
>> would be sufficient for your purpose instead?
>
> I am not quite sure if that is my problem any more. Say I have the
> following check list:
> * Do I follow the coding style?
>   * indentation by tabs and spaces
>   * no superfluous braces
>
> * The code itself
>   * Does it embed into the current logic flow?
>   * memory leaks?
>   * Does it compile and test (git rebase --exec=make --exec="make test") ?
>
> * Is a patch small enough?
>   * Does it do one thing? (move code or add code, not both!)
>
> * Do I have a proper commit message?
>   * spelling and grammar
>   * Does the commit message (still) describe the changes of the patch?

All of the above sounds like what you want to do once-per-patch, not
once-per-series, so a reminder while reviewing each individual
patch, not while writing the cover letter, would be a more
appropriate place for them, no?

> * After doing changes, wait at least 12 hours for second self-review

This is certainly once-per-series.

> * sending out:
>     git format-patch --cover-letter --notes --subject-prefix=PATCHvX

This is not even helpful reminder if that is only shown after you
run format-patch, no?

> Most of it is on a per-patch basis, but it is easier to check for the whole
> branch/series in one go after some time when you found some mental
> distance to the code you wrote yourself. And for me this is usually
> the next day, when I review it again and ask myself: Do I send out or not?
>
> As you can see there may be quite some discussion on what you want
> to put into that check list, hence it should be configurable. We could of
> course think about pre-populating the check list for new comers.

Yes, but that is a separate discussion where the check list is given
(per patch or per series?) and how it is presented (overriding the
"blurb here" comment or something else that can also be used for the
non-cover messages?).

Re: [RFC PATCH] format-patch: Add config option format.coverbodytext to change the cover letter body

[Stefan Beller]

On Tue, Jan 6, 2015 at 10:57 AM, Junio C Hamano <gitster@pobox.com> wrote:
> You said it yourself above, didn't you?  While perfecting your
> branch and while perfecting reroll of your branch.
>
>
>> * After doing changes, wait at least 12 hours for second self-review
>
> This is certainly once-per-series.

Right.

>
>> * sending out:
>>     git format-patch --cover-letter --notes --subject-prefix=PATCHvX
>
> This is not even helpful reminder if that is only shown after you
> run format-patch, no?

Right, as I currently have these notes somewhere completely
out of reach of my git tree, I manually check that list whenever I need to.
So this is for different events.

>
> Yes, but that is a separate discussion where the check list is given
> (per patch or per series?) and how it is presented (overriding the
> "blurb here" comment or something else that can also be used for the
> non-cover messages?).

One of the problems here may be that a single patch is never formatted
as a patch, but lives inside the git world. Only when I consider the
whole series
good I start formatting the patches, which is why it's hard to find a place on
a per-patch basis.

>>> Perhaps --add-header="x-reminder: what changed since the last?"
>>> would be sufficient for your purpose instead?

Maybe a similar approach of pre writing notes to some per patch
checklist would do?

So once I'd set
    git config commit.add_notes_if_empty "/in/filesystem/checklist/per/patch"

which would add notes whenever I'd commit and the notes for that commit
are not empty (i.e. commit --amend doesn't wipe existing notes).
Maybe that config option should rather be below
notes.add_on_commit though.

So before we drift into more discussion, I'd still think it makes sense to have
the ***BLURB *** replaced by some configurable questions regarding the
series as a whole, so I'd try to perfect that patch?

Thanks,
Stefan

Re: [RFC PATCH] format-patch: Add config option format.coverbodytext to change the cover letter body

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> So before we drift into more discussion, I'd still think it makes sense to have
> the ***BLURB *** replaced by some configurable questions regarding the
> series as a whole, so I'd try to perfect that patch?

So far, it is only you and me talking about that feature in response
to your request for comment, and I am personally not very interested
in it, at least not yet, so it's only you at this point.  That may
be because of new year break, or that may be because people do not
think it is worthwhile to only replace "**BLURB***", or we may have
to give others some more time to chime in.

Don't get that discourage and stop you, though.  "This was written
for my own use, friends with different workflows picked it up, have
been using it and it made their lives better, and I have been using
it myself for a few weeks while polishing it to help my own and
their workflows." might be the best way for a new feature to come
into the world, and you can do that "polish to help my own" part
without having "them".

FWIW, the format-patch itself appeared at the end of May 2005 and
after having been ignored by Linus for a little more than a month it
landed in early July 2005 ($gmane/4324, $gmane/4843, $gmane/5446 and
$gmane/5677) after exactly such a process.