[v2,1/3] notes.c: introduce "--blank-line" option

Commit Message

Teng Long Nov. 7, 2022, 1:57 p.m. UTC

From: Teng Long <[e-mail]>

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 <[e-mail]>
---
 Documentation/git-notes.txt | 11 +++++++++--
 builtin/notes.c             |  5 ++++-
 t/t3301-notes.sh            | 12 ++++++++++++
 3 files changed, 25 insertions(+), 3 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Nov. 7, 2022, 2:45 p.m. UTC | #1

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 <[e-mail]>
> ---
>  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).

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.

>  	int allow_empty = 0;
> +	int blankline = 1;

So keep this...

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

...and just make this "no-blank-line", and parse_options() will do the
right thing. I.e. PARSE_OPT_NONEG is implied.

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

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

This should be a test_when_finished "", for the previous test, otherwise
this one will presumably fail if you use the "wrong" --run="" arguments
to skip the last test.

Ævar Arnfjörð Bjarmason Nov. 7, 2022, 3:06 p.m. UTC | #2

On Mon, Nov 07 2022, Teng Long wrote:

> From: Teng Long <[e-mail]>

Something I didn't notice on the first reading:

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

Here we're growing it to add the size plus the \n.

> -		if (d.buf.len && prev_buf && size)
> +		if (blankline && d.buf.len && prev_buf && size)
>  			strbuf_insertstr(&d.buf, 0, "\n");

But now we're not going to make use of that "+ 1" anymore...

>  		if (prev_buf && size)
>  			strbuf_insert(&d.buf, 0, prev_buf, size);

..and here the prev_buf && size condition is duplicated.

I think the right thin to do is to just drop the strbuf_grow() here
altogether, since strbuf_insert() will handle it for us, but that would
make sense before this change, so maybe in pre-cleanup?

Eric Sunshine Nov. 7, 2022, 3:45 p.m. UTC | #3

On Mon, Nov 7, 2022 at 9:56 AM Ævar Arnfjörð Bjarmason <[e-mail]> 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 <[e-mail]>
> > ---
> > 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/

Ævar Arnfjörð Bjarmason Nov. 7, 2022, 5:22 p.m. UTC | #4

On Mon, Nov 07 2022, Eric Sunshine wrote:

> On Mon, Nov 7, 2022 at 9:56 AM Ævar Arnfjörð Bjarmason <[e-mail]> 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 <[e-mail]>
>> > ---
>> > 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.

I.e. both concerns can be satisfied, but whether my suggestion is
sensible UX is another matter...

Taylor Blau Nov. 7, 2022, 9:46 p.m. UTC | #5

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 <[e-mail]> 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 <[e-mail]>
> >> > ---
> >> > 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.

We can clearly support '--[no-]blank-line' and allow latter arguments to
override previous ones. The documentation is fine as-is.

Thanks,
Taylor

Taylor Blau Nov. 7, 2022, 9:47 p.m. UTC | #6

On Mon, Nov 07, 2022 at 09:57:03PM +0800, Teng Long wrote:
> From: Teng Long <[e-mail]>
>
> 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.

The discussion about whether this should support '--blank-line' or just
be '--no-blank-line' aside, I'm curious what motivated this change?

Thanks,
Taylor

Ævar Arnfjörð Bjarmason Nov. 7, 2022, 10:36 p.m. UTC | #7

On Mon, Nov 07 2022, Taylor Blau wrote:

> 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 <[e-mail]> 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 <[e-mail]>
>> >> > ---
>> >> > 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.

I see from re-reading my reply that I was conflating two things. What I
*meant* to suggest is this:

When an option is not the default, and we provide a way to turn it off
we usually document that as:

	--no-foo:
		Don't do foo.

See e.g. "git commit --no-edit", and "git clone --no-checkout".

But this is orthagonal to what you call the option in the source, and
whether your variable is "inverted". I.e. in both those cases we have a
"--edit" and "--checkout", but when we prepare the options for
parse_options() we do (or the equivalent of):

	int no_checkout = 0;
	OPT_BOOL('n', "no-checkout", &option_no_checkout,

And:

	int edit = 1;
	OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),

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.

> We can clearly support '--[no-]blank-line' and allow latter arguments to
> override previous ones.

We'll support both either way, i.e. parse_options() detects that the
name starts with "no-", so the negation of a "no-checkout" isn't
"--no-no-checkout", but a "--checkout".

> The documentation is fine as-is.

I think the above form would make it a bit better, but just my 0.02:

	--no-blank-line::
		Don't add an extra "\n" between the body of the commit
		and the note.

Or something...

Taylor Blau Nov. 8, 2022, 12:32 a.m. UTC | #8

On Mon, Nov 07, 2022 at 11:36:58PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 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.

Thanks for clarifying. For what it's worth, I slightly prefer
documenting both options as long as we clearly specify which is the
default behavior (and thus implied).

But I definitely do not feel strongly about it.

Thanks,
Taylor

Teng Long Nov. 8, 2022, 3:45 a.m. UTC | #9

"Ævar Arnfjörð Bjarmason" <[e-mail]> writes:

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

Yes, that's what I propose to do originally, but I can accept either NEG or POS
way. So, I will hang this up for a while and try to hear more inputs.

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

The current doc add the content about this circumstance corresponing to this
"if", which describes as:

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

... so you mean we should add more detailed information here?

> This should be a test_when_finished "", for the previous test, otherwise
> this one will presumably fail if you use the "wrong" --run="" arguments
> to skip the last test.

Yes, I agree, will fix (although if we replace it by "test_when_finished,
because of the dependency of the previous repo status, execute
`sh t3301-notes.sh --run=60` also failed).

Thanks.

Teng Long Nov. 8, 2022, 6:32 a.m. UTC | #10

"Ævar Arnfjörð Bjarmason" <[e-mail]> writes:

> I think the right thin to do is to just drop the strbuf_grow() here
> altogether, since strbuf_insert() will handle it for us, but that would
> make sense before this change, so maybe in pre-cleanup?

Sorry for ignoring this line of code. I think you are right, and I will
make a separate pre-cleanup commit for this in next round.

Thanks.

Teng Long Nov. 8, 2022, 7:36 a.m. UTC | #11

Taylor Blau <[e-mail]> writes:

* Actually that's my suggestion, because I found when I use "git notes appends"
to append new content to an existing note. It will always insert a blank line
between the existing and the new. Does "append" represent "a blank line first
then the content"? If the user does not want it, there may be no way to
circumvent this on "append" at present. For example:

--------example start-------

Here is a commit with notes (from a openstack community repo), the notes is as:

commit ...(HEAD -> master, origin/master, origin/HEAD)
Author: ...
Date:   ...

    .....
    Change-Id: ....

Notes (review):
    Code-Review+2: yatin <[e-mail]>
    Code-Review+2: chandan kumar <[e-mail]>
    Workflow+1: chandan kumar <[e-mail]>
    Verified+1: RDO Third Party CI <[e-mail]>
    Verified+2: Zuul
    Submitted-by: Zuul
    Submitted-at: Wed, 28 Sep 2022 12:58:46 +0000
    Reviewed-on: https://review.opendev.org/c/openstack/tripleo-quickstart/+/859516
    Project: openstack/tripleo-quickstart
    Branch: refs/heads/master

So, if user wants to append this notes, actually will add a blank line, maybe a
surprise. But I may not represent most users, so I added this compatible option,
maybe better to let the use choose, then send this as a RFC patch.

--------example finished-------

* Another reason is, I found that if I append nothing to a commit (here on my case
is HEAD) which doesn't exist notes on it, the output "Removing note for
object" shows something a little intesting to me, because there is no note to
remove, that's what 2/3 does. The following are specific examples:

--------example start-------

➜  git-notes-test git:(tl/test) ✗ git --no-pager log HEAD -n 1
commit d5a0127568e89239bb02f78d44dfa0427e726103 (HEAD -> tl/test)
Author: tenglong.tl <[e-mail]>
Date:   Thu Oct 20 18:09:16 2022 +0800

    111
➜  git-notes-test git:(tl/test) ✗ git notes append -m ""
Removing note for object d5a0127568e89239bb02f78d44dfa0427e726103

--------example finished-------

Thanks.

Teng Long Nov. 8, 2022, 1:06 p.m. UTC | #12

"Ævar Arnfjörð Bjarmason" <[e-mail]> 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.

Ævar Arnfjörð Bjarmason Nov. 8, 2022, 1:22 p.m. UTC | #13

On Tue, Nov 08 2022, Teng Long wrote:

> "Ævar Arnfjörð Bjarmason" <[e-mail]> 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[...]

Teng Long Nov. 9, 2022, 6:35 a.m. UTC | #14

"Ævar Arnfjörð Bjarmason" <[e-mail]> writes:

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

Thank you for the detailed explanation, now it's clear for me.

Patch

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).
+
 --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 be51f69225..f0fa337e8c 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, "blank-line", &blankline,
+			N_("insert paragraph break before appending to an existing note")),
 		OPT_END()
 	};
 	int edit = !strcmp(argv[0], "edit");
@@ -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 (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);
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 &&
 	git notes add -m "Initial set of notes" &&
 	git notes append -m "More notes appended with git notes append" &&
 	git notes show >actual &&