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

From: Michael J Gruber <git@grubix.eu>
Date: Nov 17, 2022, 10:13 UTC
Message-ID: <166867999215.10813.7408480606700051633.git@grubix.eu>
In-Reply-To: <221117.86wn7tj4af.gmgdl@evledraar.gmail.com>

Æ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