RFC patch, 2 partsnotes.c: introduce "--no-blankline" option

> diff --git a/builtin/notes.c b/builtin/notes.c
> index be51f69225..1ca0476a27 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -562,6 +562,7 @@ static int copy(int argc, const char **argv, const char *prefix)
>  static int append_edit(int argc, const char **argv, const char *prefix)
>  {
>  	int allow_empty = 0;
> +	int no_blankline = 0;

> @@ -584,6 +585,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>  			parse_reuse_arg),
>  		OPT_BOOL(0, "allow-empty", &allow_empty,
>  			N_("allow storing empty note")),
> +		OPT_BOOL(0, "no-blankline", &no_blankline,
> +			N_("do not initially add a blank line")),

> @@ -619,7 +622,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>  		char *prev_buf = read_object_file(note, &type, &size);
>  
>  		strbuf_grow(&d.buf, size + 1);
> -		if (d.buf.len && prev_buf && size)
> +		if (!no_blankline && d.buf.len && prev_buf && size)
>  			strbuf_insertstr(&d.buf, 0, "\n");

> Use
>
> 	int blankline = 1;
>
> to avoid double negative, which is confusing and error prone.

> 	OPT_BOOL(0, "blank-line", &blankline,
> 		 N_("insert paragraph break before appending to an existing note")),
> Then, the conditional would read more naturally without double
> negation.
>
> 		if (blank_line && d.buf.len && prev_buf && size)

> +	EOF
> +	git notes add -m "Initial set of notes" &&
> +	git notes append --no-blankline -m "Appended notes" &&
> +	git notes show >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'append to existing note with "git notes append"' '
>  	cat >expect <<-EOF &&
>  		Initial set of notes
>  
>  		More notes appended with git notes append
>  	EOF

> From: Teng Long <dyroneteng@gmail.com>
>
> When "git notes append <object>" is executed, if there is no note in
> the given object and the appended note is empty, the command line
> prompt will be as follows:
>
>      "Removing note for object <object>"
>
> Actually, this tip is not that accurate, because there is no note in
> the original <object>, and it also does no remove work on the notes
> reference, so let's fix this and give the correct tip.
>
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  builtin/notes.c  | 13 +++++++++++--
>  t/t3301-notes.sh |  3 ++-
>  2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 1ca0476a27..cc1e3aa2b6 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -567,9 +567,10 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>  	struct notes_tree *t;
>  	struct object_id object, new_note;
>  	const struct object_id *note;
> -	char *logmsg;
> +	char *logmsg = NULL;

> 
> On Thu, Oct 13 2022, Teng Long wrote:
> 
>> From: Teng Long <dyroneteng@gmail.com>
>>
>> When "git notes append <object>" is executed, if there is no note in
>> the given object and the appended note is empty, the command line
>> prompt will be as follows:
>>
>>       "Removing note for object <object>"
>>
>> Actually, this tip is not that accurate, because there is no note in
>> the original <object>, and it also does no remove work on the notes
>> reference, so let's fix this and give the correct tip.
>>
>> Signed-off-by: Teng Long <dyroneteng@gmail.com>
>> ---
>>   builtin/notes.c  | 13 +++++++++++--
>>   t/t3301-notes.sh |  3 ++-
>>   2 files changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/builtin/notes.c b/builtin/notes.c
>> index 1ca0476a27..cc1e3aa2b6 100644
>> --- a/builtin/notes.c
>> +++ b/builtin/notes.c
>> @@ -567,9 +567,10 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>>   	struct notes_tree *t;
>>   	struct object_id object, new_note;
>>   	const struct object_id *note;
>> -	char *logmsg;
>> +	char *logmsg = NULL;
> 
> Hrm, interesting that (at least my) gcc doesn't catch if we don't
> NULL-initialize this, but -fanalyzer does (usually it's not needed for
> such trivial cases0. Anyawy...

>>   	const char * const *usage;
>>   	struct note_data d = { 0, 0, NULL, STRBUF_INIT };
>> +	struct note_data cp = { 0, 0, NULL, STRBUF_INIT };
> 
> This is probably better "fixed while at it" to set both to use "{ .buf =
> STRBUF_INIT }", rather than copying the pre-C99 pattern.

> On 13/10/2022 10:36, Ævar Arnfjörð Bjarmason wrote:
>> On Thu, Oct 13 2022, Teng Long wrote:
>> 
>>> From: Teng Long <dyroneteng@gmail.com>
>>>
>>> When "git notes append <object>" is executed, if there is no note in
>>> the given object and the appended note is empty, the command line
>>> prompt will be as follows:
>>>
>>>       "Removing note for object <object>"
>>>
>>> Actually, this tip is not that accurate, because there is no note in
>>> the original <object>, and it also does no remove work on the notes
>>> reference, so let's fix this and give the correct tip.
>>>
>>> Signed-off-by: Teng Long <dyroneteng@gmail.com>
>>> ---
>>>   builtin/notes.c  | 13 +++++++++++--
>>>   t/t3301-notes.sh |  3 ++-
>>>   2 files changed, 13 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/builtin/notes.c b/builtin/notes.c
>>> index 1ca0476a27..cc1e3aa2b6 100644
>>> --- a/builtin/notes.c
>>> +++ b/builtin/notes.c
>>> @@ -567,9 +567,10 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>>>   	struct notes_tree *t;
>>>   	struct object_id object, new_note;
>>>   	const struct object_id *note;
>>> -	char *logmsg;
>>> +	char *logmsg = NULL;
>> Hrm, interesting that (at least my) gcc doesn't catch if we don't
>> NULL-initialize this, but -fanalyzer does (usually it's not needed for
>> such trivial cases0. Anyawy...
>
> I don't think its written to if we take the 'else if' branch added by
> this patch so we need to initialize it for the free() at the end.

>>>   	const char * const *usage;
>>>   	struct note_data d = { 0, 0, NULL, STRBUF_INIT };
>>> +	struct note_data cp = { 0, 0, NULL, STRBUF_INIT };
>> This is probably better "fixed while at it" to set both to use "{
>> .buf =
>> STRBUF_INIT }", rather than copying the pre-C99 pattern.
>
> We only seem to be using cp.buf.len so we can test check if the
> original note was empty so I think it would be better just to add
>
> 	int note_was_empty;
>
> `	...
>
> 	note_was_empty = !d.buf.len

> 
> On Thu, Oct 13 2022, Phillip Wood wrote:
>>>> diff --git a/builtin/notes.c b/builtin/notes.c
>>>> index 1ca0476a27..cc1e3aa2b6 100644
>>>> --- a/builtin/notes.c
>>>> +++ b/builtin/notes.c
>>>> @@ -567,9 +567,10 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>>>>    	struct notes_tree *t;
>>>>    	struct object_id object, new_note;
>>>>    	const struct object_id *note;
>>>> -	char *logmsg;
>>>> +	char *logmsg = NULL;
>>> Hrm, interesting that (at least my) gcc doesn't catch if we don't
>>> NULL-initialize this, but -fanalyzer does (usually it's not needed for
>>> such trivial cases0. Anyawy...
>>
>> I don't think its written to if we take the 'else if' branch added by
>> this patch so we need to initialize it for the free() at the end.
> 
> Yes, sorry about not being clear. It *does* need to be uninitialized, I
> was just narrating my surprise at this not being a case where my
> compiler caught it when I was locally testing this.

> 	@@ -638,21 +635,16 @@ static int append_edit(int argc, const char **argv, const char *prefix)
> 	 			BUG("combine_notes_overwrite failed");
> 	 		logmsg = xstrfmt("Notes added by 'git notes %s'", argv[0]);
> 	 		commit_notes(the_repository, t, logmsg);
> 	-	} else if (!cp.buf.len) {
> 	+	} else if (!d.buf.len) {
> 	 		fprintf(stderr,
> 	 			_("Both original and appended notes are empty in %s, do nothing\n"),
> 	 			oid_to_hex(&object));
> 	 	} else {
> 	-		fprintf(stderr, _("Removing note for object %s\n"),
> 	-			oid_to_hex(&object));
> 	-		remove_note(t, object.hash);
> 	-		logmsg = xstrfmt("Notes removed by 'git notes %s'", argv[0]);
> 	-		commit_notes(the_repository, t, logmsg);
> 	+		BUG("this is not reachable by any test now");
> 	 	}
> 	
> This 2/2 makes that "else" test-unreachable, so whatever else we do here
> we should start by making sure that by adding the "else if" we still
> have test coverage for the "else".

> We only seem to be using cp.buf.len so we can test check if the original
> note was empty so I think it would be better just to add
>
> 	int note_was_empty;
>
> `	...
>
> 	note_was_empty = !d.buf.len
>
> instead.

> We only seem to be using cp.buf.len so we can test check if the original
> note was empty so I think it would be better just to add
>
> 	int note_was_empty;
>
> `	...
>
> 	note_was_empty = !d.buf.len
>
> instead.

> >  	const char * const *usage;
> >  	struct note_data d = { 0, 0, NULL, STRBUF_INIT };
> > +	struct note_data cp = { 0, 0, NULL, STRBUF_INIT };
>
> This is probably better "fixed while at it" to set both to use "{ .buf =
> STRBUF_INIT }", rather than copying the pre-C99 pattern.

> "Ævar Arnfjörð Bjarmason" <avarab@gmail.com> writes:
>
>> Hrm, interesting that (at least my) gcc doesn't catch if we don't
>> NULL-initialize this, but -fanalyzer does (usually it's not needed for
>> such trivial cases0. Anyawy...
>
> On my local env the warnings shows , show I change the line (initialize with
> NULL to "logmsg").
>
> But it seems like different as the last time I built... However now "suggest
> braces around initialization of subobject" appears, is it normal or we should
> repair this?
>
> builtin/merge-file.c:29:23: warning: suggest braces around initialization of subobject [-Wmissing-braces]
>         mmfile_t mmfs[3] = { 0 };
>                              ^
>                              {}
> builtin/merge-file.c:31:20: warning: suggest braces around initialization of subobject [-Wmissing-braces]
>         xmparam_t xmp = { 0 };
>                           ^
>                           {}

> 2 warnings generated.
> builtin/notes.c:641:13: warning: variable 'logmsg' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
>         } else if (!cp.buf.len) {
>                    ^~~~~~~~~~~
> builtin/notes.c:653:7: note: uninitialized use occurs here
>         free(logmsg);
>              ^~~~~~
> builtin/notes.c:641:9: note: remove the 'if' if its condition is always false
>         } else if (!cp.buf.len) {
>                ^~~~~~~~~~~~~~~~~~
> builtin/notes.c:570:14: note: initialize the variable 'logmsg' to silence this warning
>         char *logmsg;
>                     ^
>                      = NULL
> 1 warning generated.

> When appending to a given object which has note and if the appended
> note is not empty too, we will insert a blank line at first. The
> blank line serves as a split line, but sometimes we just want to
> omit it then append on the heels of the target note. So, we add
> a new "OPT_BOOL()" option to determain whether a new blank line
> is insert at first.
>
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  Documentation/git-notes.txt | 11 +++++++++--
>  builtin/notes.c             |  5 ++++-
>  t/t3301-notes.sh            | 12 ++++++++++++
>  3 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
> index efbc10f0f5..43770ddf84 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] [--blank-line] [-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,9 @@ 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.
>  
>  edit::
>  	Edit the notes for a given object (defaults to HEAD).
> @@ -159,6 +161,11 @@ OPTIONS
>  	Allow an empty note object to be stored. The default behavior is
>  	to automatically remove empty notes.
>  
> +--blank-line::
> +--no-blank-line::
> +	Controls if a blank line to split paragraphs is inserted
> +	when appending (the default is true).

>  	const char *object_ref;
>  	struct notes_tree *t;
>  	struct object_id object, new_note;
> @@ -584,6 +585,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>  			parse_reuse_arg),
>  		OPT_BOOL(0, "allow-empty", &allow_empty,
>  			N_("allow storing empty note")),
> +		OPT_BOOL(0, "blank-line", &blankline,

>  		if (prev_buf && size)
>  			strbuf_insert(&d.buf, 0, prev_buf, size);
> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index 3288aaec7d..76beafdeb8 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -521,12 +521,24 @@ test_expect_success 'listing non-existing notes fails' '
>  	test_must_be_empty actual
>  '
>  
> +test_expect_success 'append to existing note without a beginning blank line' '
> +	cat >expect <<-\EOF &&
> +		Initial set of notes
> +		Appended notes
> +	EOF
> +	git notes add -m "Initial set of notes" &&
> +	git notes append --no-blank-line -m "Appended notes" &&
> +	git notes show >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'append to existing note with "git notes append"' '
>  	cat >expect <<-EOF &&
>  		Initial set of notes
>  
>  		More notes appended with git notes append
>  	EOF
> +	git notes remove HEAD &&

> On Mon, Nov 07 2022, Teng Long wrote:
> > When appending to a given object which has note and if the appended
> > note is not empty too, we will insert a blank line at first. The
> > blank line serves as a split line, but sometimes we just want to
> > omit it then append on the heels of the target note. So, we add
> > a new "OPT_BOOL()" option to determain whether a new blank line
> > is insert at first.
> >
> > Signed-off-by: Teng Long <dyroneteng@gmail.com>
> > ---
> > diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
> > @@ -159,6 +161,11 @@ OPTIONS
> > +--blank-line::
> > +--no-blank-line::
> > +     Controls if a blank line to split paragraphs is inserted
> > +     when appending (the default is true).
>
> Just make this:
>
>         --no-blank-line:
>                 Suppress the insertion of a blank line before the
>                 inserted notes.
>
> Or something, i.e. when adding a "true by default" let's add a "no-..." variant directly.

> On Mon, Nov 7, 2022 at 9:56 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> On Mon, Nov 07 2022, Teng Long wrote:
>> > When appending to a given object which has note and if the appended
>> > note is not empty too, we will insert a blank line at first. The
>> > blank line serves as a split line, but sometimes we just want to
>> > omit it then append on the heels of the target note. So, we add
>> > a new "OPT_BOOL()" option to determain whether a new blank line
>> > is insert at first.
>> >
>> > Signed-off-by: Teng Long <dyroneteng@gmail.com>
>> > ---
>> > diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
>> > @@ -159,6 +161,11 @@ OPTIONS
>> > +--blank-line::
>> > +--no-blank-line::
>> > +     Controls if a blank line to split paragraphs is inserted
>> > +     when appending (the default is true).
>>
>> Just make this:
>>
>>         --no-blank-line:
>>                 Suppress the insertion of a blank line before the
>>                 inserted notes.
>>
>> Or something, i.e. when adding a "true by default" let's add a "no-..." variant directly.
>
> This is the exact opposite of Junio's advice[1], isn't it?
>
> [1]: https://lore.kernel.org/git/xmqqsfjsi7eq.fsf@gitster.g/

>
> On Mon, Nov 07 2022, Eric Sunshine wrote:
>
> > On Mon, Nov 7, 2022 at 9:56 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> >> On Mon, Nov 07 2022, Teng Long wrote:
> >> > When appending to a given object which has note and if the appended
> >> > note is not empty too, we will insert a blank line at first. The
> >> > blank line serves as a split line, but sometimes we just want to
> >> > omit it then append on the heels of the target note. So, we add
> >> > a new "OPT_BOOL()" option to determain whether a new blank line
> >> > is insert at first.
> >> >
> >> > Signed-off-by: Teng Long <dyroneteng@gmail.com>
> >> > ---
> >> > diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
> >> > @@ -159,6 +161,11 @@ OPTIONS
> >> > +--blank-line::
> >> > +--no-blank-line::
> >> > +     Controls if a blank line to split paragraphs is inserted
> >> > +     when appending (the default is true).
> >>
> >> Just make this:
> >>
> >>         --no-blank-line:
> >>                 Suppress the insertion of a blank line before the
> >>                 inserted notes.
> >>
> >> Or something, i.e. when adding a "true by default" let's add a "no-..." variant directly.
> >
> > This is the exact opposite of Junio's advice[1], isn't it?
> >
> > [1]: https://lore.kernel.org/git/xmqqsfjsi7eq.fsf@gitster.g/
>
> I read that as him mainly talking about what we name the variable (which
> I agree with, but didn't comment on here). I'm talking about what
> interface is exposed to the user.

> On Mon, Nov 07, 2022 at 06:22:34PM +0100, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Mon, Nov 07 2022, Eric Sunshine wrote:
>>
>> > On Mon, Nov 7, 2022 at 9:56 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> >> On Mon, Nov 07 2022, Teng Long wrote:
>> >> > When appending to a given object which has note and if the appended
>> >> > note is not empty too, we will insert a blank line at first. The
>> >> > blank line serves as a split line, but sometimes we just want to
>> >> > omit it then append on the heels of the target note. So, we add
>> >> > a new "OPT_BOOL()" option to determain whether a new blank line
>> >> > is insert at first.
>> >> >
>> >> > Signed-off-by: Teng Long <dyroneteng@gmail.com>
>> >> > ---
>> >> > diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
>> >> > @@ -159,6 +161,11 @@ OPTIONS
>> >> > +--blank-line::
>> >> > +--no-blank-line::
>> >> > +     Controls if a blank line to split paragraphs is inserted
>> >> > +     when appending (the default is true).
>> >>
>> >> Just make this:
>> >>
>> >>         --no-blank-line:
>> >>                 Suppress the insertion of a blank line before the
>> >>                 inserted notes.
>> >>
>> >> Or something, i.e. when adding a "true by default" let's add a "no-..." variant directly.
>> >
>> > This is the exact opposite of Junio's advice[1], isn't it?
>> >
>> > [1]: https://lore.kernel.org/git/xmqqsfjsi7eq.fsf@gitster.g/
>>
>> I read that as him mainly talking about what we name the variable (which
>> I agree with, but didn't comment on here). I'm talking about what
>> interface is exposed to the user.
>
> Having --blank-line as an option is convenient for scripting, so I'd err
> on the side of the original interpretation of Junio's suggestion.

> So, I'm (now) saying I don't care which form we use in the sources, but
> that' it's useful to document things as e.g.:
>
> 	--no-checkout::
>         	No checkout of HEAD is performed after the clone is complete.
>
> Instead of e.g.:
>
> 	--no-checkout:
> 	--checkout:
>  		Do we do a checkout when we clone (doing a checkout is
>  		the default).
>
> Because the former convention shows the user at a glance which of the
> two is the default.

> Just make this:
>
> 	--no-blank-line:
> 		Suppress the insertion of a blank line before the
> 		inserted notes.
>
> Or something, i.e. when adding a "true by default" let's add a "no-..." variant
> directly.
>
> ...

> > -		if (d.buf.len && prev_buf && size)
> > +		if (blankline && d.buf.len && prev_buf && size)
> >  			strbuf_insertstr(&d.buf, 0, "\n");
>
> Maybe this needs to be elaborated in the docs? I.e. it sounds as if
> we'll insert a \n unconditionally, which this shows isn't the case.

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

> >  	int allow_empty = 0;
> > +	int blankline = 1;
>
> So keep this...
>
> >  	const char *object_ref;
> >G  	struct notes_tree *t;
> >  	struct object_id object, new_note;
> > @@ -584,6 +585,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)
> >  			parse_reuse_arg),
> >  		OPT_BOOL(0, "allow-empty", &allow_empty,
> >  			N_("allow storing empty note")),
> > +		OPT_BOOL(0, "blank-line", &blankline,
>
> ...and just make this "no-blank-line", and parse_options() will do the
> right thing. I.e. PARSE_OPT_NONEG is implied.

> "Ævar Arnfjörð Bjarmason" <avarab@gmail.com> writes:
>
>> >  	int allow_empty = 0;
>> > +	int blankline = 1;
>>
>> So keep this...
>>
>> >  	const char *object_ref;
>> >G  	struct notes_tree *t;
>> >  	struct object_id object, new_note;
>> > @@ -584,6 +585,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>> >  			parse_reuse_arg),
>> >  		OPT_BOOL(0, "allow-empty", &allow_empty,
>> >  			N_("allow storing empty note")),
>> > +		OPT_BOOL(0, "blank-line", &blankline,
>>
>> ...and just make this "no-blank-line", and parse_options() will do the
>> right thing. I.e. PARSE_OPT_NONEG is implied.
>
> Sorry for another question. By the explanation of "api-parse-options.txt" :
>
> `OPT_BOOL(short, long, &int_var, description)`::
> 	Introduce a boolean option. `int_var` is set to one with
> 	`--option` and set to zero with `--no-option`
>
> I think it means the same as "parse_options() will do right thing" as you said
> to me , but...after the modification the effect is opposite:
>
> diff --git a/builtin/notes.c b/builtin/notes.c
> index a6273781fb8..73427ea8dff 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -562,6 +562,7 @@ static int copy(int argc, const char **argv, const char *prefix)
>  static int append_edit(int argc, const char **argv, const char *prefix)
>  {
>         int allow_empty = 0;
> +       int blankline = 1;
>         const char *object_ref;
>         struct notes_tree *t;
>         struct object_id object, new_note;
> @@ -584,6 +585,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>                         parse_reuse_arg),
>                 OPT_BOOL(0, "allow-empty", &allow_empty,
>                         N_("allow storing empty note")),
> +               OPT_BOOL(0, "no-blank-line", &blankline,
> +                       N_("insert paragraph break before appending to an existing note")),
>                 OPT_END()
>         };
>         int edit = !strcmp(argv[0], "edit");
> @@ -618,7 +621,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>                 enum object_type type;
>                 char *prev_buf = read_object_file(note, &type, &size);
>
> -               if (d.buf.len && prev_buf && size)
> +               if (blankline && d.buf.len && prev_buf && size)
>                         strbuf_insertstr(&d.buf, 0, "\n");
>                 if (prev_buf && size)
>                         strbuf_insert(&d.buf, 0, prev_buf, size);
>
> ----
> So, I am a bit confused and I guess maybe somewhere I misunderstood or didn't
> notice some details.
>
> Thanks.

> Sorry, I meant that in both cases it will expose the same options to the
> user: --blank-line and --no-blank-line. I.e. if you create options
> named:
>
> 	"x" "x-y"
>
> Their negations are: --no-x and --no-x-y. But if their names are:
>
> 	"x" "no-x"
>
> The negations are:
>
> 	--no-x and --x
>
> But as your example shows that's unrelated to whether the *variable in
> the code* is negated.
>
> So however you structure the code, which would be:
>
> 	int blankline = 1:
>         [...]
> 	OPT_BOOL(0, "blankline", &blankline, [...]);
>
> Or:
>
> 	int no_blankline = 0:
>         [...]
> 	OPT_BOOL(0, "no-blankline", &no_blankline, [...]);
>
> The documentation could in both cases say:
>
> 	--no-blankline:
> 		describe the non-default[...]

> From: Teng Long <dyroneteng@gmail.com>
>
> When appending to a given object which has note and if the appended
> note is not empty too, we will insert a blank line at first. The
> blank line serves as a split line, but sometimes we just want to
> omit it then append on the heels of the target note. So, we add
> a new "OPT_BOOL()" option to determain whether a new blank line
> is insert at first.

> From: Teng Long <dyroneteng@gmail.com>
>
> When "git notes append <object>" is executed, if there is no note in
> the given object and the appended note is empty, the command line
> prompt will be as follows:
>
>      "Removing note for object <object>"
>
> Actually, this tip is not that accurate, because there is no note in
> the original <object>, and it also does no remove work on the notes
> reference, so let's fix this and give the correct tip.
>
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  builtin/notes.c  | 17 ++++++++++++++---
>  t/t3301-notes.sh |  3 ++-
>  2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/notes.c b/builtin/notes.c
> index f0fa337e8c..02b88e54d8 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -567,9 +567,15 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>  	struct notes_tree *t;
>  	struct object_id object, new_note;
>  	const struct object_id *note;
> -	char *logmsg;
> +	char *logmsg = NULL;
>  	const char * const *usage;
> -	struct note_data d = { 0, 0, NULL, STRBUF_INIT };
> +	struct note_data d = {
> +		.given = 0,
> +		.use_editor = 0,
> +		.edit_path = NULL,

> > @@ -567,9 +567,15 @@ static int append_edit(int argc, const char **argv, const char *prefix)
> >  	struct notes_tree *t;
> >  	struct object_id object, new_note;
> >  	const struct object_id *note;
> > -	char *logmsg;
> > +	char *logmsg = NULL;
> >  	const char * const *usage;
> > -	struct note_data d = { 0, 0, NULL, STRBUF_INIT };
> > +	struct note_data d = {
> > +		.given = 0,
> > +		.use_editor = 0,
> > +		.edit_path = NULL,
>
> Most of this is an unrelated "use designated init" cleanup, which is
> good, but let's do that in a different commit if we need it. And then
> you can drop all of these...
>
> > +		.buf = STRBUF_INIT
>
> ...and only need to keep this one.

> On Mon, Nov 07, 2022 at 03:40:03PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> > @@ -567,9 +567,15 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>> >  	struct notes_tree *t;
>> >  	struct object_id object, new_note;
>> >  	const struct object_id *note;
>> > -	char *logmsg;
>> > +	char *logmsg = NULL;
>> >  	const char * const *usage;
>> > -	struct note_data d = { 0, 0, NULL, STRBUF_INIT };
>> > +	struct note_data d = {
>> > +		.given = 0,
>> > +		.use_editor = 0,
>> > +		.edit_path = NULL,
>>
>> Most of this is an unrelated "use designated init" cleanup, which is
>> good, but let's do that in a different commit if we need it. And then
>> you can drop all of these...
>>
>> > +		.buf = STRBUF_INIT
>>
>> ...and only need to keep this one.
>
> I don't mind seeing the cleanup here, but I don't see why we need to
> keep that portion of the hunk at all for this series.
>
> IOW, your "...and only need to keep this one" does not make sense to me.

> > I don't mind seeing the cleanup here, but I don't see why we need to
> > keep that portion of the hunk at all for this series.
> >
> > IOW, your "...and only need to keep this one" does not make sense to me.
>
> I was still on my initial read-through, and was assuming that it was a
> prep change for the 3/3 adding a new field, before I saw the 3/3...

> From: Teng Long <dyroneteng@gmail.com>
>
> Situation of removing note shouldn't happen in "append_edit()",
> unless it's a bug. So, let's drop the unreachable "else" code
> in "append_edit()".
>
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  builtin/notes.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 02b88e54d8..6d42592c79 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -10,6 +10,7 @@
>  #include "cache.h"
>  #include "config.h"
>  #include "builtin.h"
> +#include "git-compat-util.h"

>  #include "notes.h"
>  #include "object-store.h"
>  #include "repository.h"
> @@ -646,11 +647,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>  			_("Both original and appended notes are empty in %s, do nothing\n"),
>  			oid_to_hex(&object));
>  	} else {
> -		fprintf(stderr, _("Removing note for object %s\n"),
> -			oid_to_hex(&object));
> -		remove_note(t, object.hash);
> -		logmsg = xstrfmt("Notes removed by 'git notes %s'", argv[0]);
> -		commit_notes(the_repository, t, logmsg);
> +		BUG("compute_notes failed");
>  	}

> From: Teng Long <dyroneteng@gmail.com>
> [...]
> * drop unreachable code in "append_edit()". Ævar found that some code has been
> unreachable in patch v1. I think it's because, after the commit "notes.c: fixed
> tip when target and append note are both empty", for example in this patch, the
> situation of "removing an existing note" should be impossible unless a BUG when
> trying to do append. The tests are passed, but I'm not sure I fully understand
> the original design.

> > From: Teng Long <dyroneteng@gmail.com>
> > [...]
> > * drop unreachable code in "append_edit()". Ævar found that some code has been
> > unreachable in patch v1. I think it's because, after the commit "notes.c: fixed
> > tip when target and append note are both empty", for example in this patch, the
> > situation of "removing an existing note" should be impossible unless a BUG when
> > trying to do append. The tests are passed, but I'm not sure I fully understand
> > the original design.
>
> I suggested squashing that BUG() in 3/3 into 2/3, but reading this again
> I think it should come first.
>
> I.e. this seems to me like the code in cd067d3bf4e (Builtin-ify
> git-notes, 2010-02-13) might have just been blindly carried forward to
> both "create" and "edit" in 52694cdabbf (builtin/notes: split
> create_note() to clarify add vs. remove logic, 2014-11-12).
>
> But it would be good to have confirmation, e.g. if you check out
> 52694cdabbf and remove that "Removing note" branch from add() does it
> fail tests at the time, but not in the case of append_edit()?

> > From: Teng Long <dyroneteng@gmail.com>
> > [...]
> > * drop unreachable code in "append_edit()". Ævar found that some code has been
> > unreachable in patch v1. I think it's because, after the commit "notes.c: fixed
> > tip when target and append note are both empty", for example in this patch, the
> > situation of "removing an existing note" should be impossible unless a BUG when
> > trying to do append. The tests are passed, but I'm not sure I fully understand
> > the original design.
>
> I suggested squashing that BUG() in 3/3 into 2/3, but reading this again
> I think it should come first.
>
> I.e. this seems to me like the code in cd067d3bf4e (Builtin-ify
> git-notes, 2010-02-13) might have just been blindly carried forward to
> both "create" and "edit" in 52694cdabbf (builtin/notes: split
> create_note() to clarify add vs. remove logic, 2014-11-12).
>
> But it would be good to have confirmation, e.g. if you check out
> 52694cdabbf and remove that "Removing note" branch from add() does it
> fail tests at the time, but not in the case of append_edit()?

> Teng Long (5):
>   notes.c: cleanup 'strbuf_grow' call in 'append_edit'
>   notes.c: cleanup for "designated init" and "char ptr init"
>   notes.c: drop unreachable code in 'append_edit()'
>   notes.c: provide tips when target and append note are both empty
>   notes.c: introduce "--no-blank-line" option

>> Teng Long (5):
>>   notes.c: cleanup 'strbuf_grow' call in 'append_edit'
>>   notes.c: cleanup for "designated init" and "char ptr init"
>>   notes.c: drop unreachable code in 'append_edit()'
>>   notes.c: provide tips when target and append note are both empty
>>   notes.c: introduce "--no-blank-line" option
>
> I'm not sure if this patch series should continue, and if there are no
> updated comments it will be temporarily suspended.
>
> Thanks for the reviews on the past patches.

> Teng Long <dyroneteng@gmail.com> writes:
>
> >> Teng Long (5):
> >>   notes.c: cleanup 'strbuf_grow' call in 'append_edit'
> >>   notes.c: cleanup for "designated init" and "char ptr init"
> >>   notes.c: drop unreachable code in 'append_edit()'
> >>   notes.c: provide tips when target and append note are both empty
> >>   notes.c: introduce "--no-blank-line" option
> >
> > I'm not sure if this patch series should continue, and if there are no
> > updated comments it will be temporarily suspended.
> >
> > Thanks for the reviews on the past patches.
>
> Taylor, This topic was marked to "expect" a reroll in the second
> issue of November "What's cooking" report you did.  Do you recall
> what remaining works there were?

> I personally do not have much opinion on this topic, other than that
> "--no-blank-link" would be a horrible name (i.e. uses concrete words
> to pretend that it clearly describes what it does, but is utterly
> unclear where these blank lines are etc.) for the feature to help
> end-users discover it.

> Junio C Hamano <gitster@pobox.com> writes:
>
>> I personally do not have much opinion on this topic, other than that
>> "--no-blank-link" would be a horrible name (i.e. uses concrete words
>> to pretend that it clearly describes what it does, but is utterly
>> unclear where these blank lines are etc.) for the feature to help
>> end-users discover it.
>
> I have some a candidates might like '--newline' and '--no-newline', or
> 'splitted-newline' or 'no-splitted-newline', but I think the latter is
> a little long.
>
> Thanks.

> but the point is "blank-line" or "newline" does not say which
> newline in the resulting notes object you are mucking with.  It is
> not like in this example:
>
>  $ git notes add -m "title of the note"
>  $ git notes append --no-blank-line -m "body of the note
>
>  that span multiple
>
>  lines" HEAD
>
> you are removving the blank lines embedded in the body of the
> message, but from the option name, it is hard to guess that.

> Junio C Hamano <gitster@pobox.com> writes:
>
> > but the point is "blank-line" or "newline" does not say which
> > newline in the resulting notes object you are mucking with.  It is
> > not like in this example:
> > [...]
> > you are removving the blank lines embedded in the body of the
> > message, but from the option name, it is hard to guess that.
>
> Fine, I see. Maybe "insert-starting-newline" or "insert-initial-newline". If we
> could not find a suitable naming for this, it's ok for me to hang up [5/5] (a
> little struggle for me to find a better name for this now in fact (⊙ˍ⊙).),
> because except it, the [4/5] still an active bugfix in my opinion.

> Taking a step back, perhaps think of this in terms of "separator". The
> default behavior is to insert "\n" as a separator between notes. If
> you add a --separator option, then users could supply their own
> separator, such as "----\n" or, in your case, "" to suppress the blank
> line.

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> Taking a step back, perhaps think of this in terms of "separator". The
>> default behavior is to insert "\n" as a separator between notes. If
>> you add a --separator option, then users could supply their own
>> separator, such as "----\n" or, in your case, "" to suppress the blank
>> line.
>
> Your idea is an enhancement of the original one of me. I think it's a suitable
> name and I could implement it, maybe we could hear about Junio's advice of the
> naming?

> Taking a step back, perhaps think of this in terms of "separator". The
> default behavior is to insert "\n" as a separator between notes. If
> you add a --separator option, then users could supply their own
> separator, such as "----\n" or, in your case, "" to suppress the blank
> line.

> Eric Sunshine <sunshine@sunshineco.com> writes:
> > Taking a step back, perhaps think of this in terms of "separator". The
> > default behavior is to insert "\n" as a separator between notes. If
> > you add a --separator option, then users could supply their own
> > separator, such as "----\n" or, in your case, "" to suppress the blank
> > line.
>
> There is another question for me, if the separator we passed contains "\n"
> string , the argument the cmd receives will need to tranfer to '\n' character
> instead to make sure it's a linebreak but not a "\n" instead.
>
> So maybe like:
> +static void insert_separator(struct strbuf *message, const char *separator)
> +{
> +               while (*separator) {
> +                       if (*separator == '\\'){
> +                               switch (separator[1]) {
> +                                       case 'n':
> +                                               strbuf_addstr(&transfered, "\n");
> +                                               separator++;
> +                                               break;
> + [...]
> +                       separator++;
> +               }
> +}
>
> If the above is understood correctly, is there an api that handles escape
> characters already in the existing code (I haven't found one so far, so I need
> to confirm and replace it if there is one). In addition, the insert_separator
> function above handles three special characters \t\n\r. Do we need more?

> Let's cleanup the unnecessary 'strbuf_grow' call in 'append_edit'. This
> "strbuf_grow(&d.buf, size + 1);" is prepared for insert a blank line if
> needed, but actually when inserting, "strbuf_insertstr(&d.buf, 0,
> "\n");" will do the "grow" for us.
>
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
> diff --git a/builtin/notes.c b/builtin/notes.c
> @@ -618,7 +618,6 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>                 char *prev_buf = read_object_file(note, &type, &size);
>
> -               strbuf_grow(&d.buf, size + 1);
>                 if (d.buf.len && prev_buf && size)
>                         strbuf_insertstr(&d.buf, 0, "\n");

> On Wed, Jan 11, 2023 at 9:48 PM Teng Long <dyroneteng@gmail.com> wrote:
> > Let's cleanup the unnecessary 'strbuf_grow' call in 'append_edit'. This
> > "strbuf_grow(&d.buf, size + 1);" is prepared for insert a blank line if
> > needed, but actually when inserting, "strbuf_insertstr(&d.buf, 0,
> > "\n");" will do the "grow" for us.
> >
> > Signed-off-by: Teng Long <dyroneteng@gmail.com>
> > ---
> > diff --git a/builtin/notes.c b/builtin/notes.c
> > @@ -618,7 +618,6 @@ static int append_edit(int argc, const char **argv, const char *prefix)
> >                 char *prev_buf = read_object_file(note, &type, &size);
> >
> > -               strbuf_grow(&d.buf, size + 1);
> >                 if (d.buf.len && prev_buf && size)
> >                         strbuf_insertstr(&d.buf, 0, "\n");
>
> Indeed, it's not clear why that was there in the first place. Digging
> through history doesn't shed any light on it. It was introduced by
> 2347fae50b (builtin-notes: Add "append" subcommand for appending to
> note objects, 2010-02-13)[1], but there's no explanation as to why it
> was coded that way. Best guess may be that the author originally
> inserted "\n" manually by direct manipulation of the strbuf rather
> than employing a strbuf function, but then switched to strbuf_insert()
> before submitting the series and forgot to remove the now-unnecessary
> strbuf_grow().

> From: Teng Long <dyroneteng@gmail.com>
>
> Let's do some cleanup for the following two places in "append_edit()".
>
> The first place is "char *logmsg;" need to be initialized with NULL.
> The second place is "struct note_data d = { 0, 0, NULL, STRBUF_INIT };"
> could be replaced with designated init format.
>
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  builtin/notes.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/notes.c b/builtin/notes.c
> index e57f024824..8ca55ec83e 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -566,9 +566,9 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>  	struct notes_tree *t;
>  	struct object_id object, new_note;
>  	const struct object_id *note;
> -	char *logmsg;
> +	char *logmsg = NULL;

> > From: Teng Long <dyroneteng@gmail.com>
> >
> > Let's do some cleanup for the following two places in "append_edit()".
> >
> > The first place is "char *logmsg;" need to be initialized with NULL.
> > The second place is "struct note_data d = { 0, 0, NULL, STRBUF_INIT };"
> > could be replaced with designated init format.
> >
> > Signed-off-by: Teng Long <dyroneteng@gmail.com>
> > ---
> >  builtin/notes.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/builtin/notes.c b/builtin/notes.c
> > index e57f024824..8ca55ec83e 100644
> > --- a/builtin/notes.c
> > +++ b/builtin/notes.c
> > @@ -566,9 +566,9 @@ static int append_edit(int argc, const char **argv, const char *prefix)
> >  	struct notes_tree *t;
> >  	struct object_id object, new_note;
> >  	const struct object_id *note;
> > -	char *logmsg;
> > +	char *logmsg = NULL;
>
> This change isn't needed, and the compiler will check that we have it
> init'd.
>
> It *is* needed when combined with your 3/5, but even then it's the wrong
> solution. In that change you move the commit_notes() into that "if"
> branch that needs the "logmsg", and it's correct that you need to
> NULL-init it to safely pass it to free().

> But let's just declare the "logmsg" in that if block then, and move the
> free() over there, that reduces the scope it's in.
>
> >  	const char * const *usage;
> > -	struct note_data d = { 0, 0, NULL, STRBUF_INIT };
> > +	struct note_data d = { .buf = STRBUF_INIT };
>
> This change is good, but then let's change the other such case in this
> file to a designated init too, and make the commit message etc. be about
> using designated init here.

> From: Teng Long <dyroneteng@gmail.com>
>
> When "git notes append <object>" is executed, if there is no note in
> the given object and the appended note is empty too, we could print
> the exact tips to end-user.
>
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  builtin/notes.c  | 5 ++++-
>  t/t3301-notes.sh | 5 +++--
>  2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/notes.c b/builtin/notes.c
> index b71a81bff1..f2efb3736c 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -631,7 +631,10 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>  			BUG("combine_notes_overwrite failed");
>  		logmsg = xstrfmt("Notes added by 'git notes %s'", argv[0]);
>  		commit_notes(the_repository, t, logmsg);
> -	}
> +	} else if (!d.buf.len && !note)
> +		fprintf(stderr,
> +			_("Both original and appended notes are empty in %s, do nothing\n"),
> +			oid_to_hex(&object));

> When "git notes append <object>" is executed, if there is no note in
> the given object and the appended note is empty too, we could print
> the exact tips to end-user.
>
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
> diff --git a/builtin/notes.c b/builtin/notes.c
> @@ -631,7 +631,10 @@ static int append_edit(int argc, const char **argv, const char *prefix)
> +       } else if (!d.buf.len && !note)
> +               fprintf(stderr,
> +                       _("Both original and appended notes are empty in %s, do nothing\n"),
> +                       oid_to_hex(&object));

> Situation of note "removing" shouldn't happen in 'append_edit()',
> unless it's a bug. So, let's drop the unreachable "else" code
> in "append_edit()".
>
> The notes operation "append" is different with "add", the latter
> supports to overwrite the existing note then let the "removing"
> happen (e.g. execute `git notes add -f -F /dev/null` on an existing
> note), but the former will not because it only does "appends" but
> not doing "overwrites".
>
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
> diff --git a/builtin/notes.c b/builtin/notes.c
> @@ -630,13 +630,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>                 if (add_note(t, &object, &new_note, combine_notes_overwrite))
>                         BUG("combine_notes_overwrite failed");
>                 logmsg = xstrfmt("Notes added by 'git notes %s'", argv[0]);
> -       } else {
> -               fprintf(stderr, _("Removing note for object %s\n"),
> -                       oid_to_hex(&object));
> -               remove_note(t, object.hash);
> -               logmsg = xstrfmt("Notes removed by 'git notes %s'", argv[0]);
> +               commit_notes(the_repository, t, logmsg);
>         }
> -       commit_notes(the_repository, t, logmsg);

> On Wed, Jan 11, 2023 at 9:48 PM Teng Long <dyroneteng@gmail.com> wrote:
> > -       } else {
> > -               fprintf(stderr, _("Removing note for object %s\n"),
> > -                       oid_to_hex(&object));
> > -               remove_note(t, object.hash);
> > -               logmsg = xstrfmt("Notes removed by 'git notes %s'", argv[0]);
> > +               commit_notes(the_repository, t, logmsg);
> >         }
> > -       commit_notes(the_repository, t, logmsg);
>
> This change breaks removal of notes using "git notes edit". Prior to
> this change, if you delete the content of a note using "git notes
> edit", then the note is removed. Following this change, the note
> remains, which contradicts documented[1] behavior.

> > Situation of note "removing" shouldn't happen in 'append_edit()',
> > unless it's a bug. So, let's drop the unreachable "else" code
> > in "append_edit()".
> >
> > The notes operation "append" is different with "add", the latter
> > supports to overwrite the existing note then let the "removing"
> > happen (e.g. execute `git notes add -f -F /dev/null` on an existing
> > note), but the former will not because it only does "appends" but
> > not doing "overwrites".
> >
> > Signed-off-by: Teng Long <dyroneteng@gmail.com>
> > ---
> > diff --git a/builtin/notes.c b/builtin/notes.c
> > @@ -630,13 +630,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)
> >                 if (add_note(t, &object, &new_note, combine_notes_overwrite))
> >                         BUG("combine_notes_overwrite failed");
> >                 logmsg = xstrfmt("Notes added by 'git notes %s'", argv[0]);
> > -       } else {
> > -               fprintf(stderr, _("Removing note for object %s\n"),
> > -                       oid_to_hex(&object));
> > -               remove_note(t, object.hash);
> > -               logmsg = xstrfmt("Notes removed by 'git notes %s'", argv[0]);
> > +               commit_notes(the_repository, t, logmsg);
> >         }
> > -       commit_notes(the_repository, t, logmsg);
>
> This change breaks removal of notes using "git notes edit". Prior to
> this change, if you delete the content of a note using "git notes
> edit", then the note is removed. Following this change, the note
> remains, which contradicts documented[1] behavior.
>
> [1]: Unfortunately, perhaps, this behavior is documented under the
> "remove" subcommand rather than the "edit" subcommand, but it is
> nevertheless documented and has worked this way for ages, so this
> patch causes a regression.

> Eric Sunshine writes:
> > > Situation of note "removing" shouldn't happen in 'append_edit()',
> > > unless it's a bug. So, let's drop the unreachable "else" code
> > > in "append_edit()".
> > >
> > > Signed-off-by: Teng Long <dyroneteng@gmail.com>
> > > ---
> > > -       } else {
> > > -               fprintf(stderr, _("Removing note for object %s\n"),
> > > -                       oid_to_hex(&object));
> > > -               remove_note(t, object.hash);
> > > -               logmsg = xstrfmt("Notes removed by 'git notes %s'", argv[0]);
> > > +               commit_notes(the_repository, t, logmsg);
> > >         }
> > > -       commit_notes(the_repository, t, logmsg);
> >
> > This change breaks removal of notes using "git notes edit". Prior to
> > this change, if you delete the content of a note using "git notes
> > edit", then the note is removed. Following this change, the note
> > remains, which contradicts documented[1] behavior.
>
> As the commit msg describes, the subcommands I understand should have clear
> responsibilities as possible (documentaion may have some effect in my mind). So,
> the removal opertion under "append subcommand" here is little wired to me, but
> your suggestion makes sense, this may have compatibility issues. Although I
> think it's weird that someone would use this in the presence of the remove
> subcommand, and my feeling is that this code is actually copied from the add
> method (introduced by 52694cdabbf68f19c8289416e7bb3bbef41d8d27), but I'm not
> sure.
>
> So, it's ok for me to drop this one.

> It's unfortunate, perhaps, that the git-notes command-set is not
> orthogonal, but it's another case of historic accretion. According to
> the history, as originally implemented, git-notes only supported two
> commands, "edit" and "show", and "edit" was responsible for _all_
> mutation operations, including deletion. git-notes only grew a more
> complete set of commands a couple years later.
>
> At any rate, even though the original behaviors may not make as much
> sense these days now that git-notes has a more complete set of
> commands, we still need to be careful not to break existing workflows
> and scripts (tooling). That's why it would be nice to beef up the
> tests so that the existing behaviors don't get broken by changes such
> as this patch wants to make. But, as mentioned earlier, the additional
> tests probably shouldn't be part of this series, but rather can be
> done as a separate series (by someone; it doesn't have to be you).

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

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

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

> +
>  --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')

> +		/* 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");

> +}
> +
> +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);
> +	}

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

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

> 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";

> @@ -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";

> +       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);
> +}

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

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

> +       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" &&

> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  builtin/notes.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 80d9dfd2..23cb6f0d 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -215,7 +215,6 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
>  
>  	BUG_ON_OPT_NEG(unset);
>  
> -	strbuf_grow(&d->buf, strlen(arg) + 2);
>  	if (d->buf.len)
>  		strbuf_addch(&d->buf, '\n');
>  	strbuf_addstr(&d->buf, arg);
> @@ -618,7 +617,6 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>  		enum object_type type;
>  		char *prev_buf = read_object_file(note, &type, &size);
>  
> -		strbuf_grow(&d.buf, size + 1);
>  		if (d.buf.len && prev_buf && size)
>  			strbuf_insertstr(&d.buf, 0, "\n");
>  		if (prev_buf && size)

> > Best guess may be that the author originally inserted "\n" manually by
> > direct manipulation of the strbuf rather than employing a strbuf
> > function, but then switched to strbuf_insert() before submitting the
> > series and forgot to remove the now-unnecessary strbuf_grow().
>
> Please do not speculate when you do not have to.  "git blame" is
> your friend.

> 348f199b (builtin-notes: Refactor handling of -F option to allow
> combining -m and -F, 2010-02-13) added these to mimic the code
> introduced by 2347fae5 (builtin-notes: Add "append" subcommand for
> appending to note objects, 2010-02-13) that reads in previous note
> before the message.  And the resulting code with explicit sizing is
> carried to this day.
>
> In the context of reading an existing note in, exact sizing may have
> made sense, but because the resulting note needs cleansing with
> stripspace() when appending with this option, such an exact sizing
> does not buy us all that much in practice.
>
> It may help avoiding overallocation due to ALLOC_GROW() slop, but
> nobody can feed so many long messages for it to matter from the
> command line.

>  builtin/notes.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 23cb6f0d..553ae2bd 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -401,7 +401,7 @@ static int add(int argc, const char **argv, const char *prefix)
>  	struct notes_tree *t;
>  	struct object_id object, new_note;
>  	const struct object_id *note;
> -	struct note_data d = { 0, 0, NULL, STRBUF_INIT };
> +	struct note_data d = { .buf = STRBUF_INIT };
>  	struct option options[] = {
>  		OPT_CALLBACK_F('m', "message", &d, N_("message"),
>  			N_("note contents as a string"), PARSE_OPT_NONEG,
> @@ -567,7 +567,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>  	const struct object_id *note;
>  	char *logmsg;
>  	const char * const *usage;
> -	struct note_data d = { 0, 0, NULL, STRBUF_INIT };
> +	struct note_data d = { .buf = STRBUF_INIT };
>  	struct option options[] = {
>  		OPT_CALLBACK_F('m', "message", &d, N_("message"),
>  			N_("note contents as a string"), PARSE_OPT_NONEG,

> Sometimes, we want to use a specified <separator> 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.

> In addition, if a user specifies multple "-m" with "--separator", the
> separator should be inserted between the messages too, so we use
> OPT_STRING_LIST instead of OPT_CALLBACK_F to parse "-m" option, make
> sure the option value of "--separator" been parsed already when we need
> it.

> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
> index efbc10f0..5abe6092 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>]

> @@ -86,7 +86,9 @@ 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 and the
> +	message are not empty, "\n" will be inserted between them.
> +	Use the `--separator` option to insert other delimiters.

> > 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.
>
> ", which as the separator" does not sound grammatically correct.
> Without that part, the above is a perfectly readable description of
> the current behaviour.

> > Sometimes, we want to use a specified <separator> 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.
>
> In a way compatible to what?  I think s/compatibly// would be even
> easier to read.  I think you are doing that for convenience.

> >     * --separator='------\n': we will insert as-is because it contains
> >     the line break at last.
>
> If this behaviour gets spelled out like this, it needs to be
> justified.
>
> After seeing that "------" gives the user these dashes on its own
> single line, wouldn't a natural expectation by the user be to see a
> line with dashes, followed by a blank line, if you give "------\n"?
> How do you justify removal of that newline in a way that is easy to
> understand to readers?
>
> I am not saying that you should allow --separator="---\n\n\n" to
> give three blank lines between paragraphs. I think it makes sense to
> keep the stripspace in the code after a paragraph gets added. I just
> prefer to see it done as our design choice, not "because there is
> stripspace that removes them", i.e. what the code happens to do.

> >     * not specified --separator option: will use '\n' as the separator
> >     when do appending and this is the default behavour.
>
> s/not specified --separator option/no --separator option/; the way
> you phrased can be misread for
>

> 	git notes --separator -m foo -m bar
>
> i.e. any additional specifics is not given but --separator is still
> on the command line.  But I do not think you meant that---rather
> this entry is what happens by default, i.e. a blank line separates
> each paragraph.

> >     * --separator='': we specify an empty separator which has the same
> >     behavour with --separator='\n' and or not specified the option.
>
> I do not quite see why it is necessary to spell this out.  Isn't
> this a natural consequence of the first one (i.e. "six dashes
> without any terminating LF gets a line with dashes plus LF"
> naturally extends to "0 dashes without any terminating LF gets a
> blank line")?

> > In addition, if a user specifies multple "-m" with "--separator", the
> > separator should be inserted between the messages too, so we use
> > OPT_STRING_LIST instead of OPT_CALLBACK_F to parse "-m" option, make
> > sure the option value of "--separator" been parsed already when we need
> > it.
>
> This is hard to grok.  Is it an instruction to whoever is
> implementing this new feature, or is it an instruct to end-users
> telling that they need to give --separator before they start giving
> -m <msg>, -F <file>, -c <object>, etc.?

> > diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
> > index efbc10f0..5abe6092 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>]
>
> Doesn't add allow you to say
>
> 	$ git notes add -m foo -m bar
>
> Shouldn't it also honor --separator to specify an alternate
> paragraph break?

> > @@ -86,7 +86,9 @@ 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 and the
> > +	message are not empty, "\n" will be inserted between them.
> > +	Use the `--separator` option to insert other delimiters.
>
> "\n" is so, ... programmer lingo?  "A blank line" is inserted
> between these paragraphs.

> > +--separator <separator>::
> > +	The '<separator>' inserted between the note and message
> > +	by 'append', "\n" by default. A custom separator can be
>
> I see no reason to single out 'append'; "add -m <msg> -m <msg>"
> follows exactly the same paragraph concatenation logic in the
> current code, no?  If we allow customized paragraph separators,
> we should use the same logic there as well.

> It probably is simpler to explain if you treat the "current note in
> 'append'" as if the text were just "earlier paragraphs", to which
> more paragraphs taken from each -m <msg> and -F <file> are
> concatenated, with paragraph break before each of them.  In the case
> of 'add', there happens to be zero "earlier paragraphs", but
> everything else gets concatenated the same way, no?

>> > In addition, if a user specifies multple "-m" with "--separator", the
>> > separator should be inserted between the messages too, so we use
>> > OPT_STRING_LIST instead of OPT_CALLBACK_F to parse "-m" option, make
>> > sure the option value of "--separator" been parsed already when we need
>> > it.
>>
>> This is hard to grok.  Is it an instruction to whoever is
>> implementing this new feature, or is it an instruct to end-users
>> telling that they need to give --separator before they start giving
>> -m <msg>, -F <file>, -c <object>, etc.?
>
> No, it's not the order of the user give, but the backend we deal.
>
> We use "parse_msg_arg" as a callback when parsing "-m " by OPT_CALLBACK_F,
> so if we have to read the separator before we parse it, so we could insert
> it correctly between the messages, So I use OPT_STRING_LIST instead.

> > We use "parse_msg_arg" as a callback when parsing "-m " by OPT_CALLBACK_F,
> > so if we have to read the separator before we parse it, so we could insert
> > it correctly between the messages, So I use OPT_STRING_LIST instead.
>
> That is an implementation detail of how you chose to implement the
> feature, and not an inherent limitation, is it?  It makes a lame
> excuse to give users a hard-to-use UI.

> For example, we could parse all the command line parameters without
> making any action other than recording the strings given to -m and
> contents of files given via -F in the order they appeared on the
> command line, all in a single string list, while remembering the
> last value of --separator you got, and then at the end concatenate
> these strings using the final value of the separator, no?

> Junio C Hamano <gitster@pobox.com> writes:
>
>> > We use "parse_msg_arg" as a callback when parsing "-m " by OPT_CALLBACK_F,
>> > so if we have to read the separator before we parse it, so we could insert
>> > it correctly between the messages, So I use OPT_STRING_LIST instead.
>>
>> That is an implementation detail of how you chose to implement the
>> feature, and not an inherent limitation, is it?  It makes a lame
>> excuse to give users a hard-to-use UI.
>
>> For example, we could parse all the command line parameters without
>> making any action other than recording the strings given to -m and
>> contents of files given via -F in the order they appeared on the
>> command line, all in a single string list, while remembering the
>> last value of --separator you got, and then at the end concatenate
>> these strings using the final value of the separator, no?
>
> Yes, please let'me clarify it, we shouldn't to give users a hard-to-use UI, so:
>
>      1. order of "-m" and "-F" matters, it determine the order of the paragraphs
>      (remain the same as before, which I need to fix in next patch).
>
>      2. order of "-m""-F" and "--separator" doesn't matter.
>
> Thanks.