| Message ID | 20240725144548.3434-3-ddiss@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] t3301-notes: check editor isn't invoked for empty notes add | expand |
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. > 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? Thanks.
On Thu, Jul 25, 2024, at 19:09, Junio C Hamano wrote: >> 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. David, if you want to mention your downstream issue you can use a “footnote” in the commit message: 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.[1] <rest of the commit message> [1] https://github.com/ddiss/icyci/issues/12 Signed-off-by: … This is widely practiced.
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
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; } @@ -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; object_ref = argc > 1 ? argv[1] : "HEAD"; @@ -692,7 +694,6 @@ static int append_edit(int argc, const char **argv, const char *prefix) if (d.msg_nr) concat_messages(&d); - d.given = !!d.buf.len; if (d.given && edit) fprintf(stderr, _("The -m/-F/-c/-C options have been deprecated "
Prior to 90bc19b3ae, note_data.given was set alongside an -m, -C or -F parameter for add / append. Following 90bc19b3ae, note_data.given is only set if the notes data buffer length is non-zero, which results in GIT_EDITOR invocation if e.g. a zero length blob object is provided. Revert to pre-90bc19b3ae note_data.given behavior, to fix non-interactive callers. Fixes: 90bc19b3ae ("notes.c: introduce '--separator=<paragraph-break>' option") Link: https://github.com/ddiss/icyci/issues/12 Signed-off-by: David Disseldorp <ddiss@suse.de> --- builtin/notes.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)