{"thread":{"id":"58620","subject":"[RFC PATCH 0/2] notes.c: introduce \"--no-blankline\" option","startedAt":"2022-10-13T05:57:16Z","lastAt":"2023-02-22T23:15:57Z","messageCount":91,"participants":["Teng Long","Junio C Hamano","Ævar Arnfjörð Bjarmason","Phillip Wood","Eric Sunshine","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"464793","messageId":"20221013055654.39628-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":null,"subject":"[RFC PATCH 0/2] notes.c: introduce \"--no-blankline\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-10-13T05:56:52Z","receivedAt":"2022-10-13T05:57:16Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nThis RFC patchset includes two patches.\n\nOne aims to introduce a new option \"--no-blankline\" for \"git-notes-append\". The\nanother one tries to fix the inaccurate output when target and append note are\nboth empty.\n\nThanks.\n\nTeng Long (2):\n notes.c: introduce \"--no-blankline\" option\n notes.c: fixed tip when target and append note are both empty\n\n Documentation/git-notes.txt | 10 ++++++++--\n builtin/notes.c | 18 +++++++++++++++---\n t/t3301-notes.sh | 15 ++++++++++++++-\n 3 files changed, 37 insertions(+), 6 deletions(-)\n\n-- \n2.38.0.rc2\n\n"},{"id":"464794","messageId":"20221013055654.39628-2-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"20221013055654.39628-1-tenglong.tl@alibaba-inc.com","subject":"[RFC PATCH 1/2] notes.c: introduce \"--no-blankline\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-10-13T05:56:53Z","receivedAt":"2022-10-13T05:57:21Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nWhen appending to a given object which has note and if the appended\nnote is not empty too, we will insert a blank line at first. The\nblank line serves as a split line, but sometimes we just want to\nomit it then append on the heels of the target note.\n\nSigned-off-by: Teng Long \n---\n Documentation/git-notes.txt | 10 ++++++++--\n builtin/notes.c | 5 ++++-\n t/t3301-notes.sh | 12 ++++++++++++\n 3 files changed, 24 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\nindex efbc10f0f5..fd0fc3d9c7 100644\n--- a/Documentation/git-notes.txt\n+++ b/Documentation/git-notes.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n 'git notes' [list []]\n 'git notes' add [-f] [--allow-empty] [-F | -m | (-c | -C) ] []\n 'git notes' copy [-f] ( --stdin | [] )\n-'git notes' append [--allow-empty] [-F | -m | (-c | -C) ] []\n+'git notes' append [--allow-empty] [--no-blankline] [-F | -m | (-c | -C) ] []\n 'git notes' edit [--allow-empty] []\n 'git notes' show []\n 'git notes' merge [-v | -q] [-s ] \n@@ -86,7 +86,9 @@ the command can read the input given to the `post-rewrite` hook.)\n \n append::\n \tAppend to the notes of an existing object (defaults to HEAD).\n-\tCreates a new notes object if needed.\n+\tCreates a new notes object if needed. If the note of the given\n+\tobject and the note to be appended are not empty, a blank line\n+\twill be inserted between them.\n \n edit::\n \tEdit the notes for a given object (defaults to HEAD).\n@@ -159,6 +161,10 @@ OPTIONS\n \tAllow an empty note object to be stored. The default behavior is\n \tto automatically remove empty notes.\n \n+--no-blank-line::\n+\tWhen appending note, do not insert a blank line between\n+\tthe note of given object and the note to be appended.\n+\n --ref ::\n \tManipulate the notes tree in . This overrides\n \t`GIT_NOTES_REF` and the \"core.notesRef\" configuration. The ref\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex be51f69225..1ca0476a27 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -562,6 +562,7 @@ static int copy(int argc, const char **argv, const char *prefix)\n static int append_edit(int argc, const char **argv, const char *prefix)\n {\n \tint allow_empty = 0;\n+\tint no_blankline = 0;\n \tconst char *object_ref;\n \tstruct notes_tree *t;\n \tstruct object_id object, new_note;\n@@ -584,6 +585,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\t\tparse_reuse_arg),\n \t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n \t\t\tN_(\"allow storing empty note\")),\n+\t\tOPT_BOOL(0, \"no-blankline\", &no_blankline,\n+\t\t\tN_(\"do not initially add a blank line\")),\n \t\tOPT_END()\n \t};\n \tint edit = !strcmp(argv[0], \"edit\");\n@@ -619,7 +622,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tchar *prev_buf = read_object_file(note, &type, &size);\n \n \t\tstrbuf_grow(&d.buf, size + 1);\n-\t\tif (d.buf.len && prev_buf && size)\n+\t\tif (!no_blankline && d.buf.len && prev_buf && size)\n \t\t\tstrbuf_insertstr(&d.buf, 0, \"\\n\");\n \t\tif (prev_buf && size)\n \t\t\tstrbuf_insert(&d.buf, 0, prev_buf, size);\ndiff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\nindex 3288aaec7d..43ac3feb78 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -521,12 +521,24 @@ test_expect_success 'listing non-existing notes fails' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'append to existing note without a beginning blank line' '\n+\tcat >expect <<-EOF &&\n+\t\tInitial set of notes\n+\t\tAppended notes\n+\tEOF\n+\tgit notes add -m \"Initial set of notes\" &&\n+\tgit notes append --no-blankline -m \"Appended notes\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'append to existing note with \"git notes append\"' '\n \tcat >expect <<-EOF &&\n \t\tInitial set of notes\n \n \t\tMore notes appended with git notes append\n \tEOF\n+\tgit notes remove HEAD &&\n \tgit notes add -m \"Initial set of notes\" &&\n \tgit notes append -m \"More notes appended with git notes append\" &&\n \tgit notes show >actual &&\n-- \n2.38.0.rc2\n\n"},{"id":"464795","messageId":"20221013055654.39628-3-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"20221013055654.39628-1-tenglong.tl@alibaba-inc.com","subject":"[RFC PATCH 2/2] notes.c: fixed tip when target and append note are both empty","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-10-13T05:56:54Z","receivedAt":"2022-10-13T05:57:29Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nWhen \"git notes append \" is executed, if there is no note in\nthe given object and the appended note is empty, the command line\nprompt will be as follows:\n\n \"Removing note for object \"\n\nActually, this tip is not that accurate, because there is no note in\nthe original , and it also does no remove work on the notes\nreference, so let's fix this and give the correct tip.\n\nSigned-off-by: Teng Long \n---\n builtin/notes.c | 13 +++++++++++--\n t/t3301-notes.sh | 3 ++-\n 2 files changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 1ca0476a27..cc1e3aa2b6 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -567,9 +567,10 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \tstruct notes_tree *t;\n \tstruct object_id object, new_note;\n \tconst struct object_id *note;\n-\tchar *logmsg;\n+\tchar *logmsg = NULL;\n \tconst char * const *usage;\n \tstruct note_data d = { 0, 0, NULL, STRBUF_INIT };\n+\tstruct note_data cp = { 0, 0, NULL, STRBUF_INIT };\n \tstruct option options[] = {\n \t\tOPT_CALLBACK_F('m', \"message\", &d, N_(\"message\"),\n \t\t\tN_(\"note contents as a string\"), PARSE_OPT_NONEG,\n@@ -615,6 +616,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \n \tprepare_note_data(&object, &d, edit && note ? note : NULL);\n \n+\tstrbuf_addbuf(&cp.buf, &d.buf);\n+\n \tif (note && !edit) {\n \t\t/* Append buf to previous note contents */\n \t\tunsigned long size;\n@@ -634,16 +637,22 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tif (add_note(t, &object, &new_note, combine_notes_overwrite))\n \t\t\tBUG(\"combine_notes_overwrite failed\");\n \t\tlogmsg = xstrfmt(\"Notes added by 'git notes %s'\", argv[0]);\n+\t\tcommit_notes(the_repository, t, logmsg);\n+\t} else if (!cp.buf.len) {\n+\t\tfprintf(stderr,\n+\t\t\t_(\"Both original and appended notes are empty in %s, do nothing\\n\"),\n+\t\t\toid_to_hex(&object));\n \t} else {\n \t\tfprintf(stderr, _(\"Removing note for object %s\\n\"),\n \t\t\toid_to_hex(&object));\n \t\tremove_note(t, object.hash);\n \t\tlogmsg = xstrfmt(\"Notes removed by 'git notes %s'\", argv[0]);\n+\t\tcommit_notes(the_repository, t, logmsg);\n \t}\n-\tcommit_notes(the_repository, t, logmsg);\n \n \tfree(logmsg);\n \tfree_note_data(&d);\n+\tfree_note_data(&cp);\n \tfree_notes(t);\n \treturn 0;\n }\ndiff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\nindex 43ac3feb78..967e6bfb67 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -574,7 +574,8 @@ test_expect_success 'git notes append == add when there is no existing note' '\n test_expect_success 'appending empty string to non-existing note does not create note' '\n \tgit notes remove HEAD &&\n \ttest_must_fail git notes list HEAD &&\n-\tgit notes append -m \"\" &&\n+\tgit notes append -m \"\" >output 2>&1 &&\n+\tgrep \"Both original and appended notes are empty\" output &&\n \ttest_must_fail git notes list HEAD\n '\n \n-- \n2.38.0.rc2\n\n"},{"id":"464796","messageId":"xmqqsfjsi7eq.fsf@gitster.g","threadId":"58620","inReplyTo":"20221013055654.39628-2-tenglong.tl@alibaba-inc.com","subject":"Re: [RFC PATCH 1/2] notes.c: introduce \"--no-blankline\" option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-13T06:06:53Z","receivedAt":"2022-10-13T06:07:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n> +--no-blank-line::\n> +\tWhen appending note, do not insert a blank line between\n> +\tthe note of given object and the note to be appended.\n> +\n\n--blank-line::\n--no-blank-line::\n\tControls if a blank line to split paragraphs is inserted\n when appending (the default is true).\n\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index be51f69225..1ca0476a27 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -562,6 +562,7 @@ static int copy(int argc, const char **argv, const char *prefix)\n> static int append_edit(int argc, const char **argv, const char *prefix)\n> {\n> \tint allow_empty = 0;\n> +\tint no_blankline = 0;\n\nUse\n\n\tint blankline = 1;\n\nto avoid double negative, which is confusing and error prone.\n\n> @@ -584,6 +585,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> \t\t\tparse_reuse_arg),\n> \t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n> \t\t\tN_(\"allow storing empty note\")),\n> +\t\tOPT_BOOL(0, \"no-blankline\", &no_blankline,\n> +\t\t\tN_(\"do not initially add a blank line\")),\n\n\tOPT_BOOL(0, \"blank-line\", &blankline,\n\t\t N_(\"insert paragraph break before appending to an existing note\")),\n\n> @@ -619,7 +622,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> \t\tchar *prev_buf = read_object_file(note, &type, &size);\n> \n> \t\tstrbuf_grow(&d.buf, size + 1);\n> -\t\tif (d.buf.len && prev_buf && size)\n> +\t\tif (!no_blankline && d.buf.len && prev_buf && size)\n> \t\t\tstrbuf_insertstr(&d.buf, 0, \"\\n\");\n\nThen, the conditional would read more naturally without double\nnegation.\n\n\t\tif (blank_line && d.buf.len && prev_buf && size)\n\nI do not know and I am not judging (yet) if the goal of the patch is\nsensible (in other words, if we should have such an option), but if\nwe were to do so, I would expect the implementation to look more\nlike what I outlined above.\n\nThanks.\n"},{"id":"464813","messageId":"221013.861qrcm5kx.gmgdl@evledraar.gmail.com","threadId":"58620","inReplyTo":"20221013055654.39628-2-tenglong.tl@alibaba-inc.com","subject":"Re: [RFC PATCH 1/2] notes.c: introduce \"--no-blankline\" option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-13T09:31:49Z","receivedAt":"2022-10-13T09:32:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Oct 13 2022, Teng Long wrote:\n\n> From: Teng Long \n> [...]\n> +test_expect_success 'append to existing note without a beginning blank line' '\n> +\tcat >expect <<-EOF &&\n\nUse <<-\\EOF here.\n\n> +\t\tInitial set of notes\n> +\t\tAppended notes\n\nWe usually indent the \"EOF\" body the same as the \"cat\", but...\n\n> +\tEOF\n> +\tgit notes add -m \"Initial set of notes\" &&\n> +\tgit notes append --no-blankline -m \"Appended notes\" &&\n> +\tgit notes show >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_expect_success 'append to existing note with \"git notes append\"' '\n> \tcat >expect <<-EOF &&\n> \t\tInitial set of notes\n> \n> \t\tMore notes appended with git notes append\n> \tEOF\n\n... I see this test might be an odd one out, so this is fine.\n"},{"id":"464814","messageId":"221013.86wn94kqq1.gmgdl@evledraar.gmail.com","threadId":"58620","inReplyTo":"20221013055654.39628-3-tenglong.tl@alibaba-inc.com","subject":"Re: [RFC PATCH 2/2] notes.c: fixed tip when target and append note are both empty","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-13T09:36:42Z","receivedAt":"2022-10-13T09:39:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Oct 13 2022, Teng Long wrote:\n\n> From: Teng Long \n>\n> When \"git notes append \" is executed, if there is no note in\n> the given object and the appended note is empty, the command line\n> prompt will be as follows:\n>\n> \"Removing note for object \"\n>\n> Actually, this tip is not that accurate, because there is no note in\n> the original , and it also does no remove work on the notes\n> reference, so let's fix this and give the correct tip.\n>\n> Signed-off-by: Teng Long \n> ---\n> builtin/notes.c | 13 +++++++++++--\n> t/t3301-notes.sh | 3 ++-\n> 2 files changed, 13 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index 1ca0476a27..cc1e3aa2b6 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -567,9 +567,10 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> \tstruct notes_tree *t;\n> \tstruct object_id object, new_note;\n> \tconst struct object_id *note;\n> -\tchar *logmsg;\n> +\tchar *logmsg = NULL;\n\nHrm, interesting that (at least my) gcc doesn't catch if we don't\nNULL-initialize this, but -fanalyzer does (usually it's not needed for\nsuch trivial cases0. Anyawy...\n\n> \tconst char * const *usage;\n> \tstruct note_data d = { 0, 0, NULL, STRBUF_INIT };\n> +\tstruct note_data cp = { 0, 0, NULL, STRBUF_INIT };\n\nThis is probably better \"fixed while at it\" to set both to use \"{ .buf =\nSTRBUF_INIT }\", rather than copying the pre-C99 pattern.\n"},{"id":"464815","messageId":"c0211f35-bb26-7ca8-6f9d-a62507e55e8a@gmail.com","threadId":"58620","inReplyTo":"221013.86wn94kqq1.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH 2/2] notes.c: fixed tip when target and append note are both empty","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-10-13T10:10:35Z","receivedAt":"2022-10-13T10:10:48Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 13/10/2022 10:36, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Thu, Oct 13 2022, Teng Long wrote:\n> \n>> From: Teng Long \n>>\n>> When \"git notes append \" is executed, if there is no note in\n>> the given object and the appended note is empty, the command line\n>> prompt will be as follows:\n>>\n>> \"Removing note for object \"\n>>\n>> Actually, this tip is not that accurate, because there is no note in\n>> the original , and it also does no remove work on the notes\n>> reference, so let's fix this and give the correct tip.\n>>\n>> Signed-off-by: Teng Long \n>> ---\n>> builtin/notes.c | 13 +++++++++++--\n>> t/t3301-notes.sh | 3 ++-\n>> 2 files changed, 13 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/builtin/notes.c b/builtin/notes.c\n>> index 1ca0476a27..cc1e3aa2b6 100644\n>> --- a/builtin/notes.c\n>> +++ b/builtin/notes.c\n>> @@ -567,9 +567,10 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n>> \tstruct notes_tree *t;\n>> \tstruct object_id object, new_note;\n>> \tconst struct object_id *note;\n>> -\tchar *logmsg;\n>> +\tchar *logmsg = NULL;\n> \n> Hrm, interesting that (at least my) gcc doesn't catch if we don't\n> NULL-initialize this, but -fanalyzer does (usually it's not needed for\n> such trivial cases0. Anyawy...\n\nI don't think its written to if we take the 'else if' branch added by \nthis patch so we need to initialize it for the free() at the end.\n\n>> \tconst char * const *usage;\n>> \tstruct note_data d = { 0, 0, NULL, STRBUF_INIT };\n>> +\tstruct note_data cp = { 0, 0, NULL, STRBUF_INIT };\n> \n> This is probably better \"fixed while at it\" to set both to use \"{ .buf =\n> STRBUF_INIT }\", rather than copying the pre-C99 pattern.\n\nWe only seem to be using cp.buf.len so we can test check if the original \nnote was empty so I think it would be better just to add\n\n\tint note_was_empty;\n\n`\t...\n\n\tnote_was_empty = !d.buf.len\n\ninstead.\n\nBest Wishes\n\nPhillip\n"},{"id":"464817","messageId":"221013.86sfjsknlw.gmgdl@evledraar.gmail.com","threadId":"58620","inReplyTo":"c0211f35-bb26-7ca8-6f9d-a62507e55e8a@gmail.com","subject":"Re: [RFC PATCH 2/2] notes.c: fixed tip when target and append note are both empty","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-13T10:23:50Z","receivedAt":"2022-10-13T10:46:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Oct 13 2022, Phillip Wood wrote:\n\n> On 13/10/2022 10:36, Ævar Arnfjörð Bjarmason wrote:\n>> On Thu, Oct 13 2022, Teng Long wrote:\n>> \n>>> From: Teng Long \n>>>\n>>> When \"git notes append \" is executed, if there is no note in\n>>> the given object and the appended note is empty, the command line\n>>> prompt will be as follows:\n>>>\n>>> \"Removing note for object \"\n>>>\n>>> Actually, this tip is not that accurate, because there is no note in\n>>> the original , and it also does no remove work on the notes\n>>> reference, so let's fix this and give the correct tip.\n>>>\n>>> Signed-off-by: Teng Long \n>>> ---\n>>> builtin/notes.c | 13 +++++++++++--\n>>> t/t3301-notes.sh | 3 ++-\n>>> 2 files changed, 13 insertions(+), 3 deletions(-)\n>>>\n>>> diff --git a/builtin/notes.c b/builtin/notes.c\n>>> index 1ca0476a27..cc1e3aa2b6 100644\n>>> --- a/builtin/notes.c\n>>> +++ b/builtin/notes.c\n>>> @@ -567,9 +567,10 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n>>> \tstruct notes_tree *t;\n>>> \tstruct object_id object, new_note;\n>>> \tconst struct object_id *note;\n>>> -\tchar *logmsg;\n>>> +\tchar *logmsg = NULL;\n>> Hrm, interesting that (at least my) gcc doesn't catch if we don't\n>> NULL-initialize this, but -fanalyzer does (usually it's not needed for\n>> such trivial cases0. Anyawy...\n>\n> I don't think its written to if we take the 'else if' branch added by\n> this patch so we need to initialize it for the free() at the end.\n\nYes, sorry about not being clear. It *does* need to be uninitialized, I\nwas just narrating my surprise at this not being a case where my\ncompiler caught it when I was locally testing this.\n\nThen I remembered this is one of those cases where clang is slightly\nbetter at analysis, gcc without -fanalyzer says nothing, but clang by\ndefault will note (if you remove the NULL initialization here):\n\n\tbuiltin/notes.c:641:13: error: variable 'logmsg' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]\n\t } else if (!cp.buf.len) {\n\t ^~~~~~~~~~~\n\tbuiltin/notes.c:653:7: note: uninitialized use occurs here\n\t free(logmsg);\n\t ^~~~~~\n\tbuiltin/notes.c:641:9: note: remove the 'if' if its condition is always false\n\t } else if (!cp.buf.len) {\n\t ^~~~~~~~~~~~~~~~~~\n\tbuiltin/notes.c:570:14: note: initialize the variable 'logmsg' to silence this warning\n\t char *logmsg;\n\t ^\n\t = NULL\n\nAnyway, nothing needs to be changed about the code here, sorry about the\ndistraction. I should have left it at something like \"this NULL\ninitialization is needed\".\n\n>>> \tconst char * const *usage;\n>>> \tstruct note_data d = { 0, 0, NULL, STRBUF_INIT };\n>>> +\tstruct note_data cp = { 0, 0, NULL, STRBUF_INIT };\n>> This is probably better \"fixed while at it\" to set both to use \"{\n>> .buf =\n>> STRBUF_INIT }\", rather than copying the pre-C99 pattern.\n>\n> We only seem to be using cp.buf.len so we can test check if the\n> original note was empty so I think it would be better just to add\n>\n> \tint note_was_empty;\n>\n> `\t...\n>\n> \tnote_was_empty = !d.buf.len\n\nThat's a good catch, and one I didn't catch on my read-through, i.e. in\ngeneral this seems like a \"was the strbuf empty?\" pattern, before we\nre-append to it.\n\nBut playing with it further:\n\t\n\tdiff --git a/builtin/notes.c b/builtin/notes.c\n\tindex cc1e3aa2b6b..262bbffa375 100644\n\t--- a/builtin/notes.c\n\t+++ b/builtin/notes.c\n\t@@ -570,7 +570,6 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n\t \tchar *logmsg = NULL;\n\t \tconst char * const *usage;\n\t \tstruct note_data d = { 0, 0, NULL, STRBUF_INIT };\n\t-\tstruct note_data cp = { 0, 0, NULL, STRBUF_INIT };\n\t \tstruct option options[] = {\n\t \t\tOPT_CALLBACK_F('m', \"message\", &d, N_(\"message\"),\n\t \t\t\tN_(\"note contents as a string\"), PARSE_OPT_NONEG,\n\t@@ -616,8 +615,6 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n\t \n\t \tprepare_note_data(&object, &d, edit && note ? note : NULL);\n\t \n\t-\tstrbuf_addbuf(&cp.buf, &d.buf);\n\t-\n\t \tif (note && !edit) {\n\t \t\t/* Append buf to previous note contents */\n\t \t\tunsigned long size;\n\t@@ -638,21 +635,16 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n\t \t\t\tBUG(\"combine_notes_overwrite failed\");\n\t \t\tlogmsg = xstrfmt(\"Notes added by 'git notes %s'\", argv[0]);\n\t \t\tcommit_notes(the_repository, t, logmsg);\n\t-\t} else if (!cp.buf.len) {\n\t+\t} else if (!d.buf.len) {\n\t \t\tfprintf(stderr,\n\t \t\t\t_(\"Both original and appended notes are empty in %s, do nothing\\n\"),\n\t \t\t\toid_to_hex(&object));\n\t \t} else {\n\t-\t\tfprintf(stderr, _(\"Removing note for object %s\\n\"),\n\t-\t\t\toid_to_hex(&object));\n\t-\t\tremove_note(t, object.hash);\n\t-\t\tlogmsg = xstrfmt(\"Notes removed by 'git notes %s'\", argv[0]);\n\t-\t\tcommit_notes(the_repository, t, logmsg);\n\t+\t\tBUG(\"this is not reachable by any test now\");\n\t \t}\n\t \n\t \tfree(logmsg);\n\t \tfree_note_data(&d);\n\t-\tfree_note_data(&cp);\n\t \tfree_notes(t);\n\t \treturn 0;\n\t }\n\t\nThis 2/2 makes that \"else\" test-unreachable, so whatever else we do here\nwe should start by making sure that by adding the \"else if\" we still\nhave test coverage for the \"else\".\n"},{"id":"465033","messageId":"b5e97527-433c-53fe-356a-e20ab3e087d2@dunelm.org.uk","threadId":"58620","inReplyTo":"221013.86sfjsknlw.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH 2/2] notes.c: fixed tip when target and append note are both empty","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-10-15T19:40:52Z","receivedAt":"2022-10-15T19:40:54Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar\n\nOn 13/10/2022 11:23, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Thu, Oct 13 2022, Phillip Wood wrote:\n>>>> diff --git a/builtin/notes.c b/builtin/notes.c\n>>>> index 1ca0476a27..cc1e3aa2b6 100644\n>>>> --- a/builtin/notes.c\n>>>> +++ b/builtin/notes.c\n>>>> @@ -567,9 +567,10 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n>>>> \tstruct notes_tree *t;\n>>>> \tstruct object_id object, new_note;\n>>>> \tconst struct object_id *note;\n>>>> -\tchar *logmsg;\n>>>> +\tchar *logmsg = NULL;\n>>> Hrm, interesting that (at least my) gcc doesn't catch if we don't\n>>> NULL-initialize this, but -fanalyzer does (usually it's not needed for\n>>> such trivial cases0. Anyawy...\n>>\n>> I don't think its written to if we take the 'else if' branch added by\n>> this patch so we need to initialize it for the free() at the end.\n> \n> Yes, sorry about not being clear. It *does* need to be uninitialized, I\n> was just narrating my surprise at this not being a case where my\n> compiler caught it when I was locally testing this.\n\nAh I think I slightly misunderstood your comment - I agree it is \nsurprising that the compiler didn't catch that.\n\n> \t@@ -638,21 +635,16 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> \t \t\t\tBUG(\"combine_notes_overwrite failed\");\n> \t \t\tlogmsg = xstrfmt(\"Notes added by 'git notes %s'\", argv[0]);\n> \t \t\tcommit_notes(the_repository, t, logmsg);\n> \t-\t} else if (!cp.buf.len) {\n> \t+\t} else if (!d.buf.len) {\n> \t \t\tfprintf(stderr,\n> \t \t\t\t_(\"Both original and appended notes are empty in %s, do nothing\\n\"),\n> \t \t\t\toid_to_hex(&object));\n> \t \t} else {\n> \t-\t\tfprintf(stderr, _(\"Removing note for object %s\\n\"),\n> \t-\t\t\toid_to_hex(&object));\n> \t-\t\tremove_note(t, object.hash);\n> \t-\t\tlogmsg = xstrfmt(\"Notes removed by 'git notes %s'\", argv[0]);\n> \t-\t\tcommit_notes(the_repository, t, logmsg);\n> \t+\t\tBUG(\"this is not reachable by any test now\");\n> \t \t}\n> \t\n> This 2/2 makes that \"else\" test-unreachable, so whatever else we do here\n> we should start by making sure that by adding the \"else if\" we still\n> have test coverage for the \"else\".\n\nOh so we can just use d.buf.len directly - nicely spotted and kudos for \nchecking the test coverage. Looking at the existing tests they are \nchecking if an empty note is removed which suggests this patch is \nfailing to distinguish between an existing empty note and no note. I \nthink we probably need to be doing \"else if (note && d.buf.len)\" but \nI've not looked very closely.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"465096","messageId":"20221017131939.62888-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"xmqqsfjsi7eq.fsf@gitster.g","subject":"Re: [RFC PATCH 1/2] notes.c: introduce \"--no-blankline\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-10-17T13:19:39Z","receivedAt":"2022-10-17T13:19:51Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Junio C Hamano writes:\n\n> --blank-line::\n> --no-blank-line::\n> \tControls if a blank line to split paragraphs is inserted\n> when appending (the default is true).\n\nWill fix, \"OPT_BOOL\" will automatically deal the \"--no\" prefix, nice design.\n\n> Use\n>\n> \tint blankline = 1;\n>\n> to avoid double negative, which is confusing and error prone.\n\nPerfectly reasonable opinion, will fix.\n\n> \tOPT_BOOL(0, \"blank-line\", &blankline,\n> \t\t N_(\"insert paragraph break before appending to an existing note\")),\n> Then, the conditional would read more naturally without double\n> negation.\n>\n> \t\tif (blank_line && d.buf.len && prev_buf && size)\n\nWill apply.\n\n> I do not know and I am not judging (yet) if the goal of the patch is\n> sensible (in other words, if we should have such an option), but if\n> we were to do so, I would expect the implementation to look more\n> like what I outlined above.\n\nIn fact, as I was learning about this feature, I thought this parameter might be\nhelpful, so I tried to send this small patch and maybe I can get some input.\n\nThank you so much for taking the time to review this patch.\n"},{"id":"465098","messageId":"20221017133323.63330-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"221013.861qrcm5kx.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH 1/2] notes.c: introduce \"--no-blankline\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-10-17T13:33:23Z","receivedAt":"2022-10-17T13:33:32Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"\"Ævar Arnfjörð Bjarmason\" writes:\n\n> Use <<-\\EOF here.\n\nAs do not escape the heredoc, will apply.\n\n> We usually indent the \"EOF\" body the same as the \"cat\", but...\n> ... I see this test might be an odd one out, so this is fine.\n\nYes, the indent sometimes make a little confusion unless you want\nto keep it as \"<<\\EOF\".\n\nThanks for your meticulous reading!\n"},{"id":"465169","messageId":"20221018031144.73730-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"221013.86wn94kqq1.gmgdl@evledraar.gmail.com","subject":"Re: [RFC PATCH 2/2] notes.c: fixed tip when target and append note are both empty","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-10-18T03:11:44Z","receivedAt":"2022-10-18T03:17:23Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"\n\"Ævar Arnfjörð Bjarmason\" writes:\n\n> Hrm, interesting that (at least my) gcc doesn't catch if we don't\n> NULL-initialize this, but -fanalyzer does (usually it's not needed for\n> such trivial cases0. Anyawy...\n\nOn my local env the warnings shows , show I change the line (initialize with\nNULL to \"logmsg\").\n\nBut it seems like different as the last time I built... However now \"suggest\nbraces around initialization of subobject\" appears, is it normal or we should\nrepair this?\n\nbuiltin/merge-file.c:29:23: warning: suggest braces around initialization of subobject [-Wmissing-braces]\n mmfile_t mmfs[3] = { 0 };\n ^\n {}\nbuiltin/merge-file.c:31:20: warning: suggest braces around initialization of subobject [-Wmissing-braces]\n xmparam_t xmp = { 0 };\n ^\n {}\n2 warnings generated.\nbuiltin/notes.c:641:13: warning: variable 'logmsg' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]\n } else if (!cp.buf.len) {\n ^~~~~~~~~~~\nbuiltin/notes.c:653:7: note: uninitialized use occurs here\n free(logmsg);\n ^~~~~~\nbuiltin/notes.c:641:9: note: remove the 'if' if its condition is always false\n } else if (!cp.buf.len) {\n ^~~~~~~~~~~~~~~~~~\nbuiltin/notes.c:570:14: note: initialize the variable 'logmsg' to silence this warning\n char *logmsg;\n ^\n = NULL\n1 warning generated.\nbuiltin/submodule--helper.c:1749:56: warning: suggest braces around initialization of subobject [-Wmissing-braces]\n struct list_objects_filter_options filter_options = { 0 };\n ^\n {}\nbuiltin/submodule--helper.c:2623:56: warning: suggest braces around initialization of subobject [-Wmissing-braces]\n struct list_objects_filter_options filter_options = { 0 };\n ^\n {}\nbuiltin/unpack-objects.c:388:26: warning: suggest braces around initialization of subobject [-Wmissing-braces]\n git_zstream zstream = { 0 };\n ^\n {}\n\n\nMy gcc version:\n Apple clang version 11.0.0 (clang-1100.0.33.17)\n Target: x86_64-apple-darwin19.6.0\n Thread model: posix\n\n\n> > \tconst char * const *usage;\n> > \tstruct note_data d = { 0, 0, NULL, STRBUF_INIT };\n> > +\tstruct note_data cp = { 0, 0, NULL, STRBUF_INIT };\n>\n> This is probably better \"fixed while at it\" to set both to use \"{ .buf =\n> STRBUF_INIT }\", rather than copying the pre-C99 pattern.\n\n\nThanks for figuring this out, will fix both.\n"},{"id":"465170","messageId":"20221018032523.74253-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"c0211f35-bb26-7ca8-6f9d-a62507e55e8a@gmail.com","subject":"Re: [RFC PATCH 2/2] notes.c: fixed tip when target and append note are both empty","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-10-18T03:25:23Z","receivedAt":"2022-10-18T03:34:49Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Phillip Wood writes:\n\n> I don't think its written to if we take the 'else if' branch added by\n> this patch so we need to initialize it for the free() at the end.\n\nActually, I didn't get it totally (maybe because my English, sorry for that),\nbut indeed the 'else if' expose this problem out, so I think to initialize it\nis needed.\n\nThanks.\n\n\n> We only seem to be using cp.buf.len so we can test check if the original\n> note was empty so I think it would be better just to add\n>\n> \tint note_was_empty;\n>\n> `\t...\n>\n> \tnote_was_empty = !d.buf.len\n>\n> instead.\n\nYes, it actually does as you describe above, your suggestion makes it look\nbetter.\n\nThank you very much for your detailed review.\n"},{"id":"465177","messageId":"20221018080827.69517-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"c0211f35-bb26-7ca8-6f9d-a62507e55e8a@gmail.com","subject":"Re: [RFC PATCH 2/2] notes.c: fixed tip when target and append note are both empty","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-10-18T08:08:27Z","receivedAt":"2022-10-18T08:08:58Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Phillip Wood writes:\n\n> I don't think its written to if we take the 'else if' branch added by\n> this patch so we need to initialize it for the free() at the end.\n\nActually, I didn't get it totally (maybe because my English, sorry for that),\nbut indeed the 'else if' expose this problem out, so I think to initialize it\nis needed.\n\nThanks.\n\n> We only seem to be using cp.buf.len so we can test check if the original\n> note was empty so I think it would be better just to add\n>\n> \tint note_was_empty;\n>\n> `\t...\n>\n> \tnote_was_empty = !d.buf.len\n>\n> instead.\n\nYes, it actually does as you describe above, your suggestion makes it look\nbetter, will apply.\n\nThank you very much for your detailed review.\n"},{"id":"465179","messageId":"221018.864jw1jxfx.gmgdl@evledraar.gmail.com","threadId":"58620","inReplyTo":"20221018031144.73730-1-tenglong.tl@alibaba-inc.com","subject":"Re: [RFC PATCH 2/2] notes.c: fixed tip when target and append note are both empty","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-18T09:23:10Z","receivedAt":"2022-10-18T09:25:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 18 2022, Teng Long wrote:\n\n> \"Ævar Arnfjörð Bjarmason\" writes:\n>\n>> Hrm, interesting that (at least my) gcc doesn't catch if we don't\n>> NULL-initialize this, but -fanalyzer does (usually it's not needed for\n>> such trivial cases0. Anyawy...\n>\n> On my local env the warnings shows , show I change the line (initialize with\n> NULL to \"logmsg\").\n>\n> But it seems like different as the last time I built... However now \"suggest\n> braces around initialization of subobject\" appears, is it normal or we should\n> repair this?\n>\n> builtin/merge-file.c:29:23: warning: suggest braces around initialization of subobject [-Wmissing-braces]\n> mmfile_t mmfs[3] = { 0 };\n> ^\n> {}\n> builtin/merge-file.c:31:20: warning: suggest braces around initialization of subobject [-Wmissing-braces]\n> xmparam_t xmp = { 0 };\n> ^\n> {}\n\nThe fix for this is in \"next\": 54795d37d9e (config.mak.dev: disable\nsuggest braces error on old clang versions, 2022-10-10)\n\n\n> 2 warnings generated.\n> builtin/notes.c:641:13: warning: variable 'logmsg' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]\n> } else if (!cp.buf.len) {\n> ^~~~~~~~~~~\n> builtin/notes.c:653:7: note: uninitialized use occurs here\n> free(logmsg);\n> ^~~~~~\n> builtin/notes.c:641:9: note: remove the 'if' if its condition is always false\n> } else if (!cp.buf.len) {\n> ^~~~~~~~~~~~~~~~~~\n> builtin/notes.c:570:14: note: initialize the variable 'logmsg' to silence this warning\n> char *logmsg;\n> ^\n> = NULL\n> 1 warning generated.\n\nYes, we should initialize it to NULL, so this is the expected\nwarning. Clang spots it.\n"},{"id":"466658","messageId":"cover.1667828335.git.dyroneteng@gmail.com","threadId":"58620","inReplyTo":"20221013055654.39628-1-tenglong.tl@alibaba-inc.com","subject":"[PATCH v2 0/3] notes.c: introduce \"--blank-line\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-07T13:57:02Z","receivedAt":"2022-11-07T13:57:17Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nDiff from RFC Patch v1:\n\n* optimize the commit-msg and docs of introducing new \"--blank-line\" option.\n\n* drop unreachable code in \"append_edit()\". Ævar found that some code has been\nunreachable in patch v1. I think it's because, after the commit \"notes.c: fixed\ntip when target and append note are both empty\", for example in this patch, the\nsituation of \"removing an existing note\" should be impossible unless a BUG when\ntrying to do append. The tests are passed, but I'm not sure I fully understand\nthe original design.\n\nThanks to Junio C Hamano, Ævar Arnfjörð Bjarmason and Phillip Wood for\nthe help in v1.\n\nTeng Long (3):\n notes.c: introduce \"--blank-line\" option\n notes.c: fixed tip when target and append note are both empty\n notes.c: drop unreachable code in \"append_edit()\"\n\n Documentation/git-notes.txt | 11 +++++++++--\n builtin/notes.c | 27 +++++++++++++++++++--------\n t/t3301-notes.sh | 15 ++++++++++++++-\n 3 files changed, 42 insertions(+), 11 deletions(-)\n\nRange-diff against v1:\n1: d69bd0a011 ! 1: 2381947abd notes.c: introduce \"--no-blankline\" option\n @@ Metadata\n Author: Teng Long \n \n ## Commit message ##\n - notes.c: introduce \"--no-blankline\" option\n + notes.c: introduce \"--blank-line\" option\n \n When appending to a given object which has note and if the appended\n note is not empty too, we will insert a blank line at first. The\n blank line serves as a split line, but sometimes we just want to\n - omit it then append on the heels of the target note.\n + omit it then append on the heels of the target note. So, we add\n + a new \"OPT_BOOL()\" option to determain whether a new blank line\n + is insert at first.\n \n Signed-off-by: Teng Long \n \n @@ Documentation/git-notes.txt: SYNOPSIS\n 'git notes' add [-f] [--allow-empty] [-F | -m | (-c | -C) ] []\n 'git notes' copy [-f] ( --stdin | [] )\n -'git notes' append [--allow-empty] [-F | -m | (-c | -C) ] []\n -+'git notes' append [--allow-empty] [--no-blankline] [-F | -m | (-c | -C) ] []\n ++'git notes' append [--allow-empty] [--blank-line] [-F | -m | (-c | -C) ] []\n 'git notes' edit [--allow-empty] []\n 'git notes' show []\n 'git notes' merge [-v | -q] [-s ] \n @@ Documentation/git-notes.txt: OPTIONS\n \tAllow an empty note object to be stored. The default behavior is\n \tto automatically remove empty notes.\n \n ++--blank-line::\n +--no-blank-line::\n -+\tWhen appending note, do not insert a blank line between\n -+\tthe note of given object and the note to be appended.\n ++\tControls if a blank line to split paragraphs is inserted\n ++\twhen appending (the default is true).\n +\n --ref ::\n \tManipulate the notes tree in . This overrides\n @@ builtin/notes.c: static int copy(int argc, const char **argv, const char *prefix\n static int append_edit(int argc, const char **argv, const char *prefix)\n {\n \tint allow_empty = 0;\n -+\tint no_blankline = 0;\n ++\tint blankline = 1;\n \tconst char *object_ref;\n \tstruct notes_tree *t;\n \tstruct object_id object, new_note;\n @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char\n \t\t\tparse_reuse_arg),\n \t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n \t\t\tN_(\"allow storing empty note\")),\n -+\t\tOPT_BOOL(0, \"no-blankline\", &no_blankline,\n -+\t\t\tN_(\"do not initially add a blank line\")),\n ++\t\tOPT_BOOL(0, \"blank-line\", &blankline,\n ++\t\t\tN_(\"insert paragraph break before appending to an existing note\")),\n \t\tOPT_END()\n \t};\n \tint edit = !strcmp(argv[0], \"edit\");\n @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char\n \n \t\tstrbuf_grow(&d.buf, size + 1);\n -\t\tif (d.buf.len && prev_buf && size)\n -+\t\tif (!no_blankline && d.buf.len && prev_buf && size)\n ++\t\tif (blankline && d.buf.len && prev_buf && size)\n \t\t\tstrbuf_insertstr(&d.buf, 0, \"\\n\");\n \t\tif (prev_buf && size)\n \t\t\tstrbuf_insert(&d.buf, 0, prev_buf, size);\n @@ t/t3301-notes.sh: test_expect_success 'listing non-existing notes fails' '\n '\n \n +test_expect_success 'append to existing note without a beginning blank line' '\n -+\tcat >expect <<-EOF &&\n ++\tcat >expect <<-\\EOF &&\n +\t\tInitial set of notes\n +\t\tAppended notes\n +\tEOF\n +\tgit notes add -m \"Initial set of notes\" &&\n -+\tgit notes append --no-blankline -m \"Appended notes\" &&\n ++\tgit notes append --no-blank-line -m \"Appended notes\" &&\n +\tgit notes show >actual &&\n +\ttest_cmp expect actual\n +'\n2: c581cb24b6 ! 2: 5dbe014a09 notes.c: fixed tip when target and append note are both empty\n @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char\n -\tchar *logmsg;\n +\tchar *logmsg = NULL;\n \tconst char * const *usage;\n - \tstruct note_data d = { 0, 0, NULL, STRBUF_INIT };\n -+\tstruct note_data cp = { 0, 0, NULL, STRBUF_INIT };\n +-\tstruct note_data d = { 0, 0, NULL, STRBUF_INIT };\n ++\tstruct note_data d = {\n ++\t\t.given = 0,\n ++\t\t.use_editor = 0,\n ++\t\t.edit_path = NULL,\n ++\t\t.buf = STRBUF_INIT\n ++\t};\n ++\n \tstruct option options[] = {\n \t\tOPT_CALLBACK_F('m', \"message\", &d, N_(\"message\"),\n \t\t\tN_(\"note contents as a string\"), PARSE_OPT_NONEG,\n @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char *prefix)\n - \n - \tprepare_note_data(&object, &d, edit && note ? note : NULL);\n - \n -+\tstrbuf_addbuf(&cp.buf, &d.buf);\n -+\n - \tif (note && !edit) {\n - \t\t/* Append buf to previous note contents */\n - \t\tunsigned long size;\n -@@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tif (add_note(t, &object, &new_note, combine_notes_overwrite))\n \t\t\tBUG(\"combine_notes_overwrite failed\");\n \t\tlogmsg = xstrfmt(\"Notes added by 'git notes %s'\", argv[0]);\n +\t\tcommit_notes(the_repository, t, logmsg);\n -+\t} else if (!cp.buf.len) {\n ++\t} else if (!d.buf.len && !note) {\n +\t\tfprintf(stderr,\n +\t\t\t_(\"Both original and appended notes are empty in %s, do nothing\\n\"),\n +\t\t\toid_to_hex(&object));\n @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char\n \n \tfree(logmsg);\n \tfree_note_data(&d);\n -+\tfree_note_data(&cp);\n - \tfree_notes(t);\n - \treturn 0;\n - }\n \n ## t/t3301-notes.sh ##\n @@ t/t3301-notes.sh: test_expect_success 'git notes append == add when there is no existing note' '\n-: ---------- > 3: 2475ea0c04 notes.c: drop unreachable code in \"append_edit()\"\n-- \n2.38.1.383.ge7205ac0a40.dirty\n\n"},{"id":"466659","messageId":"2381947abdd6b965c02e114af297fc908ed3132b.1667828335.git.dyroneteng@gmail.com","threadId":"58620","inReplyTo":"cover.1667828335.git.dyroneteng@gmail.com","subject":"[PATCH v2 1/3] notes.c: introduce \"--blank-line\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-07T13:57:03Z","receivedAt":"2022-11-07T13:57:20Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nWhen appending to a given object which has note and if the appended\nnote is not empty too, we will insert a blank line at first. The\nblank line serves as a split line, but sometimes we just want to\nomit it then append on the heels of the target note. So, we add\na new \"OPT_BOOL()\" option to determain whether a new blank line\nis insert at first.\n\nSigned-off-by: Teng Long \n---\n Documentation/git-notes.txt | 11 +++++++++--\n builtin/notes.c | 5 ++++-\n t/t3301-notes.sh | 12 ++++++++++++\n 3 files changed, 25 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\nindex efbc10f0f5..43770ddf84 100644\n--- a/Documentation/git-notes.txt\n+++ b/Documentation/git-notes.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n 'git notes' [list []]\n 'git notes' add [-f] [--allow-empty] [-F | -m | (-c | -C) ] []\n 'git notes' copy [-f] ( --stdin | [] )\n-'git notes' append [--allow-empty] [-F | -m | (-c | -C) ] []\n+'git notes' append [--allow-empty] [--blank-line] [-F | -m | (-c | -C) ] []\n 'git notes' edit [--allow-empty] []\n 'git notes' show []\n 'git notes' merge [-v | -q] [-s ] \n@@ -86,7 +86,9 @@ the command can read the input given to the `post-rewrite` hook.)\n \n append::\n \tAppend to the notes of an existing object (defaults to HEAD).\n-\tCreates a new notes object if needed.\n+\tCreates a new notes object if needed. If the note of the given\n+\tobject and the note to be appended are not empty, a blank line\n+\twill be inserted between them.\n \n edit::\n \tEdit the notes for a given object (defaults to HEAD).\n@@ -159,6 +161,11 @@ OPTIONS\n \tAllow an empty note object to be stored. The default behavior is\n \tto automatically remove empty notes.\n \n+--blank-line::\n+--no-blank-line::\n+\tControls if a blank line to split paragraphs is inserted\n+\twhen appending (the default is true).\n+\n --ref ::\n \tManipulate the notes tree in . This overrides\n \t`GIT_NOTES_REF` and the \"core.notesRef\" configuration. The ref\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex be51f69225..f0fa337e8c 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -562,6 +562,7 @@ static int copy(int argc, const char **argv, const char *prefix)\n static int append_edit(int argc, const char **argv, const char *prefix)\n {\n \tint allow_empty = 0;\n+\tint blankline = 1;\n \tconst char *object_ref;\n \tstruct notes_tree *t;\n \tstruct object_id object, new_note;\n@@ -584,6 +585,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\t\tparse_reuse_arg),\n \t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n \t\t\tN_(\"allow storing empty note\")),\n+\t\tOPT_BOOL(0, \"blank-line\", &blankline,\n+\t\t\tN_(\"insert paragraph break before appending to an existing note\")),\n \t\tOPT_END()\n \t};\n \tint edit = !strcmp(argv[0], \"edit\");\n@@ -619,7 +622,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tchar *prev_buf = read_object_file(note, &type, &size);\n \n \t\tstrbuf_grow(&d.buf, size + 1);\n-\t\tif (d.buf.len && prev_buf && size)\n+\t\tif (blankline && d.buf.len && prev_buf && size)\n \t\t\tstrbuf_insertstr(&d.buf, 0, \"\\n\");\n \t\tif (prev_buf && size)\n \t\t\tstrbuf_insert(&d.buf, 0, prev_buf, size);\ndiff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\nindex 3288aaec7d..76beafdeb8 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -521,12 +521,24 @@ test_expect_success 'listing non-existing notes fails' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'append to existing note without a beginning blank line' '\n+\tcat >expect <<-\\EOF &&\n+\t\tInitial set of notes\n+\t\tAppended notes\n+\tEOF\n+\tgit notes add -m \"Initial set of notes\" &&\n+\tgit notes append --no-blank-line -m \"Appended notes\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'append to existing note with \"git notes append\"' '\n \tcat >expect <<-EOF &&\n \t\tInitial set of notes\n \n \t\tMore notes appended with git notes append\n \tEOF\n+\tgit notes remove HEAD &&\n \tgit notes add -m \"Initial set of notes\" &&\n \tgit notes append -m \"More notes appended with git notes append\" &&\n \tgit notes show >actual &&\n-- \n2.38.1.383.ge7205ac0a40.dirty\n\n"},{"id":"466660","messageId":"5dbe014a09cca6ffa01dc9f61d0ddc095d31d518.1667828335.git.dyroneteng@gmail.com","threadId":"58620","inReplyTo":"cover.1667828335.git.dyroneteng@gmail.com","subject":"[PATCH v2 2/3] notes.c: fixed tip when target and append note are both empty","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-07T13:57:04Z","receivedAt":"2022-11-07T13:57:28Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nWhen \"git notes append \" is executed, if there is no note in\nthe given object and the appended note is empty, the command line\nprompt will be as follows:\n\n \"Removing note for object \"\n\nActually, this tip is not that accurate, because there is no note in\nthe original , and it also does no remove work on the notes\nreference, so let's fix this and give the correct tip.\n\nSigned-off-by: Teng Long \n---\n builtin/notes.c | 17 ++++++++++++++---\n t/t3301-notes.sh | 3 ++-\n 2 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex f0fa337e8c..02b88e54d8 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -567,9 +567,15 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \tstruct notes_tree *t;\n \tstruct object_id object, new_note;\n \tconst struct object_id *note;\n-\tchar *logmsg;\n+\tchar *logmsg = NULL;\n \tconst char * const *usage;\n-\tstruct note_data d = { 0, 0, NULL, STRBUF_INIT };\n+\tstruct note_data d = {\n+\t\t.given = 0,\n+\t\t.use_editor = 0,\n+\t\t.edit_path = NULL,\n+\t\t.buf = STRBUF_INIT\n+\t};\n+\n \tstruct option options[] = {\n \t\tOPT_CALLBACK_F('m', \"message\", &d, N_(\"message\"),\n \t\t\tN_(\"note contents as a string\"), PARSE_OPT_NONEG,\n@@ -634,13 +640,18 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tif (add_note(t, &object, &new_note, combine_notes_overwrite))\n \t\t\tBUG(\"combine_notes_overwrite failed\");\n \t\tlogmsg = xstrfmt(\"Notes added by 'git notes %s'\", argv[0]);\n+\t\tcommit_notes(the_repository, t, logmsg);\n+\t} else if (!d.buf.len && !note) {\n+\t\tfprintf(stderr,\n+\t\t\t_(\"Both original and appended notes are empty in %s, do nothing\\n\"),\n+\t\t\toid_to_hex(&object));\n \t} else {\n \t\tfprintf(stderr, _(\"Removing note for object %s\\n\"),\n \t\t\toid_to_hex(&object));\n \t\tremove_note(t, object.hash);\n \t\tlogmsg = xstrfmt(\"Notes removed by 'git notes %s'\", argv[0]);\n+\t\tcommit_notes(the_repository, t, logmsg);\n \t}\n-\tcommit_notes(the_repository, t, logmsg);\n \n \tfree(logmsg);\n \tfree_note_data(&d);\ndiff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\nindex 76beafdeb8..1a0fd157a5 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -574,7 +574,8 @@ test_expect_success 'git notes append == add when there is no existing note' '\n test_expect_success 'appending empty string to non-existing note does not create note' '\n \tgit notes remove HEAD &&\n \ttest_must_fail git notes list HEAD &&\n-\tgit notes append -m \"\" &&\n+\tgit notes append -m \"\" >output 2>&1 &&\n+\tgrep \"Both original and appended notes are empty\" output &&\n \ttest_must_fail git notes list HEAD\n '\n \n-- \n2.38.1.383.ge7205ac0a40.dirty\n\n"},{"id":"466661","messageId":"2475ea0c04d7b5e2812f13d0c8701a0056b305b8.1667828335.git.dyroneteng@gmail.com","threadId":"58620","inReplyTo":"cover.1667828335.git.dyroneteng@gmail.com","subject":"[PATCH v2 3/3] notes.c: drop unreachable code in \"append_edit()\"","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-07T13:57:05Z","receivedAt":"2022-11-07T13:57:30Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nSituation of removing note shouldn't happen in \"append_edit()\",\nunless it's a bug. So, let's drop the unreachable \"else\" code\nin \"append_edit()\".\n\nSigned-off-by: Teng Long \n---\n builtin/notes.c | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 02b88e54d8..6d42592c79 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -10,6 +10,7 @@\n #include \"cache.h\"\n #include \"config.h\"\n #include \"builtin.h\"\n+#include \"git-compat-util.h\"\n #include \"notes.h\"\n #include \"object-store.h\"\n #include \"repository.h\"\n@@ -646,11 +647,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\t\t_(\"Both original and appended notes are empty in %s, do nothing\\n\"),\n \t\t\toid_to_hex(&object));\n \t} else {\n-\t\tfprintf(stderr, _(\"Removing note for object %s\\n\"),\n-\t\t\toid_to_hex(&object));\n-\t\tremove_note(t, object.hash);\n-\t\tlogmsg = xstrfmt(\"Notes removed by 'git notes %s'\", argv[0]);\n-\t\tcommit_notes(the_repository, t, logmsg);\n+\t\tBUG(\"compute_notes failed\");\n \t}\n \n \tfree(logmsg);\n-- \n2.38.1.383.ge7205ac0a40.dirty\n\n"},{"id":"466664","messageId":"221107.86cz9yx1wh.gmgdl@evledraar.gmail.com","threadId":"58620","inReplyTo":"5dbe014a09cca6ffa01dc9f61d0ddc095d31d518.1667828335.git.dyroneteng@gmail.com","subject":"Re: [PATCH v2 2/3] notes.c: fixed tip when target and append note are both empty","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-07T14:40:03Z","receivedAt":"2022-11-07T14:42:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 07 2022, Teng Long wrote:\n\n> From: Teng Long \n>\n> When \"git notes append \" is executed, if there is no note in\n> the given object and the appended note is empty, the command line\n> prompt will be as follows:\n>\n> \"Removing note for object \"\n>\n> Actually, this tip is not that accurate, because there is no note in\n> the original , and it also does no remove work on the notes\n> reference, so let's fix this and give the correct tip.\n>\n> Signed-off-by: Teng Long \n> ---\n> builtin/notes.c | 17 ++++++++++++++---\n> t/t3301-notes.sh | 3 ++-\n> 2 files changed, 16 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index f0fa337e8c..02b88e54d8 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -567,9 +567,15 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> \tstruct notes_tree *t;\n> \tstruct object_id object, new_note;\n> \tconst struct object_id *note;\n> -\tchar *logmsg;\n> +\tchar *logmsg = NULL;\n> \tconst char * const *usage;\n> -\tstruct note_data d = { 0, 0, NULL, STRBUF_INIT };\n> +\tstruct note_data d = {\n> +\t\t.given = 0,\n> +\t\t.use_editor = 0,\n> +\t\t.edit_path = NULL,\n\nMost of this is an unrelated \"use designated init\" cleanup, which is\ngood, but let's do that in a different commit if we need it. And then\nyou can drop all of these...\n\n> +\t\t.buf = STRBUF_INIT\n\n...and only need to keep this one.\n"},{"id":"466665","messageId":"221107.868rkmx1pu.gmgdl@evledraar.gmail.com","threadId":"58620","inReplyTo":"2475ea0c04d7b5e2812f13d0c8701a0056b305b8.1667828335.git.dyroneteng@gmail.com","subject":"Re: [PATCH v2 3/3] notes.c: drop unreachable code in \"append_edit()\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-07T14:41:57Z","receivedAt":"2022-11-07T14:45:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 07 2022, Teng Long wrote:\n\n> From: Teng Long \n>\n> Situation of removing note shouldn't happen in \"append_edit()\",\n> unless it's a bug. So, let's drop the unreachable \"else\" code\n> in \"append_edit()\".\n>\n> Signed-off-by: Teng Long \n> ---\n> builtin/notes.c | 7 ++-----\n> 1 file changed, 2 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index 02b88e54d8..6d42592c79 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -10,6 +10,7 @@\n> #include \"cache.h\"\n> #include \"config.h\"\n> #include \"builtin.h\"\n> +#include \"git-compat-util.h\"\n\nHuh? cache.h includes this already, why are we doing it again?\n\n> #include \"notes.h\"\n> #include \"object-store.h\"\n> #include \"repository.h\"\n> @@ -646,11 +647,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> \t\t\t_(\"Both original and appended notes are empty in %s, do nothing\\n\"),\n> \t\t\toid_to_hex(&object));\n> \t} else {\n> -\t\tfprintf(stderr, _(\"Removing note for object %s\\n\"),\n> -\t\t\toid_to_hex(&object));\n> -\t\tremove_note(t, object.hash);\n> -\t\tlogmsg = xstrfmt(\"Notes removed by 'git notes %s'\", argv[0]);\n> -\t\tcommit_notes(the_repository, t, logmsg);\n> +\t\tBUG(\"compute_notes failed\");\n> \t}\n\nLet's squash this into 2/3. So you were adding to it then, but we didn't\nneed to add this commit_notes() at all?\n\nAlso, there you did:\n\n\t-\tchar *logmsg;\n\t+\tchar *logmsg = NULL;\n\nBut here we see that it's only used in the initial \"if\" arm, so the\nreason to have it declared earlier has disappeared in this 3/3. Instead\njust have 2/3 do:\n\n\tif (...) {\n\t\tchar *logmsg;\n\t\t[...]\n\n\t\tlogmsg = xstrfmt(...);\n\t commit_notes(....);\n\t\tfree(logmsg);\n\t} else if (...) {\n\t\t...\n\t}\n\nThis commit also shows that there wasn't any reason for your \"struct\nnote_data d\" cleanup, i.e. we weren't adding a field or anything...\n"},{"id":"466666","messageId":"221107.864jvax1hz.gmgdl@evledraar.gmail.com","threadId":"58620","inReplyTo":"2381947abdd6b965c02e114af297fc908ed3132b.1667828335.git.dyroneteng@gmail.com","subject":"Re: [PATCH v2 1/3] notes.c: introduce \"--blank-line\" option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-07T14:45:38Z","receivedAt":"2022-11-07T14:50:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 07 2022, Teng Long wrote:\n\n> When appending to a given object which has note and if the appended\n> note is not empty too, we will insert a blank line at first. The\n> blank line serves as a split line, but sometimes we just want to\n> omit it then append on the heels of the target note. So, we add\n> a new \"OPT_BOOL()\" option to determain whether a new blank line\n> is insert at first.\n>\n> Signed-off-by: Teng Long \n> ---\n> Documentation/git-notes.txt | 11 +++++++++--\n> builtin/notes.c | 5 ++++-\n> t/t3301-notes.sh | 12 ++++++++++++\n> 3 files changed, 25 insertions(+), 3 deletions(-)\n>\n> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\n> index efbc10f0f5..43770ddf84 100644\n> --- a/Documentation/git-notes.txt\n> +++ b/Documentation/git-notes.txt\n> @@ -11,7 +11,7 @@ SYNOPSIS\n> 'git notes' [list []]\n> 'git notes' add [-f] [--allow-empty] [-F | -m | (-c | -C) ] []\n> 'git notes' copy [-f] ( --stdin | [] )\n> -'git notes' append [--allow-empty] [-F | -m | (-c | -C) ] []\n> +'git notes' append [--allow-empty] [--blank-line] [-F | -m | (-c | -C) ] []\n> 'git notes' edit [--allow-empty] []\n> 'git notes' show []\n> 'git notes' merge [-v | -q] [-s ] \n> @@ -86,7 +86,9 @@ the command can read the input given to the `post-rewrite` hook.)\n> \n> append::\n> \tAppend to the notes of an existing object (defaults to HEAD).\n> -\tCreates a new notes object if needed.\n> +\tCreates a new notes object if needed. If the note of the given\n> +\tobject and the note to be appended are not empty, a blank line\n> +\twill be inserted between them.\n> \n> edit::\n> \tEdit the notes for a given object (defaults to HEAD).\n> @@ -159,6 +161,11 @@ OPTIONS\n> \tAllow an empty note object to be stored. The default behavior is\n> \tto automatically remove empty notes.\n> \n> +--blank-line::\n> +--no-blank-line::\n> +\tControls if a blank line to split paragraphs is inserted\n> +\twhen appending (the default is true).\n\nJust make this:\n\n\t--no-blank-line:\n\t\tSuppress the insertion of a blank line before the\n\t\tinserted notes.\n\nOr something, i.e. when adding a \"true by default\" let's add a \"no-...\" variant directly.\n\n> \tint allow_empty = 0;\n> +\tint blankline = 1;\n\nSo keep this...\n\n> \tconst char *object_ref;\n> \tstruct notes_tree *t;\n> \tstruct object_id object, new_note;\n> @@ -584,6 +585,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> \t\t\tparse_reuse_arg),\n> \t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n> \t\t\tN_(\"allow storing empty note\")),\n> +\t\tOPT_BOOL(0, \"blank-line\", &blankline,\n\n...and just make this \"no-blank-line\", and parse_options() will do the\nright thing. I.e. PARSE_OPT_NONEG is implied.\n\n> -\t\tif (d.buf.len && prev_buf && size)\n> +\t\tif (blankline && d.buf.len && prev_buf && size)\n> \t\t\tstrbuf_insertstr(&d.buf, 0, \"\\n\");\n\nMaybe this needs to be elaborated in the docs? I.e. it sounds as if\nwe'll insert a \\n unconditionally, which this shows isn't the case.\n\n> \t\tif (prev_buf && size)\n> \t\t\tstrbuf_insert(&d.buf, 0, prev_buf, size);\n> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\n> index 3288aaec7d..76beafdeb8 100755\n> --- a/t/t3301-notes.sh\n> +++ b/t/t3301-notes.sh\n> @@ -521,12 +521,24 @@ test_expect_success 'listing non-existing notes fails' '\n> \ttest_must_be_empty actual\n> '\n> \n> +test_expect_success 'append to existing note without a beginning blank line' '\n> +\tcat >expect <<-\\EOF &&\n> +\t\tInitial set of notes\n> +\t\tAppended notes\n> +\tEOF\n> +\tgit notes add -m \"Initial set of notes\" &&\n> +\tgit notes append --no-blank-line -m \"Appended notes\" &&\n> +\tgit notes show >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_expect_success 'append to existing note with \"git notes append\"' '\n> \tcat >expect <<-EOF &&\n> \t\tInitial set of notes\n> \n> \t\tMore notes appended with git notes append\n> \tEOF\n> +\tgit notes remove HEAD &&\n\nThis should be a test_when_finished \"\", for the previous test, otherwise\nthis one will presumably fail if you use the \"wrong\" --run=\"\" arguments\nto skip the last test.\n"},{"id":"466668","messageId":"221107.86zgd2vma6.gmgdl@evledraar.gmail.com","threadId":"58620","inReplyTo":"cover.1667828335.git.dyroneteng@gmail.com","subject":"Re: [PATCH v2 0/3] notes.c: introduce \"--blank-line\" option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-07T14:57:21Z","receivedAt":"2022-11-07T15:04:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 07 2022, Teng Long wrote:\n\n> From: Teng Long \n> [...]\n> * drop unreachable code in \"append_edit()\". Ævar found that some code has been\n> unreachable in patch v1. I think it's because, after the commit \"notes.c: fixed\n> tip when target and append note are both empty\", for example in this patch, the\n> situation of \"removing an existing note\" should be impossible unless a BUG when\n> trying to do append. The tests are passed, but I'm not sure I fully understand\n> the original design.\n\nI suggested squashing that BUG() in 3/3 into 2/3, but reading this again\nI think it should come first.\n\nI.e. this seems to me like the code in cd067d3bf4e (Builtin-ify\ngit-notes, 2010-02-13) might have just been blindly carried forward to\nboth \"create\" and \"edit\" in 52694cdabbf (builtin/notes: split\ncreate_note() to clarify add vs. remove logic, 2014-11-12).\n\nBut it would be good to have confirmation, e.g. if you check out\n52694cdabbf and remove that \"Removing note\" branch from add() does it\nfail tests at the time, but not in the case of append_edit()?\n\n"},{"id":"466670","messageId":"221107.86v8nqvlla.gmgdl@evledraar.gmail.com","threadId":"58620","inReplyTo":"2381947abdd6b965c02e114af297fc908ed3132b.1667828335.git.dyroneteng@gmail.com","subject":"Re: [PATCH v2 1/3] notes.c: introduce \"--blank-line\" option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-07T15:06:39Z","receivedAt":"2022-11-07T15:19:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 07 2022, Teng Long wrote:\n\n> From: Teng Long \n\nSomething I didn't notice on the first reading:\n\n> @@ -619,7 +622,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> \t\tchar *prev_buf = read_object_file(note, &type, &size);\n> \n> \t\tstrbuf_grow(&d.buf, size + 1);\n\nHere we're growing it to add the size plus the \\n.\n\n> -\t\tif (d.buf.len && prev_buf && size)\n> +\t\tif (blankline && d.buf.len && prev_buf && size)\n> \t\t\tstrbuf_insertstr(&d.buf, 0, \"\\n\");\n\nBut now we're not going to make use of that \"+ 1\" anymore...\n\n> \t\tif (prev_buf && size)\n> \t\t\tstrbuf_insert(&d.buf, 0, prev_buf, size);\n\n..and here the prev_buf && size condition is duplicated.\n\nI think the right thin to do is to just drop the strbuf_grow() here\naltogether, since strbuf_insert() will handle it for us, but that would\nmake sense before this change, so maybe in pre-cleanup?\n"},{"id":"466672","messageId":"CAPig+cS+8ER=K9byUZs9+MxZ1x9zVxnGKrKm8CGs1zJvvCoSEQ@mail.gmail.com","threadId":"58620","inReplyTo":"221107.864jvax1hz.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 1/3] notes.c: introduce \"--blank-line\" option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-11-07T15:45:49Z","receivedAt":"2022-11-07T15:46:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 7, 2022 at 9:56 AM Ævar Arnfjörð Bjarmason wrote:\n> On Mon, Nov 07 2022, Teng Long wrote:\n> > When appending to a given object which has note and if the appended\n> > note is not empty too, we will insert a blank line at first. The\n> > blank line serves as a split line, but sometimes we just want to\n> > omit it then append on the heels of the target note. So, we add\n> > a new \"OPT_BOOL()\" option to determain whether a new blank line\n> > is insert at first.\n> >\n> > Signed-off-by: Teng Long \n> > ---\n> > diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\n> > @@ -159,6 +161,11 @@ OPTIONS\n> > +--blank-line::\n> > +--no-blank-line::\n> > + Controls if a blank line to split paragraphs is inserted\n> > + when appending (the default is true).\n>\n> Just make this:\n>\n> --no-blank-line:\n> Suppress the insertion of a blank line before the\n> inserted notes.\n>\n> Or something, i.e. when adding a \"true by default\" let's add a \"no-...\" variant directly.\n\nThis is the exact opposite of Junio's advice[1], isn't it?\n\n[1]: https://lore.kernel.org/git/xmqqsfjsi7eq.fsf@gitster.g/\n"},{"id":"466680","messageId":"221107.86a652vfth.gmgdl@evledraar.gmail.com","threadId":"58620","inReplyTo":"CAPig+cS+8ER=K9byUZs9+MxZ1x9zVxnGKrKm8CGs1zJvvCoSEQ@mail.gmail.com","subject":"Re: [PATCH v2 1/3] notes.c: introduce \"--blank-line\" option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-07T17:22:34Z","receivedAt":"2022-11-07T17:24:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 07 2022, Eric Sunshine wrote:\n\n> On Mon, Nov 7, 2022 at 9:56 AM Ævar Arnfjörð Bjarmason wrote:\n>> On Mon, Nov 07 2022, Teng Long wrote:\n>> > When appending to a given object which has note and if the appended\n>> > note is not empty too, we will insert a blank line at first. The\n>> > blank line serves as a split line, but sometimes we just want to\n>> > omit it then append on the heels of the target note. So, we add\n>> > a new \"OPT_BOOL()\" option to determain whether a new blank line\n>> > is insert at first.\n>> >\n>> > Signed-off-by: Teng Long \n>> > ---\n>> > diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\n>> > @@ -159,6 +161,11 @@ OPTIONS\n>> > +--blank-line::\n>> > +--no-blank-line::\n>> > + Controls if a blank line to split paragraphs is inserted\n>> > + when appending (the default is true).\n>>\n>> Just make this:\n>>\n>> --no-blank-line:\n>> Suppress the insertion of a blank line before the\n>> inserted notes.\n>>\n>> Or something, i.e. when adding a \"true by default\" let's add a \"no-...\" variant directly.\n>\n> This is the exact opposite of Junio's advice[1], isn't it?\n>\n> [1]: https://lore.kernel.org/git/xmqqsfjsi7eq.fsf@gitster.g/\n\nI read that as him mainly talking about what we name the variable (which\nI agree with, but didn't comment on here). I'm talking about what\ninterface is exposed to the user.\n\nI.e. both concerns can be satisfied, but whether my suggestion is\nsensible UX is another matter...\n"},{"id":"466757","messageId":"Y2l8xhB+PGS+jBmQ@nand.local","threadId":"58620","inReplyTo":"221107.86a652vfth.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 1/3] notes.c: introduce \"--blank-line\" option","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-07T21:46:46Z","receivedAt":"2022-11-07T21:47:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 07, 2022 at 06:22:34PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n> On Mon, Nov 07 2022, Eric Sunshine wrote:\n>\n> > On Mon, Nov 7, 2022 at 9:56 AM Ævar Arnfjörð Bjarmason wrote:\n> >> On Mon, Nov 07 2022, Teng Long wrote:\n> >> > When appending to a given object which has note and if the appended\n> >> > note is not empty too, we will insert a blank line at first. The\n> >> > blank line serves as a split line, but sometimes we just want to\n> >> > omit it then append on the heels of the target note. So, we add\n> >> > a new \"OPT_BOOL()\" option to determain whether a new blank line\n> >> > is insert at first.\n> >> >\n> >> > Signed-off-by: Teng Long \n> >> > ---\n> >> > diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\n> >> > @@ -159,6 +161,11 @@ OPTIONS\n> >> > +--blank-line::\n> >> > +--no-blank-line::\n> >> > + Controls if a blank line to split paragraphs is inserted\n> >> > + when appending (the default is true).\n> >>\n> >> Just make this:\n> >>\n> >> --no-blank-line:\n> >> Suppress the insertion of a blank line before the\n> >> inserted notes.\n> >>\n> >> Or something, i.e. when adding a \"true by default\" let's add a \"no-...\" variant directly.\n> >\n> > This is the exact opposite of Junio's advice[1], isn't it?\n> >\n> > [1]: https://lore.kernel.org/git/xmqqsfjsi7eq.fsf@gitster.g/\n>\n> I read that as him mainly talking about what we name the variable (which\n> I agree with, but didn't comment on here). I'm talking about what\n> interface is exposed to the user.\n\nHaving --blank-line as an option is convenient for scripting, so I'd err\non the side of the original interpretation of Junio's suggestion.\n\nWe can clearly support '--[no-]blank-line' and allow latter arguments to\noverride previous ones. The documentation is fine as-is.\n\nThanks,\nTaylor\n"},{"id":"466758","messageId":"Y2l86Cvbv8nSlN4X@nand.local","threadId":"58620","inReplyTo":"2381947abdd6b965c02e114af297fc908ed3132b.1667828335.git.dyroneteng@gmail.com","subject":"Re: [PATCH v2 1/3] notes.c: introduce \"--blank-line\" option","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-07T21:47:20Z","receivedAt":"2022-11-07T21:47:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 07, 2022 at 09:57:03PM +0800, Teng Long wrote:\n> From: Teng Long \n>\n> When appending to a given object which has note and if the appended\n> note is not empty too, we will insert a blank line at first. The\n> blank line serves as a split line, but sometimes we just want to\n> omit it then append on the heels of the target note. So, we add\n> a new \"OPT_BOOL()\" option to determain whether a new blank line\n> is insert at first.\n\nThe discussion about whether this should support '--blank-line' or just\nbe '--no-blank-line' aside, I'm curious what motivated this change?\n\nThanks,\nTaylor\n"},{"id":"466759","messageId":"Y2l9yfOhiT7r2+H6@nand.local","threadId":"58620","inReplyTo":"221107.86cz9yx1wh.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 2/3] notes.c: fixed tip when target and append note are both empty","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-07T21:51:05Z","receivedAt":"2022-11-07T21:51:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 07, 2022 at 03:40:03PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> > @@ -567,9 +567,15 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> > \tstruct notes_tree *t;\n> > \tstruct object_id object, new_note;\n> > \tconst struct object_id *note;\n> > -\tchar *logmsg;\n> > +\tchar *logmsg = NULL;\n> > \tconst char * const *usage;\n> > -\tstruct note_data d = { 0, 0, NULL, STRBUF_INIT };\n> > +\tstruct note_data d = {\n> > +\t\t.given = 0,\n> > +\t\t.use_editor = 0,\n> > +\t\t.edit_path = NULL,\n>\n> Most of this is an unrelated \"use designated init\" cleanup, which is\n> good, but let's do that in a different commit if we need it. And then\n> you can drop all of these...\n>\n> > +\t\t.buf = STRBUF_INIT\n>\n> ...and only need to keep this one.\n\nI don't mind seeing the cleanup here, but I don't see why we need to\nkeep that portion of the hunk at all for this series.\n\nIOW, your \"...and only need to keep this one\" does not make sense to me.\n\nChanging \"char *logmsg\" to be initialized to NULL is important, though,\nsince we now only set it conditionally still free it unconditionally. So\nthat should be kept.\n\nThanks,\nTaylor\n"},{"id":"466761","messageId":"221107.86bkpitmw6.gmgdl@evledraar.gmail.com","threadId":"58620","inReplyTo":"Y2l9yfOhiT7r2+H6@nand.local","subject":"Re: [PATCH v2 2/3] notes.c: fixed tip when target and append note are both empty","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-07T22:33:33Z","receivedAt":"2022-11-07T22:34:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 07 2022, Taylor Blau wrote:\n\n> On Mon, Nov 07, 2022 at 03:40:03PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> > @@ -567,9 +567,15 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n>> > \tstruct notes_tree *t;\n>> > \tstruct object_id object, new_note;\n>> > \tconst struct object_id *note;\n>> > -\tchar *logmsg;\n>> > +\tchar *logmsg = NULL;\n>> > \tconst char * const *usage;\n>> > -\tstruct note_data d = { 0, 0, NULL, STRBUF_INIT };\n>> > +\tstruct note_data d = {\n>> > +\t\t.given = 0,\n>> > +\t\t.use_editor = 0,\n>> > +\t\t.edit_path = NULL,\n>>\n>> Most of this is an unrelated \"use designated init\" cleanup, which is\n>> good, but let's do that in a different commit if we need it. And then\n>> you can drop all of these...\n>>\n>> > +\t\t.buf = STRBUF_INIT\n>>\n>> ...and only need to keep this one.\n>\n> I don't mind seeing the cleanup here, but I don't see why we need to\n> keep that portion of the hunk at all for this series.\n>\n> IOW, your \"...and only need to keep this one\" does not make sense to me.\n\nI was still on my initial read-through, and was assuming that it was a\nprep change for the 3/3 adding a new field, before I saw the 3/3...\n\n> Changing \"char *logmsg\" to be initialized to NULL is important, though,\n> since we now only set it conditionally still free it unconditionally. So\n> that should be kept.\n"},{"id":"466763","messageId":"Y2mKjwpRME3EnQGn@nand.local","threadId":"58620","inReplyTo":"221107.86bkpitmw6.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 2/3] notes.c: fixed tip when target and append note are both empty","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-07T22:45:35Z","receivedAt":"2022-11-07T22:45:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 07, 2022 at 11:33:33PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> > I don't mind seeing the cleanup here, but I don't see why we need to\n> > keep that portion of the hunk at all for this series.\n> >\n> > IOW, your \"...and only need to keep this one\" does not make sense to me.\n>\n> I was still on my initial read-through, and was assuming that it was a\n> prep change for the 3/3 adding a new field, before I saw the 3/3...\n\nGotcha, makes sense. Thanks.\n\nThanks,\nTaylor\n"},{"id":"466764","messageId":"221107.867d06tluz.gmgdl@evledraar.gmail.com","threadId":"58620","inReplyTo":"Y2l8xhB+PGS+jBmQ@nand.local","subject":"Re: [PATCH v2 1/3] notes.c: introduce \"--blank-line\" option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-07T22:36:58Z","receivedAt":"2022-11-07T22:58:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 07 2022, Taylor Blau wrote:\n\n> On Mon, Nov 07, 2022 at 06:22:34PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>>\n>> On Mon, Nov 07 2022, Eric Sunshine wrote:\n>>\n>> > On Mon, Nov 7, 2022 at 9:56 AM Ævar Arnfjörð Bjarmason wrote:\n>> >> On Mon, Nov 07 2022, Teng Long wrote:\n>> >> > When appending to a given object which has note and if the appended\n>> >> > note is not empty too, we will insert a blank line at first. The\n>> >> > blank line serves as a split line, but sometimes we just want to\n>> >> > omit it then append on the heels of the target note. So, we add\n>> >> > a new \"OPT_BOOL()\" option to determain whether a new blank line\n>> >> > is insert at first.\n>> >> >\n>> >> > Signed-off-by: Teng Long \n>> >> > ---\n>> >> > diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\n>> >> > @@ -159,6 +161,11 @@ OPTIONS\n>> >> > +--blank-line::\n>> >> > +--no-blank-line::\n>> >> > + Controls if a blank line to split paragraphs is inserted\n>> >> > + when appending (the default is true).\n>> >>\n>> >> Just make this:\n>> >>\n>> >> --no-blank-line:\n>> >> Suppress the insertion of a blank line before the\n>> >> inserted notes.\n>> >>\n>> >> Or something, i.e. when adding a \"true by default\" let's add a \"no-...\" variant directly.\n>> >\n>> > This is the exact opposite of Junio's advice[1], isn't it?\n>> >\n>> > [1]: https://lore.kernel.org/git/xmqqsfjsi7eq.fsf@gitster.g/\n>>\n>> I read that as him mainly talking about what we name the variable (which\n>> I agree with, but didn't comment on here). I'm talking about what\n>> interface is exposed to the user.\n>\n> Having --blank-line as an option is convenient for scripting, so I'd err\n> on the side of the original interpretation of Junio's suggestion.\n\nI see from re-reading my reply that I was conflating two things. What I\n*meant* to suggest is this:\n\nWhen an option is not the default, and we provide a way to turn it off\nwe usually document that as:\n\n\t--no-foo:\n\t\tDon't do foo.\n\nSee e.g. \"git commit --no-edit\", and \"git clone --no-checkout\".\n\nBut this is orthagonal to what you call the option in the source, and\nwhether your variable is \"inverted\". I.e. in both those cases we have a\n\"--edit\" and \"--checkout\", but when we prepare the options for\nparse_options() we do (or the equivalent of):\n\n\tint no_checkout = 0;\n\tOPT_BOOL('n', \"no-checkout\", &option_no_checkout,\n\nAnd:\n\n\tint edit = 1;\n\tOPT_BOOL('e', \"edit\", &edit_flag, N_(\"force edit of commit\")),\n\nSo, I'm (now) saying I don't care which form we use in the sources, but\nthat' it's useful to document things as e.g.:\n\n\t--no-checkout::\n \tNo checkout of HEAD is performed after the clone is complete.\n\nInstead of e.g.:\n\n\t--no-checkout:\n\t--checkout:\n \t\tDo we do a checkout when we clone (doing a checkout is\n \t\tthe default).\n\nBecause the former convention shows the user at a glance which of the\ntwo is the default.\n\n> We can clearly support '--[no-]blank-line' and allow latter arguments to\n> override previous ones.\n\nWe'll support both either way, i.e. parse_options() detects that the\nname starts with \"no-\", so the negation of a \"no-checkout\" isn't\n\"--no-no-checkout\", but a \"--checkout\".\n\n> The documentation is fine as-is.\n\nI think the above form would make it a bit better, but just my 0.02:\n\n\t--no-blank-line::\n\t\tDon't add an extra \"\\n\" between the body of the commit\n\t\tand the note.\n\nOr something...\n"},{"id":"466774","messageId":"Y2mjqUT95dW8LMQk@nand.local","threadId":"58620","inReplyTo":"221107.867d06tluz.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 1/3] notes.c: introduce \"--blank-line\" option","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-08T00:32:41Z","receivedAt":"2022-11-08T00:32:46Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 07, 2022 at 11:36:58PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> So, I'm (now) saying I don't care which form we use in the sources, but\n> that' it's useful to document things as e.g.:\n>\n> \t--no-checkout::\n> \tNo checkout of HEAD is performed after the clone is complete.\n>\n> Instead of e.g.:\n>\n> \t--no-checkout:\n> \t--checkout:\n> \t\tDo we do a checkout when we clone (doing a checkout is\n> \t\tthe default).\n>\n> Because the former convention shows the user at a glance which of the\n> two is the default.\n\nThanks for clarifying. For what it's worth, I slightly prefer\ndocumenting both options as long as we clearly specify which is the\ndefault behavior (and thus implied).\n\nBut I definitely do not feel strongly about it.\n\nThanks,\nTaylor\n"},{"id":"466788","messageId":"20221108034538.11849-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"221107.864jvax1hz.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 1/3] notes.c: introduce \"--blank-line\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-08T03:45:38Z","receivedAt":"2022-11-08T03:45:49Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"\"Ævar Arnfjörð Bjarmason\" writes:\n\n> Just make this:\n>\n> \t--no-blank-line:\n> \t\tSuppress the insertion of a blank line before the\n> \t\tinserted notes.\n>\n> Or something, i.e. when adding a \"true by default\" let's add a \"no-...\" variant\n> directly.\n>\n> ...\n\nYes, that's what I propose to do originally, but I can accept either NEG or POS\nway. So, I will hang this up for a while and try to hear more inputs.\n\n> > -\t\tif (d.buf.len && prev_buf && size)\n> > +\t\tif (blankline && d.buf.len && prev_buf && size)\n> > \t\t\tstrbuf_insertstr(&d.buf, 0, \"\\n\");\n>\n> Maybe this needs to be elaborated in the docs? I.e. it sounds as if\n> we'll insert a \\n unconditionally, which this shows isn't the case.\n\nThe current doc add the content about this circumstance corresponing to this\n\"if\", which describes as:\n\n> append::\n> \tAppend to the notes of an existing object (defaults to HEAD).\n> -\tCreates a new notes object if needed.\n> +\tCreates a new notes object if needed. If the note of the given\n> +\tobject and the note to be appended are not empty, a blank line\n> +\twill be inserted between them.\n\n... so you mean we should add more detailed information here?\n\n> This should be a test_when_finished \"\", for the previous test, otherwise\n> this one will presumably fail if you use the \"wrong\" --run=\"\" arguments\n> to skip the last test.\n\nYes, I agree, will fix (although if we replace it by \"test_when_finished,\nbecause of the dependency of the previous repo status, execute\n`sh t3301-notes.sh --run=60` also failed).\n\nThanks.\n"},{"id":"466790","messageId":"20221108063229.13392-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"221107.86v8nqvlla.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 1/3] notes.c: introduce \"--blank-line\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-08T06:32:29Z","receivedAt":"2022-11-08T06:32:40Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"\"Ævar Arnfjörð Bjarmason\" writes:\n\n> I think the right thin to do is to just drop the strbuf_grow() here\n> altogether, since strbuf_insert() will handle it for us, but that would\n> make sense before this change, so maybe in pre-cleanup?\n\nSorry for ignoring this line of code. I think you are right, and I will\nmake a separate pre-cleanup commit for this in next round.\n\nThanks.\n"},{"id":"466792","messageId":"20221108073634.19887-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"Y2l86Cvbv8nSlN4X@nand.local","subject":"Re: [PATCH v2 1/3] notes.c: introduce \"--blank-line\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-08T07:36:34Z","receivedAt":"2022-11-08T07:37:07Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Taylor Blau writes:\n\n* Actually that's my suggestion, because I found when I use \"git notes appends\"\nto append new content to an existing note. It will always insert a blank line\nbetween the existing and the new. Does \"append\" represent \"a blank line first\nthen the content\"? If the user does not want it, there may be no way to\ncircumvent this on \"append\" at present. For example:\n\n--------example start-------\n\nHere is a commit with notes (from a openstack community repo), the notes is as:\n\ncommit ...(HEAD -> master, origin/master, origin/HEAD)\nAuthor: ...\nDate: ...\n\n .....\n Change-Id: ....\n\nNotes (review):\n Code-Review+2: yatin \n Code-Review+2: chandan kumar \n Workflow+1: chandan kumar \n Verified+1: RDO Third Party CI \n Verified+2: Zuul\n Submitted-by: Zuul\n Submitted-at: Wed, 28 Sep 2022 12:58:46 +0000\n Reviewed-on: https://review.opendev.org/c/openstack/tripleo-quickstart/+/859516\n Project: openstack/tripleo-quickstart\n Branch: refs/heads/master\n\nSo, if user wants to append this notes, actually will add a blank line, maybe a\nsurprise. But I may not represent most users, so I added this compatible option,\nmaybe better to let the use choose, then send this as a RFC patch.\n\n--------example finished-------\n\n* Another reason is, I found that if I append nothing to a commit (here on my case\nis HEAD) which doesn't exist notes on it, the output \"Removing note for\nobject\" shows something a little intesting to me, because there is no note to\nremove, that's what 2/3 does. The following are specific examples:\n\n--------example start-------\n\n➜ git-notes-test git:(tl/test) ✗ git --no-pager log HEAD -n 1\ncommit d5a0127568e89239bb02f78d44dfa0427e726103 (HEAD -> tl/test)\nAuthor: tenglong.tl \nDate: Thu Oct 20 18:09:16 2022 +0800\n\n 111\n➜ git-notes-test git:(tl/test) ✗ git notes append -m \"\"\nRemoving note for object d5a0127568e89239bb02f78d44dfa0427e726103\n\n--------example finished-------\n\nThanks.\n"},{"id":"466795","messageId":"20221108085510.24275-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"221107.86bkpitmw6.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 2/3] notes.c: fixed tip when target and append note are both empty","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-08T08:55:10Z","receivedAt":"2022-11-08T08:55:25Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"\"Ævar Arnfjörð Bjarmason\" writes:\n\n> I was still on my initial read-through, and was assuming that it was a\n> prep change for the 3/3 adding a new field, before I saw the 3/3...\n\nYes, you are right, that previous patch add a \"copy\" of \"d\", but now it's\nunnecessary, so I think make a pre-cleanup commit which includes the \"char\n*logmsg = NULL\" and the \"designated init\" of \"struct note_data d\" looks\nnice.\n\nThanks.\n"},{"id":"466817","messageId":"20221108130606.82005-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"221107.864jvax1hz.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 1/3] notes.c: introduce \"--blank-line\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-08T13:06:06Z","receivedAt":"2022-11-08T13:06:16Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"\"Ævar Arnfjörð Bjarmason\" writes:\n\n> > \tint allow_empty = 0;\n> > +\tint blankline = 1;\n>\n> So keep this...\n>\n> > \tconst char *object_ref;\n> >G \tstruct notes_tree *t;\n> > \tstruct object_id object, new_note;\n> > @@ -584,6 +585,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> > \t\t\tparse_reuse_arg),\n> > \t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n> > \t\t\tN_(\"allow storing empty note\")),\n> > +\t\tOPT_BOOL(0, \"blank-line\", &blankline,\n>\n> ...and just make this \"no-blank-line\", and parse_options() will do the\n> right thing. I.e. PARSE_OPT_NONEG is implied.\n\nSorry for another question. By the explanation of \"api-parse-options.txt\" :\n\n`OPT_BOOL(short, long, &int_var, description)`::\n\tIntroduce a boolean option. `int_var` is set to one with\n\t`--option` and set to zero with `--no-option`\n\nI think it means the same as \"parse_options() will do right thing\" as you said\nto me , but...after the modification the effect is opposite:\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex a6273781fb8..73427ea8dff 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -562,6 +562,7 @@ static int copy(int argc, const char **argv, const char *prefix)\n static int append_edit(int argc, const char **argv, const char *prefix)\n {\n int allow_empty = 0;\n+ int blankline = 1;\n const char *object_ref;\n struct notes_tree *t;\n struct object_id object, new_note;\n@@ -584,6 +585,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n parse_reuse_arg),\n OPT_BOOL(0, \"allow-empty\", &allow_empty,\n N_(\"allow storing empty note\")),\n+ OPT_BOOL(0, \"no-blank-line\", &blankline,\n+ N_(\"insert paragraph break before appending to an existing note\")),\n OPT_END()\n };\n int edit = !strcmp(argv[0], \"edit\");\n@@ -618,7 +621,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n enum object_type type;\n char *prev_buf = read_object_file(note, &type, &size);\n\n- if (d.buf.len && prev_buf && size)\n+ if (blankline && d.buf.len && prev_buf && size)\n strbuf_insertstr(&d.buf, 0, \"\\n\");\n if (prev_buf && size)\n strbuf_insert(&d.buf, 0, prev_buf, size);\n\n----\nSo, I am a bit confused and I guess maybe somewhere I misunderstood or didn't\nnotice some details.\n\nThanks.\n"},{"id":"466818","messageId":"221108.86pmdxshkt.gmgdl@evledraar.gmail.com","threadId":"58620","inReplyTo":"20221108130606.82005-1-tenglong.tl@alibaba-inc.com","subject":"Re: [PATCH v2 1/3] notes.c: introduce \"--blank-line\" option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-08T13:22:41Z","receivedAt":"2022-11-08T13:26:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 08 2022, Teng Long wrote:\n\n> \"Ævar Arnfjörð Bjarmason\" writes:\n>\n>> > \tint allow_empty = 0;\n>> > +\tint blankline = 1;\n>>\n>> So keep this...\n>>\n>> > \tconst char *object_ref;\n>> >G \tstruct notes_tree *t;\n>> > \tstruct object_id object, new_note;\n>> > @@ -584,6 +585,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n>> > \t\t\tparse_reuse_arg),\n>> > \t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n>> > \t\t\tN_(\"allow storing empty note\")),\n>> > +\t\tOPT_BOOL(0, \"blank-line\", &blankline,\n>>\n>> ...and just make this \"no-blank-line\", and parse_options() will do the\n>> right thing. I.e. PARSE_OPT_NONEG is implied.\n>\n> Sorry for another question. By the explanation of \"api-parse-options.txt\" :\n>\n> `OPT_BOOL(short, long, &int_var, description)`::\n> \tIntroduce a boolean option. `int_var` is set to one with\n> \t`--option` and set to zero with `--no-option`\n>\n> I think it means the same as \"parse_options() will do right thing\" as you said\n> to me , but...after the modification the effect is opposite:\n>\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index a6273781fb8..73427ea8dff 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -562,6 +562,7 @@ static int copy(int argc, const char **argv, const char *prefix)\n> static int append_edit(int argc, const char **argv, const char *prefix)\n> {\n> int allow_empty = 0;\n> + int blankline = 1;\n> const char *object_ref;\n> struct notes_tree *t;\n> struct object_id object, new_note;\n> @@ -584,6 +585,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> parse_reuse_arg),\n> OPT_BOOL(0, \"allow-empty\", &allow_empty,\n> N_(\"allow storing empty note\")),\n> + OPT_BOOL(0, \"no-blank-line\", &blankline,\n> + N_(\"insert paragraph break before appending to an existing note\")),\n> OPT_END()\n> };\n> int edit = !strcmp(argv[0], \"edit\");\n> @@ -618,7 +621,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> enum object_type type;\n> char *prev_buf = read_object_file(note, &type, &size);\n>\n> - if (d.buf.len && prev_buf && size)\n> + if (blankline && d.buf.len && prev_buf && size)\n> strbuf_insertstr(&d.buf, 0, \"\\n\");\n> if (prev_buf && size)\n> strbuf_insert(&d.buf, 0, prev_buf, size);\n>\n> ----\n> So, I am a bit confused and I guess maybe somewhere I misunderstood or didn't\n> notice some details.\n>\n> Thanks.\n\nSorry, I meant that in both cases it will expose the same options to the\nuser: --blank-line and --no-blank-line. I.e. if you create options\nnamed:\n\n\t\"x\" \"x-y\"\n\nTheir negations are: --no-x and --no-x-y. But if their names are:\n\n\t\"x\" \"no-x\"\n\nThe negations are:\n\n\t--no-x and --x\n\nBut as your example shows that's unrelated to whether the *variable in\nthe code* is negated.\n\nSo however you structure the code, which would be:\n\n\tint blankline = 1:\n [...]\n\tOPT_BOOL(0, \"blankline\", &blankline, [...]);\n\nOr:\n\n\tint no_blankline = 0:\n [...]\n\tOPT_BOOL(0, \"no-blankline\", &no_blankline, [...]);\n\nThe documentation could in both cases say:\n\n\t--no-blankline:\n\t\tdescribe the non-default[...]\n"},{"id":"466934","messageId":"20221109063514.93202-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"221108.86pmdxshkt.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 1/3] notes.c: introduce \"--blank-line\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-09T06:35:14Z","receivedAt":"2022-11-09T06:35:23Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"\"Ævar Arnfjörð Bjarmason\" writes:\n\n> Sorry, I meant that in both cases it will expose the same options to the\n> user: --blank-line and --no-blank-line. I.e. if you create options\n> named:\n>\n> \t\"x\" \"x-y\"\n>\n> Their negations are: --no-x and --no-x-y. But if their names are:\n>\n> \t\"x\" \"no-x\"\n>\n> The negations are:\n>\n> \t--no-x and --x\n>\n> But as your example shows that's unrelated to whether the *variable in\n> the code* is negated.\n>\n> So however you structure the code, which would be:\n>\n> \tint blankline = 1:\n> [...]\n> \tOPT_BOOL(0, \"blankline\", &blankline, [...]);\n>\n> Or:\n>\n> \tint no_blankline = 0:\n> [...]\n> \tOPT_BOOL(0, \"no-blankline\", &no_blankline, [...]);\n>\n> The documentation could in both cases say:\n>\n> \t--no-blankline:\n> \t\tdescribe the non-default[...]\n\nThank you for the detailed explanation, now it's clear for me.\n"},{"id":"466936","messageId":"20221109070526.94055-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"221107.86zgd2vma6.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 0/3] notes.c: introduce \"--blank-line\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-09T07:05:26Z","receivedAt":"2022-11-09T07:06:15Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"\"Ævar Arnfjörð Bjarmason\" writes:\n\n> > From: Teng Long \n> > [...]\n> > * drop unreachable code in \"append_edit()\". Ævar found that some code has been\n> > unreachable in patch v1. I think it's because, after the commit \"notes.c: fixed\n> > tip when target and append note are both empty\", for example in this patch, the\n> > situation of \"removing an existing note\" should be impossible unless a BUG when\n> > trying to do append. The tests are passed, but I'm not sure I fully understand\n> > the original design.\n>\n> I suggested squashing that BUG() in 3/3 into 2/3, but reading this again\n> I think it should come first.\n>\n> I.e. this seems to me like the code in cd067d3bf4e (Builtin-ify\n> git-notes, 2010-02-13) might have just been blindly carried forward to\n> both \"create\" and \"edit\" in 52694cdabbf (builtin/notes: split\n> create_note() to clarify add vs. remove logic, 2014-11-12).\n>\n> But it would be good to have confirmation, e.g. if you check out\n> 52694cdabbf and remove that \"Removing note\" branch from add() does it\n> fail tests at the time, but not in the case of append_edit()?\n\nThanks for mention that. I look back to 52694cdabbf and remove \"Removing note\"\nwill make the test fail, because the notes operation \"append\" is different with\n\"add\", the latter supports to overwrite the existing note then let the\n\"removing\" happen (e.g. execute `git notes add -f -F /dev/null` on an existing\nnote), but the former will not because it only does \"appends\" but not doing\n\"overwrites\".\n\nSo, I think may just remove the \"Removing note\" code in append_edit() is OK.\n\nThanks.\n"},{"id":"466937","messageId":"20221109070601.94086-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"221107.86zgd2vma6.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 0/3] notes.c: introduce \"--blank-line\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-09T07:06:01Z","receivedAt":"2022-11-09T07:06:32Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"\"Ævar Arnfjörð Bjarmason\" writes:\n\n> > From: Teng Long \n> > [...]\n> > * drop unreachable code in \"append_edit()\". Ævar found that some code has been\n> > unreachable in patch v1. I think it's because, after the commit \"notes.c: fixed\n> > tip when target and append note are both empty\", for example in this patch, the\n> > situation of \"removing an existing note\" should be impossible unless a BUG when\n> > trying to do append. The tests are passed, but I'm not sure I fully understand\n> > the original design.\n>\n> I suggested squashing that BUG() in 3/3 into 2/3, but reading this again\n> I think it should come first.\n>\n> I.e. this seems to me like the code in cd067d3bf4e (Builtin-ify\n> git-notes, 2010-02-13) might have just been blindly carried forward to\n> both \"create\" and \"edit\" in 52694cdabbf (builtin/notes: split\n> create_note() to clarify add vs. remove logic, 2014-11-12).\n>\n> But it would be good to have confirmation, e.g. if you check out\n> 52694cdabbf and remove that \"Removing note\" branch from add() does it\n> fail tests at the time, but not in the case of append_edit()?\n\nThanks for mention that. I look back to 52694cdabbf and remove \"Removing note\"\nwill make the test fail, because the notes operation \"append\" is different with\n\"add\", the latter supports to overwrite the existing note then let the\n\"removing\" happen (e.g. execute `git notes add -f -F /dev/null` on an existing\nnote), but the former will not because it only does \"appends\" but not doing\n\"overwrites\".\n\nSo, I think may just remove the \"Removing note\" code in append_edit() is OK.\n\nThanks.\n"},{"id":"466938","messageId":"cover.1667980450.git.dyroneteng@gmail.com","threadId":"58620","inReplyTo":"cover.1667828335.git.dyroneteng@gmail.com","subject":"[PATCH v3 0/5] notes.c: introduce \"--no-blank-line\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-09T09:06:51Z","receivedAt":"2022-11-09T09:07:23Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nDiff since v2:\n\n* [5/5] make \"--no-blank-line\" in doc.\n* [1/5] [2/5][3/5]do some cleanups and split to serval independent commit.\n* [3/5] futher explain in commit-msg why we can drop it in \"append\" but not \"add\".\n* [4/5] [4/5] use \"test_when_finished\" to cleanup notes in tests.\n\nThanks.\n\nTeng Long (5):\n notes.c: cleanup 'strbuf_grow' call in 'append_edit'\n notes.c: cleanup for \"designated init\" and \"char ptr init\"\n notes.c: drop unreachable code in 'append_edit()'\n notes.c: provide tips when target and append note are both empty\n notes.c: introduce \"--no-blank-line\" option\n\n Documentation/git-notes.txt | 10 ++++++++--\n builtin/notes.c | 20 ++++++++++----------\n t/t3301-notes.sh | 18 ++++++++++++++++--\n 3 files changed, 34 insertions(+), 14 deletions(-)\n\nRange-diff against v2:\n-: ---------- > 1: 8ae58934a1 notes.c: cleanup 'strbuf_grow' call in 'append_edit'\n-: ---------- > 2: a53576ea88 notes.c: cleanup for \"designated init\" and \"char ptr init\"\n-: ---------- > 3: 62a952ba3e notes.c: drop unreachable code in 'append_edit()'\n-: ---------- > 4: 0d8ce0b14b notes.c: provide tips when target and append note are both empty\n1: 2381947abd ! 5: 196e80358e notes.c: introduce \"--blank-line\" option\n @@ Metadata\n Author: Teng Long \n \n ## Commit message ##\n - notes.c: introduce \"--blank-line\" option\n + notes.c: introduce \"--no-blank-line\" option\n \n When appending to a given object which has note and if the appended\n note is not empty too, we will insert a blank line at first. The\n @@ Documentation/git-notes.txt: SYNOPSIS\n 'git notes' add [-f] [--allow-empty] [-F | -m | (-c | -C) ] []\n 'git notes' copy [-f] ( --stdin | [] )\n -'git notes' append [--allow-empty] [-F | -m | (-c | -C) ] []\n -+'git notes' append [--allow-empty] [--blank-line] [-F | -m | (-c | -C) ] []\n ++'git notes' append [--allow-empty] [--no-blank-line] [-F | -m | (-c | -C) ] []\n 'git notes' edit [--allow-empty] []\n 'git notes' show []\n 'git notes' merge [-v | -q] [-s ] \n @@ Documentation/git-notes.txt: OPTIONS\n \tAllow an empty note object to be stored. The default behavior is\n \tto automatically remove empty notes.\n \n -+--blank-line::\n +--no-blank-line::\n -+\tControls if a blank line to split paragraphs is inserted\n -+\twhen appending (the default is true).\n ++\tDo not insert a blank line before the inserted notes (insert\n ++\ta blank line is the default).\n +\n --ref ::\n \tManipulate the notes tree in . This overrides\n @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char\n \t};\n \tint edit = !strcmp(argv[0], \"edit\");\n @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char *prefix)\n + \t\tenum object_type type;\n \t\tchar *prev_buf = read_object_file(note, &type, &size);\n \n - \t\tstrbuf_grow(&d.buf, size + 1);\n -\t\tif (d.buf.len && prev_buf && size)\n +\t\tif (blankline && d.buf.len && prev_buf && size)\n \t\t\tstrbuf_insertstr(&d.buf, 0, \"\\n\");\n @@ t/t3301-notes.sh: test_expect_success 'listing non-existing notes fails' '\n '\n \n +test_expect_success 'append to existing note without a beginning blank line' '\n ++\ttest_when_finished git notes remove HEAD &&\n +\tcat >expect <<-\\EOF &&\n +\t\tInitial set of notes\n +\t\tAppended notes\n @@ t/t3301-notes.sh: test_expect_success 'listing non-existing notes fails' '\n \n \t\tMore notes appended with git notes append\n \tEOF\n -+\tgit notes remove HEAD &&\n ++\n \tgit notes add -m \"Initial set of notes\" &&\n \tgit notes append -m \"More notes appended with git notes append\" &&\n \tgit notes show >actual &&\n2: 5dbe014a09 < -: ---------- notes.c: fixed tip when target and append note are both empty\n3: 2475ea0c04 < -: ---------- notes.c: drop unreachable code in \"append_edit()\"\n-- \n2.38.1.386.g196e80358e8\n\n"},{"id":"466939","messageId":"8ae58934a1d98fa2bb5b9383d32b72ddd1855eb9.1667980450.git.dyroneteng@gmail.com","threadId":"58620","inReplyTo":"cover.1667980450.git.dyroneteng@gmail.com","subject":"[PATCH v3 1/5] notes.c: cleanup 'strbuf_grow' call in 'append_edit'","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-09T09:06:52Z","receivedAt":"2022-11-09T09:07:31Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nLet's cleanup the unnecessary 'strbuf_grow' call in 'append_edit'. This\n\"strbuf_grow(&d.buf, size + 1);\" is prepared for insert a blank line if\nneeded, but actually when inserting, \"strbuf_insertstr(&d.buf, 0,\n\"\\n\");\" will do the \"grow\" for us.\n\nSigned-off-by: Teng Long \n---\n builtin/notes.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex be51f69225..f4bab3b2f2 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -618,7 +618,6 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tenum object_type type;\n \t\tchar *prev_buf = read_object_file(note, &type, &size);\n \n-\t\tstrbuf_grow(&d.buf, size + 1);\n \t\tif (d.buf.len && prev_buf && size)\n \t\t\tstrbuf_insertstr(&d.buf, 0, \"\\n\");\n \t\tif (prev_buf && size)\n-- \n2.38.1.386.g196e80358e8\n\n"},{"id":"466940","messageId":"a53576ea880b00c9225f0d0855c7e357edda789c.1667980450.git.dyroneteng@gmail.com","threadId":"58620","inReplyTo":"cover.1667980450.git.dyroneteng@gmail.com","subject":"[PATCH v3 2/5] notes.c: cleanup for \"designated init\" and \"char ptr init\"","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-09T09:06:53Z","receivedAt":"2022-11-09T09:07:48Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nLet's do some cleanup for the following two places in \"append_edit()\".\n\nThe first place is \"char *logmsg;\" need to be initialized with NULL.\nThe second place is \"struct note_data d = { 0, 0, NULL, STRBUF_INIT };\"\ncould be replaced with designated init format.\n\nSigned-off-by: Teng Long \n---\n builtin/notes.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex f4bab3b2f2..485439c901 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -566,9 +566,9 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \tstruct notes_tree *t;\n \tstruct object_id object, new_note;\n \tconst struct object_id *note;\n-\tchar *logmsg;\n+\tchar *logmsg = NULL;\n \tconst char * const *usage;\n-\tstruct note_data d = { 0, 0, NULL, STRBUF_INIT };\n+\tstruct note_data d = { .buf = STRBUF_INIT };\n \tstruct option options[] = {\n \t\tOPT_CALLBACK_F('m', \"message\", &d, N_(\"message\"),\n \t\t\tN_(\"note contents as a string\"), PARSE_OPT_NONEG,\n-- \n2.38.1.386.g196e80358e8\n\n"},{"id":"466941","messageId":"0d8ce0b14b0c956c33c99e9160a4752ef7bd7df4.1667980450.git.dyroneteng@gmail.com","threadId":"58620","inReplyTo":"cover.1667980450.git.dyroneteng@gmail.com","subject":"[PATCH v3 4/5] notes.c: provide tips when target and append note are both empty","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-09T09:06:55Z","receivedAt":"2022-11-09T09:07:51Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nWhen \"git notes append \" is executed, if there is no note in\nthe given object and the appended note is empty too, we could print\nthe exact tips to end-user.\n\nSigned-off-by: Teng Long \n---\n builtin/notes.c | 5 ++++-\n t/t3301-notes.sh | 5 +++--\n 2 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 8c3a1cd913..a6273781fb 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -631,7 +631,10 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\t\tBUG(\"combine_notes_overwrite failed\");\n \t\tlogmsg = xstrfmt(\"Notes added by 'git notes %s'\", argv[0]);\n \t\tcommit_notes(the_repository, t, logmsg);\n-\t}\n+\t} else if (!d.buf.len && !note)\n+\t\tfprintf(stderr,\n+\t\t\t_(\"Both original and appended notes are empty in %s, do nothing\\n\"),\n+\t\t\toid_to_hex(&object));\n \n \tfree(logmsg);\n \tfree_note_data(&d);\ndiff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\nindex 3288aaec7d..e7807e052a 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -552,6 +552,7 @@ test_expect_success 'appending empty string does not change existing note' '\n '\n \n test_expect_success 'git notes append == add when there is no existing note' '\n+\ttest_when_finished git notes remove HEAD &&\n \tgit notes remove HEAD &&\n \ttest_must_fail git notes list HEAD &&\n \tgit notes append -m \"Initial set of notes${LF}${LF}More notes appended with git notes append\" &&\n@@ -560,9 +561,9 @@ test_expect_success 'git notes append == add when there is no existing note' '\n '\n \n test_expect_success 'appending empty string to non-existing note does not create note' '\n-\tgit notes remove HEAD &&\n \ttest_must_fail git notes list HEAD &&\n-\tgit notes append -m \"\" &&\n+\tgit notes append -m \"\" >output 2>&1 &&\n+\tgrep \"Both original and appended notes are empty\" output &&\n \ttest_must_fail git notes list HEAD\n '\n \n-- \n2.38.1.386.g196e80358e8\n\n"},{"id":"466942","messageId":"62a952ba3e7ad8599495119723b0fcd7044c819e.1667980450.git.dyroneteng@gmail.com","threadId":"58620","inReplyTo":"cover.1667980450.git.dyroneteng@gmail.com","subject":"[PATCH v3 3/5] notes.c: drop unreachable code in 'append_edit()'","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-09T09:06:54Z","receivedAt":"2022-11-09T09:07:55Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nSituation of note \"removing\" shouldn't happen in 'append_edit()',\nunless it's a bug. So, let's drop the unreachable \"else\" code\nin \"append_edit()\".\n\nThe notes operation \"append\" is different with \"add\", the latter\nsupports to overwrite the existing note then let the \"removing\"\nhappen (e.g. execute `git notes add -f -F /dev/null` on an existing\nnote), but the former will not because it only does \"appends\" but\nnot doing \"overwrites\".\n\nSigned-off-by: Teng Long \n---\n builtin/notes.c | 7 +------\n 1 file changed, 1 insertion(+), 6 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 485439c901..8c3a1cd913 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -630,13 +630,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tif (add_note(t, &object, &new_note, combine_notes_overwrite))\n \t\t\tBUG(\"combine_notes_overwrite failed\");\n \t\tlogmsg = xstrfmt(\"Notes added by 'git notes %s'\", argv[0]);\n-\t} else {\n-\t\tfprintf(stderr, _(\"Removing note for object %s\\n\"),\n-\t\t\toid_to_hex(&object));\n-\t\tremove_note(t, object.hash);\n-\t\tlogmsg = xstrfmt(\"Notes removed by 'git notes %s'\", argv[0]);\n+\t\tcommit_notes(the_repository, t, logmsg);\n \t}\n-\tcommit_notes(the_repository, t, logmsg);\n \n \tfree(logmsg);\n \tfree_note_data(&d);\n-- \n2.38.1.386.g196e80358e8\n\n"},{"id":"466943","messageId":"196e80358e80c8904cd21e6d8fca2223af93f842.1667980450.git.dyroneteng@gmail.com","threadId":"58620","inReplyTo":"cover.1667980450.git.dyroneteng@gmail.com","subject":"[PATCH v3 5/5] notes.c: introduce \"--no-blank-line\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-09T09:06:56Z","receivedAt":"2022-11-09T09:08:02Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nWhen appending to a given object which has note and if the appended\nnote is not empty too, we will insert a blank line at first. The\nblank line serves as a split line, but sometimes we just want to\nomit it then append on the heels of the target note. So, we add\na new \"OPT_BOOL()\" option to determain whether a new blank line\nis insert at first.\n\nSigned-off-by: Teng Long \n---\n Documentation/git-notes.txt | 10 ++++++++--\n builtin/notes.c | 5 ++++-\n t/t3301-notes.sh | 13 +++++++++++++\n 3 files changed, 25 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\nindex efbc10f0f5..50b198c2b2 100644\n--- a/Documentation/git-notes.txt\n+++ b/Documentation/git-notes.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n 'git notes' [list []]\n 'git notes' add [-f] [--allow-empty] [-F | -m | (-c | -C) ] []\n 'git notes' copy [-f] ( --stdin | [] )\n-'git notes' append [--allow-empty] [-F | -m | (-c | -C) ] []\n+'git notes' append [--allow-empty] [--no-blank-line] [-F | -m | (-c | -C) ] []\n 'git notes' edit [--allow-empty] []\n 'git notes' show []\n 'git notes' merge [-v | -q] [-s ] \n@@ -86,7 +86,9 @@ the command can read the input given to the `post-rewrite` hook.)\n \n append::\n \tAppend to the notes of an existing object (defaults to HEAD).\n-\tCreates a new notes object if needed.\n+\tCreates a new notes object if needed. If the note of the given\n+\tobject and the note to be appended are not empty, a blank line\n+\twill be inserted between them.\n \n edit::\n \tEdit the notes for a given object (defaults to HEAD).\n@@ -159,6 +161,10 @@ OPTIONS\n \tAllow an empty note object to be stored. The default behavior is\n \tto automatically remove empty notes.\n \n+--no-blank-line::\n+\tDo not insert a blank line before the inserted notes (insert\n+\ta blank line is the default).\n+\n --ref ::\n \tManipulate the notes tree in . This overrides\n \t`GIT_NOTES_REF` and the \"core.notesRef\" configuration. The ref\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex a6273781fb..f38e6e8b04 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -562,6 +562,7 @@ static int copy(int argc, const char **argv, const char *prefix)\n static int append_edit(int argc, const char **argv, const char *prefix)\n {\n \tint allow_empty = 0;\n+\tint blankline = 1;\n \tconst char *object_ref;\n \tstruct notes_tree *t;\n \tstruct object_id object, new_note;\n@@ -584,6 +585,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\t\tparse_reuse_arg),\n \t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n \t\t\tN_(\"allow storing empty note\")),\n+\t\tOPT_BOOL(0, \"blank-line\", &blankline,\n+\t\t\tN_(\"insert paragraph break before appending to an existing note\")),\n \t\tOPT_END()\n \t};\n \tint edit = !strcmp(argv[0], \"edit\");\n@@ -618,7 +621,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tenum object_type type;\n \t\tchar *prev_buf = read_object_file(note, &type, &size);\n \n-\t\tif (d.buf.len && prev_buf && size)\n+\t\tif (blankline && d.buf.len && prev_buf && size)\n \t\t\tstrbuf_insertstr(&d.buf, 0, \"\\n\");\n \t\tif (prev_buf && size)\n \t\t\tstrbuf_insert(&d.buf, 0, prev_buf, size);\ndiff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\nindex e7807e052a..dedad93a2f 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -521,12 +521,25 @@ test_expect_success 'listing non-existing notes fails' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'append to existing note without a beginning blank line' '\n+\ttest_when_finished git notes remove HEAD &&\n+\tcat >expect <<-\\EOF &&\n+\t\tInitial set of notes\n+\t\tAppended notes\n+\tEOF\n+\tgit notes add -m \"Initial set of notes\" &&\n+\tgit notes append --no-blank-line -m \"Appended notes\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'append to existing note with \"git notes append\"' '\n \tcat >expect <<-EOF &&\n \t\tInitial set of notes\n \n \t\tMore notes appended with git notes append\n \tEOF\n+\n \tgit notes add -m \"Initial set of notes\" &&\n \tgit notes append -m \"More notes appended with git notes append\" &&\n \tgit notes show >actual &&\n-- \n2.38.1.386.g196e80358e8\n\n"},{"id":"468103","messageId":"20221128142032.71179-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"cover.1667980450.git.dyroneteng@gmail.com","subject":"[PATCH v3 0/5] notes.c: introduce \"--no-blank-line\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-28T14:20:32Z","receivedAt":"2022-11-28T14:20:45Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"> Teng Long (5):\n> notes.c: cleanup 'strbuf_grow' call in 'append_edit'\n> notes.c: cleanup for \"designated init\" and \"char ptr init\"\n> notes.c: drop unreachable code in 'append_edit()'\n> notes.c: provide tips when target and append note are both empty\n> notes.c: introduce \"--no-blank-line\" option\n\nI'm not sure if this patch series should continue, and if there are no\nupdated comments it will be temporarily suspended.\n\nThanks for the reviews on the past patches.\n"},{"id":"468140","messageId":"xmqqzgca4lb7.fsf@gitster.g","threadId":"58620","inReplyTo":"20221128142032.71179-1-tenglong.tl@alibaba-inc.com","subject":"Re: [PATCH v3 0/5] notes.c: introduce \"--no-blank-line\" option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-29T01:10:52Z","receivedAt":"2022-11-29T01:10:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n>> Teng Long (5):\n>> notes.c: cleanup 'strbuf_grow' call in 'append_edit'\n>> notes.c: cleanup for \"designated init\" and \"char ptr init\"\n>> notes.c: drop unreachable code in 'append_edit()'\n>> notes.c: provide tips when target and append note are both empty\n>> notes.c: introduce \"--no-blank-line\" option\n>\n> I'm not sure if this patch series should continue, and if there are no\n> updated comments it will be temporarily suspended.\n>\n> Thanks for the reviews on the past patches.\n\nTaylor, This topic was marked to \"expect\" a reroll in the second\nissue of November \"What's cooking\" report you did. Do you recall\nwhat remaining works there were?\n\nI personally do not have much opinion on this topic, other than that\n\"--no-blank-link\" would be a horrible name (i.e. uses concrete words\nto pretend that it clearly describes what it does, but is utterly\nunclear where these blank lines are etc.) for the feature to help\nend-users discover it.\n"},{"id":"468176","messageId":"20221129125738.81212-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"cover.1667980450.git.dyroneteng@gmail.com","subject":"Re: [PATCH v3 0/5] notes.c: introduce \"--no-blank-line\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-29T12:57:38Z","receivedAt":"2022-11-29T12:58:14Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Junio C Hamano writes:\n\n> I personally do not have much opinion on this topic, other than that\n> \"--no-blank-link\" would be a horrible name (i.e. uses concrete words\n> to pretend that it clearly describes what it does, but is utterly\n> unclear where these blank lines are etc.) for the feature to help\n> end-users discover it.\n\nI have some a candidates might like '--newline' and '--no-newline', or\n'splitted-newline' or 'no-splitted-newline', but I think the latter is\na little long.\n\nThanks.\n"},{"id":"468184","messageId":"xmqqh6yh3nk4.fsf@gitster.g","threadId":"58620","inReplyTo":"20221129125738.81212-1-tenglong.tl@alibaba-inc.com","subject":"Re: [PATCH v3 0/5] notes.c: introduce \"--no-blank-line\" option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-29T13:19:55Z","receivedAt":"2022-11-29T13:20:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n> Junio C Hamano writes:\n>\n>> I personally do not have much opinion on this topic, other than that\n>> \"--no-blank-link\" would be a horrible name (i.e. uses concrete words\n>> to pretend that it clearly describes what it does, but is utterly\n>> unclear where these blank lines are etc.) for the feature to help\n>> end-users discover it.\n>\n> I have some a candidates might like '--newline' and '--no-newline', or\n> 'splitted-newline' or 'no-splitted-newline', but I think the latter is\n> a little long.\n>\n> Thanks.\n\nI do not care much between blank and newline. \n\nBoth alone are equally horrible, in that the option would have no\neffect when \"git notes edit\" is used and spawns an editor, or \"git\nnotes append -m one -m two\" is used and the command adds the second\nparagraph whose text is \"two\".\n\nJust like \"git commit\", the argument to each \"-m\" option becomes a\nseparate paragraph by default. I personally feel that those who\nwant to make them separate lines deserve to have an option like this\none, so that they can do\n\n $ git notes add -m foo HEAD\n $ git notes append \\\n\t--each-message-is-line-not-paragraph -m bar -m baz\n $ git notes show HEAD\n foo\n bar\n baz\n\nbut the point is \"blank-line\" or \"newline\" does not say which\nnewline in the resulting notes object you are mucking with. It is\nnot like in this example:\n\n $ git notes add -m \"title of the note\"\n $ git notes append --no-blank-line -m \"body of the note\n\n that span multiple\n\n lines\" HEAD\n\nyou are removving the blank lines embedded in the body of the\nmessage, but from the option name, it is hard to guess that.\n"},{"id":"468213","messageId":"Y4aNbc1ZMxscyvU1@nand.local","threadId":"58620","inReplyTo":"xmqqzgca4lb7.fsf@gitster.g","subject":"Re: [PATCH v3 0/5] notes.c: introduce \"--no-blank-line\" option","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-29T22:53:33Z","receivedAt":"2022-11-29T22:54:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Nov 29, 2022 at 10:10:52AM +0900, Junio C Hamano wrote:\n> Teng Long writes:\n>\n> >> Teng Long (5):\n> >> notes.c: cleanup 'strbuf_grow' call in 'append_edit'\n> >> notes.c: cleanup for \"designated init\" and \"char ptr init\"\n> >> notes.c: drop unreachable code in 'append_edit()'\n> >> notes.c: provide tips when target and append note are both empty\n> >> notes.c: introduce \"--no-blank-line\" option\n> >\n> > I'm not sure if this patch series should continue, and if there are no\n> > updated comments it will be temporarily suspended.\n> >\n> > Thanks for the reviews on the past patches.\n>\n> Taylor, This topic was marked to \"expect\" a reroll in the second\n> issue of November \"What's cooking\" report you did. Do you recall\n> what remaining works there were?\n\nLooking at the dates, I sent that WC on 2022-11-08, which means that I\nmost likely was referencing the discussion between the author and Ævar.\n\nThe following WC on 2022-11-14 likely should have changed the status to\n\"Waiting for review\".\n\nThanks,\nTaylor\n"},{"id":"469121","messageId":"20221215124807.12730-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"xmqqh6yh3nk4.fsf@gitster.g","subject":"Re: [PATCH v3 0/5] notes.c: introduce \"--no-blank-line\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-12-15T12:48:07Z","receivedAt":"2022-12-15T12:48:33Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Junio C Hamano writes:\n\n> but the point is \"blank-line\" or \"newline\" does not say which\n> newline in the resulting notes object you are mucking with. It is\n> not like in this example:\n>\n> $ git notes add -m \"title of the note\"\n> $ git notes append --no-blank-line -m \"body of the note\n>\n> that span multiple\n>\n> lines\" HEAD\n>\n> you are removving the blank lines embedded in the body of the\n> message, but from the option name, it is hard to guess that.\n\nFine, I see. Maybe \"insert-starting-newline\" or \"insert-initial-newline\". If we\ncould not find a suitable naming for this, it's ok for me to hang up [5/5] (a\nlittle struggle for me to find a better name for this now in fact (⊙ˍ⊙).),\nbecause except it, the [4/5] still an active bugfix in my opinion.\n\nThanks.\n"},{"id":"469258","messageId":"CAPig+cRcezSp4Rqt1Y9bD-FT6+7b0g9qHfbGRx65AOnw2FQXKg@mail.gmail.com","threadId":"58620","inReplyTo":"20221215124807.12730-1-tenglong.tl@alibaba-inc.com","subject":"Re: [PATCH v3 0/5] notes.c: introduce \"--no-blank-line\" option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-12-19T03:03:46Z","receivedAt":"2022-12-19T03:07:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Dec 15, 2022 at 7:58 AM Teng Long wrote:\n> Junio C Hamano writes:\n>\n> > but the point is \"blank-line\" or \"newline\" does not say which\n> > newline in the resulting notes object you are mucking with. It is\n> > not like in this example:\n> > [...]\n> > you are removving the blank lines embedded in the body of the\n> > message, but from the option name, it is hard to guess that.\n>\n> Fine, I see. Maybe \"insert-starting-newline\" or \"insert-initial-newline\". If we\n> could not find a suitable naming for this, it's ok for me to hang up [5/5] (a\n> little struggle for me to find a better name for this now in fact (⊙ˍ⊙).),\n> because except it, the [4/5] still an active bugfix in my opinion.\n\nTaking a step back, perhaps think of this in terms of \"separator\". The\ndefault behavior is to insert \"\\n\" as a separator between notes. If\nyou add a --separator option, then users could supply their own\nseparator, such as \"----\\n\" or, in your case, \"\" to suppress the blank\nline.\n"},{"id":"469426","messageId":"20221221091650.43359-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"CAPig+cRcezSp4Rqt1Y9bD-FT6+7b0g9qHfbGRx65AOnw2FQXKg@mail.gmail.com","subject":"Re: [PATCH v3 0/5] notes.c: introduce \"--no-blank-line\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-12-21T09:16:50Z","receivedAt":"2022-12-21T09:17:04Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Eric Sunshine writes:\n\n> Taking a step back, perhaps think of this in terms of \"separator\". The\n> default behavior is to insert \"\\n\" as a separator between notes. If\n> you add a --separator option, then users could supply their own\n> separator, such as \"----\\n\" or, in your case, \"\" to suppress the blank\n> line.\n\nYour idea is an enhancement of the original one of me. I think it's a suitable\nname and I could implement it, maybe we could hear about Junio's advice of the\nnaming?\n\nThanks.\n"},{"id":"469430","messageId":"xmqqili5rnts.fsf@gitster.g","threadId":"58620","inReplyTo":"20221221091650.43359-1-tenglong.tl@alibaba-inc.com","subject":"Re: [PATCH v3 0/5] notes.c: introduce \"--no-blank-line\" option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-21T11:35:59Z","receivedAt":"2022-12-21T11:36:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n> Eric Sunshine writes:\n>\n>> Taking a step back, perhaps think of this in terms of \"separator\". The\n>> default behavior is to insert \"\\n\" as a separator between notes. If\n>> you add a --separator option, then users could supply their own\n>> separator, such as \"----\\n\" or, in your case, \"\" to suppress the blank\n>> line.\n>\n> Your idea is an enhancement of the original one of me. I think it's a suitable\n> name and I could implement it, maybe we could hear about Junio's advice of the\n> naming?\n\nYeah, saying \"separator\" clarifies what that empty line is meant to\nbe (i.e. it is an inter-paragraph separator), and is much better\nthan \"newline\" or \"blankline\", I would think.\n\nThanks.\n"},{"id":"469453","messageId":"20221222093021.5251-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"CAPig+cRcezSp4Rqt1Y9bD-FT6+7b0g9qHfbGRx65AOnw2FQXKg@mail.gmail.com","subject":"Re: [PATCH v3 0/5] notes.c: introduce \"--no-blank-line\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-12-22T09:30:21Z","receivedAt":"2022-12-22T09:30:31Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Eric Sunshine writes:\n\n> Taking a step back, perhaps think of this in terms of \"separator\". The\n> default behavior is to insert \"\\n\" as a separator between notes. If\n> you add a --separator option, then users could supply their own\n> separator, such as \"----\\n\" or, in your case, \"\" to suppress the blank\n> line.\n\nThere is another question for me, if the separator we passed contains \"\\n\"\nstring , the argument the cmd receives will need to tranfer to '\\n' character\ninstead to make sure it's a linebreak but not a \"\\n\" instead.\n\nSo maybe like:\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex f38e6e8b04..64ee64eff7 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -559,15 +559,52 @@ static int copy(int argc, const char **argv, const char *prefix)\n return retval;\n }\n\n+static void insert_separator(struct strbuf *message, const char *separator)\n+{\n+ struct strbuf transfered = STRBUF_INIT;\n+\n+ if (!separator)\n+ strbuf_insertstr(message, 0, \"\\n\");\n+ else if (!strcmp(\"\", separator))\n+ return;\n+ else {\n+ while (*separator) {\n+ if (*separator == '\\\\'){\n+ switch (separator[1]) {\n+ case 'n':\n+ strbuf_addstr(&transfered, \"\\n\");\n+ separator++;\n+ break;\n+ case 'r':\n+ strbuf_addstr(&transfered, \"\\r\");\n+ separator++;\n+ break;\n+ case 't':\n+ strbuf_addstr(&transfered, \"\\t\");\n+ separator++;\n+ break;\n+ default:\n+ strbuf_addch(&transfered, *separator);\n+ }\n+ } else {\n+ strbuf_addch(&transfered, *separator);\n+ }\n+ separator++;\n+ }\n+ strbuf_insertstr(message, 0, transfered.buf);\n+ strbuf_release(&transfered);\n+ }\n+}\n+\n static int append_edit(int argc, const char **argv, const char *prefix)\n {\n int allow_empty = 0;\n- int blankline = 1;\n const char *object_ref;\n struct notes_tree *t;\n struct object_id object, new_note;\n const struct object_id *note;\n char *logmsg = NULL;\n+ const char *separator = NULL;\n const char * const *usage;\n struct note_data d = { .buf = STRBUF_INIT };\n struct option options[] = {\n@@ -585,8 +622,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n parse_reuse_arg),\n OPT_BOOL(0, \"allow-empty\", &allow_empty,\n N_(\"allow storing empty note\")),\n- OPT_BOOL(0, \"blank-line\", &blankline,\n- N_(\"insert paragraph break before appending to an existing note\")),\n+ OPT_STRING(0, \"separator\", &separator, N_(\"text\"),\n+ N_(\"insert as separator before appending to an existing note\")),\n OPT_END()\n };\n int edit = !strcmp(argv[0], \"edit\");\n@@ -621,8 +658,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n enum object_type type;\n char *prev_buf = read_object_file(note, &type, &size);\n\n- if (blankline && d.buf.len && prev_buf && size)\n- strbuf_insertstr(&d.buf, 0, \"\\n\");\n+ if (d.buf.len && prev_buf && size)\n+ insert_separator(&d.buf, separator);\n if (prev_buf && size)\n strbuf_insert(&d.buf, 0, prev_buf, size);\n free(prev_buf);\n--\n\nIf the above is understood correctly, is there an api that handles escape\ncharacters already in the existing code (I haven't found one so far, so I need\nto confirm and replace it if there is one). In addition, the insert_separator\nfunction above handles three special characters \\t\\n\\r. Do we need more?\n\nThanks\n"},{"id":"469486","messageId":"CAPig+cTFBVAL2gd3LqQEzS--cXqJXR+1OVerii-D6JqFvJwXqQ@mail.gmail.com","threadId":"58620","inReplyTo":"20221222093021.5251-1-tenglong.tl@alibaba-inc.com","subject":"Re: [PATCH v3 0/5] notes.c: introduce \"--no-blank-line\" option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-12-23T01:36:29Z","receivedAt":"2022-12-23T01:36:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Dec 22, 2022 at 4:30 AM Teng Long wrote:\n> Eric Sunshine writes:\n> > Taking a step back, perhaps think of this in terms of \"separator\". The\n> > default behavior is to insert \"\\n\" as a separator between notes. If\n> > you add a --separator option, then users could supply their own\n> > separator, such as \"----\\n\" or, in your case, \"\" to suppress the blank\n> > line.\n>\n> There is another question for me, if the separator we passed contains \"\\n\"\n> string , the argument the cmd receives will need to tranfer to '\\n' character\n> instead to make sure it's a linebreak but not a \"\\n\" instead.\n>\n> So maybe like:\n> +static void insert_separator(struct strbuf *message, const char *separator)\n> +{\n> + while (*separator) {\n> + if (*separator == '\\\\'){\n> + switch (separator[1]) {\n> + case 'n':\n> + strbuf_addstr(&transfered, \"\\n\");\n> + separator++;\n> + break;\n> + [...]\n> + separator++;\n> + }\n> +}\n>\n> If the above is understood correctly, is there an api that handles escape\n> characters already in the existing code (I haven't found one so far, so I need\n> to confirm and replace it if there is one). In addition, the insert_separator\n> function above handles three special characters \\t\\n\\r. Do we need more?\n\nYou could probably use unquote_c_style() from quote.[hc]; something like this:\n\n struct strbuf orig = STRBUF_INIT;\n struct strbuf unquoted = STRBUF_INIT;\n strbuf_addf(&orig, \"\\\"%s\\\"\", separator);\n if (unquote_c_style(&unquoted, orig.buf, NULL) < 0) {\n strbuf_release(&unquoted);\n strbuf_release(&orig);\n die(_(\"some suitable error message\"));\n }\n /* unquote succeeded -- use \"unquoted\" here */\n\nHowever, I suspect that this is overkill, and you should explore\nsimpler ideas first.\n\nFor instance, it is perfectly acceptable to embed newlines directly in\nshell strings, so this would work just fine without having to write\nany extra string-unquoting code:\n\n % git notes add --separator='---\n ' \n\nBut, I think you can make this even friendlier without having to do\nany extra coding to support string-unquoting. In particular, use this\nheuristic:\n\n - if the separator is zero-length, use it as-is\n - otherwise, if the separator ends with a newline, use it as-is\n - otherwise add a newline to the separator\n\nIn other words:\n\n if (!separator)\n separator = \"\\n\"; /* default is one blank line */\n if (*separator == '\\0')\n /* separator is empty; use as-is (no blank line) */\n else if (separator[strlen(separator) - 1] == '\\n')\n /* user supplied newline; use as-is */\n else\n /* separator lacks newline; add it ourselves */\n\nWith the above logic, this defaults to a blank line between notes:\n\n % git notes add ...\n\nthis has no blank line between notes:\n\n % git notes add --separator='' ...\n\nthis uses a \"---\" + \"\\n\" as separator:\n\n % git notes add --separator='---' ...\n\nas does this:\n\n % git notes add --separator='---\n ' ...\n\nand this places two blank lines between notes:\n\n % git notes add --separator='\n\n ' ...\n"},{"id":"470134","messageId":"cover.1673490953.git.dyroneteng@gmail.com","threadId":"58620","inReplyTo":"cover.1667980450.git.dyroneteng@gmail.com","subject":"[PATCH v4 0/5] notes.c: introduce \"--separator\" optio","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-01-12T02:48:04Z","receivedAt":"2023-01-12T02:48:23Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nSorry for the late patch, my family just recovered from COVID-19.\n\nDiff from v3:\n\n * rename option name from '--blank-line' to '--separator'\n\nThanks.\n\nTeng Long (5):\n notes.c: cleanup 'strbuf_grow' call in 'append_edit'\n notes.c: cleanup for \"designated init\" and \"char ptr init\"\n notes.c: drop unreachable code in 'append_edit()'\n notes.c: provide tips when target and append note are both empty\n notes.c: introduce \"--separator\" option\n\n Documentation/git-notes.txt | 18 +++++++++--\n builtin/notes.c | 64 +++++++++++++++++++++++++++++--------\n t/t3301-notes.sh | 64 +++++++++++++++++++++++++++++++++++--\n 3 files changed, 129 insertions(+), 17 deletions(-)\n\nRange-diff against v3:\n1: 8ae58934a1 = 1: f00a759658 notes.c: cleanup 'strbuf_grow' call in 'append_edit'\n2: a53576ea88 = 2: 29f7703b06 notes.c: cleanup for \"designated init\" and \"char ptr init\"\n3: 62a952ba3e = 3: 7b756b4c60 notes.c: drop unreachable code in 'append_edit()'\n4: 0d8ce0b14b = 4: d41ba14050 notes.c: provide tips when target and append note are both empty\n5: 196e80358e < -: ---------- notes.c: introduce \"--no-blank-line\" option\n-: ---------- > 5: f7edbd0e50 notes.c: introduce \"--separator\" option\n-- \n2.38.1.386.g6952793f2d9.dirty\n\n"},{"id":"470135","messageId":"f00a7596587bbf2d055dbf77a19506be1a6350fd.1673490953.git.dyroneteng@gmail.com","threadId":"58620","inReplyTo":"cover.1673490953.git.dyroneteng@gmail.com","subject":"[PATCH v4 1/5] notes.c: cleanup 'strbuf_grow' call in 'append_edit'","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-01-12T02:48:05Z","receivedAt":"2023-01-12T02:48:26Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nLet's cleanup the unnecessary 'strbuf_grow' call in 'append_edit'. This\n\"strbuf_grow(&d.buf, size + 1);\" is prepared for insert a blank line if\nneeded, but actually when inserting, \"strbuf_insertstr(&d.buf, 0,\n\"\\n\");\" will do the \"grow\" for us.\n\nSigned-off-by: Teng Long \n---\n builtin/notes.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 80d9dfd25c..e57f024824 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -618,7 +618,6 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tenum object_type type;\n \t\tchar *prev_buf = read_object_file(note, &type, &size);\n \n-\t\tstrbuf_grow(&d.buf, size + 1);\n \t\tif (d.buf.len && prev_buf && size)\n \t\t\tstrbuf_insertstr(&d.buf, 0, \"\\n\");\n \t\tif (prev_buf && size)\n-- \n2.38.1.386.g6952793f2d9.dirty\n\n"},{"id":"470136","messageId":"29f7703b06dfecd1daedb72a7027aa953addfa3a.1673490953.git.dyroneteng@gmail.com","threadId":"58620","inReplyTo":"cover.1673490953.git.dyroneteng@gmail.com","subject":"[PATCH v4 2/5] notes.c: cleanup for \"designated init\" and \"char ptr init\"","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-01-12T02:48:06Z","receivedAt":"2023-01-12T02:48:27Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nLet's do some cleanup for the following two places in \"append_edit()\".\n\nThe first place is \"char *logmsg;\" need to be initialized with NULL.\nThe second place is \"struct note_data d = { 0, 0, NULL, STRBUF_INIT };\"\ncould be replaced with designated init format.\n\nSigned-off-by: Teng Long \n---\n builtin/notes.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex e57f024824..8ca55ec83e 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -566,9 +566,9 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \tstruct notes_tree *t;\n \tstruct object_id object, new_note;\n \tconst struct object_id *note;\n-\tchar *logmsg;\n+\tchar *logmsg = NULL;\n \tconst char * const *usage;\n-\tstruct note_data d = { 0, 0, NULL, STRBUF_INIT };\n+\tstruct note_data d = { .buf = STRBUF_INIT };\n \tstruct option options[] = {\n \t\tOPT_CALLBACK_F('m', \"message\", &d, N_(\"message\"),\n \t\t\tN_(\"note contents as a string\"), PARSE_OPT_NONEG,\n-- \n2.38.1.386.g6952793f2d9.dirty\n\n"},{"id":"470137","messageId":"d41ba140505aa3459330afd652ff6a0f456222a0.1673490953.git.dyroneteng@gmail.com","threadId":"58620","inReplyTo":"cover.1673490953.git.dyroneteng@gmail.com","subject":"[PATCH v4 4/5] notes.c: provide tips when target and append note are both empty","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-01-12T02:48:08Z","receivedAt":"2023-01-12T02:49:06Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nWhen \"git notes append \" is executed, if there is no note in\nthe given object and the appended note is empty too, we could print\nthe exact tips to end-user.\n\nSigned-off-by: Teng Long \n---\n builtin/notes.c | 5 ++++-\n t/t3301-notes.sh | 5 +++--\n 2 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex b71a81bff1..f2efb3736c 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -631,7 +631,10 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\t\tBUG(\"combine_notes_overwrite failed\");\n \t\tlogmsg = xstrfmt(\"Notes added by 'git notes %s'\", argv[0]);\n \t\tcommit_notes(the_repository, t, logmsg);\n-\t}\n+\t} else if (!d.buf.len && !note)\n+\t\tfprintf(stderr,\n+\t\t\t_(\"Both original and appended notes are empty in %s, do nothing\\n\"),\n+\t\t\toid_to_hex(&object));\n \n \tfree(logmsg);\n \tfree_note_data(&d);\ndiff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\nindex 3288aaec7d..e7807e052a 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -552,6 +552,7 @@ test_expect_success 'appending empty string does not change existing note' '\n '\n \n test_expect_success 'git notes append == add when there is no existing note' '\n+\ttest_when_finished git notes remove HEAD &&\n \tgit notes remove HEAD &&\n \ttest_must_fail git notes list HEAD &&\n \tgit notes append -m \"Initial set of notes${LF}${LF}More notes appended with git notes append\" &&\n@@ -560,9 +561,9 @@ test_expect_success 'git notes append == add when there is no existing note' '\n '\n \n test_expect_success 'appending empty string to non-existing note does not create note' '\n-\tgit notes remove HEAD &&\n \ttest_must_fail git notes list HEAD &&\n-\tgit notes append -m \"\" &&\n+\tgit notes append -m \"\" >output 2>&1 &&\n+\tgrep \"Both original and appended notes are empty\" output &&\n \ttest_must_fail git notes list HEAD\n '\n \n-- \n2.38.1.386.g6952793f2d9.dirty\n\n"},{"id":"470138","messageId":"7b756b4c605e148f6938fee74882091661382173.1673490953.git.dyroneteng@gmail.com","threadId":"58620","inReplyTo":"cover.1673490953.git.dyroneteng@gmail.com","subject":"[PATCH v4 3/5] notes.c: drop unreachable code in 'append_edit()'","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-01-12T02:48:07Z","receivedAt":"2023-01-12T02:49:10Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nSituation of note \"removing\" shouldn't happen in 'append_edit()',\nunless it's a bug. So, let's drop the unreachable \"else\" code\nin \"append_edit()\".\n\nThe notes operation \"append\" is different with \"add\", the latter\nsupports to overwrite the existing note then let the \"removing\"\nhappen (e.g. execute `git notes add -f -F /dev/null` on an existing\nnote), but the former will not because it only does \"appends\" but\nnot doing \"overwrites\".\n\nSigned-off-by: Teng Long \n---\n builtin/notes.c | 7 +------\n 1 file changed, 1 insertion(+), 6 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 8ca55ec83e..b71a81bff1 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -630,13 +630,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tif (add_note(t, &object, &new_note, combine_notes_overwrite))\n \t\t\tBUG(\"combine_notes_overwrite failed\");\n \t\tlogmsg = xstrfmt(\"Notes added by 'git notes %s'\", argv[0]);\n-\t} else {\n-\t\tfprintf(stderr, _(\"Removing note for object %s\\n\"),\n-\t\t\toid_to_hex(&object));\n-\t\tremove_note(t, object.hash);\n-\t\tlogmsg = xstrfmt(\"Notes removed by 'git notes %s'\", argv[0]);\n+\t\tcommit_notes(the_repository, t, logmsg);\n \t}\n-\tcommit_notes(the_repository, t, logmsg);\n \n \tfree(logmsg);\n \tfree_note_data(&d);\n-- \n2.38.1.386.g6952793f2d9.dirty\n\n"},{"id":"470139","messageId":"f7edbd0e508243ab55c13721a21b78bf50278a21.1673490953.git.dyroneteng@gmail.com","threadId":"58620","inReplyTo":"cover.1673490953.git.dyroneteng@gmail.com","subject":"[PATCH v4 5/5] notes.c: introduce \"--separator\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-01-12T02:48:09Z","receivedAt":"2023-01-12T02:49:12Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nWhen appending to a given notes object and the appended note is not\nempty too, we will insert a blank line at first which separates the\nexisting note and the appended one, which as the separator.\n\nSometimes, we want to use a specified as the separator. For\nexample, if we specify as:\n\n * --separator='------': we will insert \"------\\n\" as the separator,\n because user do not provide the line break char at last, we will add\n the trailing '\\n' compatibly.\n\n * --separator='------\\n': we will insert as-is because it contains\n the line break at last.\n\n * --separator='': we specify an empty separator which means will\n append the message directly without inserting any separator at\n first.\n\n * not specified --separator option: will use '\\n' as the separator\n when do appending and this is the default behavour.\n\nSigned-off-by: Teng Long \n---\n Documentation/git-notes.txt | 18 +++++++++--\n builtin/notes.c | 49 +++++++++++++++++++++++++++---\n t/t3301-notes.sh | 59 +++++++++++++++++++++++++++++++++++++\n 3 files changed, 120 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\nindex efbc10f0f5..227fa88317 100644\n--- a/Documentation/git-notes.txt\n+++ b/Documentation/git-notes.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n 'git notes' [list []]\n 'git notes' add [-f] [--allow-empty] [-F | -m | (-c | -C) ] []\n 'git notes' copy [-f] ( --stdin | [] )\n-'git notes' append [--allow-empty] [-F | -m | (-c | -C) ] []\n+'git notes' append [--allow-empty] [--separator] [-F | -m | (-c | -C) ] []\n 'git notes' edit [--allow-empty] []\n 'git notes' show []\n 'git notes' merge [-v | -q] [-s ] \n@@ -86,7 +86,11 @@ the command can read the input given to the `post-rewrite` hook.)\n \n append::\n \tAppend to the notes of an existing object (defaults to HEAD).\n-\tCreates a new notes object if needed.\n+\tCreates a new notes object if needed. If the note of the given\n+\tobject and the note to be appended are not empty, a blank line\n+\twill be inserted between them as the separator (\"blank line\" is\n+\tthe default behavior, `--separator` option supports to specify\n+\ta customized one).\n \n edit::\n \tEdit the notes for a given object (defaults to HEAD).\n@@ -159,6 +163,16 @@ OPTIONS\n \tAllow an empty note object to be stored. The default behavior is\n \tto automatically remove empty notes.\n \n+--separator ::\n+\tSpecify the to be inserted between existing note and appended\n+\tmessage, the acts as a separator.\n+\tIf is empty (`--separator=''`), will append the message to\n+\texisting note directly without insert any separator.\n+\tIf is nonempty, will use as-is. One thing to notice is if\n+\tthe lacks newline charactor, will add the newline automatically.\n+\tIf not specify this option, a blank line will be inserted as the\n+\tseparator.\n+\n --ref ::\n \tManipulate the notes tree in . This overrides\n \t`GIT_NOTES_REF` and the \"core.notesRef\" configuration. The ref\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex f2efb3736c..6746ad3232 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -24,6 +24,8 @@\n #include \"notes-utils.h\"\n #include \"worktree.h\"\n \n+static char *separator = \"\\n\";\n+\n static const char * const git_notes_usage[] = {\n \tN_(\"git notes [--ref ] [list []]\"),\n \tN_(\"git notes [--ref ] add [-f] [--allow-empty] [-m | -F | (-c | -C) ] []\"),\n@@ -209,7 +211,7 @@ static void write_note_data(struct note_data *d, struct object_id *oid)\n \t}\n }\n \n-static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n+static int parse_msg_arg_add(const struct option *opt, const char *arg, int unset)\n {\n \tstruct note_data *d = opt->value;\n \n@@ -225,6 +227,43 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+static void insert_separator(struct strbuf *message)\n+{\n+\tconst char *insert;\n+\n+\tif (!separator)\n+\t\tseparator = \"\\n\";\n+\tif (*separator == '\\0')\n+\t\t/* separator is empty; use as-is (no blank line) */\n+\t\treturn;\n+\telse if (separator[strlen(separator) - 1] == '\\n')\n+\t\t/* user supplied newline; use as-is */\n+\t\tinsert = separator;\n+\telse\n+\t\t/* separator lacks newline; add it ourselves */\n+\t\tinsert = xstrfmt(\"%s%s\", separator,\"\\n\");\n+\tstrbuf_insertstr(message, 0, insert);\n+}\n+\n+static int parse_msg_arg_append(const struct option *opt, const char *arg, int unset)\n+{\n+\tstruct note_data *d = opt->value;\n+\tstruct strbuf append = STRBUF_INIT;\n+\n+\tBUG_ON_OPT_NEG(unset);\n+\n+\tstrbuf_addstr(&append, arg);\n+\tif (d->buf.len){\n+\t\tinsert_separator(&append);\n+\t}\n+\tstrbuf_addbuf(&d->buf, &append);\n+\tstrbuf_stripspace(&d->buf, 0);\n+\n+\td->given = 1;\n+\tstrbuf_release(&append);\n+\treturn 0;\n+}\n+\n static int parse_file_arg(const struct option *opt, const char *arg, int unset)\n {\n \tstruct note_data *d = opt->value;\n@@ -406,7 +445,7 @@ static int add(int argc, const char **argv, const char *prefix)\n \tstruct option options[] = {\n \t\tOPT_CALLBACK_F('m', \"message\", &d, N_(\"message\"),\n \t\t\tN_(\"note contents as a string\"), PARSE_OPT_NONEG,\n-\t\t\tparse_msg_arg),\n+\t\t\tparse_msg_arg_add),\n \t\tOPT_CALLBACK_F('F', \"file\", &d, N_(\"file\"),\n \t\t\tN_(\"note contents in a file\"), PARSE_OPT_NONEG,\n \t\t\tparse_file_arg),\n@@ -572,7 +611,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \tstruct option options[] = {\n \t\tOPT_CALLBACK_F('m', \"message\", &d, N_(\"message\"),\n \t\t\tN_(\"note contents as a string\"), PARSE_OPT_NONEG,\n-\t\t\tparse_msg_arg),\n+\t\t\tparse_msg_arg_append),\n \t\tOPT_CALLBACK_F('F', \"file\", &d, N_(\"file\"),\n \t\t\tN_(\"note contents in a file\"), PARSE_OPT_NONEG,\n \t\t\tparse_file_arg),\n@@ -584,6 +623,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\t\tparse_reuse_arg),\n \t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n \t\t\tN_(\"allow storing empty note\")),\n+\t\tOPT_STRING(0, \"separator\", &separator, N_(\"text\"),\n+\t\t\tN_(\"insert as separator before appending to an existing note\")),\n \t\tOPT_END()\n \t};\n \tint edit = !strcmp(argv[0], \"edit\");\n@@ -619,7 +660,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tchar *prev_buf = read_object_file(note, &type, &size);\n \n \t\tif (d.buf.len && prev_buf && size)\n-\t\t\tstrbuf_insertstr(&d.buf, 0, \"\\n\");\n+\t\t\tinsert_separator(&d.buf);\n \t\tif (prev_buf && size)\n \t\t\tstrbuf_insert(&d.buf, 0, prev_buf, size);\n \t\tfree(prev_buf);\ngit format-patch a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1 --thread -v 4 --output-directory=outgoing/git-notes-append/v4 --cover-letter --range-diff 196e80358ediff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\nindex e7807e052a..e8bc9934ed 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -521,6 +521,65 @@ test_expect_success 'listing non-existing notes fails' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'append: specify an empty separator' '\n+\ttest_when_finished git notes remove HEAD &&\n+\tcat >expect <<-\\EOF &&\n+\t\tnotes-1\n+\t\tnotes-2\n+\tEOF\n+\n+\tgit notes add -m \"notes-1\" &&\n+\tgit notes append --separator=\"\" -m \"notes-2\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+\n+'\n+\n+test_expect_success 'append: specify separatoro with line break' '\n+\ttest_when_finished git notes remove HEAD &&\n+\tcat >expect <<-\\EOF &&\n+\t\tnotes-1\n+\t\t-------\n+\t\tnotes-2\n+\tEOF\n+\n+\tgit notes add -m \"notes-1\" &&\n+\tseparator=$(printf \"%s\\n\" \"-------\") &&\n+\tgit notes append --separator=\"$separator\" -m \"notes-2\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'append: specify separator without line break' '\n+\ttest_when_finished git notes remove HEAD &&\n+\tcat >expect <<-\\EOF &&\n+\t\tnotes-1\n+\t\t-------\n+\t\tnotes-2\n+\tEOF\n+\n+\tgit notes add -m \"notes-1\" &&\n+\tgit notes append --separator=\"-------\" -m \"notes-2\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'append: specify separator with multiple messages' '\n+\ttest_when_finished git notes remove HEAD &&\n+\tcat >expect <<-\\EOF &&\n+\t\tnotes-1\n+\t\t-------\n+\t\tnotes-2\n+\t\t-------\n+\t\tnotes-3\n+\tEOF\n+\n+\tgit notes add -m \"notes-1\" &&\n+\tgit notes append --separator=\"-------\" -m \"notes-2\" -m \"notes-3\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'append to existing note with \"git notes append\"' '\n \tcat >expect <<-EOF &&\n \t\tInitial set of notes\n-- \n2.38.1.386.g6952793f2d9.dirty\n\n"},{"id":"470155","messageId":"230112.867cxs2i83.gmgdl@evledraar.gmail.com","threadId":"58620","inReplyTo":"29f7703b06dfecd1daedb72a7027aa953addfa3a.1673490953.git.dyroneteng@gmail.com","subject":"Re: [PATCH v4 2/5] notes.c: cleanup for \"designated init\" and \"char ptr init\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-12T09:51:02Z","receivedAt":"2023-01-12T09:54:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jan 12 2023, Teng Long wrote:\n\n> From: Teng Long \n>\n> Let's do some cleanup for the following two places in \"append_edit()\".\n>\n> The first place is \"char *logmsg;\" need to be initialized with NULL.\n> The second place is \"struct note_data d = { 0, 0, NULL, STRBUF_INIT };\"\n> could be replaced with designated init format.\n>\n> Signed-off-by: Teng Long \n> ---\n> builtin/notes.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index e57f024824..8ca55ec83e 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -566,9 +566,9 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> \tstruct notes_tree *t;\n> \tstruct object_id object, new_note;\n> \tconst struct object_id *note;\n> -\tchar *logmsg;\n> +\tchar *logmsg = NULL;\n\nThis change isn't needed, and the compiler will check that we have it\ninit'd.\n\nIt *is* needed when combined with your 3/5, but even then it's the wrong\nsolution. In that change you move the commit_notes() into that \"if\"\nbranch that needs the \"logmsg\", and it's correct that you need to\nNULL-init it to safely pass it to free().\n\nBut let's just declare the \"logmsg\" in that if block then, and move the\nfree() over there, that reduces the scope it's in.\n\n> \tconst char * const *usage;\n> -\tstruct note_data d = { 0, 0, NULL, STRBUF_INIT };\n> +\tstruct note_data d = { .buf = STRBUF_INIT };\n\nThis change is good, but then let's change the other such case in this\nfile to a designated init too, and make the commit message etc. be about\nusing designated init here.\n"},{"id":"470156","messageId":"230112.86358g2i6x.gmgdl@evledraar.gmail.com","threadId":"58620","inReplyTo":"d41ba140505aa3459330afd652ff6a0f456222a0.1673490953.git.dyroneteng@gmail.com","subject":"Re: [PATCH v4 4/5] notes.c: provide tips when target and append note are both empty","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-12T09:52:01Z","receivedAt":"2023-01-12T09:56:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jan 12 2023, Teng Long wrote:\n\n> From: Teng Long \n>\n> When \"git notes append \" is executed, if there is no note in\n> the given object and the appended note is empty too, we could print\n> the exact tips to end-user.\n>\n> Signed-off-by: Teng Long \n> ---\n> builtin/notes.c | 5 ++++-\n> t/t3301-notes.sh | 5 +++--\n> 2 files changed, 7 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index b71a81bff1..f2efb3736c 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -631,7 +631,10 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> \t\t\tBUG(\"combine_notes_overwrite failed\");\n> \t\tlogmsg = xstrfmt(\"Notes added by 'git notes %s'\", argv[0]);\n> \t\tcommit_notes(the_repository, t, logmsg);\n> -\t}\n> +\t} else if (!d.buf.len && !note)\n> +\t\tfprintf(stderr,\n> +\t\t\t_(\"Both original and appended notes are empty in %s, do nothing\\n\"),\n> +\t\t\toid_to_hex(&object));\n\nStyle: This arm should have {}-braces too, see the CodingGuidelines.\n"},{"id":"470158","messageId":"230112.86y1q812y4.gmgdl@evledraar.gmail.com","threadId":"58620","inReplyTo":"f7edbd0e508243ab55c13721a21b78bf50278a21.1673490953.git.dyroneteng@gmail.com","subject":"Re: [PATCH v4 5/5] notes.c: introduce \"--separator\" option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-12T09:53:04Z","receivedAt":"2023-01-12T10:11:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jan 12 2023, Teng Long wrote:\n\n> From: Teng Long \n>\n> When appending to a given notes object and the appended note is not\n> empty too, we will insert a blank line at first which separates the\n> existing note and the appended one, which as the separator.\n>\n> Sometimes, we want to use a specified as the separator. For\n> example, if we specify as:\n>\n> * --separator='------': we will insert \"------\\n\" as the separator,\n> because user do not provide the line break char at last, we will add\n> the trailing '\\n' compatibly.\n>\n> * --separator='------\\n': we will insert as-is because it contains\n> the line break at last.\n>\n> * --separator='': we specify an empty separator which means will\n> append the message directly without inserting any separator at\n> first.\n>\n> * not specified --separator option: will use '\\n' as the separator\n> when do appending and this is the default behavour.\n>\n> Signed-off-by: Teng Long \n> ---\n> Documentation/git-notes.txt | 18 +++++++++--\n> builtin/notes.c | 49 +++++++++++++++++++++++++++---\n> t/t3301-notes.sh | 59 +++++++++++++++++++++++++++++++++++++\n> 3 files changed, 120 insertions(+), 6 deletions(-)\n>\n> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\n> index efbc10f0f5..227fa88317 100644\n> --- a/Documentation/git-notes.txt\n> +++ b/Documentation/git-notes.txt\n> @@ -11,7 +11,7 @@ SYNOPSIS\n> 'git notes' [list []]\n> 'git notes' add [-f] [--allow-empty] [-F | -m | (-c | -C) ] []\n> 'git notes' copy [-f] ( --stdin | [] )\n> -'git notes' append [--allow-empty] [-F | -m | (-c | -C) ] []\n> +'git notes' append [--allow-empty] [--separator] [-F | -m | (-c | -C) ] []\n> 'git notes' edit [--allow-empty] []\n> 'git notes' show []\n> 'git notes' merge [-v | -q] [-s ] \n> @@ -86,7 +86,11 @@ the command can read the input given to the `post-rewrite` hook.)\n> \n> append::\n> \tAppend to the notes of an existing object (defaults to HEAD).\n> -\tCreates a new notes object if needed.\n> +\tCreates a new notes object if needed. If the note of the given\n> +\tobject and the note to be appended are not empty, a blank line\n> +\twill be inserted between them as the separator (\"blank line\" is\n> +\tthe default behavior, `--separator` option supports to specify\n> +\ta customized one).\n\nI think this should change to:\n\n\t[...]will be inserted between them. Use the `--separator` option\n\tto insert other delimiters.\n\nI.e. part of that's fixes for odd grammar, but mainly just offloading\nthe explanation to the --separator discussion below.\n\n\n> \n> edit::\n> \tEdit the notes for a given object (defaults to HEAD).\n> @@ -159,6 +163,16 @@ OPTIONS\n> \tAllow an empty note object to be stored. The default behavior is\n> \tto automatically remove empty notes.\n> \n> +--separator ::\n> +\tSpecify the to be inserted between existing note and appended\n> +\tmessage, the acts as a separator.\n\nMaybe let's use '' or '' here instead? e.g.:\n\n\tSpecifies the ...\n\nMaybe \"\" just looks odd to me.\n\nMore generally, let's say something like:\n\n\tWhen invoking \"git notes append\", specify the...\n\nI.e. this is only for \"append\", but nothing here says so.\n\n> +\tIf is empty (`--separator=''`), will append the message to\n> +\texisting note directly without insert any separator.\n> +\tIf is nonempty, will use as-is. One thing to notice is if\n> +\tthe lacks newline charactor, will add the newline automatically.\n> +\tIf not specify this option, a blank line will be inserted as the\n> +\tseparator.\n\n\nWe're spending a lot of text here on a pretty simple concept if I\nunderstand it correctly, I.e. just (pseudocode):\n\n\tint sep_extra_nl = 0;\n\tconst char *sep = opt_sep ? opt_sep : \"\\n\";\n\tif (!strstr(sep, '\\n'))\n\t\tsep_extra_nl = 1;\n\t[...]\n\nExcept that was written after I read your explanation, but looking at\nthe code it's incorrect, it's whether the \"*last*\" character contains a\nnewline or not.\n\nSo all in all, I think we should just say:\n\n\t--separator :\n\t\tThe '' inserted between the note and message\n\t\tby 'append', \"\\n\" by default. A custom separator can be\n\t\tprovided, if it doesn't end in a \"\\n\" one will be added\n\t\timplicitly .\n\n> +\n> --ref ::\n> \tManipulate the notes tree in . This overrides\n> \t`GIT_NOTES_REF` and the \"core.notesRef\" configuration. The ref\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index f2efb3736c..6746ad3232 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -24,6 +24,8 @@\n> #include \"notes-utils.h\"\n> #include \"worktree.h\"\n> \n> +static char *separator = \"\\n\";\n> +\n> static const char * const git_notes_usage[] = {\n> \tN_(\"git notes [--ref ] [list []]\"),\n> \tN_(\"git notes [--ref ] add [-f] [--allow-empty] [-m | -F | (-c | -C) ] []\"),\n> @@ -209,7 +211,7 @@ static void write_note_data(struct note_data *d, struct object_id *oid)\n> \t}\n> }\n> \n> -static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n> +static int parse_msg_arg_add(const struct option *opt, const char *arg, int unset)\n> {\n> \tstruct note_data *d = opt->value;\n> \n> @@ -225,6 +227,43 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n> \treturn 0;\n> }\n> \n> +static void insert_separator(struct strbuf *message)\n> +{\n> +\tconst char *insert;\n> +\n> +\tif (!separator)\n> +\t\tseparator = \"\\n\";\n> +\tif (*separator == '\\0')\n\nStyle: Don't compare to 0, NULL, '\\0' etc. Just use !*separator.\n\n> +\t\t/* separator is empty; use as-is (no blank line) */\n> +\t\treturn;\n> +\telse if (separator[strlen(separator) - 1] == '\\n')\n> +\t\t/* user supplied newline; use as-is */\n> +\t\tinsert = separator;\n> +\telse\n> +\t\t/* separator lacks newline; add it ourselves */\n> +\t\tinsert = xstrfmt(\"%s%s\", separator,\"\\n\");\n\nWe're leaking memor here, and making it hard to fix that by conflating a\nconst \"insert\" with this allocated version.\n\nI haven't read the whole context, but this seems really complex per the\ndoc feedback above. Why can't we just keep track of if we're using the\ndefault value or not? I.e. just have the \"--separator\" option default to\nNULL, if it's not set y ou don't need to do this \"\\n\" check, and just\nuse the default, otherwise append etc.\n\n> +\tstrbuf_insertstr(message, 0, insert);\n\nMaybe you were trying to get around using a more complex strbuf_splice()\nhere, but let's just avoid teh xstrfmt() and splice() that \"\\n\" in, if\nneeded?\n\n> +}\n> +\n> +static int parse_msg_arg_append(const struct option *opt, const char *arg, int unset)\n> +{\n> +\tstruct note_data *d = opt->value;\n> +\tstruct strbuf append = STRBUF_INIT;\n> +\n> +\tBUG_ON_OPT_NEG(unset);\n> +\n> +\tstrbuf_addstr(&append, arg);\n> +\tif (d->buf.len){\n> +\t\tinsert_separator(&append);\n> +\t}\n\nDrop the {} here.\n\n> +\tstrbuf_addbuf(&d->buf, &append);\n> +\tstrbuf_stripspace(&d->buf, 0);\n> +\n> +\td->given = 1;\n> +\tstrbuf_release(&append);\n\nWhy do we need this other variable, canet'w just append to d.buf\ndirectly?\n\nDo we mean to strbuf_stripspace() here over the whole buffer, or just\nwhat we're appending?\n"},{"id":"470377","messageId":"CAPig+cQKJxJCwk1GWtQ=LNNA=z9tQxYUwn9CMcXE4R9g8eKU7A@mail.gmail.com","threadId":"58620","inReplyTo":"f00a7596587bbf2d055dbf77a19506be1a6350fd.1673490953.git.dyroneteng@gmail.com","subject":"Re: [PATCH v4 1/5] notes.c: cleanup 'strbuf_grow' call in 'append_edit'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-01-15T04:53:56Z","receivedAt":"2023-01-15T05:07:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 11, 2023 at 9:48 PM Teng Long wrote:\n> Let's cleanup the unnecessary 'strbuf_grow' call in 'append_edit'. This\n> \"strbuf_grow(&d.buf, size + 1);\" is prepared for insert a blank line if\n> needed, but actually when inserting, \"strbuf_insertstr(&d.buf, 0,\n> \"\\n\");\" will do the \"grow\" for us.\n>\n> Signed-off-by: Teng Long \n> ---\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> @@ -618,7 +618,6 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> char *prev_buf = read_object_file(note, &type, &size);\n>\n> - strbuf_grow(&d.buf, size + 1);\n> if (d.buf.len && prev_buf && size)\n> strbuf_insertstr(&d.buf, 0, \"\\n\");\n\nIndeed, it's not clear why that was there in the first place. Digging\nthrough history doesn't shed any light on it. It was introduced by\n2347fae50b (builtin-notes: Add \"append\" subcommand for appending to\nnote objects, 2010-02-13)[1], but there's no explanation as to why it\nwas coded that way. Best guess may be that the author originally\ninserted \"\\n\" manually by direct manipulation of the strbuf rather\nthan employing a strbuf function, but then switched to strbuf_insert()\nbefore submitting the series and forgot to remove the now-unnecessary\nstrbuf_grow().\n\n[1]: https://lore.kernel.org/git/1266096518-2104-26-git-send-email-johan@herland.net/\n"},{"id":"470395","messageId":"CAPig+cR5s3XzmY+L_jDW2g_PEgi5E791x0GuV+VPkxFA_6sB7A@mail.gmail.com","threadId":"58620","inReplyTo":"7b756b4c605e148f6938fee74882091661382173.1673490953.git.dyroneteng@gmail.com","subject":"Re: [PATCH v4 3/5] notes.c: drop unreachable code in 'append_edit()'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-01-15T20:59:20Z","receivedAt":"2023-01-15T20:59:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 11, 2023 at 9:48 PM Teng Long wrote:\n> Situation of note \"removing\" shouldn't happen in 'append_edit()',\n> unless it's a bug. So, let's drop the unreachable \"else\" code\n> in \"append_edit()\".\n>\n> The notes operation \"append\" is different with \"add\", the latter\n> supports to overwrite the existing note then let the \"removing\"\n> happen (e.g. execute `git notes add -f -F /dev/null` on an existing\n> note), but the former will not because it only does \"appends\" but\n> not doing \"overwrites\".\n>\n> Signed-off-by: Teng Long \n> ---\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> @@ -630,13 +630,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> if (add_note(t, &object, &new_note, combine_notes_overwrite))\n> BUG(\"combine_notes_overwrite failed\");\n> logmsg = xstrfmt(\"Notes added by 'git notes %s'\", argv[0]);\n> - } else {\n> - fprintf(stderr, _(\"Removing note for object %s\\n\"),\n> - oid_to_hex(&object));\n> - remove_note(t, object.hash);\n> - logmsg = xstrfmt(\"Notes removed by 'git notes %s'\", argv[0]);\n> + commit_notes(the_repository, t, logmsg);\n> }\n> - commit_notes(the_repository, t, logmsg);\n\nThis change breaks removal of notes using \"git notes edit\". Prior to\nthis change, if you delete the content of a note using \"git notes\nedit\", then the note is removed. Following this change, the note\nremains, which contradicts documented[1] behavior.\n\n[1]: Unfortunately, perhaps, this behavior is documented under the\n\"remove\" subcommand rather than the \"edit\" subcommand, but it is\nnevertheless documented and has worked this way for ages, so this\npatch causes a regression.\n"},{"id":"470396","messageId":"CAPig+cTZ4af4YyuyogdKqMwGxyNanKejLX4Fw8Nar83gQ==WuA@mail.gmail.com","threadId":"58620","inReplyTo":"CAPig+cR5s3XzmY+L_jDW2g_PEgi5E791x0GuV+VPkxFA_6sB7A@mail.gmail.com","subject":"Re: [PATCH v4 3/5] notes.c: drop unreachable code in 'append_edit()'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-01-15T21:10:50Z","receivedAt":"2023-01-15T21:11:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Jan 15, 2023 at 3:59 PM Eric Sunshine wrote:\n> On Wed, Jan 11, 2023 at 9:48 PM Teng Long wrote:\n> > - } else {\n> > - fprintf(stderr, _(\"Removing note for object %s\\n\"),\n> > - oid_to_hex(&object));\n> > - remove_note(t, object.hash);\n> > - logmsg = xstrfmt(\"Notes removed by 'git notes %s'\", argv[0]);\n> > + commit_notes(the_repository, t, logmsg);\n> > }\n> > - commit_notes(the_repository, t, logmsg);\n>\n> This change breaks removal of notes using \"git notes edit\". Prior to\n> this change, if you delete the content of a note using \"git notes\n> edit\", then the note is removed. Following this change, the note\n> remains, which contradicts documented[1] behavior.\n\nAside from suggesting that this patch ought to be dropped, implicit in\nthe above comment is that test coverage is not as thorough as it\nshould be, otherwise this regression would have been caught early.\nThis suggests that the test coverage, at least for \"git notes edit\",\nneeds some beefing up, though doing so is probably outside the scope\nof this series; it's something which could/should be done separately,\nand it doesn't necessarily need to be done by you.\n"},{"id":"470398","messageId":"CAPig+cTtWStcDipNPHWvCVeuvtUdJtvotVH4ghtiND7-mpseeQ@mail.gmail.com","threadId":"58620","inReplyTo":"d41ba140505aa3459330afd652ff6a0f456222a0.1673490953.git.dyroneteng@gmail.com","subject":"Re: [PATCH v4 4/5] notes.c: provide tips when target and append note are both empty","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-01-15T21:28:48Z","receivedAt":"2023-01-15T21:29:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 11, 2023 at 9:48 PM Teng Long wrote:\n> When \"git notes append \" is executed, if there is no note in\n> the given object and the appended note is empty too, we could print\n> the exact tips to end-user.\n>\n> Signed-off-by: Teng Long \n> ---\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> @@ -631,7 +631,10 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> + } else if (!d.buf.len && !note)\n> + fprintf(stderr,\n> + _(\"Both original and appended notes are empty in %s, do nothing\\n\"),\n> + oid_to_hex(&object));\n\nMy knee-jerk reaction is between \"meh\" and \"thumbs down\". The commit\nmessage says we \"can do this\" but doesn't explain \"why we should do\nthis\". Is this condition important enough to break the Unix maxim of\n\"Rule of Silence\" (or \"Silence is Golden\")?\n\nI also wonder if this change is going to cause problems (or at least\nannoyance) for automated tooling. At present, tooling doesn't have to\nworry whether or not the existing or new note is empty; everything\njust works as expected without complaint. However, following this\nchange, such tooling may now be greeted with an unexpected diagnostic\nmessage.\n"},{"id":"470403","messageId":"CAPig+cSF7Fp3oM4TRU1QbiSzTeKNd1qGtqU7goPc1r-p4g8mkg@mail.gmail.com","threadId":"58620","inReplyTo":"230112.86y1q812y4.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v4 5/5] notes.c: introduce \"--separator\" option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-01-15T22:04:32Z","receivedAt":"2023-01-15T22:04:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jan 12, 2023 at 5:07 AM Ævar Arnfjörð Bjarmason\n wrote:\n> On Thu, Jan 12 2023, Teng Long wrote:\n> > When appending to a given notes object and the appended note is not\n> > empty too, we will insert a blank line at first which separates the\n> > existing note and the appended one, which as the separator.\n> > [...]\n> > Signed-off-by: Teng Long \n> > ---\n> > diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\n> > @@ -159,6 +163,16 @@ OPTIONS\n> > +--separator ::\n> > + Specify the to be inserted between existing note and appended\n> > + message, the acts as a separator.\n>\n> Maybe let's use '' or '' here instead? e.g.:\n> Specifies the ...\n> Maybe \"\" just looks odd to me.\n>\n> More generally, let's say something like:\n> When invoking \"git notes append\", specify the...\n> I.e. this is only for \"append\", but nothing here says so.\n\nAgreed on these points.\n\n> > + If is empty (`--separator=''`), will append the message to\n> > + existing note directly without insert any separator.\n> > + If is nonempty, will use as-is. One thing to notice is if\n> > + the lacks newline charactor, will add the newline automatically.\n> > + If not specify this option, a blank line will be inserted as the\n> > + separator.\n>\n> We're spending a lot of text here on a pretty simple concept if I\n> understand it correctly, I.e. just (pseudocode):\n>\n> int sep_extra_nl = 0;\n> const char *sep = opt_sep ? opt_sep : \"\\n\";\n> if (!strstr(sep, '\\n'))\n> sep_extra_nl = 1;\n> [...]\n>\n> Except that was written after I read your explanation, but looking at\n> the code it's incorrect, it's whether the \"*last*\" character contains a\n> newline or not.\n>\n> So all in all, I think we should just say:\n>\n> --separator :\n> The '' inserted between the note and message\n> by 'append', \"\\n\" by default. A custom separator can be\n> provided, if it doesn't end in a \"\\n\" one will be added\n> implicitly .\n\nUnfortunately, this misses the point. The original reason Teng Long\nstarted on this patch series was to be able to _suppress_ the blank\nline added unconditionally between notes. In the original submission,\nthis was done via a --no-blankline option, but that met with\nresistance from some reviewers as being potentially confusing and too\nspecialized. (The commit message of this patch should probably do a\nbetter job of explaining that one purpose of this change is to support\nthe case of no-separator.)\n\nA generalized --separator= option was suggested[1] as a possibly more\npalatable alternative, with which an empty string (meaning \"no\nseparator\") would cover the case for which the original --no-blankline\nwas meant to handle. So, at the very least, the documentation needs to\ncall out the empty string as being a special case for which automatic\nappending of \"\\n\" does not occur.\n\n> > diff --git a/builtin/notes.c b/builtin/notes.c\n> > +static void insert_separator(struct strbuf *message)\n> > +{\n> > + const char *insert;\n> > +\n> > + if (!separator)\n> > + separator = \"\\n\";\n> > + if (*separator == '\\0')\n>\n> Style: Don't compare to 0, NULL, '\\0' etc. Just use !*separator.\n\nMy fault[2]. Your suggestion is indeed more appropriate in this codebase.\n\n> > + /* separator is empty; use as-is (no blank line) */\n> > + return;\n> > + else if (separator[strlen(separator) - 1] == '\\n')\n> > + /* user supplied newline; use as-is */\n> > + insert = separator;\n> > + else\n> > + /* separator lacks newline; add it ourselves */\n> > + insert = xstrfmt(\"%s%s\", separator,\"\\n\");\n>\n> We're leaking memor here, and making it hard to fix that by conflating a\n> const \"insert\" with this allocated version.\n>\n> I haven't read the whole context, but this seems really complex per the\n> doc feedback above. Why can't we just keep track of if we're using the\n> default value or not? I.e. just have the \"--separator\" option default to\n> NULL, if it's not set y ou don't need to do this \"\\n\" check, and just\n> use the default, otherwise append etc.\n\nThat wouldn't work for the reason given above. The idea outlined in\n[2] is that an empty separator is treated specially as meaning\n\"nothing-between-notes, not even a blank line\".\n\n> > + strbuf_insertstr(message, 0, insert);\n>\n> Maybe you were trying to get around using a more complex strbuf_splice()\n> here, but let's just avoid teh xstrfmt() and splice() that \"\\n\" in, if\n> needed?\n\nThe code example I gave in [2] was meant to illustrate the suggested\nbehavior as clearly as possible, not necessarily to be copied\nverbatim. Being able to do this without leaking memory should\ncertainly be possible.\n\n> Do we mean to strbuf_stripspace() here over the whole buffer, or just\n> what we're appending?\n\nThat's a very good question. The note being appended might indeed have\nleading whitespace gunk which ought to be removed before the append\noperation. (Plus, it's a reasonable assumption that the existing note\ntext has already been \"stripspaced\".)\n\n[1]: https://lore.kernel.org/git/CAPig+cRcezSp4Rqt1Y9bD-FT6+7b0g9qHfbGRx65AOnw2FQXKg@mail.gmail.com/\n[2]: https://lore.kernel.org/git/CAPig+cTFBVAL2gd3LqQEzS--cXqJXR+1OVerii-D6JqFvJwXqQ@mail.gmail.com/\n"},{"id":"470404","messageId":"CAPig+cSt+vkjYi7L0pnE9g8WrqotT3djefaGQiCLqph6PAr=Lg@mail.gmail.com","threadId":"58620","inReplyTo":"f7edbd0e508243ab55c13721a21b78bf50278a21.1673490953.git.dyroneteng@gmail.com","subject":"Re: [PATCH v4 5/5] notes.c: introduce \"--separator\" option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-01-15T22:15:32Z","receivedAt":"2023-01-15T22:15:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 11, 2023 at 9:48 PM Teng Long wrote:\n> When appending to a given notes object and the appended note is not\n> empty too, we will insert a blank line at first which separates the\n> existing note and the appended one, which as the separator.\n> [...]\n> Signed-off-by: Teng Long \n> ---\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> @@ -24,6 +24,8 @@\n> +static char *separator = \"\\n\";\n\nIf you are initializing `separator` to \"\\n\"...\n\n> @@ -225,6 +227,43 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n> +static void insert_separator(struct strbuf *message)\n> +{\n> + if (!separator)\n> + separator = \"\\n\";\n\n... then these lines are not needed (and are effectively dead-code).\n\n> + if (*separator == '\\0')\n> + /* separator is empty; use as-is (no blank line) */\n> + return;\n> + else if (separator[strlen(separator) - 1] == '\\n')\n> + /* user supplied newline; use as-is */\n> + insert = separator;\n> + else\n> + /* separator lacks newline; add it ourselves */\n> + insert = xstrfmt(\"%s%s\", separator,\"\\n\");\n> + strbuf_insertstr(message, 0, insert);\n> +}\n\n> git format-patch a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1 --thread -v 4 --output-directory=outgoing/git-notes-append/v4 --cover-letter --range-diff 196e80358ediff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\n\nThere's some weird malformation going on here... this should just be a\n\"diff --git ...\" line.\n\n> @@ -521,6 +521,65 @@ test_expect_success 'listing non-existing notes fails' '\n> +test_expect_success 'append: specify an empty separator' '\n> + test_when_finished git notes remove HEAD &&\n> + cat >expect <<-\\EOF &&\n> + notes-1\n> + notes-2\n> + EOF\n\nStyle nit: We don't normally give extra indentation to the body of the\nhere-doc. Instead:\n\n cat >expect <<-\\EOF &&\n notes-1\n notes-2\n EOF\n\n> + git notes add -m \"notes-1\" &&\n> + git notes append --separator=\"\" -m \"notes-2\" &&\n> + git notes show >actual &&\n> + test_cmp expect actual\n> +\n> +'\n\nStyle nit: drop the unnecessary blank line before the closing quote.\n\n> +test_expect_success 'append: specify separatoro with line break' '\n\ns/separatoro/separator/\n\n> + test_when_finished git notes remove HEAD &&\n> + cat >expect <<-\\EOF &&\n> + notes-1\n> + -------\n> + notes-2\n> + EOF\n> +\n> + git notes add -m \"notes-1\" &&\n> + separator=$(printf \"%s\\n\" \"-------\") &&\n> + git notes append --separator=\"$separator\" -m \"notes-2\" &&\n\nIt might be easier to drop the `separator` variable and write this simply as:\n\n git notes append --separator=\"-------$LF\" -m \"notes-2\" &&\n\nLF is defined in t/test-lib.sh as \"\\n\".\n\n> + git notes show >actual &&\n> + test_cmp expect actual\n> +'\n"},{"id":"471112","messageId":"20230128112238.56553-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"CAPig+cQKJxJCwk1GWtQ=LNNA=z9tQxYUwn9CMcXE4R9g8eKU7A@mail.gmail.com","subject":"Re: [PATCH v4 1/5] notes.c: cleanup 'strbuf_grow' call in 'append_edit'","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-01-28T11:22:38Z","receivedAt":"2023-01-28T11:22:51Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Eric Sunshine writes:\n\n> On Wed, Jan 11, 2023 at 9:48 PM Teng Long wrote:\n> > Let's cleanup the unnecessary 'strbuf_grow' call in 'append_edit'. This\n> > \"strbuf_grow(&d.buf, size + 1);\" is prepared for insert a blank line if\n> > needed, but actually when inserting, \"strbuf_insertstr(&d.buf, 0,\n> > \"\\n\");\" will do the \"grow\" for us.\n> >\n> > Signed-off-by: Teng Long \n> > ---\n> > diff --git a/builtin/notes.c b/builtin/notes.c\n> > @@ -618,7 +618,6 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> > char *prev_buf = read_object_file(note, &type, &size);\n> >\n> > - strbuf_grow(&d.buf, size + 1);\n> > if (d.buf.len && prev_buf && size)\n> > strbuf_insertstr(&d.buf, 0, \"\\n\");\n>\n> Indeed, it's not clear why that was there in the first place. Digging\n> through history doesn't shed any light on it. It was introduced by\n> 2347fae50b (builtin-notes: Add \"append\" subcommand for appending to\n> note objects, 2010-02-13)[1], but there's no explanation as to why it\n> was coded that way. Best guess may be that the author originally\n> inserted \"\\n\" manually by direct manipulation of the strbuf rather\n> than employing a strbuf function, but then switched to strbuf_insert()\n> before submitting the series and forgot to remove the now-unnecessary\n> strbuf_grow().\n\nYes, I have the same opinion with you, maybe original idea to savesome array\ncreation overhead? I'm not sure, but I think the deletion here is OK.\n\nThanks.\n"},{"id":"471113","messageId":"20230128113327.56797-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"230112.867cxs2i83.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v4 2/5] notes.c: cleanup for \"designated init\" and \"char ptr init\"","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-01-28T11:33:27Z","receivedAt":"2023-01-28T11:33:37Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> > From: Teng Long \n> >\n> > Let's do some cleanup for the following two places in \"append_edit()\".\n> >\n> > The first place is \"char *logmsg;\" need to be initialized with NULL.\n> > The second place is \"struct note_data d = { 0, 0, NULL, STRBUF_INIT };\"\n> > could be replaced with designated init format.\n> >\n> > Signed-off-by: Teng Long \n> > ---\n> > builtin/notes.c | 4 ++--\n> > 1 file changed, 2 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/builtin/notes.c b/builtin/notes.c\n> > index e57f024824..8ca55ec83e 100644\n> > --- a/builtin/notes.c\n> > +++ b/builtin/notes.c\n> > @@ -566,9 +566,9 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> > \tstruct notes_tree *t;\n> > \tstruct object_id object, new_note;\n> > \tconst struct object_id *note;\n> > -\tchar *logmsg;\n> > +\tchar *logmsg = NULL;\n>\n> This change isn't needed, and the compiler will check that we have it\n> init'd.\n>\n> It *is* needed when combined with your 3/5, but even then it's the wrong\n> solution. In that change you move the commit_notes() into that \"if\"\n> branch that needs the \"logmsg\", and it's correct that you need to\n> NULL-init it to safely pass it to free().\n\nYou are correct, will fix.\n\n> But let's just declare the \"logmsg\" in that if block then, and move the\n> free() over there, that reduces the scope it's in.\n>\n> > \tconst char * const *usage;\n> > -\tstruct note_data d = { 0, 0, NULL, STRBUF_INIT };\n> > +\tstruct note_data d = { .buf = STRBUF_INIT };\n>\n> This change is good, but then let's change the other such case in this\n> file to a designated init too, and make the commit message etc. be about\n> using designated init here.\n\nOK.\n\nThanks.\n"},{"id":"471114","messageId":"20230128115027.57250-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"CAPig+cR5s3XzmY+L_jDW2g_PEgi5E791x0GuV+VPkxFA_6sB7A@mail.gmail.com","subject":"Re: [PATCH v4 3/5] notes.c: drop unreachable code in 'append_edit()'","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-01-28T11:50:27Z","receivedAt":"2023-01-28T11:50:37Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Eric Sunshine writes:\n\n> > Situation of note \"removing\" shouldn't happen in 'append_edit()',\n> > unless it's a bug. So, let's drop the unreachable \"else\" code\n> > in \"append_edit()\".\n> >\n> > The notes operation \"append\" is different with \"add\", the latter\n> > supports to overwrite the existing note then let the \"removing\"\n> > happen (e.g. execute `git notes add -f -F /dev/null` on an existing\n> > note), but the former will not because it only does \"appends\" but\n> > not doing \"overwrites\".\n> >\n> > Signed-off-by: Teng Long \n> > ---\n> > diff --git a/builtin/notes.c b/builtin/notes.c\n> > @@ -630,13 +630,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> > if (add_note(t, &object, &new_note, combine_notes_overwrite))\n> > BUG(\"combine_notes_overwrite failed\");\n> > logmsg = xstrfmt(\"Notes added by 'git notes %s'\", argv[0]);\n> > - } else {\n> > - fprintf(stderr, _(\"Removing note for object %s\\n\"),\n> > - oid_to_hex(&object));\n> > - remove_note(t, object.hash);\n> > - logmsg = xstrfmt(\"Notes removed by 'git notes %s'\", argv[0]);\n> > + commit_notes(the_repository, t, logmsg);\n> > }\n> > - commit_notes(the_repository, t, logmsg);\n>\n> This change breaks removal of notes using \"git notes edit\". Prior to\n> this change, if you delete the content of a note using \"git notes\n> edit\", then the note is removed. Following this change, the note\n> remains, which contradicts documented[1] behavior.\n>\n> [1]: Unfortunately, perhaps, this behavior is documented under the\n> \"remove\" subcommand rather than the \"edit\" subcommand, but it is\n> nevertheless documented and has worked this way for ages, so this\n> patch causes a regression.\n\nAs the commit msg describes, the subcommands I understand should have clear\nresponsibilities as possible (documentaion may have some effect in my mind). So,\nthe removal opertion under \"append subcommand\" here is little wired to me, but\nyour suggestion makes sense, this may have compatibility issues. Although I\nthink it's weird that someone would use this in the presence of the remove\nsubcommand, and my feeling is that this code is actually copied from the add\nmethod (introduced by 52694cdabbf68f19c8289416e7bb3bbef41d8d27), but I'm not\nsure.\n\nSo, it's ok for me to drop this one.\n\nThanks.\n"},{"id":"471131","messageId":"CAPig+cTVMGcRQqntPuwtV9x=WxUh-37Uy5V95A3F4ekeQgFq2g@mail.gmail.com","threadId":"58620","inReplyTo":"20230128115027.57250-1-tenglong.tl@alibaba-inc.com","subject":"Re: [PATCH v4 3/5] notes.c: drop unreachable code in 'append_edit()'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-01-30T05:38:02Z","receivedAt":"2023-01-30T05:38:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Jan 28, 2023 at 6:50 AM Teng Long wrote:\n> Eric Sunshine writes:\n> > > Situation of note \"removing\" shouldn't happen in 'append_edit()',\n> > > unless it's a bug. So, let's drop the unreachable \"else\" code\n> > > in \"append_edit()\".\n> > >\n> > > Signed-off-by: Teng Long \n> > > ---\n> > > - } else {\n> > > - fprintf(stderr, _(\"Removing note for object %s\\n\"),\n> > > - oid_to_hex(&object));\n> > > - remove_note(t, object.hash);\n> > > - logmsg = xstrfmt(\"Notes removed by 'git notes %s'\", argv[0]);\n> > > + commit_notes(the_repository, t, logmsg);\n> > > }\n> > > - commit_notes(the_repository, t, logmsg);\n> >\n> > This change breaks removal of notes using \"git notes edit\". Prior to\n> > this change, if you delete the content of a note using \"git notes\n> > edit\", then the note is removed. Following this change, the note\n> > remains, which contradicts documented[1] behavior.\n>\n> As the commit msg describes, the subcommands I understand should have clear\n> responsibilities as possible (documentaion may have some effect in my mind). So,\n> the removal opertion under \"append subcommand\" here is little wired to me, but\n> your suggestion makes sense, this may have compatibility issues. Although I\n> think it's weird that someone would use this in the presence of the remove\n> subcommand, and my feeling is that this code is actually copied from the add\n> method (introduced by 52694cdabbf68f19c8289416e7bb3bbef41d8d27), but I'm not\n> sure.\n>\n> So, it's ok for me to drop this one.\n\nIt's unfortunate, perhaps, that the git-notes command-set is not\northogonal, but it's another case of historic accretion. According to\nthe history, as originally implemented, git-notes only supported two\ncommands, \"edit\" and \"show\", and \"edit\" was responsible for _all_\nmutation operations, including deletion. git-notes only grew a more\ncomplete set of commands a couple years later.\n\nAt any rate, even though the original behaviors may not make as much\nsense these days now that git-notes has a more complete set of\ncommands, we still need to be careful not to break existing workflows\nand scripts (tooling). That's why it would be nice to beef up the\ntests so that the existing behaviors don't get broken by changes such\nas this patch wants to make. But, as mentioned earlier, the additional\ntests probably shouldn't be part of this series, but rather can be\ndone as a separate series (by someone; it doesn't have to be you).\n"},{"id":"471216","messageId":"20230201080854.28231-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"CAPig+cTVMGcRQqntPuwtV9x=WxUh-37Uy5V95A3F4ekeQgFq2g@mail.gmail.com","subject":"Re: [PATCH v4 3/5] notes.c: drop unreachable code in 'append_edit()'","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-02-01T08:08:54Z","receivedAt":"2023-02-01T08:09:04Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"On Mon, 30 Jan 2023 00:38:02 -0500 Eric Sunshine \nwrote:\n\n> It's unfortunate, perhaps, that the git-notes command-set is not\n> orthogonal, but it's another case of historic accretion. According to\n> the history, as originally implemented, git-notes only supported two\n> commands, \"edit\" and \"show\", and \"edit\" was responsible for _all_\n> mutation operations, including deletion. git-notes only grew a more\n> complete set of commands a couple years later.\n>\n> At any rate, even though the original behaviors may not make as much\n> sense these days now that git-notes has a more complete set of\n> commands, we still need to be careful not to break existing workflows\n> and scripts (tooling). That's why it would be nice to beef up the\n> tests so that the existing behaviors don't get broken by changes such\n> as this patch wants to make. But, as mentioned earlier, the additional\n> tests probably shouldn't be part of this series, but rather can be\n> done as a separate series (by someone; it doesn't have to be you).\n\nMake sense by your detailed explanation.\n\nThanks.\n"},{"id":"472196","messageId":"cover.1676551077.git.dyroneteng@gmail.com","threadId":"58620","inReplyTo":"cover.1673490953.git.dyroneteng@gmail.com","subject":"[PATCH v5 0/3] notes.c: introduce \"--separator\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-02-16T13:05:02Z","receivedAt":"2023-02-16T13:05:20Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Diff since v4:\n\n* Remove commits that may have compatibility issues\n* Fix the problems that mentioned in v4\n* Use OPT_STRING_LIST instead of OPT_CALLBACK_F to parse multiple \"-m\"\n\nThanks.\n\nTeng Long (3):\n notes.c: cleanup 'strbuf_grow' call in 'append_edit'\n notes.c: cleanup for \"designated init\"\n notes.c: introduce \"--separator\" option\n\n Documentation/git-notes.txt | 12 ++++++--\n builtin/notes.c | 37 ++++++++++++++++++-----\n t/t3301-notes.sh | 58 +++++++++++++++++++++++++++++++++++++\n 3 files changed, 97 insertions(+), 10 deletions(-)\n\nRange-diff against v4:\n1: f00a7596 ! 1: 9a450669 notes.c: cleanup 'strbuf_grow' call in 'append_edit'\n @@ Commit message\n needed, but actually when inserting, \"strbuf_insertstr(&d.buf, 0,\n \"\\n\");\" will do the \"grow\" for us.\n \n + Best guess may be that the author originally inserted \"\\n\" manually by\n + direct manipulation of the strbuf rather than employing a strbuf\n + function, but then switched to strbuf_insert() before submitting the\n + series and forgot to remove the now-unnecessary strbuf_grow().\n +\n Signed-off-by: Teng Long \n + Helped-by: Eric Sunshine \n \n ## builtin/notes.c ##\n +@@ builtin/notes.c: static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n + \n + \tBUG_ON_OPT_NEG(unset);\n + \n +-\tstrbuf_grow(&d->buf, strlen(arg) + 2);\n + \tif (d->buf.len)\n + \t\tstrbuf_addch(&d->buf, '\\n');\n + \tstrbuf_addstr(&d->buf, arg);\n @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tenum object_type type;\n \t\tchar *prev_buf = read_object_file(note, &type, &size);\n2: 29f7703b < -: -------- notes.c: cleanup for \"designated init\" and \"char ptr init\"\n3: 7b756b4c < -: -------- notes.c: drop unreachable code in 'append_edit()'\n4: d41ba140 < -: -------- notes.c: provide tips when target and append note are both empty\n5: f7edbd0e < -: -------- notes.c: introduce \"--separator\" option\n-: -------- > 2: e7bc6060 notes.c: cleanup for \"designated init\"\n-: -------- > 3: a74c96d6 notes.c: introduce \"--separator\" option\n-- \n2.39.2.459.g31d98a8e.dirty\n\n"},{"id":"472197","messageId":"9a4506692239db52c2632346c079d9ac82946c8d.1676551077.git.dyroneteng@gmail.com","threadId":"58620","inReplyTo":"cover.1676551077.git.dyroneteng@gmail.com","subject":"[PATCH v5 1/3] notes.c: cleanup 'strbuf_grow' call in 'append_edit'","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-02-16T13:05:03Z","receivedAt":"2023-02-16T13:05:22Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Let's cleanup the unnecessary 'strbuf_grow' call in 'append_edit'. This\n\"strbuf_grow(&d.buf, size + 1);\" is prepared for insert a blank line if\nneeded, but actually when inserting, \"strbuf_insertstr(&d.buf, 0,\n\"\\n\");\" will do the \"grow\" for us.\n\nBest guess may be that the author originally inserted \"\\n\" manually by\ndirect manipulation of the strbuf rather than employing a strbuf\nfunction, but then switched to strbuf_insert() before submitting the\nseries and forgot to remove the now-unnecessary strbuf_grow().\n\nSigned-off-by: Teng Long \nHelped-by: Eric Sunshine \n---\n builtin/notes.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 80d9dfd2..23cb6f0d 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -215,7 +215,6 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n \n \tBUG_ON_OPT_NEG(unset);\n \n-\tstrbuf_grow(&d->buf, strlen(arg) + 2);\n \tif (d->buf.len)\n \t\tstrbuf_addch(&d->buf, '\\n');\n \tstrbuf_addstr(&d->buf, arg);\n@@ -618,7 +617,6 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tenum object_type type;\n \t\tchar *prev_buf = read_object_file(note, &type, &size);\n \n-\t\tstrbuf_grow(&d.buf, size + 1);\n \t\tif (d.buf.len && prev_buf && size)\n \t\t\tstrbuf_insertstr(&d.buf, 0, \"\\n\");\n \t\tif (prev_buf && size)\n-- \n2.39.2.459.g31d98a8e.dirty\n\n"},{"id":"472198","messageId":"e7bc606005d8255f8951aa4f994f98b0c08d1d77.1676551077.git.dyroneteng@gmail.com","threadId":"58620","inReplyTo":"cover.1676551077.git.dyroneteng@gmail.com","subject":"[PATCH v5 2/3] notes.c: cleanup for \"designated init\"","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-02-16T13:05:04Z","receivedAt":"2023-02-16T13:05:30Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"The \"struct note_data d = { 0, 0, NULL, STRBUF_INIT };\" style could be\nreplaced with designated init format.\n---\n builtin/notes.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 23cb6f0d..553ae2bd 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -401,7 +401,7 @@ static int add(int argc, const char **argv, const char *prefix)\n \tstruct notes_tree *t;\n \tstruct object_id object, new_note;\n \tconst struct object_id *note;\n-\tstruct note_data d = { 0, 0, NULL, STRBUF_INIT };\n+\tstruct note_data d = { .buf = STRBUF_INIT };\n \tstruct option options[] = {\n \t\tOPT_CALLBACK_F('m', \"message\", &d, N_(\"message\"),\n \t\t\tN_(\"note contents as a string\"), PARSE_OPT_NONEG,\n@@ -567,7 +567,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \tconst struct object_id *note;\n \tchar *logmsg;\n \tconst char * const *usage;\n-\tstruct note_data d = { 0, 0, NULL, STRBUF_INIT };\n+\tstruct note_data d = { .buf = STRBUF_INIT };\n \tstruct option options[] = {\n \t\tOPT_CALLBACK_F('m', \"message\", &d, N_(\"message\"),\n \t\t\tN_(\"note contents as a string\"), PARSE_OPT_NONEG,\n-- \n2.39.2.459.g31d98a8e.dirty\n\n"},{"id":"472199","messageId":"a74c96d6dd23f2f1df6d3492093f3fd27451e24c.1676551077.git.dyroneteng@gmail.com","threadId":"58620","inReplyTo":"cover.1676551077.git.dyroneteng@gmail.com","subject":"[PATCH v5 3/3] notes.c: introduce \"--separator\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-02-16T13:05:05Z","receivedAt":"2023-02-16T13:05:33Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"When appending to a given notes object and the appended note is not\nempty too, we will insert a blank line at first which separates the\nexisting note and the appended one, which as the separator.\n\nSometimes, we want to use a specified as the separator. For\nexample, if we specify as:\n\n * --separator='------': we will insert \"------\\n\" as the separator,\n because user do not provide the line break char at last, we will add\n the trailing '\\n' compatibly.\n\n * --separator='------\\n': we will insert as-is because it contains\n the line break at last.\n\n * not specified --separator option: will use '\\n' as the separator\n when do appending and this is the default behavour.\n\n * --separator='': we specify an empty separator which has the same\n behavour with --separator='\\n' and or not specified the option.\n\nIn addition, if a user specifies multple \"-m\" with \"--separator\", the\nseparator should be inserted between the messages too, so we use\nOPT_STRING_LIST instead of OPT_CALLBACK_F to parse \"-m\" option, make\nsure the option value of \"--separator\" been parsed already when we need\nit.\n\nSigned-off-by: Teng Long \n---\n Documentation/git-notes.txt | 12 ++++++--\n builtin/notes.c | 31 +++++++++++++++++---\n t/t3301-notes.sh | 58 +++++++++++++++++++++++++++++++++++++\n 3 files changed, 95 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\nindex efbc10f0..5abe6092 100644\n--- a/Documentation/git-notes.txt\n+++ b/Documentation/git-notes.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n 'git notes' [list []]\n 'git notes' add [-f] [--allow-empty] [-F | -m | (-c | -C) ] []\n 'git notes' copy [-f] ( --stdin | [] )\n-'git notes' append [--allow-empty] [-F | -m | (-c | -C) ] []\n+'git notes' append [--allow-empty] [--separator] [-F | -m | (-c | -C) ] []\n 'git notes' edit [--allow-empty] []\n 'git notes' show []\n 'git notes' merge [-v | -q] [-s ] \n@@ -86,7 +86,9 @@ the command can read the input given to the `post-rewrite` hook.)\n \n append::\n \tAppend to the notes of an existing object (defaults to HEAD).\n-\tCreates a new notes object if needed.\n+\tCreates a new notes object if needed. If the note and the\n+\tmessage are not empty, \"\\n\" will be inserted between them.\n+\tUse the `--separator` option to insert other delimiters.\n \n edit::\n \tEdit the notes for a given object (defaults to HEAD).\n@@ -159,6 +161,12 @@ OPTIONS\n \tAllow an empty note object to be stored. The default behavior is\n \tto automatically remove empty notes.\n \n+--separator ::\n+\tThe '' inserted between the note and message\n+\tby 'append', \"\\n\" by default. A custom separator can be\n+\tprovided, if it doesn't end in a \"\\n\", one will be added\n+\timplicitly .\n+\n --ref ::\n \tManipulate the notes tree in . This overrides\n \t`GIT_NOTES_REF` and the \"core.notesRef\" configuration. The ref\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 553ae2bd..524976fe 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -24,6 +24,7 @@\n #include \"notes-utils.h\"\n #include \"worktree.h\"\n \n+static char *separator = NULL;\n static const char * const git_notes_usage[] = {\n \tN_(\"git notes [--ref ] [list []]\"),\n \tN_(\"git notes [--ref ] add [-f] [--allow-empty] [-m | -F | (-c | -C) ] []\"),\n@@ -209,6 +210,16 @@ static void write_note_data(struct note_data *d, struct object_id *oid)\n \t}\n }\n \n+static void insert_separator(struct strbuf *message, size_t pos)\n+{\n+\tif (!separator)\n+\t\tstrbuf_insertstr(message, pos, \"\\n\");\n+\telse if (separator[strlen(separator) - 1] == '\\n')\n+\t\tstrbuf_insertstr(message, pos, separator);\n+\telse\n+\t\tstrbuf_insertf(message, pos, \"%s%s\", separator, \"\\n\");\n+}\n+\n static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n {\n \tstruct note_data *d = opt->value;\n@@ -567,11 +578,12 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \tconst struct object_id *note;\n \tchar *logmsg;\n \tconst char * const *usage;\n+\tsize_t message_idx;\n \tstruct note_data d = { .buf = STRBUF_INIT };\n+\tstruct string_list message = STRING_LIST_INIT_DUP;\n \tstruct option options[] = {\n-\t\tOPT_CALLBACK_F('m', \"message\", &d, N_(\"message\"),\n-\t\t\tN_(\"note contents as a string\"), PARSE_OPT_NONEG,\n-\t\t\tparse_msg_arg),\n+\t\tOPT_STRING_LIST('m', \"message\", &message, N_(\"message\"),\n+\t\t\tN_(\"note contents as a string\")),\n \t\tOPT_CALLBACK_F('F', \"file\", &d, N_(\"file\"),\n \t\t\tN_(\"note contents in a file\"), PARSE_OPT_NONEG,\n \t\t\tparse_file_arg),\n@@ -583,6 +595,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\t\tparse_reuse_arg),\n \t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n \t\t\tN_(\"allow storing empty note\")),\n+\t\tOPT_STRING(0, \"separator\", &separator, N_(\"separator\"),\n+\t\t\tN_(\"insert as separator before appending a message\")),\n \t\tOPT_END()\n \t};\n \tint edit = !strcmp(argv[0], \"edit\");\n@@ -596,6 +610,15 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(usage, options);\n \t}\n \n+\tfor (message_idx = 0; message_idx < message.nr; message_idx++) {\n+\t\tif (d.buf.len)\n+\t\t\tinsert_separator(&d.buf, d.buf.len);\n+\t\tstrbuf_insertstr(&d.buf, d.buf.len,\n+\t\t\t\t message.items[message_idx].string);\n+\t\tstrbuf_stripspace(&d.buf, 0);\n+\t\td.given = 1;\n+\t}\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@@ -618,7 +641,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tchar *prev_buf = read_object_file(note, &type, &size);\n \n \t\tif (d.buf.len && prev_buf && size)\n-\t\t\tstrbuf_insertstr(&d.buf, 0, \"\\n\");\n+\t\t\tinsert_separator(&d.buf, 0);\n \t\tif (prev_buf && size)\n \t\t\tstrbuf_insert(&d.buf, 0, prev_buf, size);\n \t\tfree(prev_buf);\ndiff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\nindex 3288aaec..fe00497b 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -521,6 +521,64 @@ test_expect_success 'listing non-existing notes fails' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'append: specify an empty separator' '\n+\ttest_when_finished git notes remove HEAD &&\n+\tcat >expect <<-\\EOF &&\n+\tnotes-1\n+\n+\tnotes-2\n+\tEOF\n+\n+\tgit notes add -m \"notes-1\" &&\n+\tgit notes append --separator=\"\" -m \"notes-2\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'append: specify separatoro with line break' '\n+\ttest_when_finished git notes remove HEAD &&\n+\tcat >expect <<-\\EOF &&\n+\tnotes-1\n+\t-------\n+\tnotes-2\n+\tEOF\n+\n+\tgit notes add -m \"notes-1\" &&\n+\tgit notes append --separator=\"-------$LF\" -m \"notes-2\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'append: specify separator without line break' '\n+\ttest_when_finished git notes remove HEAD &&\n+\tcat >expect <<-\\EOF &&\n+\tnotes-1\n+\t-------\n+\tnotes-2\n+\tEOF\n+\n+\tgit notes add -m \"notes-1\" &&\n+\tgit notes append --separator=\"-------\" -m \"notes-2\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'append: specify separator with multiple messages' '\n+\ttest_when_finished git notes remove HEAD &&\n+\tcat >expect <<-\\EOF &&\n+\tnotes-1\n+\t-------\n+\tnotes-2\n+\t-------\n+\tnotes-3\n+\tEOF\n+\n+\tgit notes add -m \"notes-1\" &&\n+\tgit notes append --separator=\"-------\" -m \"notes-2\" -m \"notes-3\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'append to existing note with \"git notes append\"' '\n \tcat >expect <<-EOF &&\n \t\tInitial set of notes\n-- \n2.39.2.459.g31d98a8e.dirty\n\n"},{"id":"472215","messageId":"xmqqk00hpi9i.fsf@gitster.g","threadId":"58620","inReplyTo":"9a4506692239db52c2632346c079d9ac82946c8d.1676551077.git.dyroneteng@gmail.com","subject":"Re: [PATCH v5 1/3] notes.c: cleanup 'strbuf_grow' call in 'append_edit'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-16T18:39:05Z","receivedAt":"2023-02-16T18:39:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n> Let's cleanup the unnecessary 'strbuf_grow' call in 'append_edit'. This\n> \"strbuf_grow(&d.buf, size + 1);\" is prepared for insert a blank line if\n> needed, but actually when inserting, \"strbuf_insertstr(&d.buf, 0,\n> \"\\n\");\" will do the \"grow\" for us.\n\nCorrect. I think the code ends up trying to exactly size the strbuf\nas multiple \"-m message\" options are encountered on the command\nline, which is probably pointless (see below).\n\n> Best guess may be that the author originally inserted \"\\n\" manually by\n> direct manipulation of the strbuf rather than employing a strbuf\n> function, but then switched to strbuf_insert() before submitting the\n> series and forgot to remove the now-unnecessary strbuf_grow().\n\nPlease do not speculate when you do not have to. \"git blame\" is\nyour friend.\n\n348f199b (builtin-notes: Refactor handling of -F option to allow\ncombining -m and -F, 2010-02-13) added these to mimic the code\nintroduced by 2347fae5 (builtin-notes: Add \"append\" subcommand for\nappending to note objects, 2010-02-13) that reads in previous note\nbefore the message. And the resulting code with explicit sizing is\ncarried to this day.\n\nIn the context of reading an existing note in, exact sizing may have\nmade sense, but because the resulting note needs cleansing with\nstripspace() when appending with this option, such an exact sizing\ndoes not buy us all that much in practice.\n\nIt may help avoiding overallocation due to ALLOC_GROW() slop, but\nnobody can feed so many long messages for it to matter from the\ncommand line.\n\n> Signed-off-by: Teng Long \n> Helped-by: Eric Sunshine \n> ---\n> builtin/notes.c | 2 --\n> 1 file changed, 2 deletions(-)\n>\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index 80d9dfd2..23cb6f0d 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -215,7 +215,6 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n> \n> \tBUG_ON_OPT_NEG(unset);\n> \n> -\tstrbuf_grow(&d->buf, strlen(arg) + 2);\n> \tif (d->buf.len)\n> \t\tstrbuf_addch(&d->buf, '\\n');\n> \tstrbuf_addstr(&d->buf, arg);\n> @@ -618,7 +617,6 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> \t\tenum object_type type;\n> \t\tchar *prev_buf = read_object_file(note, &type, &size);\n> \n> -\t\tstrbuf_grow(&d.buf, size + 1);\n> \t\tif (d.buf.len && prev_buf && size)\n> \t\t\tstrbuf_insertstr(&d.buf, 0, \"\\n\");\n> \t\tif (prev_buf && size)\n"},{"id":"472216","messageId":"xmqqfsb5pi8t.fsf@gitster.g","threadId":"58620","inReplyTo":"e7bc606005d8255f8951aa4f994f98b0c08d1d77.1676551077.git.dyroneteng@gmail.com","subject":"Re: [PATCH v5 2/3] notes.c: cleanup for \"designated init\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-16T18:39:30Z","receivedAt":"2023-02-16T18:39:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n> The \"struct note_data d = { 0, 0, NULL, STRBUF_INIT };\" style could be\n> replaced with designated init format.\n> ---\n\nMissing sign-off.\n\nThe change itself with the explanation does make sense.\n\nThanks.\n\n> builtin/notes.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index 23cb6f0d..553ae2bd 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -401,7 +401,7 @@ static int add(int argc, const char **argv, const char *prefix)\n> \tstruct notes_tree *t;\n> \tstruct object_id object, new_note;\n> \tconst struct object_id *note;\n> -\tstruct note_data d = { 0, 0, NULL, STRBUF_INIT };\n> +\tstruct note_data d = { .buf = STRBUF_INIT };\n> \tstruct option options[] = {\n> \t\tOPT_CALLBACK_F('m', \"message\", &d, N_(\"message\"),\n> \t\t\tN_(\"note contents as a string\"), PARSE_OPT_NONEG,\n> @@ -567,7 +567,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> \tconst struct object_id *note;\n> \tchar *logmsg;\n> \tconst char * const *usage;\n> -\tstruct note_data d = { 0, 0, NULL, STRBUF_INIT };\n> +\tstruct note_data d = { .buf = STRBUF_INIT };\n> \tstruct option options[] = {\n> \t\tOPT_CALLBACK_F('m', \"message\", &d, N_(\"message\"),\n> \t\t\tN_(\"note contents as a string\"), PARSE_OPT_NONEG,\n"},{"id":"472237","messageId":"xmqq7cwhnql3.fsf@gitster.g","threadId":"58620","inReplyTo":"a74c96d6dd23f2f1df6d3492093f3fd27451e24c.1676551077.git.dyroneteng@gmail.com","subject":"Re: [PATCH v5 3/3] notes.c: introduce \"--separator\" option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-16T23:22:16Z","receivedAt":"2023-02-16T23:23:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n> When appending to a given notes object and the appended note is not\n> empty too, we will insert a blank line at first which separates the\n> existing note and the appended one, which as the separator.\n\n\", which as the separator\" does not sound grammatically correct.\nWithout that part, the above is a perfectly readable description of\nthe current behaviour.\n\n> Sometimes, we want to use a specified as the separator. For\n> example, if we specify as:\n>\n> * --separator='------': we will insert \"------\\n\" as the separator,\n> because user do not provide the line break char at last, we will add\n> the trailing '\\n' compatibly.\n\nIn a way compatible to what? I think s/compatibly// would be even\neasier to read. I think you are doing that for convenience.\n\n> * --separator='------\\n': we will insert as-is because it contains\n> the line break at last.\n\nIf this behaviour gets spelled out like this, it needs to be\njustified.\n\nAfter seeing that \"------\" gives the user these dashes on its own\nsingle line, wouldn't a natural expectation by the user be to see a\nline with dashes, followed by a blank line, if you give \"------\\n\"?\nHow do you justify removal of that newline in a way that is easy to\nunderstand to readers?\n\nI am not saying that you should allow --separator=\"---\\n\\n\\n\" to\ngive three blank lines between paragraphs. I think it makes sense to\nkeep the stripspace in the code after a paragraph gets added. I just\nprefer to see it done as our design choice, not \"because there is\nstripspace that removes them\", i.e. what the code happens to do. \n\n> * not specified --separator option: will use '\\n' as the separator\n> when do appending and this is the default behavour.\n\ns/not specified --separator option/no --separator option/; the way\nyou phrased can be misread for\n\n\tgit notes --separator -m foo -m bar\n\ni.e. any additional specifics is not given but --separator is still\non the command line. But I do not think you meant that---rather\nthis entry is what happens by default, i.e. a blank line separates\neach paragraph.\n\n> * --separator='': we specify an empty separator which has the same\n> behavour with --separator='\\n' and or not specified the option.\n\nI do not quite see why it is necessary to spell this out. Isn't\nthis a natural consequence of the first one (i.e. \"six dashes\nwithout any terminating LF gets a line with dashes plus LF\"\nnaturally extends to \"0 dashes without any terminating LF gets a\nblank line\")?\n\n> In addition, if a user specifies multple \"-m\" with \"--separator\", the\n> separator should be inserted between the messages too, so we use\n> OPT_STRING_LIST instead of OPT_CALLBACK_F to parse \"-m\" option, make\n> sure the option value of \"--separator\" been parsed already when we need\n> it.\n\nThis is hard to grok. Is it an instruction to whoever is\nimplementing this new feature, or is it an instruct to end-users\ntelling that they need to give --separator before they start giving\n-m , -F , -c , etc.?\n\n> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\n> index efbc10f0..5abe6092 100644\n> --- a/Documentation/git-notes.txt\n> +++ b/Documentation/git-notes.txt\n> @@ -11,7 +11,7 @@ SYNOPSIS\n> 'git notes' [list []]\n> 'git notes' add [-f] [--allow-empty] [-F | -m | (-c | -C) ] []\n\nDoesn't add allow you to say\n\n\t$ git notes add -m foo -m bar\n\nShouldn't it also honor --separator to specify an alternate\nparagraph break?\n\n> -'git notes' append [--allow-empty] [-F | -m | (-c | -C) ] []\n> +'git notes' append [--allow-empty] [--separator] [-F | -m | (-c | -C) ] []\n\n\"--separator\" -> \"--separator=\" or something?\n\n> @@ -86,7 +86,9 @@ the command can read the input given to the `post-rewrite` hook.)\n> \n> append::\n> \tAppend to the notes of an existing object (defaults to HEAD).\n> -\tCreates a new notes object if needed.\n> +\tCreates a new notes object if needed. If the note and the\n> +\tmessage are not empty, \"\\n\" will be inserted between them.\n> +\tUse the `--separator` option to insert other delimiters.\n\n\"\\n\" is so, ... programmer lingo? \"A blank line\" is inserted\nbetween these paragraphs.\n\n> +--separator ::\n> +\tThe '' inserted between the note and message\n> +\tby 'append', \"\\n\" by default. A custom separator can be\n\nI see no reason to single out 'append'; \"add -m -m \"\nfollows exactly the same paragraph concatenation logic in the\ncurrent code, no? If we allow customized paragraph separators,\nwe should use the same logic there as well.\n\nIt probably is simpler to explain if you treat the \"current note in\n'append'\" as if the text were just \"earlier paragraphs\", to which\nmore paragraphs taken from each -m and -F are\nconcatenated, with paragraph break before each of them. In the case\nof 'add', there happens to be zero \"earlier paragraphs\", but\neverything else gets concatenated the same way, no?\n\n> +\tprovided, if it doesn't end in a \"\\n\", one will be added\n> +\timplicitly .\n\nFunny punctuation.\n"},{"id":"472320","messageId":"20230220033416.3683-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"xmqqk00hpi9i.fsf@gitster.g","subject":"Re: [PATCH v5 1/3] notes.c: cleanup 'strbuf_grow' call in 'append_edit'","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-02-20T03:34:16Z","receivedAt":"2023-02-20T03:34:28Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Junio C Hamano wrote on Thu, 16 Feb 2023 10:39:05 -0800:\n\n> > Best guess may be that the author originally inserted \"\\n\" manually by\n> > direct manipulation of the strbuf rather than employing a strbuf\n> > function, but then switched to strbuf_insert() before submitting the\n> > series and forgot to remove the now-unnecessary strbuf_grow().\n>\n> Please do not speculate when you do not have to. \"git blame\" is\n> your friend.\n\nMy bad, I checked with \"git blame\" but did not describe in detail.\n\n> 348f199b (builtin-notes: Refactor handling of -F option to allow\n> combining -m and -F, 2010-02-13) added these to mimic the code\n> introduced by 2347fae5 (builtin-notes: Add \"append\" subcommand for\n> appending to note objects, 2010-02-13) that reads in previous note\n> before the message. And the resulting code with explicit sizing is\n> carried to this day.\n>\n> In the context of reading an existing note in, exact sizing may have\n> made sense, but because the resulting note needs cleansing with\n> stripspace() when appending with this option, such an exact sizing\n> does not buy us all that much in practice.\n>\n> It may help avoiding overallocation due to ALLOC_GROW() slop, but\n> nobody can feed so many long messages for it to matter from the\n> command line.\n\nWill apply by your content, thanks very much.\n"},{"id":"472334","messageId":"20230220140046.16986-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"xmqq7cwhnql3.fsf@gitster.g","subject":"Re: [PATCH v5 3/3] notes.c: introduce \"--separator\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-02-20T14:00:46Z","receivedAt":"2023-02-20T14:01:24Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Junio C Hamano wrote on Thu, 16 Feb 2023 15:22:16 -0800:\n\n> > When appending to a given notes object and the appended note is not\n> > empty too, we will insert a blank line at first which separates the\n> > existing note and the appended one, which as the separator.\n>\n> \", which as the separator\" does not sound grammatically correct.\n> Without that part, the above is a perfectly readable description of\n> the current behaviour.\n\nOK, I will remove \", which as the separator\".\n\n> > Sometimes, we want to use a specified as the separator. For\n> > example, if we specify as:\n> >\n> > * --separator='------': we will insert \"------\\n\" as the separator,\n> > because user do not provide the line break char at last, we will add\n> > the trailing '\\n' compatibly.\n>\n> In a way compatible to what? I think s/compatibly// would be even\n> easier to read. I think you are doing that for convenience.\n\nMy bad, I think I use a wrong word, maybe s/compatibly/automatically\nand I look back and think the representation of \"------\\n\" is not\ncorrect, because \"\\n\" is two characters here and will not treat\nas a newline. So, after modification, maybe like:\n\n * --separator='------': we will insert \"------\" and a trailing\n newline character, because if no newline specified at the end,\n one will be added automatically.\n\n> > * --separator='------\\n': we will insert as-is because it contains\n> > the line break at last.\n>\n> If this behaviour gets spelled out like this, it needs to be\n> justified.\n>\n> After seeing that \"------\" gives the user these dashes on its own\n> single line, wouldn't a natural expectation by the user be to see a\n> line with dashes, followed by a blank line, if you give \"------\\n\"?\n> How do you justify removal of that newline in a way that is easy to\n> understand to readers?\n>\n> I am not saying that you should allow --separator=\"---\\n\\n\\n\" to\n> give three blank lines between paragraphs. I think it makes sense to\n> keep the stripspace in the code after a paragraph gets added. I just\n> prefer to see it done as our design choice, not \"because there is\n> stripspace that removes them\", i.e. what the code happens to do.\n\nFirstly, like the problem I talked above, please let me figure out that\n\"------\\n\" is not represent as \"------\" and a blank line, but only\nas \"------\\n\" verbatim because \"\\n\" will be treated as two characters\nbut not a blank line while parsing. If the user wants to specify a\nblank line in the value of the option, they can pass \"CTRL+v CTRL+j\".\n\nThen, I think I did get some interference from old logic. Returning\nto the user scenario, I think about what the user needs and what is\nmy original idea is: consistent behavior\n\n * behavior 1: What the user enters is used as the delimiter itself\n without any special processing.\n\n * behavior 2: No matter what the user enters, we always add a\n newline at the end or other logic like stripspace, etc.\n\nI prefer the first one, sometimes users want to be next to the previous\nnote, then:\n\n git notes append -m foo -m bar --separator=\"\"\n\nand they can also choose to add as many line breaks as they want, then:\n\n export LF=\"\n \"\n git notes append -m foo -m bar --separator=\"$LF$LF$LF\"\n\nWe didn't help users make choices but I have to say it may be a little\ninconvenient to enter a newline character in the terminal, but I still\nprefer this way.\n\n> > * not specified --separator option: will use '\\n' as the separator\n> > when do appending and this is the default behavour.\n>\n> s/not specified --separator option/no --separator option/; the way\n> you phrased can be misread for\n>\nWill apply as:\n\n * no --separator option: will use '\\n' as the separator when\n do appending and this is the default behavour.\n\n> \tgit notes --separator -m foo -m bar\n>\n> i.e. any additional specifics is not given but --separator is still\n> on the command line. But I do not think you meant that---rather\n> this entry is what happens by default, i.e. a blank line separates\n> each paragraph.\n\nYes, only separate the messages(-m), but not each paragraph in it.\n\n> > * --separator='': we specify an empty separator which has the same\n> > behavour with --separator='\\n' and or not specified the option.\n>\n> I do not quite see why it is necessary to spell this out. Isn't\n> this a natural consequence of the first one (i.e. \"six dashes\n> without any terminating LF gets a line with dashes plus LF\"\n> naturally extends to \"0 dashes without any terminating LF gets a\n> blank line\")?\n\nAgree.. will remove.\n\n> > In addition, if a user specifies multple \"-m\" with \"--separator\", the\n> > separator should be inserted between the messages too, so we use\n> > OPT_STRING_LIST instead of OPT_CALLBACK_F to parse \"-m\" option, make\n> > sure the option value of \"--separator\" been parsed already when we need\n> > it.\n>\n> This is hard to grok. Is it an instruction to whoever is\n> implementing this new feature, or is it an instruct to end-users\n> telling that they need to give --separator before they start giving\n> -m , -F , -c , etc.?\n\nNo, it's not the order of the user give, but the backend we deal.\n\nWe use \"parse_msg_arg\" as a callback when parsing \"-m \" by OPT_CALLBACK_F,\nso if we have to read the separator before we parse it, so we could insert\nit correctly between the messages, So I use OPT_STRING_LIST instead.\n\n\n> > diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\n> > index efbc10f0..5abe6092 100644\n> > --- a/Documentation/git-notes.txt\n> > +++ b/Documentation/git-notes.txt\n> > @@ -11,7 +11,7 @@ SYNOPSIS\n> > 'git notes' [list []]\n> > 'git notes' add [-f] [--allow-empty] [-F | -m | (-c | -C) ] []\n>\n> Doesn't add allow you to say\n>\n> \t$ git notes add -m foo -m bar\n>\n> Shouldn't it also honor --separator to specify an alternate\n> paragraph break?\n\nAgree, you also mentioned me that, does git-notes-add need to be implies by\nthis option too? I think it needs too.\n\n> > @@ -86,7 +86,9 @@ the command can read the input given to the `post-rewrite` hook.)\n> >\n> > append::\n> > \tAppend to the notes of an existing object (defaults to HEAD).\n> > -\tCreates a new notes object if needed.\n> > +\tCreates a new notes object if needed. If the note and the\n> > +\tmessage are not empty, \"\\n\" will be inserted between them.\n> > +\tUse the `--separator` option to insert other delimiters.\n>\n> \"\\n\" is so, ... programmer lingo? \"A blank line\" is inserted\n> between these paragraphs.\n\nWill apply.\n\n> > +--separator ::\n> > +\tThe '' inserted between the note and message\n> > +\tby 'append', \"\\n\" by default. A custom separator can be\n>\n> I see no reason to single out 'append'; \"add -m -m \"\n> follows exactly the same paragraph concatenation logic in the\n> current code, no? If we allow customized paragraph separators,\n> we should use the same logic there as well.\n\nAgree, as I replied above, I think it will be done in next patch.\n\n> It probably is simpler to explain if you treat the \"current note in\n> 'append'\" as if the text were just \"earlier paragraphs\", to which\n> more paragraphs taken from each -m and -F are\n> concatenated, with paragraph break before each of them. In the case\n> of 'add', there happens to be zero \"earlier paragraphs\", but\n> everything else gets concatenated the same way, no?\n\nYes, they are the same way, you are right, so we should let add and append\nboth be implied by the option.\n\n> > +\tprovided, if it doesn't end in a \"\\n\", one will be added\n> > +\timplicitly .\n>\n> Funny punctuation.\n\nMy bad, will fix.\n\nThanks very much for your detailed review.\n"},{"id":"472404","messageId":"xmqq4jreemdn.fsf@gitster.g","threadId":"58620","inReplyTo":"20230220140046.16986-1-tenglong.tl@alibaba-inc.com","subject":"Re: [PATCH v5 3/3] notes.c: introduce \"--separator\" option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-21T21:31:32Z","receivedAt":"2023-02-21T21:31:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n>> > In addition, if a user specifies multple \"-m\" with \"--separator\", the\n>> > separator should be inserted between the messages too, so we use\n>> > OPT_STRING_LIST instead of OPT_CALLBACK_F to parse \"-m\" option, make\n>> > sure the option value of \"--separator\" been parsed already when we need\n>> > it.\n>>\n>> This is hard to grok. Is it an instruction to whoever is\n>> implementing this new feature, or is it an instruct to end-users\n>> telling that they need to give --separator before they start giving\n>> -m , -F , -c , etc.?\n>\n> No, it's not the order of the user give, but the backend we deal.\n>\n> We use \"parse_msg_arg\" as a callback when parsing \"-m \" by OPT_CALLBACK_F,\n> so if we have to read the separator before we parse it, so we could insert\n> it correctly between the messages, So I use OPT_STRING_LIST instead.\n\nThat is an implementation detail of how you chose to implement the\nfeature, and not an inherent limitation, is it? It makes a lame\nexcuse to give users a hard-to-use UI.\n\nFor example, we could parse all the command line parameters without\nmaking any action other than recording the strings given to -m and\ncontents of files given via -F in the order they appeared on the\ncommand line, all in a single string list, while remembering the\nlast value of --separator you got, and then at the end concatenate\nthese strings using the final value of the separator, no?\n"},{"id":"472437","messageId":"20230222081708.8010-1-tenglong.tl@alibaba-inc.com","threadId":"58620","inReplyTo":"xmqq4jreemdn.fsf@gitster.g","subject":"Re: [PATCH v5 3/3] notes.c: introduce \"--separator\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-02-22T08:17:08Z","receivedAt":"2023-02-22T08:17:25Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Junio C Hamano writes:\n\n> > We use \"parse_msg_arg\" as a callback when parsing \"-m \" by OPT_CALLBACK_F,\n> > so if we have to read the separator before we parse it, so we could insert\n> > it correctly between the messages, So I use OPT_STRING_LIST instead.\n>\n> That is an implementation detail of how you chose to implement the\n> feature, and not an inherent limitation, is it? It makes a lame\n> excuse to give users a hard-to-use UI.\n\n> For example, we could parse all the command line parameters without\n> making any action other than recording the strings given to -m and\n> contents of files given via -F in the order they appeared on the\n> command line, all in a single string list, while remembering the\n> last value of --separator you got, and then at the end concatenate\n> these strings using the final value of the separator, no?\n\nYes, please let'me clarify it, we shouldn't to give users a hard-to-use UI, so:\n\n 1. order of \"-m\" and \"-F\" matters, it determine the order of the paragraphs\n (remain the same as before, which I need to fix in next patch).\n\n 2. order of \"-m\"\"-F\" and \"--separator\" doesn't matter.\n\nThanks.\n"},{"id":"472464","messageId":"xmqqy1opb8bb.fsf@gitster.g","threadId":"58620","inReplyTo":"20230222081708.8010-1-tenglong.tl@alibaba-inc.com","subject":"Re: [PATCH v5 3/3] notes.c: introduce \"--separator\" option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-22T23:15:52Z","receivedAt":"2023-02-22T23:15:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n> Junio C Hamano writes:\n>\n>> > We use \"parse_msg_arg\" as a callback when parsing \"-m \" by OPT_CALLBACK_F,\n>> > so if we have to read the separator before we parse it, so we could insert\n>> > it correctly between the messages, So I use OPT_STRING_LIST instead.\n>>\n>> That is an implementation detail of how you chose to implement the\n>> feature, and not an inherent limitation, is it? It makes a lame\n>> excuse to give users a hard-to-use UI.\n>\n>> For example, we could parse all the command line parameters without\n>> making any action other than recording the strings given to -m and\n>> contents of files given via -F in the order they appeared on the\n>> command line, all in a single string list, while remembering the\n>> last value of --separator you got, and then at the end concatenate\n>> these strings using the final value of the separator, no?\n>\n> Yes, please let'me clarify it, we shouldn't to give users a hard-to-use UI, so:\n>\n> 1. order of \"-m\" and \"-F\" matters, it determine the order of the paragraphs\n> (remain the same as before, which I need to fix in next patch).\n>\n> 2. order of \"-m\"\"-F\" and \"--separator\" doesn't matter.\n>\n> Thanks.\n\nSounds good.\n"}]}