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

[David Disseldorp <ddiss@suse.de>]

On Thu, 25 Jul 2024 10:09:26 -0700, Junio C Hamano wrote:

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

Sure, sounds good to me.

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

Ack, will do.

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

Okay, will drop the Fixes: and go with a footnote for the downstream
issue, as proposed by Kristoffer.

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

Yes, I also noticed that msg_nr could also be used for this, but chose
to revert back to the old given logic for clarity. I'll revisit the
msg_nr approach for v2.

Thanks for the feedback,
David