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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 15, 2023, 22:15 UTC
Message-ID: <CAPig+cSt+vkjYi7L0pnE9g8WrqotT3djefaGQiCLqph6PAr=Lg@mail.gmail.com>
In-Reply-To: <f7edbd0e508243ab55c13721a21b78bf50278a21.1673490953.git.dyroneteng@gmail.com>

On Wed, Jan 11, 2023 at 9:48 PM Teng Long <dyroneteng@gmail.com> wrote:

Show 9 quoted lines

> 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/builtin/notes.c b/builtin/notes.c
> @@ -24,6 +24,8 @@
> +static char *separator = "\n";

If you are initializing `separator` to "\n"...

Show 5 quoted lines

> @@ -225,6 +227,43 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
> +static void insert_separator(struct strbuf *message)
> +{
> +       if (!separator)
> +               separator = "\n";

... then these lines are not needed (and are effectively dead-code).

Show 11 quoted lines

> +       if (*separator == '\0')
> +               /* 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");
> +       strbuf_insertstr(message, 0, insert);
> +}

> git format-patch a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1  --thread -v 4 --output-directory=outgoing/git-notes-append/v4 --cover-letter  --range-diff 196e80358ediff --git a/t/t3301-notes.sh b/t/t3301-notes.sh

There's some weird malformation going on here... this should just be a "diff --git ..." line.

Show 7 quoted lines

> @@ -521,6 +521,65 @@ test_expect_success 'listing non-existing notes fails' '
> +test_expect_success 'append: specify an empty separator' '
> +       test_when_finished git notes remove HEAD &&
> +       cat >expect <<-\EOF &&
> +               notes-1
> +               notes-2
> +       EOF

Style nit: We don't normally give extra indentation to the body of the here-doc. Instead:

    cat >expect <<-\EOF &&
    notes-1
    notes-2
    EOF

Show 6 quoted lines

> +       git notes add -m "notes-1" &&
> +       git notes append --separator="" -m "notes-2" &&
> +       git notes show >actual &&
> +       test_cmp expect actual
> +
> +'

Style nit: drop the unnecessary blank line before the closing quote.

> +test_expect_success 'append: specify separatoro with line break' '

s/separatoro/separator/

Show 10 quoted lines

> +       test_when_finished git notes remove HEAD &&
> +       cat >expect <<-\EOF &&
> +               notes-1
> +               -------
> +               notes-2
> +       EOF
> +
> +       git notes add -m "notes-1" &&
> +       separator=$(printf "%s\n" "-------") &&
> +       git notes append --separator="$separator" -m "notes-2" &&

It might be easier to drop the `separator` variable and write this simply as:

    git notes append --separator="-------$LF" -m "notes-2" &&

LF is defined in t/test-lib.sh as "\n".

> +       git notes show >actual &&
> +       test_cmp expect actual
> +'