| Message ID | 20240729151639.19192-2-ddiss@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/1] notes: do not trigger editor when adding an empty note | expand |
David Disseldorp <ddiss@suse.de> writes: > Restore the original behaviour of "git note" that takes the contents > given via the "-m", "-C", "-F" options without invoking an editor, by > checking for any prior parameter callbacks, indicated by a non-zero > note_data.msg_nr. Remove the now-unneeded note_data.given flag. > > Add a test for this regression by checking whether GIT_EDITOR is > invoked alongside "git notes add -C $empty_blob --allow-empty" Makes perfect sense. A #leftoverbit that we may want to look into after this topic settles is to teach the "-e" option, similar to "git commit" and "git tag", so that messages seeded with -m/-F can still be edited. In a sense, supporting both "-c/-C" was unnecessary if these commands supported "-e" in the first place, and that mistake has been inherited from "git commit" and "git tag" into this command. > diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh > index 536bd11ff4..c0dbacc161 100755 > --- a/t/t3301-notes.sh > +++ b/t/t3301-notes.sh > @@ -1557,4 +1557,9 @@ test_expect_success 'empty notes are displayed by git log' ' > test_cmp expect actual > ' > > +test_expect_success 'empty notes do not invoke the editor' ' > + test_commit 18th && > + GIT_EDITOR="false" git notes add -C "$empty_blob" --allow-empty > +' > + > test_done I am tempted to squash in diff --git i/t/t3301-notes.sh w/t/t3301-notes.sh index c0dbacc161..99137fb235 100755 --- i/t/t3301-notes.sh +++ w/t/t3301-notes.sh @@ -1559,7 +1559,12 @@ test_expect_success 'empty notes are displayed by git log' ' test_expect_success 'empty notes do not invoke the editor' ' test_commit 18th && - GIT_EDITOR="false" git notes add -C "$empty_blob" --allow-empty + GIT_EDITOR="false" git notes add -C "$empty_blob" --allow-empty && + git notes remove HEAD && + GIT_EDITOR="false" git notes add -m "" --allow-empty && + git notes remove HEAD && + GIT_EDITOR="false" git notes add -F /dev/null --allow-empty && + git notes remove HEAD ' test_done to make sure that all three options mentioned in the proposed commit log message are tested. It is not too much more effort to do so. Will queue. Thanks.
On Mon, 29 Jul 2024 14:37:35 -0700, Junio C Hamano wrote: ... > I am tempted to squash in > > diff --git i/t/t3301-notes.sh w/t/t3301-notes.sh > index c0dbacc161..99137fb235 100755 > --- i/t/t3301-notes.sh > +++ w/t/t3301-notes.sh > @@ -1559,7 +1559,12 @@ test_expect_success 'empty notes are displayed by git log' ' > > test_expect_success 'empty notes do not invoke the editor' ' > test_commit 18th && > - GIT_EDITOR="false" git notes add -C "$empty_blob" --allow-empty > + GIT_EDITOR="false" git notes add -C "$empty_blob" --allow-empty && > + git notes remove HEAD && > + GIT_EDITOR="false" git notes add -m "" --allow-empty && > + git notes remove HEAD && > + GIT_EDITOR="false" git notes add -F /dev/null --allow-empty && > + git notes remove HEAD > ' > > test_done > > to make sure that all three options mentioned in the proposed commit > log message are tested. It is not too much more effort to do so. This looks good and works for me - feel free to squash it in. Thanks, David
David Disseldorp <ddiss@suse.de> writes: > On Mon, 29 Jul 2024 14:37:35 -0700, Junio C Hamano wrote: > ... >> I am tempted to squash in >> ... >> to make sure that all three options mentioned in the proposed commit >> log message are tested. It is not too much more effort to do so. > > This looks good and works for me - feel free to squash it in. Will do. Thanks for a quick response.
diff --git a/builtin/notes.c b/builtin/notes.c index d9c356e354..4cc5bfedc3 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -114,7 +114,6 @@ struct note_msg { }; struct note_data { - int given; int use_editor; int stripspace; char *edit_path; @@ -193,7 +192,7 @@ static void write_commented_object(int fd, const struct object_id *object) static void prepare_note_data(const struct object_id *object, struct note_data *d, const struct object_id *old_note) { - if (d->use_editor || !d->given) { + if (d->use_editor || !d->msg_nr) { int fd; struct strbuf buf = STRBUF_INIT; @@ -201,7 +200,7 @@ static void prepare_note_data(const struct object_id *object, struct note_data * d->edit_path = git_pathdup("NOTES_EDITMSG"); fd = xopen(d->edit_path, O_CREAT | O_TRUNC | O_WRONLY, 0600); - if (d->given) + if (d->msg_nr) write_or_die(fd, d->buf.buf, d->buf.len); else if (old_note) copy_obj_to_fd(fd, old_note); @@ -515,7 +514,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"; @@ -528,7 +526,7 @@ static int add(int argc, const char **argv, const char *prefix) if (note) { if (!force) { free_notes(t); - if (d.given) { + if (d.msg_nr) { free_note_data(&d); return error(_("Cannot add notes. " "Found existing notes for object %s. " @@ -690,14 +688,14 @@ static int append_edit(int argc, const char **argv, const char *prefix) usage_with_options(usage, options); } - if (d.msg_nr) + 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 " - "for the 'edit' subcommand.\n" - "Please use 'git notes add -f -m/-F/-c/-C' instead.\n")); + if (edit) + fprintf(stderr, _("The -m/-F/-c/-C options have been " + "deprecated for the 'edit' subcommand.\n" + "Please use 'git notes add -f -m/-F/-c/-C' " + "instead.\n")); + } object_ref = 1 < argc ? argv[1] : "HEAD"; diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh index 536bd11ff4..c0dbacc161 100755 --- a/t/t3301-notes.sh +++ b/t/t3301-notes.sh @@ -1557,4 +1557,9 @@ test_expect_success 'empty notes are displayed by git log' ' test_cmp expect actual ' +test_expect_success 'empty notes do not invoke the editor' ' + test_commit 18th && + GIT_EDITOR="false" git notes add -C "$empty_blob" --allow-empty +' + test_done
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, which can cause problems for non-interactive callers[1]. This behaviour started with 90bc19b3ae (notes.c: introduce '--separator=<paragraph-break>' option, 2023-05-27), which changed editor invocation logic to check for a zero length note_data buffer. Restore the original behaviour of "git note" that takes the contents given via the "-m", "-C", "-F" options without invoking an editor, by checking for any prior parameter callbacks, indicated by a non-zero note_data.msg_nr. Remove the now-unneeded note_data.given flag. Add a test for this regression by checking whether GIT_EDITOR is invoked alongside "git notes add -C $empty_blob --allow-empty" [1] https://github.com/ddiss/icyci/issues/12 Signed-off-by: David Disseldorp <ddiss@suse.de> --- builtin/notes.c | 22 ++++++++++------------ t/t3301-notes.sh | 5 +++++ 2 files changed, 15 insertions(+), 12 deletions(-)