On Thu, 25 Jul 2024 10:09:26 -0700, Junio C Hamano wrote:
> David Disseldorp <ddiss@xxxxxxx> 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@xxxxxxx>
> > ---
> > 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
