threads / patch / 58805

patchnotes: avoid empty line in template

Subject: [PATCH] notes: avoid empty line in template

## tl;dr

6 messages between Nov 16, 2022 and Nov 17, 2022. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

Michael J Gruber· Nov 16, 2022, 15:56 UTC · lore

When `git notes` prepares the template it adds an empty newline between the comment header and the content:

Show 6 quoted lines
>
> #
> # 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`:
Show 6 quoted lines
>
> #
> # 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(-)
Show changes to builtin/notes.c +1 −1
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
Jeff King· Nov 16, 2022, 18:52 UTC · re: Michael J Gruber · lore

Re: [PATCH] notes: avoid empty line in template

On Wed, Nov 16, 2022 at 04:56:40PM +0100, Michael J Gruber wrote:
Show 23 quoted lines
> 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.

Show 10 quoted lines
> 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
Taylor Blau· Nov 16, 2022, 19:57 UTC · re: Jeff King · lore

Re: [PATCH] notes: avoid empty line in template

On Wed, Nov 16, 2022 at 01:52:55PM -0500, Jeff King wrote:
Show 13 quoted lines
> > 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

Ævar Arnfjörð Bjarmason· Nov 17, 2022, 09:48 UTC · re: Michael J Gruber · lore

Re: [PATCH] notes: avoid empty line in template

On Wed, Nov 16 2022, Michael J Gruber wrote:
Show 16 quoted lines
> 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?

Show 7 quoted lines
>> #
>> # 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.

Show 16 quoted lines
> 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.

Michael J Gruber· Nov 17, 2022, 10:13 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] notes: avoid empty line in template

Ævar Arnfjörð Bjarmason venit, vidit, dixit 2022-11-17 10:48:48:
Show 23 quoted lines
> 
> 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.
Show 14 quoted lines
> >> #
> >> # 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.
Show 19 quoted lines
> > 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
Jeff King· Nov 17, 2022, 15:12 UTC · re: Michael J Gruber · lore

Re: [PATCH] notes: avoid empty line in template

On Thu, Nov 17, 2022 at 11:13:12AM +0100, Michael J Gruber wrote:
Show 7 quoted lines
> > 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

← back to recent threads