{"thread":{"id":"38285","subject":"[RFC PATCH] format-patch: Add config option format.coverbodytext to change the cover letter body","startedAt":"2015-01-05T19:28:25Z","lastAt":"2015-01-06T22:35:36Z","messageCount":6,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"254298","messageId":"1420486105-30242-1-git-send-email-sbeller@google.com","threadId":"38285","inReplyTo":null,"subject":"[RFC PATCH] format-patch: Add config option format.coverbodytext to change the cover letter body","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-05T19:28:25Z","receivedAt":"2015-01-05T19:28:25Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When sending out patch series one of the last things doing is writing\nthe cover letter. The cover letter would be a good place to remind\npeople to check the todo list for sending patches. As people have\ndifferent levels of confidence and sloppiness this todo list may be\nlengthier for some people compared to others. To make this possible\nthis adds a way to put your personal todo list for sending patches in\nthe cover letter, so you'll see it every time you intend to send patches.\n\nThis intentionally doesn't let you configure the subject line of the cover letter\nas send email will stop you if you want to send out the coverletter with untouched\nsubject line (*** SUBJECT HERE***).\n\nNot-signed-off-by: Stefan Beller <sbeller@google.com> as it's RFC.\n\n---\n builtin/log.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 734aab3..84da54d 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -34,6 +34,7 @@ static int default_show_root = 1;\n static int decoration_style;\n static int decoration_given;\n static int use_mailmap_config;\n+static const char *fmt_patch_body_text = \"*** BLURB HERE ***\";\n static const char *fmt_patch_subject_prefix = \"PATCH\";\n static const char *fmt_pretty;\n \n@@ -374,6 +375,8 @@ static int git_log_config(const char *var, const char *value, void *cb)\n \t\treturn git_config_string(&fmt_pretty, var, value);\n \tif (!strcmp(var, \"format.subjectprefix\"))\n \t\treturn git_config_string(&fmt_patch_subject_prefix, var, value);\n+\tif (!strcmp(var, \"format.coverbodytext\"))\n+\t\treturn git_config_string(&fmt_patch_body_text, var, value);\n \tif (!strcmp(var, \"log.abbrevcommit\")) {\n \t\tdefault_abbrev_commit = git_config_bool(var, value);\n \t\treturn 0;\n@@ -904,8 +907,8 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \t\t\t      int quiet)\n {\n \tconst char *committer;\n-\tconst char *body = \"*** SUBJECT HERE ***\\n\\n*** BLURB HERE ***\\n\";\n-\tconst char *msg;\n+\tconst char *subject = \"*** SUBJECT HERE ***\\n\\n\";\n+\tstruct strbuf msg = STRBUF_INIT;\n \tstruct shortlog log;\n \tstruct strbuf sb = STRBUF_INIT;\n \tint i;\n@@ -937,16 +940,18 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \tif (!branch_name)\n \t\tbranch_name = find_branch_name(rev);\n \n-\tmsg = body;\n+\tstrbuf_addstr(&msg, subject);\n+\tstrbuf_addstr(&msg, fmt_patch_body_text);\n \tpp.fmt = CMIT_FMT_EMAIL;\n \tpp.date_mode = DATE_RFC2822;\n \tpp_user_info(&pp, NULL, &sb, committer, encoding);\n-\tpp_title_line(&pp, &msg, &sb, encoding, need_8bit_cte);\n-\tpp_remainder(&pp, &msg, &sb, 0);\n+\tpp_title_line(&pp, &msg.buf, &sb, encoding, need_8bit_cte);\n+\tpp_remainder(&pp, &msg.buf, &sb, 0);\n \tadd_branch_description(&sb, branch_name);\n \tprintf(\"%s\\n\", sb.buf);\n \n \tstrbuf_release(&sb);\n+\tstrbuf_release(&msg);\n \n \tshortlog_init(&log);\n \tlog.wrap_lines = 1;\n-- \n2.2.1.62.g3f15098\n"},{"id":"254325","messageId":"xmqqzj9wp52y.fsf@gitster.dls.corp.google.com","threadId":"38285","inReplyTo":"1420486105-30242-1-git-send-email-sbeller@google.com","subject":"Re: [RFC PATCH] format-patch: Add config option format.coverbodytext to change the cover letter body","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-06T10:37:57Z","receivedAt":"2015-01-06T10:37:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> When sending out patch series one of the last things doing is writing\n> the cover letter. The cover letter would be a good place to remind\n> people to check the todo list for sending patches.\n\nI do not quite understand.  Wouldn't a check-list be useful _before_\nyou start series of things (I am assuming that you meant a list like\n1. run spell check; 2. run checkpatch; 3. run full test suite;\n4. format the docs for HTML and manpage)?  Time to write cover\nletter (or running format-patch in general) is way too late for\nmany of these things.\n\nThere may be a check-list that is still useful after commits to be\nsent are perfect and ready to be formatted.  \"Describe change since\nthe last round after three-dash line.\" would be one of them\n(\"Sign-off the patch\" is not---without one, the commits would not\nhave been perfect yet).  But for such a check-list, wouldn't we want\nremainder not only on the cover but on each individual patch?\n\nPerhaps --add-header=\"x-reminder: what changed since the last?\"\nwould be sufficient for your purpose instead?\n"},{"id":"254351","messageId":"CAGZ79kabhx1q2pxY0_UXAy64G94GQZaP4nSUgeUjV0c_sjt+QQ@mail.gmail.com","threadId":"38285","inReplyTo":"xmqqzj9wp52y.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC PATCH] format-patch: Add config option format.coverbodytext to change the cover letter body","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-06T18:08:46Z","receivedAt":"2015-01-06T18:08:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jan 6, 2015 at 2:37 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> When sending out patch series one of the last things doing is writing\n>> the cover letter. The cover letter would be a good place to remind\n>> people to check the todo list for sending patches.\n>\n> I do not quite understand.  Wouldn't a check-list be useful _before_\n> you start series of things (I am assuming that you meant a list like\n> 1. run spell check; 2. run checkpatch; 3. run full test suite;\n> 4. format the docs for HTML and manpage)?  Time to write cover\n> letter (or running format-patch in general) is way too late for\n> many of these things.\n\nActually I do mean these things.\nAs I have all changes to be sent in branches, each commit having all the\nnotes, sending them to the mailing list is becoming less of a hassle.\nAll I need to do is\n\n    git format-patch --cover-letter --notes -subject-prefix=PATCHvX\norigin/master.. &&\n    $editor 0000-cover-letter.patch &&\n    git send-email 00* --to=git@vger.kernel.org --cc=...\n\nThere is *no* postprocessing of other *.patch files apart from the cover letter\ninvolved any more in my workflow. If something needs to change I need to\nchange it directly in the commit with\n\n    git rebase -i origin/master\n     ... and edit/reword the specific patch\n\nBy choosing this way of working I am able to send out any branch for review\nin a heartbeat. I think the advantage of this approach is to have git\nsupport for\nmost of the time, i.e. I can use gitk for reviewing my patches\nincluding notes as\nwell as inspecting the filesystem. Also I try to force myself into\nreviewing each\npatch twice (with time in between) before sending it out for public review.\n\nThe disadvantage is that I need to have the high quality in a branch before\nsending it out for review. But I personally find it easier to deal\nwith git branches\nthan with patch files of different versions. Branches do not forget\nanything once\nI edited it in which turns that point into an advantage for me.\n\n>\n> There may be a check-list that is still useful after commits to be\n> sent are perfect and ready to be formatted.\n\n> \"Describe change since\n> the last round after three-dash line.\" would be one of them\n> (\"Sign-off the patch\" is not---without one, the commits would not\n> have been perfect yet).\n\nBut how do I know if a patch is perfect?\nBy checking all points of my checklist for each patch. But during the\niterations of reviews, patches come and go, so I need to have\na checklist after I think each patch is perfect on its own.\nSo what about:\n    * Are the patches in the correct order? (Implement the feature\n      before advertising it)\n    * Do some of the patches still make sense in the context of this\n      series? (Sometimes the focus of a series changes quite a bit\n      after input from reviewers, so some patches may be dropped)\n\n\n> But for such a check-list, wouldn't we want\n> remainder not only on the cover but on each individual patch?\n\n>\n> Perhaps --add-header=\"x-reminder: what changed since the last?\"\n> would be sufficient for your purpose instead?\n\nI am not quite sure if that is my problem any more. Say I have the\nfollowing check list:\n* Do I follow the coding style?\n  * indentation by tabs and spaces\n  * no superfluous braces\n\n* The code itself\n  * Does it embed into the current logic flow?\n  * memory leaks?\n  * Does it compile and test (git rebase --exec=make --exec=\"make test\") ?\n\n* Is a patch small enough?\n  * Does it do one thing? (move code or add code, not both!)\n\n* Do I have a proper commit message?\n  * spelling and grammar\n  * Does the commit message (still) describe the changes of the patch?\n\n* After doing changes, wait at least 12 hours for second self-review\n\n* sending out:\n    git format-patch --cover-letter --notes --subject-prefix=PATCHvX\n\nMost of it is on a per-patch basis, but it is easier to check for the whole\nbranch/series in one go after some time when you found some mental\ndistance to the code you wrote yourself. And for me this is usually\nthe next day, when I review it again and ask myself: Do I send out or not?\n\nAs you can see there may be quite some discussion on what you want\nto put into that check list, hence it should be configurable. We could of\ncourse think about pre-populating the check list for new comers.\n\nThanks,\nStefan\n"},{"id":"254356","messageId":"xmqqd26rpwjg.fsf@gitster.dls.corp.google.com","threadId":"38285","inReplyTo":"CAGZ79kabhx1q2pxY0_UXAy64G94GQZaP4nSUgeUjV0c_sjt+QQ@mail.gmail.com","subject":"Re: [RFC PATCH] format-patch: Add config option format.coverbodytext to change the cover letter body","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-06T18:57:07Z","receivedAt":"2015-01-06T18:57:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> The disadvantage is that I need to have the high quality in a branch before\n> sending it out for review. But I personally find it easier to deal\n> with git branches\n> than with patch files of different versions. Branches do not forget\n> anything once\n> I edited it in which turns that point into an advantage for me.\n> ...\n> But how do I know if a patch is perfect?\n\nYou said it yourself above, didn't you?  While perfecting your\nbranch and while perfecting reroll of your branch.\n\n>> Perhaps --add-header=\"x-reminder: what changed since the last?\"\n>> would be sufficient for your purpose instead?\n>\n> I am not quite sure if that is my problem any more. Say I have the\n> following check list:\n> * Do I follow the coding style?\n>   * indentation by tabs and spaces\n>   * no superfluous braces\n>\n> * The code itself\n>   * Does it embed into the current logic flow?\n>   * memory leaks?\n>   * Does it compile and test (git rebase --exec=make --exec=\"make test\") ?\n>\n> * Is a patch small enough?\n>   * Does it do one thing? (move code or add code, not both!)\n>\n> * Do I have a proper commit message?\n>   * spelling and grammar\n>   * Does the commit message (still) describe the changes of the patch?\n\nAll of the above sounds like what you want to do once-per-patch, not\nonce-per-series, so a reminder while reviewing each individual\npatch, not while writing the cover letter, would be a more\nappropriate place for them, no?\n\n> * After doing changes, wait at least 12 hours for second self-review\n\nThis is certainly once-per-series.\n\n> * sending out:\n>     git format-patch --cover-letter --notes --subject-prefix=PATCHvX\n\nThis is not even helpful reminder if that is only shown after you\nrun format-patch, no?\n\n> Most of it is on a per-patch basis, but it is easier to check for the whole\n> branch/series in one go after some time when you found some mental\n> distance to the code you wrote yourself. And for me this is usually\n> the next day, when I review it again and ask myself: Do I send out or not?\n>\n> As you can see there may be quite some discussion on what you want\n> to put into that check list, hence it should be configurable. We could of\n> course think about pre-populating the check list for new comers.\n\nYes, but that is a separate discussion where the check list is given\n(per patch or per series?) and how it is presented (overriding the\n\"blurb here\" comment or something else that can also be used for the\nnon-cover messages?).\n"},{"id":"254359","messageId":"CAGZ79kZE+zBgYANE8+NppnBsAVfLXPdgxnvFRbLdMkafSqduWA@mail.gmail.com","threadId":"38285","inReplyTo":"xmqqd26rpwjg.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC PATCH] format-patch: Add config option format.coverbodytext to change the cover letter body","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-01-06T19:14:59Z","receivedAt":"2015-01-06T19:14:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jan 6, 2015 at 10:57 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> You said it yourself above, didn't you?  While perfecting your\n> branch and while perfecting reroll of your branch.\n>\n>\n>> * After doing changes, wait at least 12 hours for second self-review\n>\n> This is certainly once-per-series.\n\nRight.\n\n>\n>> * sending out:\n>>     git format-patch --cover-letter --notes --subject-prefix=PATCHvX\n>\n> This is not even helpful reminder if that is only shown after you\n> run format-patch, no?\n\nRight, as I currently have these notes somewhere completely\nout of reach of my git tree, I manually check that list whenever I need to.\nSo this is for different events.\n\n>\n> Yes, but that is a separate discussion where the check list is given\n> (per patch or per series?) and how it is presented (overriding the\n> \"blurb here\" comment or something else that can also be used for the\n> non-cover messages?).\n\nOne of the problems here may be that a single patch is never formatted\nas a patch, but lives inside the git world. Only when I consider the\nwhole series\ngood I start formatting the patches, which is why it's hard to find a place on\na per-patch basis.\n\n>>> Perhaps --add-header=\"x-reminder: what changed since the last?\"\n>>> would be sufficient for your purpose instead?\n\nMaybe a similar approach of pre writing notes to some per patch\nchecklist would do?\n\nSo once I'd set\n    git config commit.add_notes_if_empty \"/in/filesystem/checklist/per/patch\"\n\nwhich would add notes whenever I'd commit and the notes for that commit\nare not empty (i.e. commit --amend doesn't wipe existing notes).\nMaybe that config option should rather be below\nnotes.add_on_commit though.\n\nSo before we drift into more discussion, I'd still think it makes sense to have\nthe ***BLURB *** replaced by some configurable questions regarding the\nseries as a whole, so I'd try to perfect that patch?\n\nThanks,\nStefan\n"},{"id":"254373","messageId":"xmqqa91vo7uv.fsf@gitster.dls.corp.google.com","threadId":"38285","inReplyTo":"CAGZ79kZE+zBgYANE8+NppnBsAVfLXPdgxnvFRbLdMkafSqduWA@mail.gmail.com","subject":"Re: [RFC PATCH] format-patch: Add config option format.coverbodytext to change the cover letter body","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-06T22:35:36Z","receivedAt":"2015-01-06T22:35:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> So before we drift into more discussion, I'd still think it makes sense to have\n> the ***BLURB *** replaced by some configurable questions regarding the\n> series as a whole, so I'd try to perfect that patch?\n\nSo far, it is only you and me talking about that feature in response\nto your request for comment, and I am personally not very interested\nin it, at least not yet, so it's only you at this point.  That may\nbe because of new year break, or that may be because people do not\nthink it is worthwhile to only replace \"**BLURB***\", or we may have\nto give others some more time to chime in.\n\nDon't get that discourage and stop you, though.  \"This was written\nfor my own use, friends with different workflows picked it up, have\nbeen using it and it made their lives better, and I have been using\nit myself for a few weeks while polishing it to help my own and\ntheir workflows.\" might be the best way for a new feature to come\ninto the world, and you can do that \"polish to help my own\" part\nwithout having \"them\".\n\nFWIW, the format-patch itself appeared at the end of May 2005 and\nafter having been ignored by Linus for a little more than a month it\nlanded in early July 2005 ($gmane/4324, $gmane/4843, $gmane/5446 and\n$gmane/5677) after exactly such a process.\n"}]}