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

> 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.

> > +     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 .

> > 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.

> > +             /* 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.

> > +     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?