* [PATCH] notes: avoid empty line in template @ 2022-11-16 15:56 Michael J Gruber 2022-11-16 18:52 ` Jeff King 2022-11-17 9:48 ` Ævar Arnfjörð Bjarmason 0 siblings, 2 replies; 6+ messages in thread From: Michael J Gruber @ 2022-11-16 15:56 UTC (permalink / raw) To: git When `git notes` prepares the template it adds an empty newline between the comment header and the content: > > # > # Write/edit the notes for the following object: > > # commit 0f3c55d4c2b7864bffb2d92278eff08d0b2e083f > # etc This is wrong structurally because that newline is part of the comment, too, and thus should be commented. Also, it throws off some positioning strategies of editors and plugins, and it differs from how we do commit templates. Change this to follow the standard set by `git commit`: > > # > # Write/edit the notes for the following object: > # > # commit 0f3c55d4c2b7864bffb2d92278eff08d0b2e083f > Tests pass unchanged after this code change. Signed-off-by: Michael J Gruber <git@grubix.eu> --- builtin/notes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/notes.c b/builtin/notes.c index be51f69225..80d9dfd25c 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -181,7 +181,7 @@ static void prepare_note_data(const struct object_id *object, struct note_data * strbuf_addch(&buf, '\n'); strbuf_add_commented_lines(&buf, "\n", strlen("\n")); strbuf_add_commented_lines(&buf, _(note_template), strlen(_(note_template))); - strbuf_addch(&buf, '\n'); + strbuf_add_commented_lines(&buf, "\n", strlen("\n")); write_or_die(fd, buf.buf, buf.len); write_commented_object(fd, object); -- 2.38.1.672.gc8cd8f59d3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] notes: avoid empty line in template 2022-11-16 15:56 [PATCH] notes: avoid empty line in template Michael J Gruber @ 2022-11-16 18:52 ` Jeff King 2022-11-16 19:57 ` Taylor Blau 2022-11-17 9:48 ` Ævar Arnfjörð Bjarmason 1 sibling, 1 reply; 6+ messages in thread From: Jeff King @ 2022-11-16 18:52 UTC (permalink / raw) To: Michael J Gruber; +Cc: git On Wed, Nov 16, 2022 at 04:56:40PM +0100, Michael J Gruber wrote: > When `git notes` prepares the template it adds an empty newline between > the comment header and the content: > > > > > # > > # Write/edit the notes for the following object: > > > > # commit 0f3c55d4c2b7864bffb2d92278eff08d0b2e083f > > # etc > > This is wrong structurally because that newline is part of the comment, > too, and thus should be commented. Also, it throws off some positioning > strategies of editors and plugins, and it differs from how we do commit > templates. > > Change this to follow the standard set by `git commit`: > > > > > # > > # Write/edit the notes for the following object: > > # > > # commit 0f3c55d4c2b7864bffb2d92278eff08d0b2e083f > > Yeah, this makes perfect sense. > Tests pass unchanged after this code change. I'm mildly surprised that there wasn't some test depending on this in a stupid way. :) But grepping for "Write/edit" shows that nobody did (and obviously the tests pass). We could add a new test here, but it probably is trivial enough not to worry about. > diff --git a/builtin/notes.c b/builtin/notes.c > index be51f69225..80d9dfd25c 100644 > --- a/builtin/notes.c > +++ b/builtin/notes.c > @@ -181,7 +181,7 @@ static void prepare_note_data(const struct object_id *object, struct note_data * > strbuf_addch(&buf, '\n'); > strbuf_add_commented_lines(&buf, "\n", strlen("\n")); > strbuf_add_commented_lines(&buf, _(note_template), strlen(_(note_template))); > - strbuf_addch(&buf, '\n'); > + strbuf_add_commented_lines(&buf, "\n", strlen("\n")); And the patch looks obviously good. The irony is the version two lines above which does it correctly. ;) -Peff ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] notes: avoid empty line in template 2022-11-16 18:52 ` Jeff King @ 2022-11-16 19:57 ` Taylor Blau 0 siblings, 0 replies; 6+ messages in thread From: Taylor Blau @ 2022-11-16 19:57 UTC (permalink / raw) To: Jeff King; +Cc: Michael J Gruber, git On Wed, Nov 16, 2022 at 01:52:55PM -0500, Jeff King wrote: > > diff --git a/builtin/notes.c b/builtin/notes.c > > index be51f69225..80d9dfd25c 100644 > > --- a/builtin/notes.c > > +++ b/builtin/notes.c > > @@ -181,7 +181,7 @@ static void prepare_note_data(const struct object_id *object, struct note_data * > > strbuf_addch(&buf, '\n'); > > strbuf_add_commented_lines(&buf, "\n", strlen("\n")); > > strbuf_add_commented_lines(&buf, _(note_template), strlen(_(note_template))); > > - strbuf_addch(&buf, '\n'); > > + strbuf_add_commented_lines(&buf, "\n", strlen("\n")); > > And the patch looks obviously good. The irony is the version two lines > above which does it correctly. ;) Thanks, both. ;-). Thanks, Taylor ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] notes: avoid empty line in template 2022-11-16 15:56 [PATCH] notes: avoid empty line in template Michael J Gruber 2022-11-16 18:52 ` Jeff King @ 2022-11-17 9:48 ` Ævar Arnfjörð Bjarmason 2022-11-17 10:13 ` Michael J Gruber 1 sibling, 1 reply; 6+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-11-17 9:48 UTC (permalink / raw) To: Michael J Gruber; +Cc: git On Wed, Nov 16 2022, Michael J Gruber wrote: > When `git notes` prepares the template it adds an empty newline between > the comment header and the content: > >> >> # >> # Write/edit the notes for the following object: >> >> # commit 0f3c55d4c2b7864bffb2d92278eff08d0b2e083f >> # etc > > This is wrong structurally because that newline is part of the comment, > too, and thus should be commented. Also, it throws off some positioning > strategies of editors and plugins, and it differs from how we do commit > templates. > > Change this to follow the standard set by `git commit`: I don't mind the consistency here, but what does "wrong structurally" mean? Doesn't the usual removing of duplicate newlines make this amount to the same? >> # >> # Write/edit the notes for the following object: >> # >> # commit 0f3c55d4c2b7864bffb2d92278eff08d0b2e083f >> > > Tests pass unchanged after this code change. Because it did change something and we've got bad test coverage, or just because it's really a stylistic change? I don't mind it being a stylistic change, but the proposed commit doesn't really make that clear, and leaves one wondering about potential missing test coverage etc. > Signed-off-by: Michael J Gruber <git@grubix.eu> > --- > builtin/notes.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/builtin/notes.c b/builtin/notes.c > index be51f69225..80d9dfd25c 100644 > --- a/builtin/notes.c > +++ b/builtin/notes.c > @@ -181,7 +181,7 @@ static void prepare_note_data(const struct object_id *object, struct note_data * > strbuf_addch(&buf, '\n'); > strbuf_add_commented_lines(&buf, "\n", strlen("\n")); > strbuf_add_commented_lines(&buf, _(note_template), strlen(_(note_template))); > - strbuf_addch(&buf, '\n'); > + strbuf_add_commented_lines(&buf, "\n", strlen("\n")); > write_or_die(fd, buf.buf, buf.len); Nothing new as the pre-image shows, but I wondered why not just add a "#\n", before I remembered core.commentChar, so this is correct. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] notes: avoid empty line in template 2022-11-17 9:48 ` Ævar Arnfjörð Bjarmason @ 2022-11-17 10:13 ` Michael J Gruber 2022-11-17 15:12 ` Jeff King 0 siblings, 1 reply; 6+ messages in thread From: Michael J Gruber @ 2022-11-17 10:13 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git Ævar Arnfjörð Bjarmason venit, vidit, dixit 2022-11-17 10:48:48: > > On Wed, Nov 16 2022, Michael J Gruber wrote: > > > When `git notes` prepares the template it adds an empty newline between > > the comment header and the content: > > > >> > >> # > >> # Write/edit the notes for the following object: > >> > >> # commit 0f3c55d4c2b7864bffb2d92278eff08d0b2e083f > >> # etc > > > > This is wrong structurally because that newline is part of the comment, > > too, and thus should be commented. Also, it throws off some positioning > > strategies of editors and plugins, and it differs from how we do commit > > templates. > > > > Change this to follow the standard set by `git commit`: > > I don't mind the consistency here, but what does "wrong structurally" > mean? Doesn't the usual removing of duplicate newlines make this amount > to the same? I am talking about what we present to the user as a template, and that contains two newlines. Whether they will be reduced afterwards depends on the cleanup policy. > >> # > >> # Write/edit the notes for the following object: > >> # > >> # commit 0f3c55d4c2b7864bffb2d92278eff08d0b2e083f > >> > > > > Tests pass unchanged after this code change. > > Because it did change something and we've got bad test coverage, or just > because it's really a stylistic change? > > I don't mind it being a stylistic change, but the proposed commit > doesn't really make that clear, and leaves one wondering about potential > missing test coverage etc. Yes, missing, as noted by peff also. We do test the resulting notes objects, and those tests pass unchanged which proves that the code changes only what we present to the user in the template, not what we create in the object store. > > Signed-off-by: Michael J Gruber <git@grubix.eu> > > --- > > builtin/notes.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/builtin/notes.c b/builtin/notes.c > > index be51f69225..80d9dfd25c 100644 > > --- a/builtin/notes.c > > +++ b/builtin/notes.c > > @@ -181,7 +181,7 @@ static void prepare_note_data(const struct object_id *object, struct note_data * > > strbuf_addch(&buf, '\n'); > > strbuf_add_commented_lines(&buf, "\n", strlen("\n")); > > strbuf_add_commented_lines(&buf, _(note_template), strlen(_(note_template))); > > - strbuf_addch(&buf, '\n'); > > + strbuf_add_commented_lines(&buf, "\n", strlen("\n")); > > write_or_die(fd, buf.buf, buf.len); > > Nothing new as the pre-image shows, but I wondered why not just add a > "#\n", before I remembered core.commentChar, so this is correct. Introduction of that feature was the source of all these lines in the preimage, but due to the extent of the proposed cosmetic change I passed on the usual blaming business. Michael ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] notes: avoid empty line in template 2022-11-17 10:13 ` Michael J Gruber @ 2022-11-17 15:12 ` Jeff King 0 siblings, 0 replies; 6+ messages in thread From: Jeff King @ 2022-11-17 15:12 UTC (permalink / raw) To: Michael J Gruber; +Cc: Ævar Arnfjörð Bjarmason, git On Thu, Nov 17, 2022 at 11:13:12AM +0100, Michael J Gruber wrote: > > I don't mind the consistency here, but what does "wrong structurally" > > mean? Doesn't the usual removing of duplicate newlines make this amount > > to the same? > > I am talking about what we present to the user as a template, and that > contains two newlines. Whether they will be reduced afterwards depends > on the cleanup policy. I wondered if this might actually be a non-cosmetic bug, if you could so something like: git notes add --cleanup=only-comments which would retain the extra newlines. But there is no --cleanup option for git-notes at all (it always calls strbuf_stripspace() internally). And there is no "comments only" cleanup mode; it is either "clean nothing", "clean space but leave comments", or "clean both". So I think it really is just cosmetic. -Peff ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-11-17 15:18 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-11-16 15:56 [PATCH] notes: avoid empty line in template Michael J Gruber 2022-11-16 18:52 ` Jeff King 2022-11-16 19:57 ` Taylor Blau 2022-11-17 9:48 ` Ævar Arnfjörð Bjarmason 2022-11-17 10:13 ` Michael J Gruber 2022-11-17 15:12 ` Jeff King
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).