{"thread":{"id":"61844","subject":"[PATCH 0/2] notes: fix editor invocation regression","startedAt":"2024-07-25T14:46:04Z","lastAt":"2024-07-25T23:26:07Z","messageCount":11,"participants":["David Disseldorp","Kristoffer Haugsbakk","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"499343","messageId":"20240725144548.3434-1-ddiss@suse.de","threadId":"61844","inReplyTo":null,"subject":"[PATCH 0/2] notes: fix editor invocation regression","fromName":"David Disseldorp","fromEmail":"ddiss@suse.de","sentAt":"2024-07-25T14:41:05Z","receivedAt":"2024-07-25T14:46:04Z","isPatch":true,"sender":{"key":"ddiss@suse.de","avatar":"https://avatars.githubusercontent.com/u/3995472?v=4"},"body":"This patchset attempts to revert editor invocation behavior for empty\nnotes, which was inadvertantly changed via 90bc19b3ae (\"notes.c:\nintroduce '--separator=' option\").\nThe new test passes prior to 90bc19b3ae and after 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\nad57f148c6b5f8735b62238dda8f571c582e0e54:\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\n\nfor you to fetch changes up to fa40003e560348e146f11aa85ac1fc5a3d86e31e:\n\n notes: revert note_data.given behavior with empty notes add\n(2024-07-25 15:41:56 +0200)\n\n----------------------------------------------------------------\nDavid Disseldorp (2):\n t3301-notes: check editor isn't invoked for empty notes add\n notes: revert note_data.given behavior with empty notes add\n\n builtin/notes.c | 5 +++--\n t/t3301-notes.sh | 5 +++++\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\n"},{"id":"499344","messageId":"20240725144548.3434-2-ddiss@suse.de","threadId":"61844","inReplyTo":"20240725144548.3434-1-ddiss@suse.de","subject":"[PATCH 1/2] t3301-notes: check editor isn't invoked for empty notes add","fromName":"David Disseldorp","fromEmail":"ddiss@suse.de","sentAt":"2024-07-25T14:41:06Z","receivedAt":"2024-07-25T14:46:07Z","isPatch":true,"sender":{"key":"ddiss@suse.de","avatar":"https://avatars.githubusercontent.com/u/3995472?v=4"},"body":"90bc19b3ae (\"notes.c: introduce '--separator=' option\")\nchanged note_data.given logic such that it's no longer set if a zero\nlength file or blob object is provided.\n\nAdd a test for this regression by checking whether GIT_EDITOR is\ninvoked.\n\nSigned-off-by: David Disseldorp \n---\n t/t3301-notes.sh | 5 +++++\n 1 file changed, 5 insertions(+)\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":"499345","messageId":"20240725144548.3434-3-ddiss@suse.de","threadId":"61844","inReplyTo":"20240725144548.3434-1-ddiss@suse.de","subject":"[PATCH 2/2] notes: revert note_data.given behavior with empty notes add","fromName":"David Disseldorp","fromEmail":"ddiss@suse.de","sentAt":"2024-07-25T14:41:07Z","receivedAt":"2024-07-25T14:46:09Z","isPatch":true,"sender":{"key":"ddiss@suse.de","avatar":"https://avatars.githubusercontent.com/u/3995472?v=4"},"body":"Prior to 90bc19b3ae, note_data.given was set alongside an -m, -C or -F\nparameter for add / append. Following 90bc19b3ae, note_data.given is\nonly set if the notes data buffer length is non-zero, which results in\nGIT_EDITOR invocation if e.g. a zero length blob object is provided.\n\nRevert to pre-90bc19b3ae note_data.given behavior, to fix\nnon-interactive callers.\n\nFixes: 90bc19b3ae (\"notes.c: introduce '--separator=' option\")\nLink: https://github.com/ddiss/icyci/issues/12\nSigned-off-by: David Disseldorp \n---\n builtin/notes.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex d9c356e354..3ccb3eb602 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -282,6 +282,7 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n \tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n \td->messages[d->msg_nr - 1] = msg;\n \tmsg->stripspace = STRIPSPACE;\n+\td->given = 1;\n \treturn 0;\n }\n \n@@ -302,6 +303,7 @@ static int parse_file_arg(const struct option *opt, const char *arg, int unset)\n \tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n \td->messages[d->msg_nr - 1] = msg;\n \tmsg->stripspace = STRIPSPACE;\n+\td->given = 1;\n \treturn 0;\n }\n \n@@ -335,6 +337,7 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)\n \tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n \td->messages[d->msg_nr - 1] = msg;\n \tmsg->stripspace = NO_STRIPSPACE;\n+\td->given = 1;\n \treturn 0;\n }\n \n@@ -515,7 +518,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@@ -692,7 +694,6 @@ static int append_edit(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 \tif (d.given && edit)\n \t\tfprintf(stderr, _(\"The -m/-F/-c/-C options have been deprecated \"\n-- \n2.43.0\n\n"},{"id":"499352","messageId":"5fdccb84-1e60-477e-99d9-b22e3ef83f54@app.fastmail.com","threadId":"61844","inReplyTo":"20240725144548.3434-2-ddiss@suse.de","subject":"Re: [PATCH 1/2] t3301-notes: check editor isn't invoked for empty notes add","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-07-25T15:52:25Z","receivedAt":"2024-07-25T15:52:47Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Hi\n\nOn Thu, Jul 25, 2024, at 16:41, David Disseldorp wrote:\n> 90bc19b3ae (\"notes.c: introduce '--separator=' option\")\n> changed note_data.given logic such that it's no longer set if a zero\n> length file or blob object is provided.\n\nThis project uses the `git show -s --pretty=reference` format:\n\n 90bc19b3ae (notes.c: introduce '--separator='\n option, 2023-05-27)\n\n> t/t3301-notes.sh | 5 +++++\n> 1 file changed, 5 insertions(+)\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\n> 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> --\n> 2.43.0\n\nThis test fails, obviously. Maybe you can reorder the patches so that\nboth two patches pass the test suite?\n\nIntroducing a regression test in one patch and then fixing the bug (and\nmaking the test pass) in the next patch is a style that some prefer. But\nI have received feedback before that it’s best to avoid that.\n\n-- \nKristoffer Haugsbakk\n\n"},{"id":"499354","messageId":"20240725180322.5b6d3a0d.ddiss@suse.de","threadId":"61844","inReplyTo":"5fdccb84-1e60-477e-99d9-b22e3ef83f54@app.fastmail.com","subject":"Re: [PATCH 1/2] t3301-notes: check editor isn't invoked for empty notes add","fromName":"David Disseldorp","fromEmail":"ddiss@suse.de","sentAt":"2024-07-25T16:03:22Z","receivedAt":"2024-07-25T16:03:27Z","isPatch":true,"sender":{"key":"ddiss@suse.de","avatar":"https://avatars.githubusercontent.com/u/3995472?v=4"},"body":"On Thu, 25 Jul 2024 17:52:25 +0200, Kristoffer Haugsbakk wrote:\n\n> Hi\n> \n> On Thu, Jul 25, 2024, at 16:41, David Disseldorp wrote:\n> > 90bc19b3ae (\"notes.c: introduce '--separator=' option\")\n> > changed note_data.given logic such that it's no longer set if a zero\n> > length file or blob object is provided. \n> \n> This project uses the `git show -s --pretty=reference` format:\n> \n> 90bc19b3ae (notes.c: introduce '--separator='\n> option, 2023-05-27)\n\nOkay, will fix in a subsequent version. \n\n> > t/t3301-notes.sh | 5 +++++\n> > 1 file changed, 5 insertions(+)\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\n> > 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> > --\n> > 2.43.0 \n> \n> This test fails, obviously. Maybe you can reorder the patches so that\n> both two patches pass the test suite?\n> \n> Introducing a regression test in one patch and then fixing the bug (and\n> making the test pass) in the next patch is a style that some prefer. But\n> I have received feedback before that it’s best to avoid that.\n\nMakes sense, thanks for the feedback.\n"},{"id":"499360","messageId":"xmqqle1pigbk.fsf@gitster.g","threadId":"61844","inReplyTo":"20240725144548.3434-2-ddiss@suse.de","subject":"Re: [PATCH 1/2] t3301-notes: check editor isn't invoked for empty notes add","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-25T16:31:27Z","receivedAt":"2024-07-25T16:31:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Disseldorp writes:\n\n> 90bc19b3ae (\"notes.c: introduce '--separator=' option\")\n> changed note_data.given logic such that it's no longer set if a zero\n> length file or blob object is provided.\n>\n> Add a test for this regression by checking whether GIT_EDITOR is\n> invoked.\n>\n> Signed-off-by: David Disseldorp \n> ---\n> t/t3301-notes.sh | 5 +++++\n> 1 file changed, 5 insertions(+)\n\nHaving this test separate from 2/2 breaks bisectability. For a\nsmall test like this, it is often a lot more preferrable to squash\nit into the patch that updates the behaviour. It is easy to apply\nthe whole thing, and when the reviewer/tester is curious, it is easy\nto tentatively \"revert\" only the behaviour changes out of the\nworking tree to see how the original code behaved against the test\nto verify the alleged breakages were indeed there in the original.\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\nClever. By setting the editor to something that always fails, we\nwill notice if the command invoked it, when we do not expect the\neditor to be used.\n\nNot questioning the usefulness of this fix, and not suggesting to\nremove the \"--allow-empty\" support out of the \"git notes\" command,\nbut out of curiosity, what are these empty notes used for?\n\nThanks.\n\n"},{"id":"499367","messageId":"xmqqzfq5flfd.fsf@gitster.g","threadId":"61844","inReplyTo":"20240725144548.3434-3-ddiss@suse.de","subject":"Re: [PATCH 2/2] notes: revert note_data.given behavior with empty notes add","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-25T17:09:26Z","receivedAt":"2024-07-25T17:09:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Disseldorp writes:\n\n> Subject: Re: [PATCH 2/2] notes: revert note_data.given behavior with empty notes add\n\nMaybe it is just me, but \"revert\" has a specific connotation in the\ncontext of the command this project develops, and when your patch is\nnot doing so, it gets misleading. If you said you are restoring the\nbehaviour, that would be more easily understood.\n\n notes: do not trigger editor when adding an empty note\n\nperhaps?\n\n> Prior to 90bc19b3ae, note_data.given was set alongside an -m, -C or -F\n\nPlease make the first mention of a commit using \"git show -s --pretty=reference\"\nformat, i.e.\n\n 90bc19b3ae (notes.c: introduce '--separator=' option, 2023-05-27)\n\nUsing the reference format, besides being consistent, is very much\npreferrable to allow us to tell how old the problem goes back\nimmediately by having the datestamp at the end.\n\nmore generally, this proposed log message starts with implementation\ndetails. When we have a user-visible breakage, please start from\ndescribing that instead. The usual way to compose a log message of\nthis project is to\n\n - Give an observation on how the current system work in the present\n tense (so no need to say \"Currently X is Y\", just \"X is Y\" or \"X\n does Y\"), and discuss what you perceive as a problem in it.\n\n - Propose a solution (optional---often, problem description\n trivially leads to an obvious solution in reader's minds).\n\n - Give commands to the codebase to \"become like so\".\n\nin this order. So something along this line:\n\n\tWith \"git notes add -C $blob\", the given blob contents are\n\tto be made into a note without involving an editor. But\n\twhen \"--allow-empty\" is given, the editor is invoked.\n\n\tThis behaviour started when 90bc19b3 (notes.c: introduce\n\t'--separator=' option, 2023-05-27). Prior\n\tto that commit, we used to do this and that ... describe\n\timplementation details here if you want ...\n\n\tRestore the original behaviour of \"git note\" that takes the\n\tcontents given via the \"-m\", \"-C\", \"-F\" options without\n\tinvoking an editor, by doing ... this and that ... describe\n\timplementation details here if you want ...\n\nwould be more customary.\n\nThis is not a fault of this patch, and certainly not a fault of\n90bc19b3 (notes.c: introduce '--separator=' option,\n2023-05-27), but unlike \"git commit\" and \"git tag\" that can take\npre-prepared contents from some source and by default use that\nintact without involving an editor, \"git notes\" seems to lack the\nability to countermand this default and spawn an editor (e.g., \"git\ncommit -F myfile -e\"). We may want to #leftoverbits fix that.\n\n> Fixes: 90bc19b3ae (\"notes.c: introduce '--separator=' option\")\n> Link: https://github.com/ddiss/icyci/issues/12\n\nWe generally refrain from using these two trailers. Please drop them.\n\nEspecially \"Fixes\" claim can later prove incorrect (we thought this\nwas a good fix when committing, but it later turned out to be a bad\none), and besides you will be referring to the problematic commit in\nyour proposed log message already anyway.\n\n> Signed-off-by: David Disseldorp \n> ---\n> builtin/notes.c | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index d9c356e354..3ccb3eb602 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -282,6 +282,7 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n> \tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n> \td->messages[d->msg_nr - 1] = msg;\n> \tmsg->stripspace = STRIPSPACE;\n> +\td->given = 1;\n> \treturn 0;\n> }\n> \n> @@ -302,6 +303,7 @@ static int parse_file_arg(const struct option *opt, const char *arg, int unset)\n> \tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n> \td->messages[d->msg_nr - 1] = msg;\n> \tmsg->stripspace = STRIPSPACE;\n> +\td->given = 1;\n> \treturn 0;\n> }\n> \n> @@ -335,6 +337,7 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)\n> \tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n> \td->messages[d->msg_nr - 1] = msg;\n> \tmsg->stripspace = NO_STRIPSPACE;\n> +\td->given = 1;\n> \treturn 0;\n> }\n\nAll the above places that resurrects setting d.given looks OK, but\nit makes me wonder if you need to add them in the first place.\n\nWouldn't it be sufficient to see if d->msg_nr is non-zero after all\nthe parsing is done? If any of the message came from \"-c\", a\nseparate flag d->use_editor is set to force you run the editor, and\notherwise you'd not be using the editor, right?\n\n> @@ -515,7 +518,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\nHere is what I mean. If you didn't do any of the \"d->given = 1\"\nabove during parsing, and instead say\n\n\tif (d.msg_nr)\n\t\tconcat_messages(&d);\n\td.given = d.msg_nr;\n\nshouldn't it be sufficient to convince prepare_note_data() to do the\nright thing?\n\nThanks.\n"},{"id":"499368","messageId":"xmqqv80tfldo.fsf@gitster.g","threadId":"61844","inReplyTo":"5fdccb84-1e60-477e-99d9-b22e3ef83f54@app.fastmail.com","subject":"Re: [PATCH 1/2] t3301-notes: check editor isn't invoked for empty notes add","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-25T17:10:27Z","receivedAt":"2024-07-25T17:10:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" writes:\n\n> Introducing a regression test in one patch and then fixing the bug (and\n> making the test pass) in the next patch is a style that some prefer. But\n> I have received feedback before that it’s best to avoid that.\n\nEspecially for something very small like this topic, yes.\n"},{"id":"499370","messageId":"ae5dc48b-07df-45ab-a3b7-4d5c27fdbfa2@app.fastmail.com","threadId":"61844","inReplyTo":"xmqqzfq5flfd.fsf@gitster.g","subject":"Re: [PATCH 2/2] notes: revert note_data.given behavior with empty notes add","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-07-25T17:22:39Z","receivedAt":"2024-07-25T17:23:07Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Thu, Jul 25, 2024, at 19:09, Junio C Hamano wrote:\n>> Fixes: 90bc19b3ae (\"notes.c: introduce '--separator=' option\")\n>> Link: https://github.com/ddiss/icyci/issues/12\n>\n> We generally refrain from using these two trailers. Please drop them.\n>\n> Especially \"Fixes\" claim can later prove incorrect (we thought this\n> was a good fix when committing, but it later turned out to be a bad\n> one), and besides you will be referring to the problematic commit in\n> your proposed log message already anyway.\n\nDavid, if you want to mention your downstream issue you can use a\n“footnote” in the commit message:\n\n With \"git notes add -C $blob\", the given blob contents are\n to be made into a note without involving an editor. But\n when \"--allow-empty\" is given, the editor is invoked.[1]\n\n \n\n [1] https://github.com/ddiss/icyci/issues/12\n\n Signed-off-by: …\n\nThis is widely practiced.\n\n-- \nKristoffer Haugsbakk\n\n"},{"id":"499381","messageId":"20240726011004.6bde3938.ddiss@suse.de","threadId":"61844","inReplyTo":"xmqqle1pigbk.fsf@gitster.g","subject":"Re: [PATCH 1/2] t3301-notes: check editor isn't invoked for empty notes add","fromName":"David Disseldorp","fromEmail":"ddiss@suse.de","sentAt":"2024-07-25T23:10:04Z","receivedAt":"2024-07-25T23:10:09Z","isPatch":true,"sender":{"key":"ddiss@suse.de","avatar":"https://avatars.githubusercontent.com/u/3995472?v=4"},"body":"On Thu, 25 Jul 2024 09:31:27 -0700, Junio C Hamano wrote:\n\n> David Disseldorp writes:\n> \n> > 90bc19b3ae (\"notes.c: introduce '--separator=' option\")\n> > changed note_data.given logic such that it's no longer set if a zero\n> > length file or blob object is provided.\n> >\n> > Add a test for this regression by checking whether GIT_EDITOR is\n> > invoked.\n> >\n> > Signed-off-by: David Disseldorp \n> > ---\n> > t/t3301-notes.sh | 5 +++++\n> > 1 file changed, 5 insertions(+) \n> \n> Having this test separate from 2/2 breaks bisectability. For a\n> small test like this, it is often a lot more preferrable to squash\n> it into the patch that updates the behaviour. It is easy to apply\n> the whole thing, and when the reviewer/tester is curious, it is easy\n> to tentatively \"revert\" only the behaviour changes out of the\n> working tree to see how the original code behaved against the test\n> to verify the alleged breakages were indeed there in the original.\n\nUnderstood. In that case I'll squash both patches together in the next\nversion.\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> Clever. By setting the editor to something that always fails, we\n> will notice if the command invoked it, when we do not expect the\n> editor to be used.\n> \n> Not questioning the usefulness of this fix, and not suggesting to\n> remove the \"--allow-empty\" support out of the \"git notes\" command,\n> but out of curiosity, what are these empty notes used for?\n\nicyci (https://github.com/ddiss/icyci) attaches test-suite stderr and\nstdout notes following test completion. There's no real value in\nattaching empty e.g. stderr notes, but tests can also provide their own\narbitrary notes files and may wish to use (empty) note existence as a\nflag in results reporting.\n"},{"id":"499383","messageId":"20240726012604.057a645a.ddiss@suse.de","threadId":"61844","inReplyTo":"xmqqzfq5flfd.fsf@gitster.g","subject":"Re: [PATCH 2/2] notes: revert note_data.given behavior with empty notes add","fromName":"David Disseldorp","fromEmail":"ddiss@suse.de","sentAt":"2024-07-25T23:26:04Z","receivedAt":"2024-07-25T23:26:07Z","isPatch":true,"sender":{"key":"ddiss@suse.de","avatar":"https://avatars.githubusercontent.com/u/3995472?v=4"},"body":"On Thu, 25 Jul 2024 10:09:26 -0700, Junio C Hamano wrote:\n\n> David Disseldorp writes:\n> \n> > Subject: Re: [PATCH 2/2] notes: revert note_data.given behavior with empty notes add \n> \n> Maybe it is just me, but \"revert\" has a specific connotation in the\n> context of the command this project develops, and when your patch is\n> not doing so, it gets misleading. If you said you are restoring the\n> behaviour, that would be more easily understood.\n> \n> notes: do not trigger editor when adding an empty note\n> \n> perhaps?\n\nSure, sounds good to me.\n\n> > Prior to 90bc19b3ae, note_data.given was set alongside an -m, -C or -F \n> \n> Please make the first mention of a commit using \"git show -s --pretty=reference\"\n> format, i.e.\n> \n> 90bc19b3ae (notes.c: introduce '--separator=' option, 2023-05-27)\n> \n> Using the reference format, besides being consistent, is very much\n> preferrable to allow us to tell how old the problem goes back\n> immediately by having the datestamp at the end.\n\nAck, will do.\n\n> more generally, this proposed log message starts with implementation\n> details. When we have a user-visible breakage, please start from\n> describing that instead. The usual way to compose a log message of\n> this project is to\n> \n> - Give an observation on how the current system work in the present\n> tense (so no need to say \"Currently X is Y\", just \"X is Y\" or \"X\n> does Y\"), and discuss what you perceive as a problem in it.\n> \n> - Propose a solution (optional---often, problem description\n> trivially leads to an obvious solution in reader's minds).\n> \n> - Give commands to the codebase to \"become like so\".\n> \n> in this order. So something along this line:\n> \n> \tWith \"git notes add -C $blob\", the given blob contents are\n> \tto be made into a note without involving an editor. But\n> \twhen \"--allow-empty\" is given, the editor is invoked.\n> \n> \tThis behaviour started when 90bc19b3 (notes.c: introduce\n> \t'--separator=' option, 2023-05-27). Prior\n> \tto that commit, we used to do this and that ... describe\n> \timplementation details here if you want ...\n> \n> \tRestore the original behaviour of \"git note\" that takes the\n> \tcontents given via the \"-m\", \"-C\", \"-F\" options without\n> \tinvoking an editor, by doing ... this and that ... describe\n> \timplementation details here if you want ...\n> \n> would be more customary.\n> \n> This is not a fault of this patch, and certainly not a fault of\n> 90bc19b3 (notes.c: introduce '--separator=' option,\n> 2023-05-27), but unlike \"git commit\" and \"git tag\" that can take\n> pre-prepared contents from some source and by default use that\n> intact without involving an editor, \"git notes\" seems to lack the\n> ability to countermand this default and spawn an editor (e.g., \"git\n> commit -F myfile -e\"). We may want to #leftoverbits fix that.\n> \n> > Fixes: 90bc19b3ae (\"notes.c: introduce '--separator=' option\")\n> > Link: https://github.com/ddiss/icyci/issues/12 \n> \n> We generally refrain from using these two trailers. Please drop them.\n> \n> Especially \"Fixes\" claim can later prove incorrect (we thought this\n> was a good fix when committing, but it later turned out to be a bad\n> one), and besides you will be referring to the problematic commit in\n> your proposed log message already anyway.\n\nOkay, will drop the Fixes: and go with a footnote for the downstream\nissue, as proposed by Kristoffer.\n\n> > Signed-off-by: David Disseldorp \n> > ---\n> > builtin/notes.c | 5 +++--\n> > 1 file changed, 3 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/builtin/notes.c b/builtin/notes.c\n> > index d9c356e354..3ccb3eb602 100644\n> > --- a/builtin/notes.c\n> > +++ b/builtin/notes.c\n> > @@ -282,6 +282,7 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n> > \tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n> > \td->messages[d->msg_nr - 1] = msg;\n> > \tmsg->stripspace = STRIPSPACE;\n> > +\td->given = 1;\n> > \treturn 0;\n> > }\n> > \n> > @@ -302,6 +303,7 @@ static int parse_file_arg(const struct option *opt, const char *arg, int unset)\n> > \tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n> > \td->messages[d->msg_nr - 1] = msg;\n> > \tmsg->stripspace = STRIPSPACE;\n> > +\td->given = 1;\n> > \treturn 0;\n> > }\n> > \n> > @@ -335,6 +337,7 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)\n> > \tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n> > \td->messages[d->msg_nr - 1] = msg;\n> > \tmsg->stripspace = NO_STRIPSPACE;\n> > +\td->given = 1;\n> > \treturn 0;\n> > } \n> \n> All the above places that resurrects setting d.given looks OK, but\n> it makes me wonder if you need to add them in the first place.\n> \n> Wouldn't it be sufficient to see if d->msg_nr is non-zero after all\n> the parsing is done? If any of the message came from \"-c\", a\n> separate flag d->use_editor is set to force you run the editor, and\n> otherwise you'd not be using the editor, right?\n> \n> > @@ -515,7 +518,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> Here is what I mean. If you didn't do any of the \"d->given = 1\"\n> above during parsing, and instead say\n> \n> \tif (d.msg_nr)\n> \t\tconcat_messages(&d);\n> \td.given = d.msg_nr;\n> \n> shouldn't it be sufficient to convince prepare_note_data() to do the\n> right thing?\n\nYes, I also noticed that msg_nr could also be used for this, but chose\nto revert back to the old given logic for clarity. I'll revisit the\nmsg_nr approach for v2.\n\nThanks for the feedback,\nDavid\n"}]}