{"thread":{"id":"58805","subject":"[PATCH] notes: avoid empty line in template","startedAt":"2022-11-16T16:04:25Z","lastAt":"2022-11-17T15:18:48Z","messageCount":6,"participants":["Michael J Gruber","Jeff King","Taylor Blau","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"467374","messageId":"347ee7ad6fea7cf96bb1e51772802102082b58cc.1668614158.git.git@grubix.eu","threadId":"58805","inReplyTo":null,"subject":"[PATCH] notes: avoid empty line in template","fromName":"Michael J Gruber","fromEmail":"git@grubix.eu","sentAt":"2022-11-16T15:56:40Z","receivedAt":"2022-11-16T16:04:25Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"When `git notes` prepares the template it adds an empty newline between\nthe comment header and the content:\n\n>\n> #\n> # Write/edit the notes for the following object:\n>\n> # commit 0f3c55d4c2b7864bffb2d92278eff08d0b2e083f\n> # etc\n\nThis is wrong structurally because that newline is part of the comment,\ntoo, and thus should be commented. Also, it throws off some positioning\nstrategies of editors and plugins, and it differs from how we do commit\ntemplates.\n\nChange this to follow the standard set by `git commit`:\n\n>\n> #\n> # Write/edit the notes for the following object:\n> #\n> # commit 0f3c55d4c2b7864bffb2d92278eff08d0b2e083f\n>\n\nTests pass unchanged after this code change.\n\nSigned-off-by: Michael J Gruber <git@grubix.eu>\n---\n builtin/notes.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex be51f69225..80d9dfd25c 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -181,7 +181,7 @@ static void prepare_note_data(const struct object_id *object, struct note_data *\n \t\tstrbuf_addch(&buf, '\\n');\n \t\tstrbuf_add_commented_lines(&buf, \"\\n\", strlen(\"\\n\"));\n \t\tstrbuf_add_commented_lines(&buf, _(note_template), strlen(_(note_template)));\n-\t\tstrbuf_addch(&buf, '\\n');\n+\t\tstrbuf_add_commented_lines(&buf, \"\\n\", strlen(\"\\n\"));\n \t\twrite_or_die(fd, buf.buf, buf.len);\n \n \t\twrite_commented_object(fd, object);\n-- \n2.38.1.672.gc8cd8f59d3\n\n"},{"id":"467379","messageId":"Y3Uxh25Gh5fPQrMq@coredump.intra.peff.net","threadId":"58805","inReplyTo":"347ee7ad6fea7cf96bb1e51772802102082b58cc.1668614158.git.git@grubix.eu","subject":"Re: [PATCH] notes: avoid empty line in template","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-16T18:52:55Z","receivedAt":"2022-11-16T18:54:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 16, 2022 at 04:56:40PM +0100, Michael J Gruber wrote:\n\n> When `git notes` prepares the template it adds an empty newline between\n> the comment header and the content:\n> \n> >\n> > #\n> > # Write/edit the notes for the following object:\n> >\n> > # commit 0f3c55d4c2b7864bffb2d92278eff08d0b2e083f\n> > # etc\n> \n> This is wrong structurally because that newline is part of the comment,\n> too, and thus should be commented. Also, it throws off some positioning\n> strategies of editors and plugins, and it differs from how we do commit\n> templates.\n> \n> Change this to follow the standard set by `git commit`:\n> \n> >\n> > #\n> > # Write/edit the notes for the following object:\n> > #\n> > # commit 0f3c55d4c2b7864bffb2d92278eff08d0b2e083f\n> >\n\nYeah, this makes perfect sense.\n\n> Tests pass unchanged after this code change.\n\nI'm mildly surprised that there wasn't some test depending on this in a\nstupid way. :) But grepping for \"Write/edit\" shows that nobody did (and\nobviously the tests pass).\n\nWe could add a new test here, but it probably is trivial enough not to\nworry about.\n\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index be51f69225..80d9dfd25c 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -181,7 +181,7 @@ static void prepare_note_data(const struct object_id *object, struct note_data *\n>  \t\tstrbuf_addch(&buf, '\\n');\n>  \t\tstrbuf_add_commented_lines(&buf, \"\\n\", strlen(\"\\n\"));\n>  \t\tstrbuf_add_commented_lines(&buf, _(note_template), strlen(_(note_template)));\n> -\t\tstrbuf_addch(&buf, '\\n');\n> +\t\tstrbuf_add_commented_lines(&buf, \"\\n\", strlen(\"\\n\"));\n\nAnd the patch looks obviously good. The irony is the version two lines\nabove which does it correctly. ;)\n\n-Peff\n"},{"id":"467390","messageId":"Y3VAxO+EQVXVu92z@nand.local","threadId":"58805","inReplyTo":"Y3Uxh25Gh5fPQrMq@coredump.intra.peff.net","subject":"Re: [PATCH] notes: avoid empty line in template","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-16T19:57:56Z","receivedAt":"2022-11-16T19:58:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 16, 2022 at 01:52:55PM -0500, Jeff King wrote:\n> > diff --git a/builtin/notes.c b/builtin/notes.c\n> > index be51f69225..80d9dfd25c 100644\n> > --- a/builtin/notes.c\n> > +++ b/builtin/notes.c\n> > @@ -181,7 +181,7 @@ static void prepare_note_data(const struct object_id *object, struct note_data *\n> >  \t\tstrbuf_addch(&buf, '\\n');\n> >  \t\tstrbuf_add_commented_lines(&buf, \"\\n\", strlen(\"\\n\"));\n> >  \t\tstrbuf_add_commented_lines(&buf, _(note_template), strlen(_(note_template)));\n> > -\t\tstrbuf_addch(&buf, '\\n');\n> > +\t\tstrbuf_add_commented_lines(&buf, \"\\n\", strlen(\"\\n\"));\n>\n> And the patch looks obviously good. The irony is the version two lines\n> above which does it correctly. ;)\n\nThanks, both. ;-).\n\nThanks,\nTaylor\n"},{"id":"467425","messageId":"221117.86wn7tj4af.gmgdl@evledraar.gmail.com","threadId":"58805","inReplyTo":"347ee7ad6fea7cf96bb1e51772802102082b58cc.1668614158.git.git@grubix.eu","subject":"Re: [PATCH] notes: avoid empty line in template","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-17T09:48:48Z","receivedAt":"2022-11-17T09:54:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 16 2022, Michael J Gruber wrote:\n\n> When `git notes` prepares the template it adds an empty newline between\n> the comment header and the content:\n>\n>>\n>> #\n>> # Write/edit the notes for the following object:\n>>\n>> # commit 0f3c55d4c2b7864bffb2d92278eff08d0b2e083f\n>> # etc\n>\n> This is wrong structurally because that newline is part of the comment,\n> too, and thus should be commented. Also, it throws off some positioning\n> strategies of editors and plugins, and it differs from how we do commit\n> templates.\n>\n> Change this to follow the standard set by `git commit`:\n\nI don't mind the consistency here, but what does \"wrong structurally\"\nmean? Doesn't the usual removing of duplicate newlines make this amount\nto the same?\n\n>> #\n>> # Write/edit the notes for the following object:\n>> #\n>> # commit 0f3c55d4c2b7864bffb2d92278eff08d0b2e083f\n>>\n>\n> Tests pass unchanged after this code change.\n\nBecause it did change something and we've got bad test coverage, or just\nbecause it's really a stylistic change?\n\nI don't mind it being a stylistic change, but the proposed commit\ndoesn't really make that clear, and leaves one wondering about potential\nmissing test coverage etc.\n\n> Signed-off-by: Michael J Gruber <git@grubix.eu>\n> ---\n>  builtin/notes.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index be51f69225..80d9dfd25c 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -181,7 +181,7 @@ static void prepare_note_data(const struct object_id *object, struct note_data *\n>  \t\tstrbuf_addch(&buf, '\\n');\n>  \t\tstrbuf_add_commented_lines(&buf, \"\\n\", strlen(\"\\n\"));\n>  \t\tstrbuf_add_commented_lines(&buf, _(note_template), strlen(_(note_template)));\n> -\t\tstrbuf_addch(&buf, '\\n');\n> +\t\tstrbuf_add_commented_lines(&buf, \"\\n\", strlen(\"\\n\"));\n>  \t\twrite_or_die(fd, buf.buf, buf.len);\n\nNothing new as the pre-image shows, but I wondered why not just add a\n\"#\\n\", before I remembered core.commentChar, so this is correct.\n"},{"id":"467428","messageId":"166867999215.10813.7408480606700051633.git@grubix.eu","threadId":"58805","inReplyTo":"221117.86wn7tj4af.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] notes: avoid empty line in template","fromName":"Michael J Gruber","fromEmail":"git@grubix.eu","sentAt":"2022-11-17T10:13:12Z","receivedAt":"2022-11-17T10:22:33Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Ævar Arnfjörð Bjarmason venit, vidit, dixit 2022-11-17 10:48:48:\n> \n> On Wed, Nov 16 2022, Michael J Gruber wrote:\n> \n> > When `git notes` prepares the template it adds an empty newline between\n> > the comment header and the content:\n> >\n> >>\n> >> #\n> >> # Write/edit the notes for the following object:\n> >>\n> >> # commit 0f3c55d4c2b7864bffb2d92278eff08d0b2e083f\n> >> # etc\n> >\n> > This is wrong structurally because that newline is part of the comment,\n> > too, and thus should be commented. Also, it throws off some positioning\n> > strategies of editors and plugins, and it differs from how we do commit\n> > templates.\n> >\n> > Change this to follow the standard set by `git commit`:\n> \n> I don't mind the consistency here, but what does \"wrong structurally\"\n> mean? Doesn't the usual removing of duplicate newlines make this amount\n> to the same?\n\nI am talking about what we present to the user as a template, and that\ncontains two newlines. Whether they will be reduced afterwards depends\non the cleanup policy.\n \n> >> #\n> >> # Write/edit the notes for the following object:\n> >> #\n> >> # commit 0f3c55d4c2b7864bffb2d92278eff08d0b2e083f\n> >>\n> >\n> > Tests pass unchanged after this code change.\n> \n> Because it did change something and we've got bad test coverage, or just\n> because it's really a stylistic change?\n> \n> I don't mind it being a stylistic change, but the proposed commit\n> doesn't really make that clear, and leaves one wondering about potential\n> missing test coverage etc.\n\nYes, missing, as noted by peff also. We do test the resulting notes\nobjects, and those tests pass unchanged which proves that the code\nchanges only what we present to the user in the template, not what we\ncreate in the object store.\n \n> > Signed-off-by: Michael J Gruber <git@grubix.eu>\n> > ---\n> >  builtin/notes.c | 2 +-\n> >  1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/builtin/notes.c b/builtin/notes.c\n> > index be51f69225..80d9dfd25c 100644\n> > --- a/builtin/notes.c\n> > +++ b/builtin/notes.c\n> > @@ -181,7 +181,7 @@ static void prepare_note_data(const struct object_id *object, struct note_data *\n> >               strbuf_addch(&buf, '\\n');\n> >               strbuf_add_commented_lines(&buf, \"\\n\", strlen(\"\\n\"));\n> >               strbuf_add_commented_lines(&buf, _(note_template), strlen(_(note_template)));\n> > -             strbuf_addch(&buf, '\\n');\n> > +             strbuf_add_commented_lines(&buf, \"\\n\", strlen(\"\\n\"));\n> >               write_or_die(fd, buf.buf, buf.len);\n> \n> Nothing new as the pre-image shows, but I wondered why not just add a\n> \"#\\n\", before I remembered core.commentChar, so this is correct.\n\nIntroduction of that feature was the source of all these lines in the\npreimage, but due to the extent of the proposed cosmetic change I passed\non the usual blaming business.\n\nMichael\n"},{"id":"467449","messageId":"Y3ZPW/seP0auAtKZ@coredump.intra.peff.net","threadId":"58805","inReplyTo":"166867999215.10813.7408480606700051633.git@grubix.eu","subject":"Re: [PATCH] notes: avoid empty line in template","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-17T15:12:27Z","receivedAt":"2022-11-17T15:18:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 17, 2022 at 11:13:12AM +0100, Michael J Gruber wrote:\n\n> > I don't mind the consistency here, but what does \"wrong structurally\"\n> > mean? Doesn't the usual removing of duplicate newlines make this amount\n> > to the same?\n> \n> I am talking about what we present to the user as a template, and that\n> contains two newlines. Whether they will be reduced afterwards depends\n> on the cleanup policy.\n\nI wondered if this might actually be a non-cosmetic bug, if you could so\nsomething like:\n\n  git notes add --cleanup=only-comments\n\nwhich would retain the extra newlines. But there is no --cleanup option\nfor git-notes at all (it always calls strbuf_stripspace() internally).\nAnd there is no \"comments only\" cleanup mode; it is either \"clean\nnothing\", \"clean space but leave comments\", or \"clean both\".\n\nSo I think it really is just cosmetic.\n\n-Peff\n"}]}