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