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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Jan 12 2023, Teng Long wrote:

> From: Teng Long <dyroneteng@gmail.com>
>
> 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.
>
> Sometimes, we want to use a specified <text> as the separator. For
> example, if we specify as:
>
>     * --separator='------': we will insert "------\n" as the separator,
>     because user do not provide the line break char at last, we will add
>     the trailing '\n' compatibly.
>
>     * --separator='------\n': we will insert as-is because it contains
>     the line break at last.
>
>     * --separator='': we specify an empty separator which means will
>     append the message directly without inserting any separator at
>     first.
>
>     * not specified --separator option: will use '\n' as the separator
>     when do appending and this is the default behavour.
>
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  Documentation/git-notes.txt | 18 +++++++++--
>  builtin/notes.c             | 49 +++++++++++++++++++++++++++---
>  t/t3301-notes.sh            | 59 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 120 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
> index efbc10f0f5..227fa88317 100644
> --- a/Documentation/git-notes.txt
> +++ b/Documentation/git-notes.txt
> @@ -11,7 +11,7 @@ SYNOPSIS
>  'git notes' [list [<object>]]
>  'git notes' add [-f] [--allow-empty] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
>  'git notes' copy [-f] ( --stdin | <from-object> [<to-object>] )
> -'git notes' append [--allow-empty] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
> +'git notes' append [--allow-empty] [--separator] [-F <file> | -m <msg> | (-c | -C) <object>] [<object>]
>  'git notes' edit [--allow-empty] [<object>]
>  'git notes' show [<object>]
>  'git notes' merge [-v | -q] [-s <strategy> ] <notes-ref>
> @@ -86,7 +86,11 @@ the command can read the input given to the `post-rewrite` hook.)
>  
>  append::
>  	Append to the notes of an existing object (defaults to HEAD).
> -	Creates a new notes object if needed.
> +	Creates a new notes object if needed. If the note of the given
> +	object and the note to be appended are not empty, a blank line
> +	will be inserted between them as the separator ("blank line" is
> +	the default behavior, `--separator` option supports to specify
> +	a customized one).

I think this should change to:

	[...]will be inserted between them. Use the `--separator` option
	to insert other delimiters.

I.e. part of that's fixes for odd grammar, but mainly just offloading
the explanation to the --separator discussion below.


>  
>  edit::
>  	Edit the notes for a given object (defaults to HEAD).
> @@ -159,6 +163,16 @@ OPTIONS
>  	Allow an empty note object to be stored. The default behavior is
>  	to automatically remove empty notes.
>  
> +--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 .

> +
>  --ref <ref>::
>  	Manipulate the notes tree in <ref>.  This overrides
>  	`GIT_NOTES_REF` and the "core.notesRef" configuration.  The ref
> diff --git a/builtin/notes.c b/builtin/notes.c
> index f2efb3736c..6746ad3232 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -24,6 +24,8 @@
>  #include "notes-utils.h"
>  #include "worktree.h"
>  
> +static char *separator = "\n";
> +
>  static const char * const git_notes_usage[] = {
>  	N_("git notes [--ref <notes-ref>] [list [<object>]]"),
>  	N_("git notes [--ref <notes-ref>] add [-f] [--allow-empty] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]"),
> @@ -209,7 +211,7 @@ static void write_note_data(struct note_data *d, struct object_id *oid)
>  	}
>  }
>  
> -static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
> +static int parse_msg_arg_add(const struct option *opt, const char *arg, int unset)
>  {
>  	struct note_data *d = opt->value;
>  
> @@ -225,6 +227,43 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
>  	return 0;
>  }
>  
> +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?

> +}
> +
> +static int parse_msg_arg_append(const struct option *opt, const char *arg, int unset)
> +{
> +	struct note_data *d = opt->value;
> +	struct strbuf append = STRBUF_INIT;
> +
> +	BUG_ON_OPT_NEG(unset);
> +
> +	strbuf_addstr(&append, arg);
> +	if (d->buf.len){
> +		insert_separator(&append);
> +	}

Drop the {} here.

> +	strbuf_addbuf(&d->buf, &append);
> +	strbuf_stripspace(&d->buf, 0);
> +
> +	d->given = 1;
> +	strbuf_release(&append);

Why do we need this other variable, canet'w just append to d.buf
directly?

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