On Wed, Jan 11, 2023 at 9:48 PM Teng Long <dyroneteng@xxxxxxxxx> 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/builtin/notes.c b/builtin/notes.c
> @@ -24,6 +24,8 @@
> +static char *separator = "\n";
If you are initializing `separator` to "\n"...
> @@ -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).
> + 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.
> @@ -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
> + 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/
> + 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
> +'
