{"thread":{"id":"46677","subject":"[PATCH] format-patch: use raw format for notes","startedAt":"2017-08-28T04:23:32Z","lastAt":"2017-09-13T13:03:40Z","messageCount":6,"participants":["Sam Bobroff","Junio C Hamano","Stefan Beller","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"327270","messageId":"334a7be4f61c02db24008181eb1d6c80c95772f7.1503894009.git.sam.bobroff@au1.ibm.com","threadId":"46677","inReplyTo":null,"subject":"[PATCH] format-patch: use raw format for notes","fromName":"Sam Bobroff","fromEmail":"sam.bobroff@au1.ibm.com","sentAt":"2017-08-28T04:23:20Z","receivedAt":"2017-08-28T04:23:32Z","isPatch":true,"sender":{"key":"sam.bobroff@au1.ibm.com","avatar":null},"body":"If \"--notes=...\" is used with \"git format-patch\", the notes are\nprefixed with the ref's local name and indented, which looks odd and\nexposes the path of the ref.\n\nExtend the test that suppresses this behaviour so that it also catches\nthis case, causing the notes to be included without additional\nprocessing.\n\nSigned-off-by: Sam Bobroff \n---\n\nNotes (foo):\n Hi,\n \n I've noticed what appears to be a small cosmetic bug in git format-patch, as\n I've described in the commit message.\n \n I'm not sure if this patch is the right way to fix it (or perhaps it's not even\n a bug), but it should at least help to explain what I'm talking about.\n \n I've used \"git format-patch --notes=foo\" to prepare this email so that it is an\n example of the issue :-)\n \n Cheers,\n Sam.\n\n log-tree.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 410ab4f02..26bc21ad3 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -655,7 +655,8 @@ void show_log(struct rev_info *opt)\n \t\tint raw;\n \t\tstruct strbuf notebuf = STRBUF_INIT;\n \n-\t\traw = (opt->commit_format == CMIT_FMT_USERFORMAT);\n+\t\traw = (opt->commit_format == CMIT_FMT_USERFORMAT) ||\n+\t\t (opt->commit_format == CMIT_FMT_EMAIL);\n \t\tformat_display_notes(&commit->object.oid, ¬ebuf,\n \t\t\t\t get_log_output_encoding(), raw);\n \t\tctx.notes_message = notebuf.len\n-- \n2.11.0\n\n"},{"id":"327628","messageId":"xmqqingw8ppj.fsf@gitster.mtv.corp.google.com","threadId":"46677","inReplyTo":"334a7be4f61c02db24008181eb1d6c80c95772f7.1503894009.git.sam.bobroff@au1.ibm.com","subject":"Re: [PATCH] format-patch: use raw format for notes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-06T03:34:48Z","receivedAt":"2017-09-06T03:34:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sam Bobroff writes:\n\n> If \"--notes=...\" is used with \"git format-patch\", the notes are\n> prefixed with the ref's local name and indented, which looks odd and\n> exposes the path of the ref.\n>\n> Extend the test that suppresses this behaviour so that it also catches\n> this case, causing the notes to be included without additional\n> processing.\n>\n> Signed-off-by: Sam Bobroff \n> ---\n>\n> Notes (foo):\n> Hi,\n> \n> I've noticed what appears to be a small cosmetic bug in git format-patch, as\n> I've described in the commit message.\n> \n> I'm not sure if this patch is the right way to fix it (or perhaps it's not even\n> a bug), but it should at least help to explain what I'm talking about.\n> \n> I've used \"git format-patch --notes=foo\" to prepare this email so that it is an\n> example of the issue :-)\n> \n> Cheers,\n> Sam.\n\nIs the above addition from your 'foo' notes with or without this\npatch? I think the answer is \"without\", and the above \"example\"\nlooks just fine to me.\n\n - It is very much intended to allow The \"(foo)\" after the \"Notes\"\n label to show which notes ref the note comes from, because there\n can be more than one notes refs that annotate the same commit.\n\n - And the contents are indented, just like the diffstat and other\n stuff we place after \"---\" but before the first \"diff\", to ensure\n no matter what text appears there it will not be mistaken as part\n sure that the contents from the notes will not be mistaken as part\n of the patch.\n\nI do not think an unconditional change of the established format,\nlike your patch does, is acceptable, as existing users have relied\non, and expect to be able to continue relying on, the above two\naspect of the current format.\n\nBut I am somewhat curious what your use case that wants to insert\nthe raw contents there is. We may be able to construct a valid\nargument to add such an output as an optional feature if there is a\ngood use case for it.\n\nThanks.\n\n> log-tree.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/log-tree.c b/log-tree.c\n> index 410ab4f02..26bc21ad3 100644\n> --- a/log-tree.c\n> +++ b/log-tree.c\n> @@ -655,7 +655,8 @@ void show_log(struct rev_info *opt)\n> \t\tint raw;\n> \t\tstruct strbuf notebuf = STRBUF_INIT;\n> \n> -\t\traw = (opt->commit_format == CMIT_FMT_USERFORMAT);\n> +\t\traw = (opt->commit_format == CMIT_FMT_USERFORMAT) ||\n> +\t\t (opt->commit_format == CMIT_FMT_EMAIL);\n> \t\tformat_display_notes(&commit->object.oid, ¬ebuf,\n> \t\t\t\t get_log_output_encoding(), raw);\n> \t\tctx.notes_message = notebuf.len\n"},{"id":"327843","messageId":"20170911042737.4h5b2jygdeu7cpmf@tungsten.ozlabs.ibm.com","threadId":"46677","inReplyTo":"xmqqingw8ppj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] format-patch: use raw format for notes","fromName":"Sam Bobroff","fromEmail":"sam.bobroff@au1.ibm.com","sentAt":"2017-09-11T04:27:38Z","receivedAt":"2017-09-11T04:27:51Z","isPatch":true,"sender":{"key":"sam.bobroff@au1.ibm.com","avatar":null},"body":"On Wed, Sep 06, 2017 at 12:34:48PM +0900, Junio C Hamano wrote:\n> Sam Bobroff writes:\n> \n> > If \"--notes=...\" is used with \"git format-patch\", the notes are\n> > prefixed with the ref's local name and indented, which looks odd and\n> > exposes the path of the ref.\n> >\n> > Extend the test that suppresses this behaviour so that it also catches\n> > this case, causing the notes to be included without additional\n> > processing.\n> >\n> > Signed-off-by: Sam Bobroff \n> > ---\n> >\n> > Notes (foo):\n> > Hi,\n> > \n> > I've noticed what appears to be a small cosmetic bug in git format-patch, as\n> > I've described in the commit message.\n> > \n> > I'm not sure if this patch is the right way to fix it (or perhaps it's not even\n> > a bug), but it should at least help to explain what I'm talking about.\n> > \n> > I've used \"git format-patch --notes=foo\" to prepare this email so that it is an\n> > example of the issue :-)\n> > \n> > Cheers,\n> > Sam.\n> \n> Is the above addition from your 'foo' notes with or without this\n> patch? I think the answer is \"without\", and the above \"example\"\n> looks just fine to me.\n\nYes that's correct, it is without the patch.\n\n> - It is very much intended to allow The \"(foo)\" after the \"Notes\"\n> label to show which notes ref the note comes from, because there\n> can be more than one notes refs that annotate the same commit.\n\nRight, that makes perfect sense to me when it's being output locally.\n\nBut the ref names are local to my git repo and there is no reaason why\nthey should be meaningful or even known to the recipients of the patch\nemail.\n\n> - And the contents are indented, just like the diffstat and other\n> stuff we place after \"---\" but before the first \"diff\", to ensure\n> no matter what text appears there it will not be mistaken as part\n> sure that the contents from the notes will not be mistaken as part\n> of the patch.\n\nI don't quite agree here. I think it would be fine to indent the whole\nblock by one space, like the diffstat, but the main text is indented an\nadditional four which looks odd and breaks line wrapping. (Yes, the line\nwrapping can be worked around, but still...)\n\n> I do not think an unconditional change of the established format,\n> like your patch does, is acceptable, as existing users have relied\n> on, and expect to be able to continue relying on, the above two\n> aspect of the current format.\n\nSure and I'm happy look at some kind of optional change but, just out of\ncuriousity, is there some specific use case that this might break? (Not\nthat there needs to be one, I agree with the general \"don't break\nthings\" approach.)\n\n> But I am somewhat curious what your use case that wants to insert\n> the raw contents there is. We may be able to construct a valid\n> argument to add such an output as an optional feature if there is a\n> good use case for it.\n\nPerhaps I'm misunderstanding something about it but I just want to\ninsert text into the comments section automatically, so that I don't\nhave to post-process the output of format-patch with some horrible sed\nscript :-)\n\n(If only there were a way to set the coverletter text automatically as\nwell...)\n\n> Thanks.\n\nThanks for the reply!\nCheers,\nSam.\n\n> \n> > log-tree.c | 3 ++-\n> > 1 file changed, 2 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/log-tree.c b/log-tree.c\n> > index 410ab4f02..26bc21ad3 100644\n> > --- a/log-tree.c\n> > +++ b/log-tree.c\n> > @@ -655,7 +655,8 @@ void show_log(struct rev_info *opt)\n> > \t\tint raw;\n> > \t\tstruct strbuf notebuf = STRBUF_INIT;\n> > \n> > -\t\traw = (opt->commit_format == CMIT_FMT_USERFORMAT);\n> > +\t\traw = (opt->commit_format == CMIT_FMT_USERFORMAT) ||\n> > +\t\t (opt->commit_format == CMIT_FMT_EMAIL);\n> > \t\tformat_display_notes(&commit->object.oid, ¬ebuf,\n> > \t\t\t\t get_log_output_encoding(), raw);\n> > \t\tctx.notes_message = notebuf.len\n\n"},{"id":"327901","messageId":"CAGZ79kbcufMxbBdH=a9YzPWWet5nEdmUN6P+xmMEcGna-VL9=Q@mail.gmail.com","threadId":"46677","inReplyTo":"20170911042737.4h5b2jygdeu7cpmf@tungsten.ozlabs.ibm.com","subject":"Re: [PATCH] format-patch: use raw format for notes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-12T17:33:37Z","receivedAt":"2017-09-12T17:33:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Sep 10, 2017 at 9:27 PM, Sam Bobroff wrote:\n\n> (If only there were a way to set the coverletter text automatically as\n> well...)\n>\n\nCheckout branch..description\n"},{"id":"327910","messageId":"20170913003821.xn43yosuieoivmqk@tungsten.ozlabs.ibm.com","threadId":"46677","inReplyTo":"CAGZ79kbcufMxbBdH=a9YzPWWet5nEdmUN6P+xmMEcGna-VL9=Q@mail.gmail.com","subject":"Re: [PATCH] format-patch: use raw format for notes","fromName":"Sam Bobroff","fromEmail":"sam.bobroff@au1.ibm.com","sentAt":"2017-09-13T00:38:21Z","receivedAt":"2017-09-13T00:38:35Z","isPatch":true,"sender":{"key":"sam.bobroff@au1.ibm.com","avatar":null},"body":"On Tue, Sep 12, 2017 at 10:33:37AM -0700, Stefan Beller wrote:\n> On Sun, Sep 10, 2017 at 9:27 PM, Sam Bobroff wrote:\n> \n> > (If only there were a way to set the coverletter text automatically as\n> > well...)\n> >\n> \n> Checkout branch..description\n\nAH! I had seen this section of the format-patch man page...\n\n --[no-]cover-letter\n In addition to the patches, generate a cover letter file containing the branch description, shortlog and the overall diffstat. You can fill in a description in the file before\n sending it out.\n\n... but I didn't realize that it meant that it would insert the text\nfrom branch..description into the cover letter. Perhaps there\nshould be a hint to point you to the branch..description section\nof \"git config\"?\n\nAlso, it's not very useful for automating patch submission, as it\ndoesn't set the subject line or remove the \"*** BLURB HERE ***\" marker,\nso it must still be post-processed.\n\n(To be honest, I was surprised that it didn't use first line as the\nsubject and leave out the markers.)\n\nCheers,\nSam.\n\n"},{"id":"327931","messageId":"20170913130333.y4sgn5yjdebbvwzn@sigill.intra.peff.net","threadId":"46677","inReplyTo":"20170911042737.4h5b2jygdeu7cpmf@tungsten.ozlabs.ibm.com","subject":"Re: [PATCH] format-patch: use raw format for notes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-13T13:03:34Z","receivedAt":"2017-09-13T13:03:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 11, 2017 at 02:27:38PM +1000, Sam Bobroff wrote:\n\n> > - It is very much intended to allow The \"(foo)\" after the \"Notes\"\n> > label to show which notes ref the note comes from, because there\n> > can be more than one notes refs that annotate the same commit.\n> \n> Right, that makes perfect sense to me when it's being output locally.\n> \n> But the ref names are local to my git repo and there is no reaason why\n> they should be meaningful or even known to the recipients of the patch\n> email.\n\nI can see how your notes names might not be of interest to others. But I\ncan also see how they _could_ be. For instance, if you kept test result\nannotations in a notes ref, you would want to mark them as such.\n\nThe idea of the current output is that you'd put general text into\n\"refs/notes/commits\" (which shows up only as \"Notes:\"). And if you are\nputting notes in another ref, you have some reason to do so, which\nimplies that it's worth showing that they're not in the default ref.\n\nI grant that there are reasons to do so which might not be worth showing\n(e.g., you might be pushing and fetching refs, and keep some hierarchy).\nBut I don't think \"are we emailing them\" is a robust determiner of \"are\nthey worth showing\". So this probably needs to be a separate option,\nrather than tied to the output format.\n\nOr possibly there should be a naming convention (e.g., that everything\nthat ends in \"/commits\" doesn't have its name shown, which would allow\nmultiple hierarchies). It's hard to say without knowing the reason you\nchose a non-default refname in the first place.\n\n-Peff\n"}]}