On Thu, Jan 12, 2023 at 5:07 AM Ævar Arnfjörð Bjarmason
<avarab@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxx/
[2]: https://lore.kernel.org/git/CAPig+cTFBVAL2gd3LqQEzS--cXqJXR+1OVerii-D6JqFvJwXqQ@xxxxxxxxxxxxxx/
