Re: [PATCH 2/2] notes: revert note_data.given behavior with empty notes add

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 25, 2024, 17:09 UTC
Message-ID: <xmqqzfq5flfd.fsf@gitster.g>
In-Reply-To: <20240725144548.3434-3-ddiss@suse.de>

David Disseldorp <ddiss@suse.de> writes:

> Subject: Re: [PATCH 2/2] notes: revert note_data.given behavior with empty notes add

Maybe it is just me, but "revert" has a specific connotation in the context of the command this project develops, and when your patch is not doing so, it gets misleading. If you said you are restoring the behaviour, that would be more easily understood.

    notes: do not trigger editor when adding an empty note

perhaps?

> Prior to 90bc19b3ae, note_data.given was set alongside an -m, -C or -F

Please make the first mention of a commit using "git show -s --pretty=reference" format, i.e.

    90bc19b3ae (notes.c: introduce '--separator=<paragraph-break>' option, 2023-05-27)

Using the reference format, besides being consistent, is very much preferrable to allow us to tell how old the problem goes back immediately by having the datestamp at the end.

more generally, this proposed log message starts with implementation details. When we have a user-visible breakage, please start from describing that instead. The usual way to compose a log message of this project is to

 - Give an observation on how the current system work in the present
   tense (so no need to say "Currently X is Y", just "X is Y" or "X
   does Y"), and discuss what you perceive as a problem in it.

 - Propose a solution (optional---often, problem description
   trivially leads to an obvious solution in reader's minds).

 - Give commands to the codebase to "become like so".

in this order.  So something along this line:

	With "git notes add -C $blob", the given blob contents are
	to be made into a note without involving an editor.  But
	when "--allow-empty" is given, the editor is invoked.

	This behaviour started when 90bc19b3 (notes.c: introduce
	'--separator=<paragraph-break>' option, 2023-05-27).  Prior
	to that commit, we used to do this and that ... describe
	implementation details here if you want ...

	Restore the original behaviour of "git note" that takes the
	contents given via the "-m", "-C", "-F" options without
	invoking an editor, by doing ... this and that ... describe
	implementation details here if you want ...

would be more customary.

This is not a fault of this patch, and certainly not a fault of 90bc19b3 (notes.c: introduce '--separator=<paragraph-break>' option, 2023-05-27), but unlike "git commit" and "git tag" that can take pre-prepared contents from some source and by default use that intact without involving an editor, "git notes" seems to lack the ability to countermand this default and spawn an editor (e.g., "git commit -F myfile -e"). We may want to #leftoverbits fix that.

> Fixes: 90bc19b3ae ("notes.c: introduce '--separator=<paragraph-break>' option")
> Link: https://github.com/ddiss/icyci/issues/12

We generally refrain from using these two trailers.  Please drop them.

Especially "Fixes" claim can later prove incorrect (we thought this was a good fix when committing, but it later turned out to be a bad one), and besides you will be referring to the problematic commit in your proposed log message already anyway.

Show 32 quoted lines

> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
>  builtin/notes.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/notes.c b/builtin/notes.c
> index d9c356e354..3ccb3eb602 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -282,6 +282,7 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
>  	ALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);
>  	d->messages[d->msg_nr - 1] = msg;
>  	msg->stripspace = STRIPSPACE;
> +	d->given = 1;
>  	return 0;
>  }
>  
> @@ -302,6 +303,7 @@ static int parse_file_arg(const struct option *opt, const char *arg, int unset)
>  	ALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);
>  	d->messages[d->msg_nr - 1] = msg;
>  	msg->stripspace = STRIPSPACE;
> +	d->given = 1;
>  	return 0;
>  }
>  
> @@ -335,6 +337,7 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)
>  	ALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);
>  	d->messages[d->msg_nr - 1] = msg;
>  	msg->stripspace = NO_STRIPSPACE;
> +	d->given = 1;
>  	return 0;
>  }

All the above places that resurrects setting d.given looks OK, but it makes me wonder if you need to add them in the first place.

Wouldn't it be sufficient to see if d->msg_nr is non-zero after all the parsing is done? If any of the message came from "-c", a separate flag d->use_editor is set to force you run the editor, and otherwise you'd not be using the editor, right?

Show 5 quoted lines

> @@ -515,7 +518,6 @@ static int add(int argc, const char **argv, const char *prefix)
>  
>  	if (d.msg_nr)
>  		concat_messages(&d);
> -	d.given = !!d.buf.len;

Here is what I mean. If you didn't do any of the "d->given = 1" above during parsing, and instead say

	if (d.msg_nr)
		concat_messages(&d);
	d.given = d.msg_nr;

shouldn't it be sufficient to convince prepare_note_data() to do the right thing?

Thanks.