{"thread":{"id":"61859","subject":"[PATCH v2 0/1] notes: fix editor invocation regression","startedAt":"2024-07-29T15:16:56Z","lastAt":"2024-07-29T22:50:24Z","messageCount":5,"participants":["David Disseldorp","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":1},"messages":[{"id":"499521","messageId":"20240729151639.19192-1-ddiss@suse.de","threadId":"61859","inReplyTo":null,"subject":"[PATCH v2 0/1] notes: fix editor invocation regression","fromName":"David Disseldorp","fromEmail":"ddiss@suse.de","sentAt":"2024-07-29T15:13:59Z","receivedAt":"2024-07-29T15:16:56Z","isPatch":true,"sender":{"key":"ddiss@suse.de","avatar":"https://avatars.githubusercontent.com/u/3995472?v=4"},"body":"The following patch attempts to restore previous editor invocation\nbehavior for empty notes, which was inadvertantly changed via 90bc19b3ae\n(notes.c: introduce '--separator=<paragraph-break>' option, 2023-05-27).\nA new regression test is included, which passes prior to 90bc19b3ae and\nafter this fix.\n\nPlease cc me in any replies as I'm not subscribed.\n\nCheers, David\n\n--\n\nThe following changes since commit ad57f148c6b5f8735b62238dda8f571c582e0e54:\n\n  Git 2.46-rc2 (2024-07-23 16:54:35 -0700)\n\nare available in the Git repository at:\n\n  https://github.com/ddiss/git.git notes_empty_editor_add_v2\n\nfor you to fetch changes up to 63019aa2c7bbee5989944a990ca2c0229cef8ae2:\n\n  notes: do not trigger editor when adding an empty note (2024-07-29 13:22:26 +0200)\n\n----------------------------------------------------------------\nDavid Disseldorp (1):\n      notes: do not trigger editor when adding an empty note\n\n builtin/notes.c  | 22 ++++++++++------------\n t/t3301-notes.sh |  5 +++++\n 2 files changed, 15 insertions(+), 12 deletions(-)\n"},{"id":"499522","messageId":"20240729151639.19192-2-ddiss@suse.de","threadId":"61859","inReplyTo":"20240729151639.19192-1-ddiss@suse.de","subject":"[PATCH v2 1/1] notes: do not trigger editor when adding an empty note","fromName":"David Disseldorp","fromEmail":"ddiss@suse.de","sentAt":"2024-07-29T15:14:00Z","receivedAt":"2024-07-29T15:17:03Z","isPatch":true,"sender":{"key":"ddiss@suse.de","avatar":"https://avatars.githubusercontent.com/u/3995472?v=4"},"body":"With \"git notes add -C $blob\", the given blob contents are to be made\ninto a note without involving an editor.  But when \"--allow-empty\" is\ngiven, the editor is invoked, which can cause problems for\nnon-interactive callers[1].\n\nThis behaviour started with 90bc19b3ae (notes.c: introduce\n'--separator=<paragraph-break>' option, 2023-05-27), which changed\neditor invocation logic to check for a zero length note_data buffer.\n\nRestore the original behaviour of \"git note\" that takes the contents\ngiven via the \"-m\", \"-C\", \"-F\" options without invoking an editor, by\nchecking for any prior parameter callbacks, indicated by a non-zero\nnote_data.msg_nr.  Remove the now-unneeded note_data.given flag.\n\nAdd a test for this regression by checking whether GIT_EDITOR is\ninvoked alongside \"git notes add -C $empty_blob --allow-empty\"\n\n[1] https://github.com/ddiss/icyci/issues/12\n\nSigned-off-by: David Disseldorp <ddiss@suse.de>\n---\n builtin/notes.c  | 22 ++++++++++------------\n t/t3301-notes.sh |  5 +++++\n 2 files changed, 15 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex d9c356e354..4cc5bfedc3 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -114,7 +114,6 @@ struct note_msg {\n };\n \n struct note_data {\n-\tint given;\n \tint use_editor;\n \tint stripspace;\n \tchar *edit_path;\n@@ -193,7 +192,7 @@ static void write_commented_object(int fd, const struct object_id *object)\n static void prepare_note_data(const struct object_id *object, struct note_data *d,\n \t\tconst struct object_id *old_note)\n {\n-\tif (d->use_editor || !d->given) {\n+\tif (d->use_editor || !d->msg_nr) {\n \t\tint fd;\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n@@ -201,7 +200,7 @@ static void prepare_note_data(const struct object_id *object, struct note_data *\n \t\td->edit_path = git_pathdup(\"NOTES_EDITMSG\");\n \t\tfd = xopen(d->edit_path, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n \n-\t\tif (d->given)\n+\t\tif (d->msg_nr)\n \t\t\twrite_or_die(fd, d->buf.buf, d->buf.len);\n \t\telse if (old_note)\n \t\t\tcopy_obj_to_fd(fd, old_note);\n@@ -515,7 +514,6 @@ static int add(int argc, const char **argv, const char *prefix)\n \n \tif (d.msg_nr)\n \t\tconcat_messages(&d);\n-\td.given = !!d.buf.len;\n \n \tobject_ref = argc > 1 ? argv[1] : \"HEAD\";\n \n@@ -528,7 +526,7 @@ static int add(int argc, const char **argv, const char *prefix)\n \tif (note) {\n \t\tif (!force) {\n \t\t\tfree_notes(t);\n-\t\t\tif (d.given) {\n+\t\t\tif (d.msg_nr) {\n \t\t\t\tfree_note_data(&d);\n \t\t\t\treturn error(_(\"Cannot add notes. \"\n \t\t\t\t\t\"Found existing notes for object %s. \"\n@@ -690,14 +688,14 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(usage, options);\n \t}\n \n-\tif (d.msg_nr)\n+\tif (d.msg_nr) {\n \t\tconcat_messages(&d);\n-\td.given = !!d.buf.len;\n-\n-\tif (d.given && edit)\n-\t\tfprintf(stderr, _(\"The -m/-F/-c/-C options have been deprecated \"\n-\t\t\t\"for the 'edit' subcommand.\\n\"\n-\t\t\t\"Please use 'git notes add -f -m/-F/-c/-C' instead.\\n\"));\n+\t\tif (edit)\n+\t\t\tfprintf(stderr, _(\"The -m/-F/-c/-C options have been \"\n+\t\t\t\t\"deprecated for the 'edit' subcommand.\\n\"\n+\t\t\t\t\"Please use 'git notes add -f -m/-F/-c/-C' \"\n+\t\t\t\t\"instead.\\n\"));\n+\t}\n \n \tobject_ref = 1 < argc ? argv[1] : \"HEAD\";\n \ndiff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\nindex 536bd11ff4..c0dbacc161 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -1557,4 +1557,9 @@ test_expect_success 'empty notes are displayed by git log' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'empty notes do not invoke the editor' '\n+\ttest_commit 18th &&\n+\tGIT_EDITOR=\"false\" git notes add -C \"$empty_blob\" --allow-empty\n+'\n+\n test_done\n-- \n2.43.0\n\n"},{"id":"499535","messageId":"xmqqed7bhobk.fsf@gitster.g","threadId":"61859","inReplyTo":"20240729151639.19192-2-ddiss@suse.de","subject":"Re: [PATCH v2 1/1] notes: do not trigger editor when adding an empty note","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-29T21:37:35Z","receivedAt":"2024-07-29T21:37:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Disseldorp <ddiss@suse.de> writes:\n\n> Restore the original behaviour of \"git note\" that takes the contents\n> given via the \"-m\", \"-C\", \"-F\" options without invoking an editor, by\n> checking for any prior parameter callbacks, indicated by a non-zero\n> note_data.msg_nr.  Remove the now-unneeded note_data.given flag.\n>\n> Add a test for this regression by checking whether GIT_EDITOR is\n> invoked alongside \"git notes add -C $empty_blob --allow-empty\"\n\nMakes perfect sense.\n\nA #leftoverbit that we may want to look into after this topic\nsettles is to teach the \"-e\" option, similar to \"git commit\" and\n\"git tag\", so that messages seeded with -m/-F can still be edited.\nIn a sense, supporting both \"-c/-C\" was unnecessary if these\ncommands supported \"-e\" in the first place, and that mistake has\nbeen inherited from \"git commit\" and \"git tag\" into this command.\n\n> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\n> index 536bd11ff4..c0dbacc161 100755\n> --- a/t/t3301-notes.sh\n> +++ b/t/t3301-notes.sh\n> @@ -1557,4 +1557,9 @@ test_expect_success 'empty notes are displayed by git log' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'empty notes do not invoke the editor' '\n> +\ttest_commit 18th &&\n> +\tGIT_EDITOR=\"false\" git notes add -C \"$empty_blob\" --allow-empty\n> +'\n> +\n>  test_done\n\nI am tempted to squash in\n\n        diff --git i/t/t3301-notes.sh w/t/t3301-notes.sh\n        index c0dbacc161..99137fb235 100755\n        --- i/t/t3301-notes.sh\n        +++ w/t/t3301-notes.sh\n        @@ -1559,7 +1559,12 @@ test_expect_success 'empty notes are displayed by git log' '\n\n         test_expect_success 'empty notes do not invoke the editor' '\n                test_commit 18th &&\n        -\tGIT_EDITOR=\"false\" git notes add -C \"$empty_blob\" --allow-empty\n        +\tGIT_EDITOR=\"false\" git notes add -C \"$empty_blob\" --allow-empty &&\n        +\tgit notes remove HEAD &&\n        +\tGIT_EDITOR=\"false\" git notes add -m \"\" --allow-empty &&\n        +\tgit notes remove HEAD &&\n        +\tGIT_EDITOR=\"false\" git notes add -F /dev/null --allow-empty &&\n        +\tgit notes remove HEAD\n         '\n\n         test_done\n\nto make sure that all three options mentioned in the proposed commit\nlog message are tested.  It is not too much more effort to do so.\n\nWill queue.\n\nThanks.\n\n"},{"id":"499536","messageId":"20240729235333.513eb1d9.ddiss@suse.de","threadId":"61859","inReplyTo":"xmqqed7bhobk.fsf@gitster.g","subject":"Re: [PATCH v2 1/1] notes: do not trigger editor when adding an empty note","fromName":"David Disseldorp","fromEmail":"ddiss@suse.de","sentAt":"2024-07-29T21:53:33Z","receivedAt":"2024-07-29T21:53:37Z","isPatch":true,"sender":{"key":"ddiss@suse.de","avatar":"https://avatars.githubusercontent.com/u/3995472?v=4"},"body":"On Mon, 29 Jul 2024 14:37:35 -0700, Junio C Hamano wrote:\n...\n> I am tempted to squash in\n> \n>         diff --git i/t/t3301-notes.sh w/t/t3301-notes.sh\n>         index c0dbacc161..99137fb235 100755\n>         --- i/t/t3301-notes.sh\n>         +++ w/t/t3301-notes.sh\n>         @@ -1559,7 +1559,12 @@ test_expect_success 'empty notes are displayed by git log' '\n> \n>          test_expect_success 'empty notes do not invoke the editor' '\n>                 test_commit 18th &&\n>         -\tGIT_EDITOR=\"false\" git notes add -C \"$empty_blob\" --allow-empty\n>         +\tGIT_EDITOR=\"false\" git notes add -C \"$empty_blob\" --allow-empty &&\n>         +\tgit notes remove HEAD &&\n>         +\tGIT_EDITOR=\"false\" git notes add -m \"\" --allow-empty &&\n>         +\tgit notes remove HEAD &&\n>         +\tGIT_EDITOR=\"false\" git notes add -F /dev/null --allow-empty &&\n>         +\tgit notes remove HEAD\n>          '\n> \n>          test_done\n> \n> to make sure that all three options mentioned in the proposed commit\n> log message are tested.  It is not too much more effort to do so.\n\nThis looks good and works for me - feel free to squash it in.\n\nThanks, David\n"},{"id":"499538","messageId":"xmqqa5hzhkye.fsf@gitster.g","threadId":"61859","inReplyTo":"20240729235333.513eb1d9.ddiss@suse.de","subject":"Re: [PATCH v2 1/1] notes: do not trigger editor when adding an empty note","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-29T22:50:17Z","receivedAt":"2024-07-29T22:50:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Disseldorp <ddiss@suse.de> writes:\n\n> On Mon, 29 Jul 2024 14:37:35 -0700, Junio C Hamano wrote:\n> ...\n>> I am tempted to squash in\n>> ...\n>> to make sure that all three options mentioned in the proposed commit\n>> log message are tested.  It is not too much more effort to do so.\n>\n> This looks good and works for me - feel free to squash it in.\n\nWill do.  Thanks for a quick response.\n"}]}