Re: [PATCH v4 5/5] notes.c: introduce "--separator" option

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Jan 12, 2023 at 5:07 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Thu, Jan 12 2023, Teng Long wrote:
> > When appending to a given notes object and the appended note is not
> > empty too, we will insert a blank line at first which separates the
> > existing note and the appended one, which as the separator.
> > [...]
> > Signed-off-by: Teng Long <dyroneteng@gmail.com>
> > ---
> > diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
> > @@ -159,6 +163,16 @@ OPTIONS
> > +--separator <text>::
> > +     Specify the <text> to be inserted between existing note and appended
> > +     message, the <text> acts as a separator.
>
> Maybe let's use '<string>' or '<separator>' here instead? e.g.:
>         Specifies the <string> ...
> Maybe "<text>" just looks odd to me.
>
> More generally, let's say something like:
>         When invoking "git notes append", specify the...
> I.e. this is only for "append", but nothing here says so.

Agreed on these points.

> > +     If <text> is empty (`--separator=''`), will append the message to
> > +     existing note directly without insert any separator.
> > +     If <text> is nonempty, will use as-is. One thing to notice is if
> > +     the <text> lacks newline charactor, will add the newline automatically.
> > +     If not specify this option, a blank line will be inserted as the
> > +     separator.
>
> We're spending a lot of text here on a pretty simple concept if I
> understand it correctly, I.e. just (pseudocode):
>
>         int sep_extra_nl = 0;
>         const char *sep = opt_sep ? opt_sep : "\n";
>         if (!strstr(sep, '\n'))
>                 sep_extra_nl = 1;
>         [...]
>
> Except that was written after I read your explanation, but looking at
> the code it's incorrect, it's whether the "*last*" character contains a
> newline or not.
>
> So all in all, I think we should just say:
>
>         --separator <separator>:
>                 The '<separator>' inserted between the note and message
>                 by 'append', "\n" by default. A custom separator can be
>                 provided, if it doesn't end in a "\n" one will be added
>                 implicitly .

Unfortunately, this misses the point. The original reason Teng Long
started on this patch series was to be able to _suppress_ the blank
line added unconditionally between notes. In the original submission,
this was done via a --no-blankline option, but that met with
resistance from some reviewers as being potentially confusing and too
specialized. (The commit message of this patch should probably do a
better job of explaining that one purpose of this change is to support
the case of no-separator.)

A generalized --separator= option was suggested[1] as a possibly more
palatable alternative, with which an empty string (meaning "no
separator") would cover the case for which the original --no-blankline
was meant to handle. So, at the very least, the documentation needs to
call out the empty string as being a special case for which automatic
appending of "\n" does not occur.

> > diff --git a/builtin/notes.c b/builtin/notes.c
> > +static void insert_separator(struct strbuf *message)
> > +{
> > +     const char *insert;
> > +
> > +     if (!separator)
> > +             separator = "\n";
> > +     if (*separator == '\0')
>
> Style: Don't compare to 0, NULL, '\0' etc. Just use !*separator.

My fault[2]. Your suggestion is indeed more appropriate in this codebase.

> > +             /* separator is empty; use as-is (no blank line) */
> > +             return;
> > +     else if (separator[strlen(separator) - 1] == '\n')
> > +             /* user supplied newline; use as-is */
> > +             insert = separator;
> > +     else
> > +             /* separator lacks newline; add it ourselves */
> > +             insert = xstrfmt("%s%s", separator,"\n");
>
> We're leaking memor here, and making it hard to fix that by conflating a
> const "insert" with this allocated version.
>
> I haven't read the whole context, but this seems really complex per the
> doc feedback above. Why can't we just keep track of if we're using the
> default value or not? I.e. just have the "--separator" option default to
> NULL, if it's not set y ou don't need to do this "\n" check, and just
> use the default, otherwise append etc.

That wouldn't work for the reason given above. The idea outlined in
[2] is that an empty separator is treated specially as meaning
"nothing-between-notes, not even a blank line".

> > +     strbuf_insertstr(message, 0, insert);
>
> Maybe you were trying to get around using a more complex strbuf_splice()
> here, but let's just avoid teh xstrfmt() and splice() that "\n" in, if
> needed?

The code example I gave in [2] was meant to illustrate the suggested
behavior as clearly as possible, not necessarily to be copied
verbatim. Being able to do this without leaking memory should
certainly be possible.

> Do we mean to strbuf_stripspace() here over the whole buffer, or just
> what we're appending?

That's a very good question. The note being appended might indeed have
leading whitespace gunk which ought to be removed before the append
operation. (Plus, it's a reasonable assumption that the existing note
text has already been "stripspaced".)

[1]: https://lore.kernel.org/git/CAPig+cRcezSp4Rqt1Y9bD-FT6+7b0g9qHfbGRx65AOnw2FQXKg@mail.gmail.com/
[2]: https://lore.kernel.org/git/CAPig+cTFBVAL2gd3LqQEzS--cXqJXR+1OVerii-D6JqFvJwXqQ@mail.gmail.com/