{"thread":{"id":"47076","subject":"On the nature of cover letters [WAS Re: [PATCH 0/6] Create Git/Packet.pm]","startedAt":"2017-10-30T18:29:43Z","lastAt":"2017-11-02T18:03:44Z","messageCount":8,"participants":["Stefan Beller","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"331402","messageId":"CAGZ79kbHwXpWXhUkokA9=XGN9D1YZevO_8MhbOztMXpkkHMFbg@mail.gmail.com","threadId":"47076","inReplyTo":null,"subject":"On the nature of cover letters [WAS Re: [PATCH 0/6] Create Git/Packet.pm]","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-30T18:29:37Z","receivedAt":"2017-10-30T18:29:43Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Oct 30, 2017 at 11:08 AM, Jeff King <peff@peff.net> wrote:\n> On Mon, Oct 23, 2017 at 01:26:41PM +0100, Philip Oakley wrote:\n>\n>> > Totally offtopic, but is it only me who finds these \"section\n>> > headers\" in cover letters from some people irritating and/or\n>> > jarring?\n>>\n>> Personally I find that, for significant patch series, that clearly breaking\n>> out these distinct sections is of advantage. At this stage (the very first\n>> patch 0/n) there is no specific conversation, so the subject line is a short\n>> 'hello' to the topic, and then the contributor is (it is to be hoped)\n>> setting out their proposal in a clear manner.\n>>\n>> So I do like these headings for larger series, though there is some\n>> judgement to be made as to when the subject line alone is sufficient.\n>\n> I can live with fancily-formatted cover letters. BUT. I would say if\n> your cover letter is getting quite long, you might consider whether some\n> of its content ought to be going elsewhere (either into commit messages\n> themselves, or into a design document or other place inside the repo).\n\nI am of the opinion that in an ideal workflow the cover letter would be\npart of the merge commit message. It may even be editorialized or\nannotated by the maintainer performing the merge, but not necessarily so.\n\nCurrently I rarely pay attention to merges, because they are not exciting\n(in a good way). I mostly know the texts that Junio puts into the merge\nmessage[1] from the cooking email, and otherwise there is not much\ninformation added there.\n\nHowever looking at *what* Junio puts there, is \"why is this worthwhile\nto merge from the *projects* point of view?\", whereas the cover letter\nmost of the time talks about \"Why the *contributor* wants this merged\".\nOften these are subtly different, so it would be nice to have these\ntwo competing views around.\n\n[1] e.g. cf. da15b78e52 Merge branch 'jk/ui-color-always-to-auto'\n\n>> As a separate follow on, one thing that does annoy me is that in subsequent\n>> versions of the various patch series, folk tend to drop all explanation of\n>> why the series is of any relevance, leaving just the 'changed since last\n>> time' part. This means that new readers who try and pick up / review /\n>> contribute to a series later on in its development are not told the purpose.\n>> When the list is active it can, accidentally, do a disservice to the\n>> potential contributors who may feel that only core contributors are able to\n>> contribute.\n>\n> I actually have the opposite opinion. I find it annoying to have to wade\n> through the same unchanged content for each round just to find the\n> little snippet of \"here's what's changed\".\n\nWould you be happier if the \"What changed?\" goes first[2]?\nThough I think whether to just reply to the previous version, put an\nexplicit link or copy the cover letter verbatim from last time is up\nfor discussion. I tent to think a link ought to be enough, because\nthose familiar with the topic would not follow it (so they have no\nadditional burden compared to direct copy), and new comers to\nthat topic may be ok with links .\n\n[2] I tried following that style, e.g.\nhttps://public-inbox.org/git/20170630205310.7380-1-sbeller@google.com/\n\n\n> I don't mind following a link to the previous iteration to read the\n> back-story if I wasn't involved (it's a good idea to do that anyway to\n> see what previous reviews have already discussed).\n\nSuch a back story would make an excellent merge message, too,\nas it explains the big picture more accurately, often shows\nalternatives considered etc.\n\nStefan\n"},{"id":"331549","messageId":"20171101071422.c2k4plhntlgpdnbk@sigill.intra.peff.net","threadId":"47076","inReplyTo":"CAGZ79kbHwXpWXhUkokA9=XGN9D1YZevO_8MhbOztMXpkkHMFbg@mail.gmail.com","subject":"Re: On the nature of cover letters [WAS Re: [PATCH 0/6] Create Git/Packet.pm]","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-01T07:14:22Z","receivedAt":"2017-11-01T07:14:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 30, 2017 at 11:29:37AM -0700, Stefan Beller wrote:\n\n> > I can live with fancily-formatted cover letters. BUT. I would say if\n> > your cover letter is getting quite long, you might consider whether some\n> > of its content ought to be going elsewhere (either into commit messages\n> > themselves, or into a design document or other place inside the repo).\n> \n> I am of the opinion that in an ideal workflow the cover letter would be\n> part of the merge commit message. It may even be editorialized or\n> annotated by the maintainer performing the merge, but not necessarily so.\n> \n> Currently I rarely pay attention to merges, because they are not exciting\n> (in a good way). I mostly know the texts that Junio puts into the merge\n> message[1] from the cooking email, and otherwise there is not much\n> information added there.\n\nYes, I'd agree that for some topics it would be nice for the merge\nmessage to give any \"big picture\" details that wouldn't have made sense\ninside a single commit message.\n\n> However looking at *what* Junio puts there, is \"why is this worthwhile\n> to merge from the *projects* point of view?\", whereas the cover letter\n> most of the time talks about \"Why the *contributor* wants this merged\".\n> Often these are subtly different, so it would be nice to have these\n> two competing views around.\n\nYes, there's really no reason we couldn't have both (e.g., Junio's\nmaintainer synopsis followed by a marker, and then the original author's\ncover letter).\n\nThe workflow within git is a little awkward, though. You'd want to pick\nup the cover letter information via \"git am\" when the branch is applied.\nBut it doesn't go into a commit message until the merge. So where is it\nstored in the meantime? You'd need a branch->msg key/value store of some\nkind.\n\nJunio's workflow now actually uses the \"pu\" merges as the storage\nlocation while a topic is working its way up. But there's a period\nbetween \"am\" and running the integration cycle where it would need to go\nsomewhere else.\n\nOne other consideration is that the cover letter may get updated between\nrounds (e.g., because you changed patches in response to review, or even\nto discuss alternatives that came up and were rejected). That places a\nburden on the maintainer to update the prospective merge-message.\n\nSo it may make more sense just to cross-reference those merges with the\ntopics that spawned them on the mailing list. I.e., instead of copying\nthe cover letter contents, just record the message-id (and update it\nwhenever a new iteration of a topic is picked up via \"git am\"). That\nlets you get the cover letter information _and_ see any discussion\nor review around the patch. (But it has the same \"where does this\nmessage-id go until the merge-commit is created\" question).\n\n> > I actually have the opposite opinion. I find it annoying to have to wade\n> > through the same unchanged content for each round just to find the\n> > little snippet of \"here's what's changed\".\n> \n> Would you be happier if the \"What changed?\" goes first[2]?\n\nYes, I think that would help. And marking the start of \"old\" information\nclearly so that the reader knows when they can stop looking.\n\nBut then links do that pretty well, too (it's easy to choose whether to\nfollow them or not).\n\n-Peff\n"},{"id":"331574","messageId":"CAGZ79kYKK69Xw0-2OxFpo9Q=Kv1hvw8D7YkfhMFFcgzTuevTCQ@mail.gmail.com","threadId":"47076","inReplyTo":"20171101071422.c2k4plhntlgpdnbk@sigill.intra.peff.net","subject":"Re: On the nature of cover letters [WAS Re: [PATCH 0/6] Create Git/Packet.pm]","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-01T16:42:27Z","receivedAt":"2017-11-01T16:42:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Nov 1, 2017 at 12:14 AM, Jeff King <peff@peff.net> wrote:\n> On Mon, Oct 30, 2017 at 11:29:37AM -0700, Stefan Beller wrote:\n>\n>> > I can live with fancily-formatted cover letters. BUT. I would say if\n>> > your cover letter is getting quite long, you might consider whether some\n>> > of its content ought to be going elsewhere (either into commit messages\n>> > themselves, or into a design document or other place inside the repo).\n>>\n>> I am of the opinion that in an ideal workflow the cover letter would be\n>> part of the merge commit message. It may even be editorialized or\n>> annotated by the maintainer performing the merge, but not necessarily so.\n>>\n>> Currently I rarely pay attention to merges, because they are not exciting\n>> (in a good way). I mostly know the texts that Junio puts into the merge\n>> message[1] from the cooking email, and otherwise there is not much\n>> information added there.\n>\n> Yes, I'd agree that for some topics it would be nice for the merge\n> message to give any \"big picture\" details that wouldn't have made sense\n> inside a single commit message.\n>\n>> However looking at *what* Junio puts there, is \"why is this worthwhile\n>> to merge from the *projects* point of view?\", whereas the cover letter\n>> most of the time talks about \"Why the *contributor* wants this merged\".\n>> Often these are subtly different, so it would be nice to have these\n>> two competing views around.\n>\n> Yes, there's really no reason we couldn't have both (e.g., Junio's\n> maintainer synopsis followed by a marker, and then the original author's\n> cover letter).\n>\n> The workflow within git is a little awkward, though. You'd want to pick\n> up the cover letter information via \"git am\" when the branch is applied.\n> But it doesn't go into a commit message until the merge. So where is it\n> stored in the meantime? You'd need a branch->msg key/value store of some\n> kind.\n\nbranch.<name.>description already exists on the contributors side.\nMaybe we can teach git-am to populate that field with either the\nmessage-id only (of the coverletter), or the cover letter text.\n\nOr we introduce a git-am hook, that could populate the value\nof that setting.\n\nOnce we have that setting there, our man page of git-merge claims\nmerging pays attention to that setting via git-fmt-merge-msg.\n\nI guess if we put these pieces in place, it may be easier to convince\nJunio to try out that workflow.\n\n> Junio's workflow now actually uses the \"pu\" merges as the storage\n> location while a topic is working its way up. But there's a period\n> between \"am\" and running the integration cycle where it would need to go\n> somewhere else.\n\nAn empty commit on top is a clunky idea. (Though from the data model,\nthat empty commit \"just learns about a new parent\" in the merge process).\nI think the branch description field will do.\n\n> One other consideration is that the cover letter may get updated between\n> rounds (e.g., because you changed patches in response to review, or even\n> to discuss alternatives that came up and were rejected). That places a\n> burden on the maintainer to update the prospective merge-message.\n\nif there was a git-am hook, that could be smart enough to *always*\nupdate the branch description to the latest cover letter, (or even just\nthe latest patch of the series, if just the last patch changed)\n\n> So it may make more sense just to cross-reference those merges with the\n> topics that spawned them on the mailing list. I.e., instead of copying\n> the cover letter contents, just record the message-id (and update it\n> whenever a new iteration of a topic is picked up via \"git am\"). That\n> lets you get the cover letter information _and_ see any discussion\n> or review around the patch.\n\nThat sounds good.\n\nStefan\n"},{"id":"331625","messageId":"CAGZ79kYghF6=AzL1N96fe2zixHCERXu=RpxHw2-kdUrU+j4CXw@mail.gmail.com","threadId":"47076","inReplyTo":"CAGZ79kYKK69Xw0-2OxFpo9Q=Kv1hvw8D7YkfhMFFcgzTuevTCQ@mail.gmail.com","subject":"Re: On the nature of cover letters [WAS Re: [PATCH 0/6] Create Git/Packet.pm]","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-01T22:31:42Z","receivedAt":"2017-11-01T22:31:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Nov 1, 2017 at 9:42 AM, Stefan Beller <sbeller@google.com> wrote:\n\n>> So it may make more sense just to cross-reference those merges with the\n>> topics that spawned them on the mailing list. I.e., instead of copying\n>> the cover letter contents, just record the message-id (and update it\n>> whenever a new iteration of a topic is picked up via \"git am\"). That\n>> lets you get the cover letter information _and_ see any discussion\n>> or review around the patch.\n>\n> That sounds good.\n>\n\nActually I just found out about `am.messageId`, which adds the individual\nmessage id as a footer. Maybe that is good enough? (Though it would\nclutter every commit, not just the merge commits)\n"},{"id":"331658","messageId":"20171102072237.wbmrzrgfyxdb3m3v@sigill.intra.peff.net","threadId":"47076","inReplyTo":"CAGZ79kYghF6=AzL1N96fe2zixHCERXu=RpxHw2-kdUrU+j4CXw@mail.gmail.com","subject":"Re: On the nature of cover letters [WAS Re: [PATCH 0/6] Create Git/Packet.pm]","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-02T07:22:37Z","receivedAt":"2017-11-02T07:22:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 01, 2017 at 03:31:42PM -0700, Stefan Beller wrote:\n\n> On Wed, Nov 1, 2017 at 9:42 AM, Stefan Beller <sbeller@google.com> wrote:\n> \n> >> So it may make more sense just to cross-reference those merges with the\n> >> topics that spawned them on the mailing list. I.e., instead of copying\n> >> the cover letter contents, just record the message-id (and update it\n> >> whenever a new iteration of a topic is picked up via \"git am\"). That\n> >> lets you get the cover letter information _and_ see any discussion\n> >> or review around the patch.\n> >\n> > That sounds good.\n> \n> Actually I just found out about `am.messageId`, which adds the individual\n> message id as a footer. Maybe that is good enough? (Though it would\n> clutter every commit, not just the merge commits)\n\nIt also means digging around to find the apex of the thread (though\ngenerally that can be done automatically with sufficiently smart\ntooling; I think public-inbox can do it pretty easily).\n\nI also like the idea that I could read \"log --first-parent\" to get an\noverview of the topics (with links). But probably associating a message\nid with each patch smooths out a lot of corner cases (it sidesteps the\n\"where do you store it until the commit is made\" question, and it\nworks when there's no cover letter). And it gives enough hint for other\nsoftware to figure out everything else.\n\nIf the clutter is too much, it could also go into a git-notes ref\n(that's not already implemented, but it seems like it would be pretty\neasy to teach \"git am\" to do that).\n\nFor a while, Thomas Rast used a script to heuristically create that\nmapping via git-notes after the fact. But if \"git am\" just did it\nautomatically on behalf of Junio, that would be more robust.\n\nI will admit that I found I didn't use the mapping generated by Thomas's\nscript all that much. But I do keep a local mailing list index and often\nsearch for the commit's subject as a mail subject, which roughly\naccomplishes the same thing.\n\n-Peff\n"},{"id":"331660","messageId":"xmqqh8udqfxe.fsf@gitster.mtv.corp.google.com","threadId":"47076","inReplyTo":"20171102072237.wbmrzrgfyxdb3m3v@sigill.intra.peff.net","subject":"Re: On the nature of cover letters [WAS Re: [PATCH 0/6] Create Git/Packet.pm]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-02T07:48:45Z","receivedAt":"2017-11-02T07:48:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> If the clutter is too much, it could also go into a git-notes ref\n> (that's not already implemented, but it seems like it would be pretty\n> easy to teach \"git am\" to do that).\n\nFWIW, that is what I use a notes ref \"amlog\" for.\n"},{"id":"331662","messageId":"20171102080111.edh5wqyklkoqkvlg@sigill.intra.peff.net","threadId":"47076","inReplyTo":"xmqqh8udqfxe.fsf@gitster.mtv.corp.google.com","subject":"Re: On the nature of cover letters [WAS Re: [PATCH 0/6] Create Git/Packet.pm]","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-02T08:01:12Z","receivedAt":"2017-11-02T08:01:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 02, 2017 at 04:48:45PM +0900, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > If the clutter is too much, it could also go into a git-notes ref\n> > (that's not already implemented, but it seems like it would be pretty\n> > easy to teach \"git am\" to do that).\n> \n> FWIW, that is what I use a notes ref \"amlog\" for.\n\nRight, I completely forgot that you were already doing that. So jumping\nto the thread for a commit is basically:\n\n  $browser $(git notes --ref=amlog show $commit |\n             perl -pe 's{Message-Id: <(.*)>}{https://public-inbox.org/git/$1/t}')\n\n(Or whichever archive you prefer to use). Thanks for the reminder.\n\n-Peff\n"},{"id":"331680","messageId":"CAGZ79kZRymJxosAFOjGDzmzMk_ZxqDwdNv-V2510L2DwDdkr2Q@mail.gmail.com","threadId":"47076","inReplyTo":"20171102080111.edh5wqyklkoqkvlg@sigill.intra.peff.net","subject":"Re: On the nature of cover letters [WAS Re: [PATCH 0/6] Create Git/Packet.pm]","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-02T18:03:36Z","receivedAt":"2017-11-02T18:03:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Nov 2, 2017 at 1:01 AM, Jeff King <peff@peff.net> wrote:\n> On Thu, Nov 02, 2017 at 04:48:45PM +0900, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>>\n>> > If the clutter is too much, it could also go into a git-notes ref\n>> > (that's not already implemented, but it seems like it would be pretty\n>> > easy to teach \"git am\" to do that).\n>>\n>> FWIW, that is what I use a notes ref \"amlog\" for.\n>\n> Right, I completely forgot that you were already doing that. So jumping\n> to the thread for a commit is basically:\n>\n>   $browser $(git notes --ref=amlog show $commit |\n>              perl -pe 's{Message-Id: <(.*)>}{https://public-inbox.org/git/$1/t}')\n>\n> (Or whichever archive you prefer to use). Thanks for the reminder.\n>\n\nThanks from my side as well. I did not pay attention when this was\nintroduced and see the notes ref for the first time today.\n\nI'll incorporate that into my digging repertoire.\n\nThanks,\nStefan\n"}]}