threads / rfc / 38285

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

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

## tl;dr

6 messages between Jan 5, 2015 and Jan 6, 2015. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Stefan Beller· Jan 5, 2015, 19:28 UTC · lore

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(-)
Show changes to builtin/log.c +10 −5
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
Junio C Hamano· Jan 6, 2015, 10:37 UTC · re: Stefan Beller · lore

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

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?

Stefan Beller· Jan 6, 2015, 18:08 UTC · re: Junio C Hamano · lore

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

On Tue, Jan 6, 2015 at 2:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
> 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

Junio C Hamano· Jan 6, 2015, 18:57 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 8 quoted lines
> 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.

Show 20 quoted lines
>> 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?

Show 8 quoted lines
> 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?).

Stefan Beller· Jan 6, 2015, 19:14 UTC · re: Junio C Hamano · lore

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

On Tue, Jan 6, 2015 at 10:57 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
> 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.
Show 6 quoted lines
>
>> * 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.

Show 5 quoted lines
>
> 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

Junio C Hamano· Jan 6, 2015, 22:35 UTC · re: Stefan Beller · lore

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

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.

← back to recent threads