[PATCH] notes: avoid empty line in template

[PATCH] notes: avoid empty line in template

[Michael J Gruber]

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

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

[Jeff King]

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

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

[Taylor Blau]

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

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

[Ævar Arnfjörð Bjarmason]

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.

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

[Michael J Gruber]

Æ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

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

[Jeff King]

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