{"thread":{"id":"59295","subject":"[PATCH v6 2/3] notes.c: cleanup for \"designated init\"","startedAt":"2023-02-23T07:30:09Z","lastAt":"2023-04-28T18:21:54Z","messageCount":40,"participants":["Teng Long","Junio C Hamano"],"isPatch":true,"patchVersion":6,"patchTotal":3},"messages":[{"id":"472485","messageId":"043db6311b0af9311764d8e763e85867ed3381a6.1677136319.git.dyroneteng@gmail.com","threadId":"59295","inReplyTo":"cover.1677136319.git.dyroneteng@gmail.com","subject":"[PATCH v6 2/3] notes.c: cleanup for \"designated init\"","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-02-23T07:29:46Z","receivedAt":"2023-02-23T07:30:09Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nThe \"struct note_data d = { 0, 0, NULL, STRBUF_INIT };\" style could be\nreplaced 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 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.gd5a6c747\n\n"},{"id":"472486","messageId":"b029ee0b51151b054c6f2c76968f88fb2ceec1af.1677136319.git.dyroneteng@gmail.com","threadId":"59295","inReplyTo":"cover.1677136319.git.dyroneteng@gmail.com","subject":"[PATCH v6 1/3] notes.c: cleanup 'strbuf_grow' call in 'append_edit'","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-02-23T07:29:45Z","receivedAt":"2023-02-23T07:30:10Z","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\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\nSigned-off-by: Teng Long \nHelped-by: Eric Sunshine \nHelped-by: Junio C Hamano \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.gd5a6c747\n\n"},{"id":"472487","messageId":"cover.1677136319.git.dyroneteng@gmail.com","threadId":"59295","inReplyTo":"cover.1676551077.git.dyroneteng@gmail.com","subject":"[PATCH v6 0/3] notes.c: introduce \"--separator\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-02-23T07:29:44Z","receivedAt":"2023-02-23T07:30:10Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nDiff since v5:\n\n1. Optimized and fixed the commit-message problems.\n2. Optimized and fixed the related documentation.\n3. the order of \"-m\" and -F should matter, but not, it was newly\nintroduced problem in v4, now fixed in v5.\n\nBy the way, In order to add a separator when parsing '-F and\n'-m', I used 'string_list' to save all messages for subsequent\nprocessing instead of processing in callback, because the value\nof '--separator' has not been initialized and set at this time and\nI have not found a similar \"OPT_XXX\" API for doing this. I'm not\nsure if it's worth adding a similar API to parse the value of an\noption in advance and initialize it for this scenario. If so, it\nmay not be suitable to continue to incubate in the current patch\nseries, but I will try to contribute it.\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 | 20 ++++++--\n builtin/notes.c | 78 ++++++++++++++++++----------\n t/t3301-notes.sh | 100 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 168 insertions(+), 30 deletions(-)\n\nRange-diff against v5:\n1: 9a450669 ! 1: b029ee0b 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 + 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 \n Signed-off-by: Teng Long \n Helped-by: Eric Sunshine \n + Helped-by: Junio C Hamano \n \n ## builtin/notes.c ##\n @@ builtin/notes.c: static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n2: e7bc6060 ! 2: 043db631 notes.c: cleanup for \"designated init\"\n @@ Commit message\n The \"struct note_data d = { 0, 0, NULL, STRBUF_INIT };\" style could be\n replaced with designated init format.\n \n + Signed-off-by: Teng Long \n +\n ## builtin/notes.c ##\n @@ builtin/notes.c: static int add(int argc, const char **argv, const char *prefix)\n \tstruct notes_tree *t;\n3: a74c96d6 ! 3: d5a6c747 notes.c: introduce \"--separator\" option\n @@ Metadata\n Author: Teng Long \n \n ## Commit message ##\n - notes.c: introduce \"--separator\" option\n + notes.c: introduce '--separator=' option\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 + When adding new notes or appending to an existing notes, we will\n + insert a blank line between the paragraphs, like:\n \n - Sometimes, we want to use a specified as the separator. For\n - example, if we specify as:\n + $ git notes add -m foo -m bar\n + $ git notes show HEAD | cat\n + foo\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 + bar\n \n - * --separator='------\\n': we will insert as-is because it contains\n - the line break at last.\n + The default behavour sometimes is not enough, the user may want\n + to use a custom delimiter between paragraphs, like when\n + specifiy one or more '-m' or '-F' options. So this commit\n + introduces a new '--separator' option for 'git-notes-add' and\n + 'git-notes-append', for example when execute:\n \n - * not specified --separator option: will use '\\n' as the separator\n - when do appending and this is the default behavour.\n + $ git notes add -m foo -m bar --separator=\"-\"\n + $ git notes show HEAD | cat\n + foo\n + -\n + bar\n \n - * --separator='': we specify an empty separator which has the same\n - behavour with --separator='\\n' and or not specified the option.\n + We will check the option value and if the value doesn't contail\n + a trailing '\\n', will add it automatically, so execute\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 + $ git notes add -m foo -m bar --separator=\"-\"\n + and\n + $ export LF=\"\n + \"\n + $ git notes add -m foo -m bar --separator=\"-$LF\"\n +\n + have the same behavour.\n \n Signed-off-by: Teng Long \n \n ## Documentation/git-notes.txt ##\n @@ Documentation/git-notes.txt: SYNOPSIS\n + --------\n + [verse]\n 'git notes' [list []]\n - 'git notes' add [-f] [--allow-empty] [-F | -m | (-c | -C) ] []\n +-'git notes' add [-f] [--allow-empty] [-F | -m | (-c | -C) ] []\n ++'git notes' add [-f] [--allow-empty] [--separator=] [-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' 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 +@@ Documentation/git-notes.txt: add::\n + \tHowever, if you're using `add` interactively (using an editor\n + \tto supply the notes contents), then - instead of aborting -\n + \tthe existing notes will be opened in the editor (like the `edit`\n +-\tsubcommand).\n ++\tsubcommand). If you specify multiple `-m` and `-F`, a blank\n ++\tline will be inserted between the messages. Use the `--separator`\n ++\toption to insert other delimiters. \n + \n + copy::\n + \tCopy the notes for the first object onto the second object (defaults to\n @@ Documentation/git-notes.txt: 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 ++\tCreates a new notes object if needed. \n ++\tThe default delimiter is a blank line, use the `--separator`\n ++\toption to insert other delimiters. More specifically, if the\n ++\tnote and the message are not empty, the delimiter will be\n ++\tinserted between them. If you specify multiple `-m` and `-F`\n ++\toptions, the delimiter will be inserted between the messages\n ++\ttoo.\n \n edit::\n \tEdit the notes for a given object (defaults to HEAD).\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 -+--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 ++--separator ::\n ++\tThe '' inserted between paragraphs.\n ++\tA blank line by default.\n +\n --ref ::\n \tManipulate the notes tree in . This overrides\n @@ builtin/notes.c\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 +-\tN_(\"git notes [--ref ] add [-f] [--allow-empty] [-m | -F | (-c | -C) ] []\"),\n ++\tN_(\"git notes [--ref ] add [-f] [--allow-empty] [--separator=] [-m | -F | (-c | -C) ] []\"),\n + \tN_(\"git notes [--ref ] copy [-f] \"),\n +-\tN_(\"git notes [--ref ] append [--allow-empty] [-m | -F | (-c | -C) ] []\"),\n ++\tN_(\"git notes [--ref ] append [--allow-empty] [--separator=] [-m | -F | (-c | -C) ] []\"),\n + \tN_(\"git notes [--ref ] edit [--allow-empty] []\"),\n + \tN_(\"git notes [--ref ] show []\"),\n + \tN_(\"git notes [--ref ] merge [-v | -q] [-s ] \"),\n @@ builtin/notes.c: static void write_note_data(struct note_data *d, struct object_id *oid)\n \t}\n }\n @@ builtin/notes.c: static void write_note_data(struct note_data *d, struct object_\n +\telse\n +\t\tstrbuf_insertf(message, pos, \"%s%s\", separator, \"\\n\");\n +}\n ++\n ++static void parse_messages(struct string_list *messages, struct note_data *d)\n ++{\n ++\tsize_t i;\n ++\tfor (i = 0; i < messages->nr; i++) {\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 messages->items[i].string);\n ++\t\tstrbuf_stripspace(&d->buf, 0);\n ++\t\td->given = 1;\n ++\t}\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 -@@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char *prefix)\n +-\tstruct note_data *d = opt->value;\n ++\tstruct string_list *msg = opt->value;\n + \n + \tBUG_ON_OPT_NEG(unset);\n + \n +-\tif (d->buf.len)\n +-\t\tstrbuf_addch(&d->buf, '\\n');\n +-\tstrbuf_addstr(&d->buf, arg);\n +-\tstrbuf_stripspace(&d->buf, 0);\n +-\n +-\td->given = 1;\n ++\tstring_list_append(msg, arg);\n + \treturn 0;\n + }\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 ++\tstruct string_list *msg = opt->value;\n ++\tstruct strbuf buf = STRBUF_INIT;\n + \n + \tBUG_ON_OPT_NEG(unset);\n + \n +-\tif (d->buf.len)\n +-\t\tstrbuf_addch(&d->buf, '\\n');\n + \tif (!strcmp(arg, \"-\")) {\n +-\t\tif (strbuf_read(&d->buf, 0, 1024) < 0)\n ++\t\tif (strbuf_read(&buf, 0, 1024) < 0)\n + \t\t\tdie_errno(_(\"cannot read '%s'\"), arg);\n +-\t} else if (strbuf_read_file(&d->buf, arg, 1024) < 0)\n ++\t} else if (strbuf_read_file(&buf, arg, 1024) < 0)\n + \t\tdie_errno(_(\"could not open or read '%s'\"), arg);\n +-\tstrbuf_stripspace(&d->buf, 0);\n + \n +-\td->given = 1;\n ++\tstring_list_append(msg, buf.buf);\n ++\tstrbuf_release(&buf);\n + \treturn 0;\n + }\n + \n +@@ builtin/notes.c: static int add(int argc, const char **argv, const char *prefix)\n + \tstruct object_id object, new_note;\n \tconst struct object_id *note;\n + \tstruct note_data d = { .buf = STRBUF_INIT };\n ++\tstruct string_list messages = STRING_LIST_INIT_DUP;\n + \tstruct option options[] = {\n +-\t\tOPT_CALLBACK_F('m', \"message\", &d, N_(\"message\"),\n ++\t\tOPT_CALLBACK_F('m', \"message\", &messages, N_(\"message\"),\n + \t\t\tN_(\"note contents as a string\"), PARSE_OPT_NONEG,\n + \t\t\tparse_msg_arg),\n +-\t\tOPT_CALLBACK_F('F', \"file\", &d, N_(\"file\"),\n ++\t\tOPT_CALLBACK_F('F', \"file\", &messages, N_(\"file\"),\n + \t\t\tN_(\"note contents in a file\"), PARSE_OPT_NONEG,\n + \t\t\tparse_file_arg),\n + \t\tOPT_CALLBACK_F('c', \"reedit-message\", &d, N_(\"object\"),\n +@@ builtin/notes.c: static int add(int argc, const char **argv, const char *prefix)\n + \t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n + \t\t\tN_(\"allow storing empty note\")),\n + \t\tOPT__FORCE(&force, N_(\"replace existing notes\"), PARSE_OPT_NOCOMPLETE),\n ++\t\tOPT_STRING(0, \"separator\", &separator, N_(\"separator\"),\n ++\t\t\tN_(\"insert between paragraphs\")),\n + \t\tOPT_END()\n + \t};\n + \n +@@ builtin/notes.c: static int add(int argc, const char **argv, const char *prefix)\n + \t\tusage_with_options(git_notes_add_usage, options);\n + \t}\n + \n ++\tparse_messages(&messages, &d);\n + \tobject_ref = argc > 1 ? argv[1] : \"HEAD\";\n + \n + \tif (get_oid(object_ref, &object))\n +@@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char *prefix)\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 string_list messages = 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\tOPT_CALLBACK_F('m', \"message\", &messages, N_(\"message\"),\n ++\t\t\tN_(\"note contents as a string\"), PARSE_OPT_NONEG, \n + \t\t\tparse_msg_arg),\n +-\t\tOPT_CALLBACK_F('F', \"file\", &d, N_(\"file\"),\n ++\t\tOPT_CALLBACK_F('F', \"file\", &messages, N_(\"file\"),\n \t\t\tN_(\"note contents in a file\"), PARSE_OPT_NONEG,\n \t\t\tparse_file_arg),\n + \t\tOPT_CALLBACK_F('c', \"reedit-message\", &d, N_(\"object\"),\n @@ builtin/notes.c: 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\t\tN_(\"insert between paragraphs\")),\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 \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 ++\tparse_messages(&messages, &d);\n +\n \tif (d.given && edit)\n \t\tfprintf(stderr, _(\"The -m/-F/-c/-C options have been deprecated \"\n @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char\n \t\tfree(prev_buf);\n \n ## t/t3301-notes.sh ##\n +@@ t/t3301-notes.sh: test_expect_success 'do not create empty note with -m \"\"' '\n + '\n + \n + test_expect_success 'create note with combination of -m and -F' '\n ++\ttest_when_finished git notes remove HEAD &&\n + \tcat >expect-combine_m_and_F <<-EOF &&\n + \t\tfoo\n + \n +@@ t/t3301-notes.sh: test_expect_success 'create note with combination of -m and -F' '\n + \ttest_cmp expect-combine_m_and_F actual\n + '\n + \n ++test_expect_success 'create note with combination of -m and -F and --separator' '\n ++\tcat >expect-combine_m_and_F <<-\\EOF &&\n ++\tfoo\n ++\t-------\n ++\txyzzy\n ++\t-------\n ++\tbar\n ++\t-------\n ++\tzyxxy\n ++\t-------\n ++\tbaz\n ++\tEOF\n ++\techo \"xyzzy\" >note_a &&\n ++\techo \"zyxxy\" >note_b &&\n ++\tgit notes add -m \"foo\" -F note_a -m \"bar\" -F note_b -m \"baz\" --separator \"-------\" &&\n ++\tgit notes show >actual &&\n ++\ttest_cmp expect-combine_m_and_F actual\n ++\t\n ++'\n ++\n + test_expect_success 'remove note with \"git notes remove\"' '\n + \tgit notes remove HEAD^ &&\n + \tgit notes remove &&\n @@ t/t3301-notes.sh: test_expect_success 'listing non-existing notes fails' '\n \ttest_must_be_empty actual\n '\n @@ t/t3301-notes.sh: test_expect_success 'listing non-existing notes fails' '\n +\ttest_cmp expect actual\n +'\n +\n -+test_expect_success 'append: specify separatoro with line break' '\n ++test_expect_success 'append: specify separator with line break' '\n +\ttest_when_finished git notes remove HEAD &&\n +\tcat >expect <<-\\EOF &&\n +\tnotes-1\n @@ t/t3301-notes.sh: test_expect_success 'listing non-existing notes fails' '\n +\tgit notes show >actual &&\n +\ttest_cmp expect actual\n +'\n ++\n ++test_expect_success 'append note with combination of -m and -F and --separator' '\n ++\ttest_when_finished git notes remove HEAD &&\n ++\tcat >expect-combine_m_and_F <<-\\EOF &&\n ++\tm-notes-1\n ++\t-------\n ++\tf-notes-1\n ++\t-------\n ++\tm-notes-2\n ++\t-------\n ++\tf-notes-2\n ++\t-------\n ++\tm-notes-3\n ++\tEOF\n ++\n ++\techo \"f-notes-1\" >note_a &&\n ++\techo \"f-notes-2\" >note_b &&\n ++\tgit notes append -m \"m-notes-1\" -F note_a -m \"m-notes-2\" -F note_b -m \"m-notes-3\" --separator \"-------\" &&\n ++\tgit notes show >actual &&\n ++\ttest_cmp expect-combine_m_and_F actual\n ++'\n +\n test_expect_success 'append to existing note with \"git notes append\"' '\n \tcat >expect <<-EOF &&\n-- \n2.39.2.459.gd5a6c747\n\n"},{"id":"472488","messageId":"d5a6c74792c15e2f83c2ed0758fb99eac11a8174.1677136319.git.dyroneteng@gmail.com","threadId":"59295","inReplyTo":"cover.1677136319.git.dyroneteng@gmail.com","subject":"[PATCH v6 3/3] notes.c: introduce '--separator=' option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-02-23T07:29:47Z","receivedAt":"2023-02-23T07:30:10Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nWhen adding new notes or appending to an existing notes, we will\ninsert a blank line between the paragraphs, like:\n\n $ git notes add -m foo -m bar\n $ git notes show HEAD | cat\n foo\n\n bar\n\nThe default behavour sometimes is not enough, the user may want\nto use a custom delimiter between paragraphs, like when\nspecifiy one or more '-m' or '-F' options. So this commit\nintroduces a new '--separator' option for 'git-notes-add' and\n'git-notes-append', for example when execute:\n\n $ git notes add -m foo -m bar --separator=\"-\"\n $ git notes show HEAD | cat\n foo\n -\n bar\n\nWe will check the option value and if the value doesn't contail\na trailing '\\n', will add it automatically, so execute\n\n $ git notes add -m foo -m bar --separator=\"-\"\nand\n $ export LF=\"\n \"\n $ git notes add -m foo -m bar --separator=\"-$LF\"\n\nhave the same behavour.\n\nSigned-off-by: Teng Long \n---\n Documentation/git-notes.txt | 20 ++++++--\n builtin/notes.c | 72 ++++++++++++++++++--------\n t/t3301-notes.sh | 100 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 166 insertions(+), 26 deletions(-)\n\ndiff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\nindex efbc10f0..53d63888 100644\n--- a/Documentation/git-notes.txt\n+++ b/Documentation/git-notes.txt\n@@ -9,9 +9,9 @@ SYNOPSIS\n --------\n [verse]\n 'git notes' [list []]\n-'git notes' add [-f] [--allow-empty] [-F | -m | (-c | -C) ] []\n+'git notes' add [-f] [--allow-empty] [--separator=] [-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@@ -65,7 +65,9 @@ add::\n \tHowever, if you're using `add` interactively (using an editor\n \tto supply the notes contents), then - instead of aborting -\n \tthe existing notes will be opened in the editor (like the `edit`\n-\tsubcommand).\n+\tsubcommand). If you specify multiple `-m` and `-F`, a blank\n+\tline will be inserted between the messages. Use the `--separator`\n+\toption to insert other delimiters. \n \n copy::\n \tCopy the notes for the first object onto the second object (defaults to\n@@ -86,7 +88,13 @@ 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. \n+\tThe default delimiter is a blank line, use the `--separator`\n+\toption to insert other delimiters. More specifically, if the\n+\tnote and the message are not empty, the delimiter will be\n+\tinserted between them. If you specify multiple `-m` and `-F`\n+\toptions, the delimiter will be inserted between the messages\n+\ttoo.\n \n edit::\n \tEdit the notes for a given object (defaults to HEAD).\n@@ -159,6 +167,10 @@ 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 paragraphs.\n+\tA blank line by 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 553ae2bd..e0ada862 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -24,11 +24,12 @@\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+\tN_(\"git notes [--ref ] add [-f] [--allow-empty] [--separator=] [-m | -F | (-c | -C) ] []\"),\n \tN_(\"git notes [--ref ] copy [-f] \"),\n-\tN_(\"git notes [--ref ] append [--allow-empty] [-m | -F | (-c | -C) ] []\"),\n+\tN_(\"git notes [--ref ] append [--allow-empty] [--separator=] [-m | -F | (-c | -C) ] []\"),\n \tN_(\"git notes [--ref ] edit [--allow-empty] []\"),\n \tN_(\"git notes [--ref ] show []\"),\n \tN_(\"git notes [--ref ] merge [-v | -q] [-s ] \"),\n@@ -209,37 +210,55 @@ 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 void parse_messages(struct string_list *messages, struct note_data *d)\n+{\n+\tsize_t i;\n+\tfor (i = 0; i < messages->nr; i++) {\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 messages->items[i].string);\n+\t\tstrbuf_stripspace(&d->buf, 0);\n+\t\td->given = 1;\n+\t}\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+\tstruct string_list *msg = opt->value;\n \n \tBUG_ON_OPT_NEG(unset);\n \n-\tif (d->buf.len)\n-\t\tstrbuf_addch(&d->buf, '\\n');\n-\tstrbuf_addstr(&d->buf, arg);\n-\tstrbuf_stripspace(&d->buf, 0);\n-\n-\td->given = 1;\n+\tstring_list_append(msg, arg);\n \treturn 0;\n }\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+\tstruct string_list *msg = opt->value;\n+\tstruct strbuf buf = STRBUF_INIT;\n \n \tBUG_ON_OPT_NEG(unset);\n \n-\tif (d->buf.len)\n-\t\tstrbuf_addch(&d->buf, '\\n');\n \tif (!strcmp(arg, \"-\")) {\n-\t\tif (strbuf_read(&d->buf, 0, 1024) < 0)\n+\t\tif (strbuf_read(&buf, 0, 1024) < 0)\n \t\t\tdie_errno(_(\"cannot read '%s'\"), arg);\n-\t} else if (strbuf_read_file(&d->buf, arg, 1024) < 0)\n+\t} else if (strbuf_read_file(&buf, arg, 1024) < 0)\n \t\tdie_errno(_(\"could not open or read '%s'\"), arg);\n-\tstrbuf_stripspace(&d->buf, 0);\n \n-\td->given = 1;\n+\tstring_list_append(msg, buf.buf);\n+\tstrbuf_release(&buf);\n \treturn 0;\n }\n \n@@ -402,11 +421,12 @@ static int add(int argc, const char **argv, const char *prefix)\n \tstruct object_id object, new_note;\n \tconst struct object_id *note;\n \tstruct note_data d = { .buf = STRBUF_INIT };\n+\tstruct string_list messages = STRING_LIST_INIT_DUP;\n \tstruct option options[] = {\n-\t\tOPT_CALLBACK_F('m', \"message\", &d, N_(\"message\"),\n+\t\tOPT_CALLBACK_F('m', \"message\", &messages, N_(\"message\"),\n \t\t\tN_(\"note contents as a string\"), PARSE_OPT_NONEG,\n \t\t\tparse_msg_arg),\n-\t\tOPT_CALLBACK_F('F', \"file\", &d, N_(\"file\"),\n+\t\tOPT_CALLBACK_F('F', \"file\", &messages, N_(\"file\"),\n \t\t\tN_(\"note contents in a file\"), PARSE_OPT_NONEG,\n \t\t\tparse_file_arg),\n \t\tOPT_CALLBACK_F('c', \"reedit-message\", &d, N_(\"object\"),\n@@ -418,6 +438,8 @@ static int add(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n \t\t\tN_(\"allow storing empty note\")),\n \t\tOPT__FORCE(&force, N_(\"replace existing notes\"), PARSE_OPT_NOCOMPLETE),\n+\t\tOPT_STRING(0, \"separator\", &separator, N_(\"separator\"),\n+\t\t\tN_(\"insert between paragraphs\")),\n \t\tOPT_END()\n \t};\n \n@@ -429,6 +451,7 @@ static int add(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_notes_add_usage, options);\n \t}\n \n+\tparse_messages(&messages, &d);\n \tobject_ref = argc > 1 ? argv[1] : \"HEAD\";\n \n \tif (get_oid(object_ref, &object))\n@@ -568,11 +591,12 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \tchar *logmsg;\n \tconst char * const *usage;\n \tstruct note_data d = { .buf = STRBUF_INIT };\n+\tstruct string_list messages = 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\tOPT_CALLBACK_F('m', \"message\", &messages, N_(\"message\"),\n+\t\t\tN_(\"note contents as a string\"), PARSE_OPT_NONEG, \n \t\t\tparse_msg_arg),\n-\t\tOPT_CALLBACK_F('F', \"file\", &d, N_(\"file\"),\n+\t\tOPT_CALLBACK_F('F', \"file\", &messages, N_(\"file\"),\n \t\t\tN_(\"note contents in a file\"), PARSE_OPT_NONEG,\n \t\t\tparse_file_arg),\n \t\tOPT_CALLBACK_F('c', \"reedit-message\", &d, N_(\"object\"),\n@@ -583,6 +607,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 between paragraphs\")),\n \t\tOPT_END()\n \t};\n \tint edit = !strcmp(argv[0], \"edit\");\n@@ -596,6 +622,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(usage, options);\n \t}\n \n+\tparse_messages(&messages, &d);\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 +646,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..c2c09f32 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -362,6 +362,7 @@ test_expect_success 'do not create empty note with -m \"\"' '\n '\n \n test_expect_success 'create note with combination of -m and -F' '\n+\ttest_when_finished git notes remove HEAD &&\n \tcat >expect-combine_m_and_F <<-EOF &&\n \t\tfoo\n \n@@ -380,6 +381,26 @@ test_expect_success 'create note with combination of -m and -F' '\n \ttest_cmp expect-combine_m_and_F actual\n '\n \n+test_expect_success 'create note with combination of -m and -F and --separator' '\n+\tcat >expect-combine_m_and_F <<-\\EOF &&\n+\tfoo\n+\t-------\n+\txyzzy\n+\t-------\n+\tbar\n+\t-------\n+\tzyxxy\n+\t-------\n+\tbaz\n+\tEOF\n+\techo \"xyzzy\" >note_a &&\n+\techo \"zyxxy\" >note_b &&\n+\tgit notes add -m \"foo\" -F note_a -m \"bar\" -F note_b -m \"baz\" --separator \"-------\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect-combine_m_and_F actual\n+\t\n+'\n+\n test_expect_success 'remove note with \"git notes remove\"' '\n \tgit notes remove HEAD^ &&\n \tgit notes remove &&\n@@ -521,6 +542,85 @@ 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 separator 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 note with combination of -m and -F and --separator' '\n+\ttest_when_finished git notes remove HEAD &&\n+\tcat >expect-combine_m_and_F <<-\\EOF &&\n+\tm-notes-1\n+\t-------\n+\tf-notes-1\n+\t-------\n+\tm-notes-2\n+\t-------\n+\tf-notes-2\n+\t-------\n+\tm-notes-3\n+\tEOF\n+\n+\techo \"f-notes-1\" >note_a &&\n+\techo \"f-notes-2\" >note_b &&\n+\tgit notes append -m \"m-notes-1\" -F note_a -m \"m-notes-2\" -F note_b -m \"m-notes-3\" --separator \"-------\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect-combine_m_and_F 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.gd5a6c747\n\n"},{"id":"472561","messageId":"xmqqcz608cpk.fsf@gitster.g","threadId":"59295","inReplyTo":"d5a6c74792c15e2f83c2ed0758fb99eac11a8174.1677136319.git.dyroneteng@gmail.com","subject":"Re: [PATCH v6 3/3] notes.c: introduce '--separator=' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-23T18:21:27Z","receivedAt":"2023-02-23T18:21:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n> So this commit\n> introduces a new '--separator' option for 'git-notes-add' and\n> 'git-notes-append', for example when execute:\n\n Introduce a new '--separator' option for 'git notes add' and\n 'git notes append'.\n\n> We will check the option value and if the value doesn't contail\n> a trailing '\\n', will add it automatically,\n\ncontail?\n\n\tA newline is added to the value given to --separator if it\n\tdoes not end with one already.\n\n> so execute\n> $ git notes add -m foo -m bar --separator=\"-\"\n> and\n> $ export LF=\"\n> \"\n> $ git notes add -m foo -m bar --separator=\"-$LF\"\n>\n> have the same behavour.\n\n\tRunning A and B produces the same result.\n\n> +\tsubcommand). If you specify multiple `-m` and `-F`, a blank\n> +\tline will be inserted between the messages. Use the `--separator`\n> +\toption to insert other delimiters. \n\nConcise and readable. Nice.\n\n> @@ -86,7 +88,13 @@ 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. \n> +\tThe default delimiter is a blank line, use the `--separator`\n\nRe-read the above three lines, pretending that it is already 6\nmonths and you've forgotten about adding the feature yourself.\n\nNotice something?\n\nA reader with fresh mind would read and think along the description\nbut\n\n - OK, append command is used to append to the note to an existing\n object;\n\n - OK, if the object does not have a note yet, we will create one;\n\n - The default delimiter? Delimiter for what? I am puzzled.\n\nat the third step, gets puzzled. The command takes the existing\nnote's contents, adds a delimiter and then appends the new material\ngiven by the user, but because that is not clear after reading the\nfirst two lines, the sudden appearance of \"delimiter\" would confuse\nreaders.\n\n> +\toption to insert other delimiters. More specifically, if the\n> +\tnote and the message are not empty, the delimiter will be\n> +\tinserted between them. If you specify multiple `-m` and `-F`\n\nAgain, the order of presentation is somewhat backwards and that is\nwhy we need to say \"More specifically\" here.\n\n> +\toptions, the delimiter will be inserted between the messages\n> +\ttoo.\n\n\tAppend new message(s) given by `-m` or `-F` options to an\n\texisting note, or add them as a new note if one does not\n\texist, for the object (defaults to HEAD). When appending to\n\tan existing note, a blank line is added before each new\n\tmessage as an inter-paragraph separator. The separator can\n\tbe customized with the `--separator` option.\n\nor something along that line, perhaps?\n\n> +--separator ::\n> +\tThe '' inserted between paragraphs.\n> +\tA blank line by default.\n\nHere is where you need to say about promoting incomplete line\nseparator more than the proposed log message.\n\n\tSpecify a string used as a custom inter-paragraph separator\n\t(a newline is added at the end as needed). Defaults to a\n\tblank line.\n\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index 553ae2bd..e0ada862 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -24,11 +24,12 @@\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\nIt looks like you are very fond of \"insert\", but aren't we always\nappending with the latest control flow? In other words, is it worth\ncarrying 'pos' around?\n\n> +static void parse_messages(struct string_list *messages, struct note_data *d)\n> +{\n> +\tsize_t i;\n> +\tfor (i = 0; i < messages->nr; i++) {\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 messages->items[i].string);\n> +\t\tstrbuf_stripspace(&d->buf, 0);\n\nThis is not a new problem, but if we get three 100-byte messages, we\n\n - add the first 100-byte message to d->buf and then run\n stripspace() over that 100-byte.\n\n - add separator and then the second 100-byte message to d->buf, and\n then run stripspace() over that 200-plus-byte.\n\n - add separator and then the third 100-byte message to d->buf, and\n then run stripspace() over that 300-plus-byte.\n\nShouldn't we be doing better?\n\n> +\t\td->given = 1;\n\nDo we understand what d->given flag represents? My understanding is\nthat it becomes true only when any of the -m/-F/-c/-C options are given\nto tell the command what message to use, so that we can automatically\nopen the editor to ask for the message when nothing is given.\n\nSo, I suspect that\n\n\td->given = !!messages->nr;\n\nat the beginning of the function, or\n\n\td->given = !!d->buf.len;\n\nmay be equivalent[*], instead of setting it once every iteration?\n\n\tSide note: The latter is slightly more strict, as giving a\n\trun of empty strings with the default separator would result\n\tin an empty d->buf and d->given will become false.\n\n> +\t}\n> +}\n\nThis helper is not parsing, but just processing after the whole\nthing was parsed. \"parse_messages\" -> \"concatenate_messages\" or\nsomething, perhaps?\n\nNow d->given is set in parse_reuse_arg() and parse_reedit_arg(),\nbecause -c/-C is another source of a paragraph. Shouldn't the\nparagraph taken by these options go in the message list to be\nconcatenated together with other messages, or are -c/-C incompatible\nwith -m/-F and we are OK with two separate and distinct behaviour?\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> +\tstruct string_list *msg = opt->value;\n> \n> \tBUG_ON_OPT_NEG(unset);\n> \n> -\tif (d->buf.len)\n> -\t\tstrbuf_addch(&d->buf, '\\n');\n> -\tstrbuf_addstr(&d->buf, arg);\n> -\tstrbuf_stripspace(&d->buf, 0);\n> -\n> -\td->given = 1;\n> +\tstring_list_append(msg, arg);\n> \treturn 0;\n> }\n\nOK, this one now does not concatenate; just accumulates to the \"msg\"\nstring list.\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> +\tstruct string_list *msg = opt->value;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> \n> \tBUG_ON_OPT_NEG(unset);\n> \n> -\tif (d->buf.len)\n> -\t\tstrbuf_addch(&d->buf, '\\n');\n> \tif (!strcmp(arg, \"-\")) {\n> -\t\tif (strbuf_read(&d->buf, 0, 1024) < 0)\n> +\t\tif (strbuf_read(&buf, 0, 1024) < 0)\n> \t\t\tdie_errno(_(\"cannot read '%s'\"), arg);\n> -\t} else if (strbuf_read_file(&d->buf, arg, 1024) < 0)\n> +\t} else if (strbuf_read_file(&buf, arg, 1024) < 0)\n> \t\tdie_errno(_(\"could not open or read '%s'\"), arg);\n> -\tstrbuf_stripspace(&d->buf, 0);\n> \n> -\td->given = 1;\n> +\tstring_list_append(msg, buf.buf);\n> +\tstrbuf_release(&buf);\n> \treturn 0;\n> }\n\nDitto.\n\n> @@ -418,6 +438,8 @@ static int add(int argc, const char **argv, const char *prefix)\n> \t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n> \t\t\tN_(\"allow storing empty note\")),\n> \t\tOPT__FORCE(&force, N_(\"replace existing notes\"), PARSE_OPT_NOCOMPLETE),\n> +\t\tOPT_STRING(0, \"separator\", &separator, N_(\"separator\"),\n> +\t\t\tN_(\"insert between paragraphs\")),\n> \t\tOPT_END()\n> \t};\n> \n> @@ -429,6 +451,7 @@ static int add(int argc, const char **argv, const char *prefix)\n> \t\tusage_with_options(git_notes_add_usage, options);\n> \t}\n> \n> +\tparse_messages(&messages, &d);\n\nYup, this one is \"concatenate_messages\". We do not read the\nexisting one, we accumulated everything the user gave us in\nmessages, and concatenate them using the separator. The result\nis stored in d->buf.\n\nI wonder why separator is not a parameter into the helper function?\n\n\nIn short, I think handling of -m/-F got vastly better from the\nprevious rounds in this version, but I am not sure about the only\nother remaining \"strbuf_addch(&d->buf, '\\n')\" in parse_reuse_arg().\nI am inclined to say that that codepath should behave the same way,\nbut I didn't think it as deeply as you did, so...?\n\nThanks.\n"},{"id":"472732","messageId":"xmqqk005v3ex.fsf@gitster.g","threadId":"59295","inReplyTo":"d5a6c74792c15e2f83c2ed0758fb99eac11a8174.1677136319.git.dyroneteng@gmail.com","subject":"Re: [PATCH v6 3/3] notes.c: introduce '--separator=' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-25T21:30:30Z","receivedAt":"2023-02-25T21:30:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n> +static void parse_messages(struct string_list *messages, struct note_data *d)\n> +{\n> +\tsize_t i;\n> +\tfor (i = 0; i < messages->nr; i++) {\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 messages->items[i].string);\n> +\t\tstrbuf_stripspace(&d->buf, 0);\n> +\t\td->given = 1;\n> +\t}\n> +}\n\nThe two callers of this function prepares the string_list, and have\nthis function consume it by concatenating its contents to d->buf.\nAfter calling this function, neither of them talks about messages,\nwhich means we are leaking the strings kept in the string_list.\n\nI could eject the topic from today's integration run (because the\ntopic is not ready to be merged to 'next' as-is; \"-C/-c\" codepaths\nneed to be adjusted, at least), but as I took a look already, I'll\nqueue this fix-up on top of the topic for now. Feel free to squash\nit in (or address the leaks in your own way) when sending in an\nupdate.\n\nThanks.\n\n builtin/notes.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git c/builtin/notes.c w/builtin/notes.c\nindex 97c18fc02f..cd7af76e2f 100644\n--- c/builtin/notes.c\n+++ w/builtin/notes.c\n@@ -220,7 +220,8 @@ static void insert_separator(struct strbuf *message, size_t pos)\n \t\tstrbuf_insertf(message, pos, \"%s%s\", separator, \"\\n\");\n }\n \n-static void parse_messages(struct string_list *messages, struct note_data *d)\n+/* Consume messages and append them into d->buf */\n+static void concat_messages(struct string_list *messages, struct note_data *d)\n {\n \tsize_t i;\n \tfor (i = 0; i < messages->nr; i++) {\n@@ -231,6 +232,7 @@ static void parse_messages(struct string_list *messages, struct note_data *d)\n \t\tstrbuf_stripspace(&d->buf, 0);\n \t\td->given = 1;\n \t}\n+\tstring_list_clear(messages, 0);\n }\n \n static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n@@ -451,7 +453,7 @@ static int add(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_notes_add_usage, options);\n \t}\n \n-\tparse_messages(&messages, &d);\n+\tconcat_messages(&messages, &d);\n \tobject_ref = argc > 1 ? argv[1] : \"HEAD\";\n \n \tif (get_oid(object_ref, &object))\n@@ -622,7 +624,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(usage, options);\n \t}\n \n-\tparse_messages(&messages, &d);\n+\tconcat_messages(&messages, &d);\n \n \tif (d.given && edit)\n \t\tfprintf(stderr, _(\"The -m/-F/-c/-C options have been deprecated \"\n"},{"id":"472845","messageId":"20230228141103.47275-1-tenglong.tl@alibaba-inc.com","threadId":"59295","inReplyTo":"xmqqcz608cpk.fsf@gitster.g","subject":"Re: [PATCH v6 3/3] notes.c: introduce '--separator=' option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-02-28T14:11:03Z","receivedAt":"2023-02-28T14:11:15Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Junio C Hamano :\n\n> > So this commit\n> > introduces a new '--separator' option for 'git-notes-add' and\n> > 'git-notes-append', for example when execute:\n>\n> Introduce a new '--separator' option for 'git notes add' and\n> 'git notes append'.\n\nThanks, I will avoid this like 'git-subcommand' expression in the future.\n\n> > We will check the option value and if the value doesn't contail\n> > a trailing '\\n', will add it automatically,\n>\n> contail?\n\n\"contain\", sorry.\n\n> \tA newline is added to the value given to --separator if it\n> \tdoes not end with one already.\n\nWill apply.\n\n> > so execute\n> > $ git notes add -m foo -m bar --separator=\"-\"\n> > and\n> > $ export LF=\"\n> > \"\n> > $ git notes add -m foo -m bar --separator=\"-$LF\"\n> >\n> > have the same behavour.\n>\n> \tRunning A and B produces the same result.\n\nWill s/Running A and B produces the same result/have the same behavour\n\n> - The default delimiter? Delimiter for what? I am puzzled.\n>\n> at the third step, gets puzzled. The command takes the existing\n> note's contents, adds a delimiter and then appends the new material\n> given by the user, but because that is not clear after reading the\n> first two lines, the sudden appearance of \"delimiter\" would confuse\n> readers.\n>\n> > +\toption to insert other delimiters. More specifically, if the\n> > +\tnote and the message are not empty, the delimiter will be\n> > +\tinserted between them. If you specify multiple `-m` and `-F`\n>\n> Again, the order of presentation is somewhat backwards and that is\n> why we need to say \"More specifically\" here.\n>\n> > +\toptions, the delimiter will be inserted between the messages\n> > +\ttoo.\n>\n> \tAppend new message(s) given by `-m` or `-F` options to an\n> \texisting note, or add them as a new note if one does not\n> \texist, for the object (defaults to HEAD). When appending to\n> \tan existing note, a blank line is added before each new\n> \tmessage as an inter-paragraph separator. The separator can\n> \tbe customized with the `--separator` option.\n\nWill apply.\n\n> Here is where you need to say about promoting incomplete line\n> separator more than the proposed log message.\n>\n> \tSpecify a string used as a custom inter-paragraph separator\n> \t(a newline is added at the end as needed). Defaults to a\n> \tblank line.\n\nWill apply.\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> It looks like you are very fond of \"insert\", but aren't we always\n> appending with the latest control flow? In other words, is it worth\n> carrying 'pos' around?\n\nActually I can't find a more suitable verb. I think it worth, but I'm\nnot sure whether there is a better way, that is, when we add separator which\nuse to contact messages like multiple '-m' and '-F', the 'pos' is at the end.\nWhen we add separator which use to contact the prev_notes and the new notes\n, the 'pos' is at the head.\n\n> > +static void parse_messages(struct string_list *messages, struct note_data *d)\n> > +{\n> > +\tsize_t i;\n> > +\tfor (i = 0; i < messages->nr; i++) {\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 messages->items[i].string);\n> > +\t\tstrbuf_stripspace(&d->buf, 0);\n>\n> This is not a new problem, but if we get three 100-byte messages, we\n>\n> - add the first 100-byte message to d->buf and then run\n> stripspace() over that 100-byte.\n>\n> - add separator and then the second 100-byte message to d->buf, and\n> then run stripspace() over that 200-plus-byte.\n>\n> - add separator and then the third 100-byte message to d->buf, and\n> then run stripspace() over that 300-plus-byte.\n>\n> Shouldn't we be doing better?\n\nAfter I read the comments of 'strbuf_stripspace', it does :\n\n1. remove empty lines from beginning and end.\n2. turn multiple empty lines between paragraphs into just one empty line.\n3. if the input has only empty lines and spaces, no output will be produced.\n4. if the last line doesn't have a newline at the end, one is added.\n5. skip every commentted line if skip_comments is enabled.\n\nI think above the five functions in 'strbuf_stripspace', we may just care about\nthe fourth, the others just like the logic when we edit the commit message when\ndoing 'git commit' or 'git merge', etc. My opinion is a \"commit note\" and a\n\"commit message\" maybe not be the same, and sometimes the user might want a\nblank line or something at the beginning of the notes. But after I read the\ntests I think they are just the same. So, let me just fix this by stripspace\nthe message individually, thanks.\n\n> Do we understand what d->given flag represents? My understanding is\n> that it becomes true only when any of the -m/-F/-c/-C options are given\n> to tell the command what message to use, so that we can automatically\n> open the editor to ask for the message when nothing is given.\n> \n> So, I suspect that\n> \n> \td->given = !!messages->nr;\n> \n> at the beginning of the function, or\n> \n> \td->given = !!d->buf.len;\n> \n> may be equivalent[*], instead of setting it once every iteration?\n> \n> \tSide note: The latter is slightly more strict, as giving a\n> \trun of empty strings with the default separator would result\n> \tin an empty d->buf and d->given will become false.\n\nI agree, we should choose the latter and move \"d->given = !!d->buf.len;\"\nbehind the place where we concat the messages.\n\n\n> Now d->given is set in parse_reuse_arg() and parse_reedit_arg(),\n> because -c/-C is another source of a paragraph. Shouldn't the\n> paragraph taken by these options go in the message list to be\n> concatenated together with other messages, or are -c/-C incompatible\n> with -m/-F and we are OK with two separate and distinct behaviour?\n\nYes, my bad. -c/-C should be considered as well and dealing them with the same\nmessages-contacting logic, I will fix these two place in next patch.\n\n> I wonder why separator is not a parameter into the helper function?\n\nIn my opinion, your proposal is probably similar to the current approach, but\nyour solution may be a bit more readable. I will consider changing it in the\nnext patch.\n\n> other remaining \"strbuf_addch(&d->buf, '\\n')\" in parse_reuse_arg().\n> I am inclined to say that that codepath should behave the same way,\n> but I didn't think it as deeply as you did, so...?\n\nYou really have a good eye, I didn't noticed them. There are two place remained,\nthe first one is \"parse_reuse_arg\", second one is \"prepare_note_data\" it's made\nfor separate the note and the commented lines (note template), I think they\nshould both be replaced/impacted by \"--separator\".\n\nThanks.\n"},{"id":"472847","messageId":"20230228141419.47430-1-tenglong.tl@alibaba-inc.com","threadId":"59295","inReplyTo":"xmqqk005v3ex.fsf@gitster.g","subject":"Re: [PATCH v6 3/3] notes.c: introduce '--separator=' option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-02-28T14:14:19Z","receivedAt":"2023-02-28T14:14:29Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Junio C Hamano writes:\n\n> The two callers of this function prepares the string_list, and have\n> this function consume it by concatenating its contents to d->buf.\n> After calling this function, neither of them talks about messages,\n> which means we are leaking the strings kept in the string_list.\n>\n> I could eject the topic from today's integration run (because the\n> topic is not ready to be merged to 'next' as-is; \"-C/-c\" codepaths\n> need to be adjusted, at least), but as I took a look already, I'll\n> queue this fix-up on top of the topic for now. Feel free to squash\n> it in (or address the leaks in your own way) when sending in an\n> update.\n>\n> Thanks.\n>\n> builtin/notes.c | 8 +++++---\n> 1 file changed, 5 insertions(+), 3 deletions(-)\n>\n> diff --git c/builtin/notes.c w/builtin/notes.c\n> index 97c18fc02f..cd7af76e2f 100644\n> --- c/builtin/notes.c\n> +++ w/builtin/notes.c\n> @@ -220,7 +220,8 @@ static void insert_separator(struct strbuf *message, size_t pos)\n> \t\tstrbuf_insertf(message, pos, \"%s%s\", separator, \"\\n\");\n> }\n>\n> -static void parse_messages(struct string_list *messages, struct note_data *d)\n> +/* Consume messages and append them into d->buf */\n> +static void concat_messages(struct string_list *messages, struct note_data *d)\n> {\n> \tsize_t i;\n> \tfor (i = 0; i < messages->nr; i++) {\n> @@ -231,6 +232,7 @@ static void parse_messages(struct string_list *messages, struct note_data *d)\n> \t\tstrbuf_stripspace(&d->buf, 0);\n> \t\td->given = 1;\n> \t}\n> +\tstring_list_clear(messages, 0);\n> }\n>\n> static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n> @@ -451,7 +453,7 @@ static int add(int argc, const char **argv, const char *prefix)\n> \t\tusage_with_options(git_notes_add_usage, options);\n> \t}\n>\n> -\tparse_messages(&messages, &d);\n> +\tconcat_messages(&messages, &d);\n> \tobject_ref = argc > 1 ? argv[1] : \"HEAD\";\n>\n> \tif (get_oid(object_ref, &object))\n> @@ -622,7 +624,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> \t\tusage_with_options(usage, options);\n> \t}\n>\n> -\tparse_messages(&messages, &d);\n> +\tconcat_messages(&messages, &d);\n>\n> \tif (d.given && edit)\n> \t\tfprintf(stderr, _(\"The -m/-F/-c/-C options have been deprecated \"\n\nThanks, will apply in next patch.\n"},{"id":"474234","messageId":"20230327131333.35036-1-tenglong.tl@alibaba-inc.com","threadId":"59295","inReplyTo":"cover.1677136319.git.dyroneteng@gmail.com","subject":"[PATCH v6 0/3] notes.c: introduce \"--separator\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-03-27T13:13:33Z","receivedAt":"2023-03-27T13:13:49Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"I'm working on the new patchset recently, and I found there may be a\nweired test code writting like this in t3301:\n\n\ntest_expect_success 'refusing to edit notes in refs/remotes/' '\n\ttest_must_fail env MSG=1 GIT_NOTES_REF=refs/heads/bogus git notes edit\n'\n\nshould it be like:\n\ntest_expect_success 'refusing to edit notes in refs/remotes/' '\n\ttest_must_fail env MSG=1 GIT_NOTES_REF=refs/remotes/bogus git notes edit\n'\n\nThanks.\n"},{"id":"474312","messageId":"cover.1680012650.git.dyroneteng@gmail.com","threadId":"59295","inReplyTo":"cover.1677136319.git.dyroneteng@gmail.com","subject":"[PATCH v7 0/4] notes.c: introduce \"--separator\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-03-28T14:28:43Z","receivedAt":"2023-03-28T14:29:00Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nDiff since v6:\n\n1. apply Junio's code[1] in 3/4 \n2. apply Junio's code[2] and text in 3/4\n3. adjust -c/-C codepath in 3/4\n4. add new test cases in 3/4\n5. in v6, I used a string_list struct to temporarily save the message\nthen concat them with the separator. But, I found it will be a problem\nwhen we use -C/-F to save the content of binary file(break the t3307)\nbecause binary file maybe contains the NUL in the middle, so I use\nstrbuf struct instead to make sure the whole content is saved to the\nnotes.\n6. 4/4 is a new commit, which fix the problem about the binary file\ntoo. The binary file maybe contains multiple consecutive empty lines,\nwe shouldn't make them into just one empty line, which may corrupt\nthe file I think.\n\n[1] https://public-inbox.org/git/xmqqk005v3ex.fsf@gitster.g/\n[2] https://public-inbox.org/git/xmqqcz608cpk.fsf@gitster.g/\n\nThanks.\n\nTeng Long (4):\n notes.c: cleanup 'strbuf_grow' call in 'append_edit'\n notes.c: cleanup for \"designated init\"\n notes.c: introduce '--separator=' option\n notes.c: don't do stripespace when parse file arg\n\n Documentation/git-notes.txt | 21 ++++--\n builtin/notes.c | 106 +++++++++++++++++++++-------\n t/t3301-notes.sh | 137 +++++++++++++++++++++++++++++++++++-\n 3 files changed, 232 insertions(+), 32 deletions(-)\n\nRange-diff against v6:\n1: b029ee0b = 1: ea9c199e notes.c: cleanup 'strbuf_grow' call in 'append_edit'\n2: 043db631 = 2: 9dc123c8 notes.c: cleanup for \"designated init\"\n3: d5a6c747 ! 3: d1febf86 notes.c: introduce '--separator=' option\n @@ Commit message\n \n The default behavour sometimes is not enough, the user may want\n to use a custom delimiter between paragraphs, like when\n - specifiy one or more '-m' or '-F' options. So this commit\n - introduces a new '--separator' option for 'git-notes-add' and\n - 'git-notes-append', for example when execute:\n + specifiy '-m', '-F', '-C', '-c' options. So this commit\n + introduce a new '--separator' option for 'git notes add' and\n + 'git notes append', for example when execute:\n \n $ git notes add -m foo -m bar --separator=\"-\"\n $ git notes show HEAD | cat\n @@ Commit message\n -\n bar\n \n - We will check the option value and if the value doesn't contail\n - a trailing '\\n', will add it automatically, so execute\n + A newline is added to the value given to --separator if it\n + does not end with one already. So execute:\n \n $ git notes add -m foo -m bar --separator=\"-\"\n and\n @@ Commit message\n \"\n $ git notes add -m foo -m bar --separator=\"-$LF\"\n \n - have the same behavour.\n + Running A and B produces the same result.\n \n Signed-off-by: Teng Long \n \n @@ Documentation/git-notes.txt: add::\n -\tsubcommand).\n +\tsubcommand). If you specify multiple `-m` and `-F`, a blank\n +\tline will be inserted between the messages. Use the `--separator`\n -+\toption to insert other delimiters. \n ++\toption to insert other delimiters.\n \n copy::\n \tCopy the notes for the first object onto the second object (defaults to\n -@@ Documentation/git-notes.txt: the command can read the input given to the `post-rewrite` hook.)\n +@@ Documentation/git-notes.txt: corresponding . (The optional `` is ignored so that\n + 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 +-\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. \n -+\tThe default delimiter is a blank line, use the `--separator`\n -+\toption to insert other delimiters. More specifically, if the\n -+\tnote and the message are not empty, the delimiter will be\n -+\tinserted between them. If you specify multiple `-m` and `-F`\n -+\toptions, the delimiter will be inserted between the messages\n -+\ttoo.\n ++\tAppend new message(s) given by `-m` or `-F` options to an\n ++\texisting note, or add them as a new note if one does not\n ++\texist, for the object (defaults to HEAD). When appending to\n ++\tan existing note, a blank line is added before each new\n ++\tmessage as an inter-paragraph separator. The separator can\n ++\tbe customized with the `--separator` option.\n \n edit::\n \tEdit the notes for a given object (defaults to HEAD).\n @@ Documentation/git-notes.txt: OPTIONS\n \tto automatically remove empty notes.\n \n +--separator ::\n -+\tThe '' inserted between paragraphs.\n -+\tA blank line by default.\n ++\tSpecify a string used as a custom inter-paragraph separator\n ++\t(a newline is added at the end as needed). Defaults to a\n ++\tblank line.\n +\n --ref ::\n \tManipulate the notes tree in . This overrides\n @@ builtin/notes.c\n \tN_(\"git notes [--ref ] edit [--allow-empty] []\"),\n \tN_(\"git notes [--ref ] show []\"),\n \tN_(\"git notes [--ref ] merge [-v | -q] [-s ] \"),\n +@@ builtin/notes.c: struct note_data {\n + \tint use_editor;\n + \tchar *edit_path;\n + \tstruct strbuf buf;\n ++\tstruct strbuf **messages;\n ++\tsize_t msg_nr;\n ++\tsize_t msg_alloc;\n + };\n + \n + static void free_note_data(struct note_data *d)\n @@ builtin/notes.c: static void write_note_data(struct note_data *d, struct object_id *oid)\n \t}\n }\n @@ builtin/notes.c: static void write_note_data(struct note_data *d, struct object_\n +\t\tstrbuf_insertf(message, pos, \"%s%s\", separator, \"\\n\");\n +}\n +\n -+static void parse_messages(struct string_list *messages, struct note_data *d)\n ++/* Consume messages and append them into d->buf, then free them */\n ++static void concat_messages(struct note_data *d)\n +{\n ++\tstruct strbuf msg = STRBUF_INIT;\n ++\n +\tsize_t i;\n -+\tfor (i = 0; i < messages->nr; i++) {\n ++\tfor (i = 0; i < d->msg_nr ; i++) {\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 messages->items[i].string);\n -+\t\tstrbuf_stripspace(&d->buf, 0);\n -+\t\td->given = 1;\n ++\t\tstrbuf_add(&msg, d->messages[i]->buf, d->messages[i]->len);\n ++\t\tstrbuf_addbuf(&d->buf, &msg);\n ++\t\tstrbuf_reset(&msg);\n ++\t\tstrbuf_release(d->messages[i]);\n ++\t\tfree(d->messages[i]);\n +\t}\n ++\tstrbuf_release(&msg);\n ++\tfree(d->messages);\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 -+\tstruct string_list *msg = opt->value;\n + \tstruct note_data *d = opt->value;\n ++\tstruct strbuf *buf;\n \n \tBUG_ON_OPT_NEG(unset);\n \n @@ builtin/notes.c: static void write_note_data(struct note_data *d, struct object_\n -\t\tstrbuf_addch(&d->buf, '\\n');\n -\tstrbuf_addstr(&d->buf, arg);\n -\tstrbuf_stripspace(&d->buf, 0);\n --\n ++\tbuf = xmalloc(sizeof(*buf));\n ++\tstrbuf_init(buf, strlen(arg));\n ++\tstrbuf_addstr(buf, arg);\n ++\tstrbuf_stripspace(buf, 0);\n + \n -\td->given = 1;\n -+\tstring_list_append(msg, arg);\n ++\tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n ++\td->messages[d->msg_nr - 1] = buf;\n \treturn 0;\n }\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 -+\tstruct string_list *msg = opt->value;\n -+\tstruct strbuf buf = STRBUF_INIT;\n + \tstruct note_data *d = opt->value;\n ++\tstruct strbuf *buf;\n \n \tBUG_ON_OPT_NEG(unset);\n \n -\tif (d->buf.len)\n -\t\tstrbuf_addch(&d->buf, '\\n');\n ++\tbuf = xmalloc(sizeof(*buf));\n ++\tstrbuf_init(buf, 0);\n ++\n \tif (!strcmp(arg, \"-\")) {\n -\t\tif (strbuf_read(&d->buf, 0, 1024) < 0)\n -+\t\tif (strbuf_read(&buf, 0, 1024) < 0)\n ++\t\tif (strbuf_read(buf, 0, 1024) < 0)\n \t\t\tdie_errno(_(\"cannot read '%s'\"), arg);\n -\t} else if (strbuf_read_file(&d->buf, arg, 1024) < 0)\n -+\t} else if (strbuf_read_file(&buf, arg, 1024) < 0)\n ++\t} else if (strbuf_read_file(buf, arg, 1024) < 0)\n \t\tdie_errno(_(\"could not open or read '%s'\"), arg);\n -\tstrbuf_stripspace(&d->buf, 0);\n ++\tstrbuf_stripspace(buf, 0);\n + \n +-\td->given = 1;\n ++\t// we will note stripspace the buf here, because the file maybe\n ++\t// is a binary and it maybe contains multiple continuous line breaks.\n ++\tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n ++\td->messages[d->msg_nr - 1] = buf;\n + \treturn 0;\n + }\n + \n + static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)\n + {\n + \tstruct note_data *d = opt->value;\n +-\tchar *buf;\n ++\tstruct strbuf *buf;\n + \tstruct object_id object;\n + \tenum object_type type;\n + \tunsigned long len;\n + \n + \tBUG_ON_OPT_NEG(unset);\n + \n +-\tif (d->buf.len)\n +-\t\tstrbuf_addch(&d->buf, '\\n');\n +-\n ++\tbuf = xmalloc(sizeof(*buf));\n + \tif (get_oid(arg, &object))\n + \t\tdie(_(\"failed to resolve '%s' as a valid ref.\"), arg);\n +-\tif (!(buf = read_object_file(&object, &type, &len)))\n ++\tif (!(buf->buf = read_object_file(&object, &type, &len)))\n + \t\tdie(_(\"failed to read object '%s'.\"), arg);\n + \tif (type != OBJ_BLOB) {\n ++\t\tstrbuf_release(buf);\n + \t\tfree(buf);\n + \t\tdie(_(\"cannot read note data from non-blob object '%s'.\"), arg);\n + \t}\n +-\tstrbuf_add(&d->buf, buf, len);\n +-\tfree(buf);\n \n -\td->given = 1;\n -+\tstring_list_append(msg, buf.buf);\n -+\tstrbuf_release(&buf);\n ++\t// The reuse object maybe is a binary content which could\n ++\t// contains '\\0' in the middle, so let's set the correct\n ++\t// lenth of strbuf to concat all of content.\n ++\tbuf->len = len;\n ++\tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n ++\td->messages[d->msg_nr - 1] = buf;\n \treturn 0;\n }\n \n + static int parse_reedit_arg(const struct option *opt, const char *arg, int unset)\n + {\n + \tstruct note_data *d = opt->value;\n ++\n + \tBUG_ON_OPT_NEG(unset);\n + \td->use_editor = 1;\n + \treturn parse_reuse_arg(opt, arg, unset);\n @@ builtin/notes.c: 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 = { .buf = STRBUF_INIT };\n -+\tstruct string_list messages = STRING_LIST_INIT_DUP;\n +-\tstruct note_data d = { .buf = STRBUF_INIT };\n ++\tstruct note_data d = { .buf = STRBUF_INIT, .messages = NULL };\n ++\n \tstruct option options[] = {\n --\t\tOPT_CALLBACK_F('m', \"message\", &d, N_(\"message\"),\n -+\t\tOPT_CALLBACK_F('m', \"message\", &messages, N_(\"message\"),\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_CALLBACK_F('F', \"file\", &d, N_(\"file\"),\n -+\t\tOPT_CALLBACK_F('F', \"file\", &messages, N_(\"file\"),\n - \t\t\tN_(\"note contents in a file\"), PARSE_OPT_NONEG,\n - \t\t\tparse_file_arg),\n - \t\tOPT_CALLBACK_F('c', \"reedit-message\", &d, N_(\"object\"),\n @@ builtin/notes.c: static int add(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n \t\t\tN_(\"allow storing empty note\")),\n @@ builtin/notes.c: static int add(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_notes_add_usage, options);\n \t}\n \n -+\tparse_messages(&messages, &d);\n ++\tif (d.msg_nr)\n ++\t\tconcat_messages(&d);\n ++\td.given = !!d.buf.len;\n ++\n \tobject_ref = argc > 1 ? argv[1] : \"HEAD\";\n \n \tif (get_oid(object_ref, &object))\n @@ builtin/notes.c: 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 = { .buf = STRBUF_INIT };\n -+\tstruct string_list messages = STRING_LIST_INIT_DUP;\n +-\tstruct note_data d = { .buf = STRBUF_INIT };\n ++\tstruct note_data d = { .buf = STRBUF_INIT, .messages = NULL };\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 -+\t\tOPT_CALLBACK_F('m', \"message\", &messages, N_(\"message\"),\n -+\t\t\tN_(\"note contents as a string\"), PARSE_OPT_NONEG, \n - \t\t\tparse_msg_arg),\n --\t\tOPT_CALLBACK_F('F', \"file\", &d, N_(\"file\"),\n -+\t\tOPT_CALLBACK_F('F', \"file\", &messages, N_(\"file\"),\n - \t\t\tN_(\"note contents in a file\"), PARSE_OPT_NONEG,\n - \t\t\tparse_file_arg),\n - \t\tOPT_CALLBACK_F('c', \"reedit-message\", &d, N_(\"object\"),\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 \t\t\tparse_reuse_arg),\n \t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char\n \t\tusage_with_options(usage, options);\n \t}\n \n -+\tparse_messages(&messages, &d);\n ++\tif (d.msg_nr)\n ++\t\tconcat_messages(&d);\n ++\td.given = !!d.buf.len;\n +\n \tif (d.given && edit)\n \t\tfprintf(stderr, _(\"The -m/-F/-c/-C options have been deprecated \"\n @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char\n \t\tfree(prev_buf);\n \n ## t/t3301-notes.sh ##\n +@@\n + \n + test_description='Test commit notes'\n + \n ++TEST_PASSES_SANITIZE_LEAK=true\n + . ./test-lib.sh\n + \n + write_script fake_editor <<\\EOF\n @@ t/t3301-notes.sh: test_expect_success 'do not create empty note with -m \"\"' '\n '\n \n @@ t/t3301-notes.sh: test_expect_success 'create note with combination of -m and -F\n +\tgit notes add -m \"foo\" -F note_a -m \"bar\" -F note_b -m \"baz\" --separator \"-------\" &&\n +\tgit notes show >actual &&\n +\ttest_cmp expect-combine_m_and_F actual\n -+\t\n +'\n +\n test_expect_success 'remove note with \"git notes remove\"' '\n @@ t/t3301-notes.sh: test_expect_success 'listing non-existing notes fails' '\n test_expect_success 'append to existing note with \"git notes append\"' '\n \tcat >expect <<-EOF &&\n \t\tInitial set of notes\n +@@ t/t3301-notes.sh: test_expect_success 'create note from blob with \"git notes add -C\" reuses blob i\n + \ttest_cmp blob actual\n + '\n + \n ++test_expect_success 'create note from blob with \"-C\", also specify \"-m\", \"-F\" and \"--separator\"' '\n ++\t# 8th will be reuseed in following tests, so rollback when the test is done\n ++\ttest_when_finished \"git notes remove && git notes add -C $(cat blob)\" &&\n ++\tcommit=$(git rev-parse HEAD) &&\n ++\tcat >expect <<-EOF &&\n ++\t\tcommit $commit\n ++\t\tAuthor: A U Thor \n ++\t\tDate: Thu Apr 7 15:20:13 2005 -0700\n ++\n ++\t\t${indent}8th\n ++\n ++\t\tNotes:\n ++\t\t${indent}This is a blob object\n ++\t\t${indent}-------\n ++\t\t${indent}This is created by -m\n ++\t\t${indent}-------\n ++\t\t${indent}This is created by -F\n ++\tEOF\n ++\n ++\tgit notes remove &&\n ++\techo \"This is a blob object\" | git hash-object -w --stdin >blob &&\n ++\techo \"This is created by -F\" >note_a &&\n ++\tgit notes add -C $(cat blob) -m \"This is created by -m\" -F note_a --separator=\"-------\" &&\n ++\tgit log -1 >actual &&\n ++\ttest_cmp expect actual\n ++'\n ++\n + test_expect_success 'create note from other note with \"git notes add -c\"' '\n + \ttest_commit 9th &&\n + \tcommit=$(git rev-parse HEAD) &&\n-: -------- > 4: b9d12f0c notes.c: don't do stripespace when parse file arg\n-- \n2.40.0.rc2.4.gb9d12f0c\n\n"},{"id":"474313","messageId":"ea9c199ecf78fe8edaa157a51824b31bb983cfcb.1680012650.git.dyroneteng@gmail.com","threadId":"59295","inReplyTo":"cover.1680012650.git.dyroneteng@gmail.com","subject":"[PATCH v7 1/4] notes.c: cleanup 'strbuf_grow' call in 'append_edit'","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-03-28T14:28:44Z","receivedAt":"2023-03-28T14:29:03Z","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\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\nSigned-off-by: Teng Long \nHelped-by: Eric Sunshine \nHelped-by: Junio C Hamano \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.40.0.rc2.4.gb9d12f0c\n\n"},{"id":"474314","messageId":"9dc123c8f7a1cc2e7a146fb336939d6076369e2e.1680012650.git.dyroneteng@gmail.com","threadId":"59295","inReplyTo":"cover.1680012650.git.dyroneteng@gmail.com","subject":"[PATCH v7 2/4] notes.c: cleanup for \"designated init\"","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-03-28T14:28:45Z","receivedAt":"2023-03-28T14:29:11Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nThe \"struct note_data d = { 0, 0, NULL, STRBUF_INIT };\" style could be\nreplaced 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 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.40.0.rc2.4.gb9d12f0c\n\n"},{"id":"474315","messageId":"d1febf86d8471c24c9a045d6962fd86f63f414d5.1680012650.git.dyroneteng@gmail.com","threadId":"59295","inReplyTo":"cover.1680012650.git.dyroneteng@gmail.com","subject":"[PATCH v7 3/4] notes.c: introduce '--separator=' option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-03-28T14:28:46Z","receivedAt":"2023-03-28T14:29:13Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nWhen adding new notes or appending to an existing notes, we will\ninsert a blank line between the paragraphs, like:\n\n $ git notes add -m foo -m bar\n $ git notes show HEAD | cat\n foo\n\n bar\n\nThe default behavour sometimes is not enough, the user may want\nto use a custom delimiter between paragraphs, like when\nspecifiy '-m', '-F', '-C', '-c' options. So this commit\nintroduce a new '--separator' option for 'git notes add' and\n'git notes append', for example when execute:\n\n $ git notes add -m foo -m bar --separator=\"-\"\n $ git notes show HEAD | cat\n foo\n -\n bar\n\nA newline is added to the value given to --separator if it\ndoes not end with one already. So execute:\n\n $ git notes add -m foo -m bar --separator=\"-\"\nand\n $ export LF=\"\n \"\n $ git notes add -m foo -m bar --separator=\"-$LF\"\n\nRunning A and B produces the same result.\n\nSigned-off-by: Teng Long \n---\n Documentation/git-notes.txt | 21 ++++--\n builtin/notes.c | 105 ++++++++++++++++++++++-------\n t/t3301-notes.sh | 127 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 224 insertions(+), 29 deletions(-)\n\ndiff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\nindex efbc10f0..59980b21 100644\n--- a/Documentation/git-notes.txt\n+++ b/Documentation/git-notes.txt\n@@ -9,9 +9,9 @@ SYNOPSIS\n --------\n [verse]\n 'git notes' [list []]\n-'git notes' add [-f] [--allow-empty] [-F | -m | (-c | -C) ] []\n+'git notes' add [-f] [--allow-empty] [--separator=] [-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@@ -65,7 +65,9 @@ add::\n \tHowever, if you're using `add` interactively (using an editor\n \tto supply the notes contents), then - instead of aborting -\n \tthe existing notes will be opened in the editor (like the `edit`\n-\tsubcommand).\n+\tsubcommand). If you specify multiple `-m` and `-F`, a blank\n+\tline will be inserted between the messages. Use the `--separator`\n+\toption to insert other delimiters.\n \n copy::\n \tCopy the notes for the first object onto the second object (defaults to\n@@ -85,8 +87,12 @@ corresponding . (The optional `` is ignored so that\n 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+\tAppend new message(s) given by `-m` or `-F` options to an\n+\texisting note, or add them as a new note if one does not\n+\texist, for the object (defaults to HEAD). When appending to\n+\tan existing note, a blank line is added before each new\n+\tmessage as an inter-paragraph separator. The separator can\n+\tbe customized with the `--separator` option.\n \n edit::\n \tEdit the notes for a given object (defaults to HEAD).\n@@ -159,6 +165,11 @@ 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 a string used as a custom inter-paragraph separator\n+\t(a newline is added at the end as needed). Defaults to a\n+\tblank line.\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..94b69607 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -24,11 +24,12 @@\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+\tN_(\"git notes [--ref ] add [-f] [--allow-empty] [--separator=] [-m | -F | (-c | -C) ] []\"),\n \tN_(\"git notes [--ref ] copy [-f] \"),\n-\tN_(\"git notes [--ref ] append [--allow-empty] [-m | -F | (-c | -C) ] []\"),\n+\tN_(\"git notes [--ref ] append [--allow-empty] [--separator=] [-m | -F | (-c | -C) ] []\"),\n \tN_(\"git notes [--ref ] edit [--allow-empty] []\"),\n \tN_(\"git notes [--ref ] show []\"),\n \tN_(\"git notes [--ref ] merge [-v | -q] [-s ] \"),\n@@ -101,6 +102,9 @@ struct note_data {\n \tint use_editor;\n \tchar *edit_path;\n \tstruct strbuf buf;\n+\tstruct strbuf **messages;\n+\tsize_t msg_nr;\n+\tsize_t msg_alloc;\n };\n \n static void free_note_data(struct note_data *d)\n@@ -209,71 +213,110 @@ 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+/* Consume messages and append them into d->buf, then free them */\n+static void concat_messages(struct note_data *d)\n+{\n+\tstruct strbuf msg = STRBUF_INIT;\n+\n+\tsize_t i;\n+\tfor (i = 0; i < d->msg_nr ; i++) {\n+\t\tif (d->buf.len)\n+\t\t\tinsert_separator(&d->buf, d->buf.len);\n+\t\tstrbuf_add(&msg, d->messages[i]->buf, d->messages[i]->len);\n+\t\tstrbuf_addbuf(&d->buf, &msg);\n+\t\tstrbuf_reset(&msg);\n+\t\tstrbuf_release(d->messages[i]);\n+\t\tfree(d->messages[i]);\n+\t}\n+\tstrbuf_release(&msg);\n+\tfree(d->messages);\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+\tstruct strbuf *buf;\n \n \tBUG_ON_OPT_NEG(unset);\n \n-\tif (d->buf.len)\n-\t\tstrbuf_addch(&d->buf, '\\n');\n-\tstrbuf_addstr(&d->buf, arg);\n-\tstrbuf_stripspace(&d->buf, 0);\n+\tbuf = xmalloc(sizeof(*buf));\n+\tstrbuf_init(buf, strlen(arg));\n+\tstrbuf_addstr(buf, arg);\n+\tstrbuf_stripspace(buf, 0);\n \n-\td->given = 1;\n+\tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n+\td->messages[d->msg_nr - 1] = buf;\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+\tstruct strbuf *buf;\n \n \tBUG_ON_OPT_NEG(unset);\n \n-\tif (d->buf.len)\n-\t\tstrbuf_addch(&d->buf, '\\n');\n+\tbuf = xmalloc(sizeof(*buf));\n+\tstrbuf_init(buf, 0);\n+\n \tif (!strcmp(arg, \"-\")) {\n-\t\tif (strbuf_read(&d->buf, 0, 1024) < 0)\n+\t\tif (strbuf_read(buf, 0, 1024) < 0)\n \t\t\tdie_errno(_(\"cannot read '%s'\"), arg);\n-\t} else if (strbuf_read_file(&d->buf, arg, 1024) < 0)\n+\t} else if (strbuf_read_file(buf, arg, 1024) < 0)\n \t\tdie_errno(_(\"could not open or read '%s'\"), arg);\n-\tstrbuf_stripspace(&d->buf, 0);\n+\tstrbuf_stripspace(buf, 0);\n \n-\td->given = 1;\n+\t// we will note stripspace the buf here, because the file maybe\n+\t// is a binary and it maybe contains multiple continuous line breaks.\n+\tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n+\td->messages[d->msg_nr - 1] = buf;\n \treturn 0;\n }\n \n static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)\n {\n \tstruct note_data *d = opt->value;\n-\tchar *buf;\n+\tstruct strbuf *buf;\n \tstruct object_id object;\n \tenum object_type type;\n \tunsigned long len;\n \n \tBUG_ON_OPT_NEG(unset);\n \n-\tif (d->buf.len)\n-\t\tstrbuf_addch(&d->buf, '\\n');\n-\n+\tbuf = xmalloc(sizeof(*buf));\n \tif (get_oid(arg, &object))\n \t\tdie(_(\"failed to resolve '%s' as a valid ref.\"), arg);\n-\tif (!(buf = read_object_file(&object, &type, &len)))\n+\tif (!(buf->buf = read_object_file(&object, &type, &len)))\n \t\tdie(_(\"failed to read object '%s'.\"), arg);\n \tif (type != OBJ_BLOB) {\n+\t\tstrbuf_release(buf);\n \t\tfree(buf);\n \t\tdie(_(\"cannot read note data from non-blob object '%s'.\"), arg);\n \t}\n-\tstrbuf_add(&d->buf, buf, len);\n-\tfree(buf);\n \n-\td->given = 1;\n+\t// The reuse object maybe is a binary content which could\n+\t// contains '\\0' in the middle, so let's set the correct\n+\t// lenth of strbuf to concat all of content.\n+\tbuf->len = len;\n+\tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n+\td->messages[d->msg_nr - 1] = buf;\n \treturn 0;\n }\n \n static int parse_reedit_arg(const struct option *opt, const char *arg, int unset)\n {\n \tstruct note_data *d = opt->value;\n+\n \tBUG_ON_OPT_NEG(unset);\n \td->use_editor = 1;\n \treturn parse_reuse_arg(opt, arg, unset);\n@@ -401,7 +444,8 @@ 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 = { .buf = STRBUF_INIT };\n+\tstruct note_data d = { .buf = STRBUF_INIT, .messages = NULL };\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@@ -418,6 +462,8 @@ static int add(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n \t\t\tN_(\"allow storing empty note\")),\n \t\tOPT__FORCE(&force, N_(\"replace existing notes\"), PARSE_OPT_NOCOMPLETE),\n+\t\tOPT_STRING(0, \"separator\", &separator, N_(\"separator\"),\n+\t\t\tN_(\"insert between paragraphs\")),\n \t\tOPT_END()\n \t};\n \n@@ -429,6 +475,10 @@ static int add(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_notes_add_usage, options);\n \t}\n \n+\tif (d.msg_nr)\n+\t\tconcat_messages(&d);\n+\td.given = !!d.buf.len;\n+\n \tobject_ref = argc > 1 ? argv[1] : \"HEAD\";\n \n \tif (get_oid(object_ref, &object))\n@@ -567,7 +617,8 @@ 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 = { .buf = STRBUF_INIT };\n+\tstruct note_data d = { .buf = STRBUF_INIT, .messages = NULL };\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@@ -583,6 +634,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 between paragraphs\")),\n \t\tOPT_END()\n \t};\n \tint edit = !strcmp(argv[0], \"edit\");\n@@ -596,6 +649,10 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(usage, options);\n \t}\n \n+\tif (d.msg_nr)\n+\t\tconcat_messages(&d);\n+\td.given = !!d.buf.len;\n+\n \tif (d.given && edit)\n \t\tfprintf(stderr, _(\"The -m/-F/-c/-C options have been deprecated \"\n \t\t\t\"for the 'edit' subcommand.\\n\"\n@@ -618,7 +675,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..716192b5 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -5,6 +5,7 @@\n \n test_description='Test commit notes'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n write_script fake_editor <<\\EOF\n@@ -362,6 +363,7 @@ test_expect_success 'do not create empty note with -m \"\"' '\n '\n \n test_expect_success 'create note with combination of -m and -F' '\n+\ttest_when_finished git notes remove HEAD &&\n \tcat >expect-combine_m_and_F <<-EOF &&\n \t\tfoo\n \n@@ -380,6 +382,25 @@ test_expect_success 'create note with combination of -m and -F' '\n \ttest_cmp expect-combine_m_and_F actual\n '\n \n+test_expect_success 'create note with combination of -m and -F and --separator' '\n+\tcat >expect-combine_m_and_F <<-\\EOF &&\n+\tfoo\n+\t-------\n+\txyzzy\n+\t-------\n+\tbar\n+\t-------\n+\tzyxxy\n+\t-------\n+\tbaz\n+\tEOF\n+\techo \"xyzzy\" >note_a &&\n+\techo \"zyxxy\" >note_b &&\n+\tgit notes add -m \"foo\" -F note_a -m \"bar\" -F note_b -m \"baz\" --separator \"-------\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect-combine_m_and_F actual\n+'\n+\n test_expect_success 'remove note with \"git notes remove\"' '\n \tgit notes remove HEAD^ &&\n \tgit notes remove &&\n@@ -521,6 +542,85 @@ 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 separator 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 note with combination of -m and -F and --separator' '\n+\ttest_when_finished git notes remove HEAD &&\n+\tcat >expect-combine_m_and_F <<-\\EOF &&\n+\tm-notes-1\n+\t-------\n+\tf-notes-1\n+\t-------\n+\tm-notes-2\n+\t-------\n+\tf-notes-2\n+\t-------\n+\tm-notes-3\n+\tEOF\n+\n+\techo \"f-notes-1\" >note_a &&\n+\techo \"f-notes-2\" >note_b &&\n+\tgit notes append -m \"m-notes-1\" -F note_a -m \"m-notes-2\" -F note_b -m \"m-notes-3\" --separator \"-------\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect-combine_m_and_F 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@@ -818,6 +918,33 @@ test_expect_success 'create note from blob with \"git notes add -C\" reuses blob i\n \ttest_cmp blob actual\n '\n \n+test_expect_success 'create note from blob with \"-C\", also specify \"-m\", \"-F\" and \"--separator\"' '\n+\t# 8th will be reuseed in following tests, so rollback when the test is done\n+\ttest_when_finished \"git notes remove && git notes add -C $(cat blob)\" &&\n+\tcommit=$(git rev-parse HEAD) &&\n+\tcat >expect <<-EOF &&\n+\t\tcommit $commit\n+\t\tAuthor: A U Thor \n+\t\tDate: Thu Apr 7 15:20:13 2005 -0700\n+\n+\t\t${indent}8th\n+\n+\t\tNotes:\n+\t\t${indent}This is a blob object\n+\t\t${indent}-------\n+\t\t${indent}This is created by -m\n+\t\t${indent}-------\n+\t\t${indent}This is created by -F\n+\tEOF\n+\n+\tgit notes remove &&\n+\techo \"This is a blob object\" | git hash-object -w --stdin >blob &&\n+\techo \"This is created by -F\" >note_a &&\n+\tgit notes add -C $(cat blob) -m \"This is created by -m\" -F note_a --separator=\"-------\" &&\n+\tgit log -1 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'create note from other note with \"git notes add -c\"' '\n \ttest_commit 9th &&\n \tcommit=$(git rev-parse HEAD) &&\n-- \n2.40.0.rc2.4.gb9d12f0c\n\n"},{"id":"474316","messageId":"b9d12f0c5f31d11f89993e476d461fe3f13de705.1680012650.git.dyroneteng@gmail.com","threadId":"59295","inReplyTo":"cover.1680012650.git.dyroneteng@gmail.com","subject":"[PATCH v7 4/4] notes.c: don't do stripespace when parse file arg","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-03-28T14:28:47Z","receivedAt":"2023-03-28T14:29:15Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nThe file maybe is a binary and it could contains multiple line\nbreaks, if we do the stripespace on it's content, the notes will\nbe different to the original content.\n\nThe binary file as the notes maybe a rare scenario, but there're\nsome related tests about it in t3307, so let's fix the problem\nand add a test in t3301.\n\nSigned-off-by: Teng Long \n---\n builtin/notes.c | 1 -\n t/t3301-notes.sh | 10 +++++++++-\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 94b69607..ade8fb73 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -274,7 +274,6 @@ static int parse_file_arg(const struct option *opt, const char *arg, int unset)\n \t\t\tdie_errno(_(\"cannot read '%s'\"), arg);\n \t} else if (strbuf_read_file(buf, arg, 1024) < 0)\n \t\tdie_errno(_(\"could not open or read '%s'\"), arg);\n-\tstrbuf_stripspace(buf, 0);\n \n \t// we will note stripspace the buf here, because the file maybe\n \t// is a binary and it maybe contains multiple continuous line breaks.\ndiff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\nindex 716192b5..2af318f5 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -192,8 +192,16 @@ test_expect_success 'show multi-line notes' '\n \ttest_cmp expect-multiline actual\n '\n \n-test_expect_success 'show -F notes' '\n+test_expect_success 'add -F notes with binary' '\n+\ttest_when_finished \"git notes remove\" &&\n \ttest_commit 4th &&\n+\tcp \"$TEST_DIRECTORY\"/test-binary-1.png . &&\n+\tgit notes add -F test-binary-1.png &&\n+\tgit notes show >actual &&\n+\ttest_cmp test-binary-1.png actual\n+'\n+\n+test_expect_success 'show -F notes' '\n \techo \"xyzzy\" >note5 &&\n \tgit notes add -F note5 &&\n \tcommit=$(git rev-parse HEAD) &&\n-- \n2.40.0.rc2.4.gb9d12f0c\n\n"},{"id":"474319","messageId":"xmqq5yakhoo9.fsf@gitster.g","threadId":"59295","inReplyTo":"d1febf86d8471c24c9a045d6962fd86f63f414d5.1680012650.git.dyroneteng@gmail.com","subject":"Re: [PATCH v7 3/4] notes.c: introduce '--separator=' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-28T15:37:58Z","receivedAt":"2023-03-28T15:39:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n> From: Teng Long \n>\n> When adding new notes or appending to an existing notes, we will\n> insert a blank line between the paragraphs, like:\n>\n> $ git notes add -m foo -m bar\n> $ git notes show HEAD | cat\n> foo\n>\n> bar\n>\n> The default behavour sometimes is not enough, the user may want\n> to use a custom delimiter between paragraphs, like when\n> specifiy '-m', '-F', '-C', '-c' options. So this commit\n\n\"like when specifying\", you mean?\n\n> introduce a new '--separator' option for 'git notes add' and\n> 'git notes append', for example when execute:\n\n\"when executing\"?\n\n> $ git notes add -m foo -m bar --separator=\"-\"\n> $ git notes show HEAD | cat\n> foo\n> -\n> bar\n>\n> A newline is added to the value given to --separator if it\n\n\"a newline is ...\", because this is not starting a new sentence, but\ncontinuing from the \"for example, when ...\" above.\n\n> does not end with one already. So execute:\n>\n> $ git notes add -m foo -m bar --separator=\"-\"\n> and\n> $ export LF=\"\n> \"\n> $ git notes add -m foo -m bar --separator=\"-$LF\"\n>\n> Running A and B produces the same result.\n\nHere, it is totally unclear what A and B refers to. \"both\n... and ... produce the same result\" or something?\n\n> @@ -85,8 +87,12 @@ corresponding . (The optional `` is ignored so that\n> 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> +\tAppend new message(s) given by `-m` or `-F` options to an\n> +\texisting note, or add them as a new note if one does not\n> +\texist, for the object (defaults to HEAD). When appending to\n> +\tan existing note, a blank line is added before each new\n> +\tmessage as an inter-paragraph separator. The separator can\n> +\tbe customized with the `--separator` option.\n\nExcellent.\n\n> @@ -101,6 +102,9 @@ struct note_data {\n> \tint use_editor;\n> \tchar *edit_path;\n> \tstruct strbuf buf;\n> +\tstruct strbuf **messages;\n> +\tsize_t msg_nr;\n> +\tsize_t msg_alloc;\n> };\n\nHmph, OK. I'd think twice before allowing an array of strbufs,\nthough. strbuf is an excellent data structure to allow editing\nstring safely, and an array of strbufs may be very useful when these\nstrings need to be edited simultaneously, but such a use case is\nvery rare and I suspect this would not be it---rather, don't we take\none string at a time while processing each option, perhaps running\nstripspace, and then after that aren't we done with the string until\nwhen we finally have these N strings to loop over and concatenate\ninto a single string? It would be sensible to use a strbuf to\nproduce the concatenation, but the strings to be concatenated do not\nhave to come from strbufs. So my intuition, before reading the code\nbut after seeing the data structure, says that \"const char **messages\"\nmight be more appropriate here. It sends a strong message about the\ncode structure and \"phases\" of processing, i.e. \"we read each piece\nand store it in the array once we are done preprocessing; then in\nthe next phase we concatenate them\".\n\n> @@ -209,71 +213,110 @@ 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\nDo we need \"insert\" separator? The caller in \"concat\" certainly\nshouldn't---all it needs is \"append\".\n\n> +/* Consume messages and append them into d->buf, then free them */\n> +static void concat_messages(struct note_data *d)\n> +{\n> +\tstruct strbuf msg = STRBUF_INIT;\n> +\n> +\tsize_t i;\n> +\tfor (i = 0; i < d->msg_nr ; i++) {\n> +\t\tif (d->buf.len)\n> +\t\t\tinsert_separator(&d->buf, d->buf.len);\n> +\t\tstrbuf_add(&msg, d->messages[i]->buf, d->messages[i]->len);\n> +\t\tstrbuf_addbuf(&d->buf, &msg);\n> +\t\tstrbuf_reset(&msg);\n> +\t\tstrbuf_release(d->messages[i]);\n> +\t\tfree(d->messages[i]);\n> +\t}\n> +\tstrbuf_release(&msg);\n> +\tfree(d->messages);\n> +}\n\nAs I suspected earlier, with the way d->messages[i] are used, they\nhave no reason to be strbufs---they can and should be a simple\nstring \"const char *\".\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> +\tstruct strbuf *buf;\n> \n> \tBUG_ON_OPT_NEG(unset);\n> \n> -\tif (d->buf.len)\n> -\t\tstrbuf_addch(&d->buf, '\\n');\n> -\tstrbuf_addstr(&d->buf, arg);\n> -\tstrbuf_stripspace(&d->buf, 0);\n> +\tbuf = xmalloc(sizeof(*buf));\n> +\tstrbuf_init(buf, strlen(arg));\n> +\tstrbuf_addstr(buf, arg);\n> +\tstrbuf_stripspace(buf, 0);\n> \n> -\td->given = 1;\n> +\tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n> +\td->messages[d->msg_nr - 1] = buf;\n> \treturn 0;\n> }\n\nAnd this one can use an on-stack strbuf (initialized with STRBUF_INIT),\nand strbuf_detach() the result into d->messages[].\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> +\tstruct strbuf *buf;\n\nLikewise.\n\n> \n> \tBUG_ON_OPT_NEG(unset);\n> \n> -\tif (d->buf.len)\n> -\t\tstrbuf_addch(&d->buf, '\\n');\n> +\tbuf = xmalloc(sizeof(*buf));\n> +\tstrbuf_init(buf, 0);\n> +\n> \tif (!strcmp(arg, \"-\")) {\n> -\t\tif (strbuf_read(&d->buf, 0, 1024) < 0)\n> +\t\tif (strbuf_read(buf, 0, 1024) < 0)\n> \t\t\tdie_errno(_(\"cannot read '%s'\"), arg);\n> -\t} else if (strbuf_read_file(&d->buf, arg, 1024) < 0)\n> +\t} else if (strbuf_read_file(buf, arg, 1024) < 0)\n> \t\tdie_errno(_(\"could not open or read '%s'\"), arg);\n> -\tstrbuf_stripspace(&d->buf, 0);\n> +\tstrbuf_stripspace(buf, 0);\n> \n> -\td->given = 1;\n> +\t// we will note stripspace the buf here, because the file maybe\n> +\t// is a binary and it maybe contains multiple continuous line breaks.\n\nNo // comments in our codebase.\n\n> +\tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n> +\td->messages[d->msg_nr - 1] = buf;\n> \treturn 0;\n> }\n\n> static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)\n> {\n> ...\n> }\n\nDitto.\n\n> @@ -567,7 +617,8 @@ 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 = { .buf = STRBUF_INIT };\n> +\tstruct note_data d = { .buf = STRBUF_INIT, .messages = NULL };\n\nWhy explicitly initialize .messages to NULL when we are leaving\nother members to zero initialized implicitly by using designated\ninitializers?\n\n> @@ -618,7 +675,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);\n\nHaving to insert two things in front of d.buf feels somewhat\nwasteful. I wonder if we can easily reorder the logic to first read\nthe previous one, and then append new stuff to it, perhaps?\n\nIt wouldn't be a huge deal. If this weren't the only reason why we\nneed to invent insertstr that takes \"where\", I wouldn't even be\nmentioning it.\n\n> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\n> index 3288aaec..716192b5 100755\n> --- a/t/t3301-notes.sh\n> +++ b/t/t3301-notes.sh\n> @@ -5,6 +5,7 @@\n> \n> test_description='Test commit notes'\n> \n> +TEST_PASSES_SANITIZE_LEAK=true\n> . ./test-lib.sh\n\nThis is a strange commit to add this. If the test and the code\ninvolved in the test were leak-sanitizer clean before this commit,\nthen you should have been able to insert this without any other\nchange, and we should do it that way in a separate commit. If we\nare fixing an existing leak that the sanitizer complains, then that\nis a different matter. If that is the case, it makes perfect sense\nto have this here, but the proposed commit log message should\nexplain that it is what is happening.\n\nOther than that, looks very nicely done.\n\nThanks.\n"},{"id":"474320","messageId":"xmqqsfdog9d1.fsf@gitster.g","threadId":"59295","inReplyTo":"b9d12f0c5f31d11f89993e476d461fe3f13de705.1680012650.git.dyroneteng@gmail.com","subject":"Re: [PATCH v7 4/4] notes.c: don't do stripespace when parse file arg","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-28T15:54:02Z","receivedAt":"2023-03-28T15:54:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n> From: Teng Long \n>\n> The file maybe is a binary and it could contains multiple line\n\n\"could contain\"?\n\n> breaks, if we do the stripespace on it's content, the notes will\n\n\"stripspace\"?\n\n> be different to the original content.\n\nIf a file is \"binary\" then by definition there is no \"line\" and no\n\"line break\" in it, so while I can see what you are trying to say,\nthis needs a bit rephrasing to make it easier to follow for future\nreaders. Perhaps something like...\n\n The file given to the \"-F\" option could be binary, in which case\n we want to store it verbatim without corrupting its contents.\n\nBut I do not necessarily agree with the logic here, regardless of\nhow it is phrased. Existing users are used to seeing the contents\nfed from \"-F \" get cleaned up, and to them, this unconditional\nremoval of stripspace() will be a regression. Besides, don't they\nexpect that \n\n\t-m=\"$(cat file)\"\n\nbe equivalent to\n\n\t-F file\n\nfor their text file?\n\nA --binary-file= option that is incompatible with -m/-F and\n\"append\" action may make sense if we really want to have a feature\nto help those who want to attach binary contents as a note to\nobjects.\n\n"},{"id":"474405","messageId":"20230329120613.11415-1-tenglong.tl@alibaba-inc.com","threadId":"59295","inReplyTo":"xmqqsfdog9d1.fsf@gitster.g","subject":"Re: [PATCH v7 4/4] notes.c: don't do stripespace when parse file arg","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-03-29T12:06:13Z","receivedAt":"2023-03-29T12:06:49Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Junio C Hamano writes:\n\n> > The file maybe is a binary and it could contains multiple line\n>\n> \"could contain\"?\n\nWill fix.\n\n> > breaks, if we do the stripespace on it's content, the notes will\n>\n> \"stripspace\"?\n\nWill fix, also in the commit subject.\n\n> If a file is \"binary\" then by definition there is no \"line\" and no\n> \"line break\" in it, so while I can see what you are trying to say,\n> this needs a bit rephrasing to make it easier to follow for future\n> readers. Perhaps something like...\n>\n> The file given to the \"-F\" option could be binary, in which case\n> we want to store it verbatim without corrupting its contents.\n\nThanks, the phrasing is obviously better.\n\n> But I do not necessarily agree with the logic here, regardless of\n> how it is phrased. Existing users are used to seeing the contents\n> fed from \"-F \" get cleaned up, and to them, this unconditional\n> removal of stripspace() will be a regression. Besides, don't they\n> expect that\n>\n> \t-m=\"$(cat file)\"\n>\n> be equivalent to\n>\n> \t-F file\n\n> for their text file?\n\nYes, for text file. I agree and I actually notice it will be\na regression, because I didn't figure out when user might\nstore a binary file's content in notes, so I'd like to discuss\nabout this issue.\n\n> A --binary-file= option that is incompatible with -m/-F and\n> \"append\" action may make sense if we really want to have a feature\n> to help those who want to attach binary contents as a note to\n> objects.\n\nActually, -m/-F/-c/-C in 'git notes add' is quite like the\noptions in 'git commit', rather than \"--binary-file=\", I think\nmaybe '--[no-]stripspace' is a bit better. Defaults to\n'--stripspace' which keeps old logic (stripspace the string fed by\n-m/-F). If specifying '--no-stripspace', we do not stripespace.\n\nThanks.\n"},{"id":"474408","messageId":"20230329141559.14244-1-tenglong.tl@alibaba-inc.com","threadId":"59295","inReplyTo":"xmqq5yakhoo9.fsf@gitster.g","subject":"Re: [PATCH v7 3/4] notes.c: introduce '--separator=' option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-03-29T14:15:59Z","receivedAt":"2023-03-29T14:20:00Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Junio C Hamano writes:\n\n> > The default behavour sometimes is not enough, the user may want\n> > to use a custom delimiter between paragraphs, like when\n> > specifiy '-m', '-F', '-C', '-c' options. So this commit\n>\n> \"like when specifying\", you mean?\n>\n> \"when executing\"?\n>\n> > $ git notes add -m foo -m bar --separator=\"-\"\n> > $ git notes show HEAD | cat\n> > foo\n> > -\n> > bar\n> >\n> > A newline is added to the value given to --separator if it\n>\n> \"a newline is ...\", because this is not starting a new sentence, but\n> continuing from the \"for example, when ...\" above.\n>\n> \"a newline is ...\", because this is not starting a new sentence, but\n> continuing from the \"for example, when ...\" above.\n>\n> > does not end with one already. So execute:\n> >\n> > $ git notes add -m foo -m bar --separator=\"-\"\n> > and\n> > $ export LF=\"\n> > \"\n> > $ git notes add -m foo -m bar --separator=\"-$LF\"\n> >\n> > Running A and B produces the same result.\n>\n> Here, it is totally unclear what A and B refers to. \"both\n> ... and ... produce the same result\" or something?\n\nwill fix, thanks.\n\n> > @@ -101,6 +102,9 @@ struct note_data {\n> > \tint use_editor;\n> > \tchar *edit_path;\n> > \tstruct strbuf buf;\n> > +\tstruct strbuf **messages;\n> > +\tsize_t msg_nr;\n> > +\tsize_t msg_alloc;\n> > };\n>\n> Hmph, OK. I'd think twice before allowing an array of strbufs,\n> though. strbuf is an excellent data structure to allow editing\n> string safely, and an array of strbufs may be very useful when these\n> strings need to be edited simultaneously, but such a use case is\n> very rare and I suspect this would not be it---rather, don't we take\n> one string at a time while processing each option, perhaps running\n> stripspace, and then after that aren't we done with the string until\n> when we finally have these N strings to loop over and concatenate\n> into a single string? It would be sensible to use a strbuf to\n> produce the concatenation, but the strings to be concatenated do not\n> have to come from strbufs. So my intuition, before reading the code\n> but after seeing the data structure, says that \"const char **messages\"\n> might be more appropriate here. It sends a strong message about the\n> code structure and \"phases\" of processing, i.e. \"we read each piece\n> and store it in the array once we are done preprocessing; then in\n> the next phase we concatenate them\".\n\nI used string in previous patch, but I found there will be a problem when\nspecify a binary file to '-F', that is, a binary file maybe contains NUL in\nthe middle, then if we feed the content to a string, it will be cut off when\nappending to the 'string_list'. So, it seems that strbuf could solve the\nissue (will be reproduced as a failure in t3307).\n\n> > @@ -209,71 +213,110 @@ 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> Do we need \"insert\" separator? The caller in \"concat\" certainly\n> shouldn't---all it needs is \"append\".\n\nOK, I'm willing to try to do that, \"insert at a position\" actually because\nthere is a related codepath need to add the separator in the very beginning.\n\n> > +/* Consume messages and append them into d->buf, then free them */\n> > +static void concat_messages(struct note_data *d)\n> > +{\n> > +\tstruct strbuf msg = STRBUF_INIT;\n> > +\n> > +\tsize_t i;\n> > +\tfor (i = 0; i < d->msg_nr ; i++) {\n> > +\t\tif (d->buf.len)\n> > +\t\t\tinsert_separator(&d->buf, d->buf.len);\n> > +\t\tstrbuf_add(&msg, d->messages[i]->buf, d->messages[i]->len);\n> > +\t\tstrbuf_addbuf(&d->buf, &msg);\n> > +\t\tstrbuf_reset(&msg);\n> > +\t\tstrbuf_release(d->messages[i]);\n> > +\t\tfree(d->messages[i]);\n> > +\t}\n> > +\tstrbuf_release(&msg);\n> > +\tfree(d->messages);\n> > +}\n>\n> As I suspected earlier, with the way d->messages[i] are used, they\n> have no reason to be strbufs---they can and should be a simple\n> string \"const char *\".\n\nAs I said about why I choose strbuf in the above, but I expect your\nfurther advice on that.\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> > +\tstruct strbuf *buf;\n> >\n> > \tBUG_ON_OPT_NEG(unset);\n> >\n> > -\tif (d->buf.len)\n> > -\t\tstrbuf_addch(&d->buf, '\\n');\n> > -\tstrbuf_addstr(&d->buf, arg);\n> > -\tstrbuf_stripspace(&d->buf, 0);\n> > +\tbuf = xmalloc(sizeof(*buf));\n> > +\tstrbuf_init(buf, strlen(arg));\n> > +\tstrbuf_addstr(buf, arg);\n> > +\tstrbuf_stripspace(buf, 0);\n> >\n> > -\td->given = 1;\n> > +\tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n> > +\td->messages[d->msg_nr - 1] = buf;\n> > \treturn 0;\n> > }\n>\n> And this one can use an on-stack strbuf (initialized with STRBUF_INIT),\n> and strbuf_detach() the result into d->messages[].\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> > +\tstruct strbuf *buf;\n\n> Likewise.\n\nFor the same reason as above I think.\n\n> >\n> > \tBUG_ON_OPT_NEG(unset);\n> >\n> > -\tif (d->buf.len)\n> > -\t\tstrbuf_addch(&d->buf, '\\n');\n> > +\tbuf = xmalloc(sizeof(*buf));\n> > +\tstrbuf_init(buf, 0);\n> > +\n> > \tif (!strcmp(arg, \"-\")) {\n> > -\t\tif (strbuf_read(&d->buf, 0, 1024) < 0)\n> > +\t\tif (strbuf_read(buf, 0, 1024) < 0)\n> > \t\t\tdie_errno(_(\"cannot read '%s'\"), arg);\n> > -\t} else if (strbuf_read_file(&d->buf, arg, 1024) < 0)\n> > +\t} else if (strbuf_read_file(buf, arg, 1024) < 0)\n> > \t\tdie_errno(_(\"could not open or read '%s'\"), arg);\n> > -\tstrbuf_stripspace(&d->buf, 0);\n> > +\tstrbuf_stripspace(buf, 0);\n> >\n> > -\td->given = 1;\n> > +\t// we will note stripspace the buf here, because the file maybe\n> > +\t// is a binary and it maybe contains multiple continuous line breaks.\n>\n> No // comments in our codebase.\n\nWill fix.\n\n> > @@ -567,7 +617,8 @@ 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 = { .buf = STRBUF_INIT };\n> > +\tstruct note_data d = { .buf = STRBUF_INIT, .messages = NULL };\n>\n> Why explicitly initialize .messages to NULL when we are leaving\n> other members to zero initialized implicitly by using designated\n> initializers?\n\nMy bad, will fix.\n\n> > @@ -618,7 +675,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);\n>\n> Having to insert two things in front of d.buf feels somewhat\n> wasteful. ...\n\n\"wasteful\" for?\n\n> ... I wonder if we can easily reorder the logic to first read\n> the previous one, and then append new stuff to it, perhaps?\n\nI think it can be done in this way, but in another single commit, perhaps?\n\n> It wouldn't be a huge deal. If this weren't the only reason why we\n> need to invent insertstr that takes \"where\", I wouldn't even be\n> mentioning it.\n\nThis makes me think that there is a need for a method 'insert_separator'? For\nexample, to initialize 'separator' and then use 'strbuf_insertstr' directly?\nI'm not sure, but it feels like it could work.\n\n\n> > diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\n> > index 3288aaec..716192b5 100755\n> > --- a/t/t3301-notes.sh\n> > +++ b/t/t3301-notes.sh\n> > @@ -5,6 +5,7 @@\n> >\n> > test_description='Test commit notes'\n> >\n> > +TEST_PASSES_SANITIZE_LEAK=true\n> > . ./test-lib.sh\n>\n> This is a strange commit to add this. If the test and the code\n> involved in the test were leak-sanitizer clean before this commit,\n> then you should have been able to insert this without any other\n> change, and we should do it that way in a separate commit. If we\n> are fixing an existing leak that the sanitizer complains, then that\n> is a different matter. If that is the case, it makes perfect sense\n> to have this here, but the proposed commit log message should\n> explain that it is what is happening.\n\nWill fix.\n\nThanks.\n"},{"id":"474412","messageId":"xmqqo7obbka3.fsf@gitster.g","threadId":"59295","inReplyTo":"20230329120613.11415-1-tenglong.tl@alibaba-inc.com","subject":"Re: [PATCH v7 4/4] notes.c: don't do stripespace when parse file arg","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-29T16:21:40Z","receivedAt":"2023-03-29T16:21:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n> Actually, -m/-F/-c/-C in 'git notes add' is quite like the\n> options in 'git commit', rather than \"--binary-file=\", I think\n> maybe '--[no-]stripspace' is a bit better. Defaults to\n> '--stripspace' which keeps old logic (stripspace the string fed by\n> -m/-F). If specifying '--no-stripspace', we do not stripespace.\n\nIt is a fairly easy way to both implement and explain. The approach\nis certainly attractive from the \"expectation management\" point of\nview.\n\nWith \"git notes add --no-stripspace -F file0 -F file1 ...\", we may\nstill be making the resulting note unusable as a binary file due to\nsplicing the inter-paragraph separator, but the option name does not\nhint that we are giving this option to users to help them use binary\ncontents at all, to prevent them from complaining. If it is truly\nuseful is a separate matter, but I somehow suspect that there will\nbe a lot more users who want \"no stripspace on text contents\" than\nthose who want \"binary contents in note\", so it probably is a good\nway to go after all. I like that.\n\nBefore proceeding further on this front, we'd probably need a bit\nmore tests that are specifically designed to catch behaviour change\nbefore and after this series with respect to stripspace behaviour.\nThe current code without these patches calls stripspace() on the\nwhole thing every time it receives and appends an additional piece\nof contents. The parse_msg_arg() function, for example, handles \"-m\n\" by first appending a blank line (if we\nalready have some contents) and then appending the new message, and\nruns stripspace on the whole thing. \"-F \" is handled by\nparse_file_arg() the same way, i.e. append the file data and then\nstripspace the result. This can of course be optimized somewhat\n(e.g. an early part of the buffer may have already been cleansed by\nstripspace before appending new data, so as long as we do not\nintroduce double-blanks and other things that are to be corrected at\nthe boundary of appending new data, we shouldn't have to run\nstripspace on the earlier part in order to behave \"as if we did a\nstripspace the whole thing over and over again\"), but we should make\nsure we do not change the behaviour to cause regression. After\ndoing something silly like this:\n\n cat >file1 <<-\\EOF &&\n\n With a leading blank line, this is the second line of the file.\n And with a trailing blank line, this is the second from the last.\n\n EOF\n git notes add -m '\n Message from the command line.\n\n\n Two blank lines above.' -F file1 \n\nwe should see the leading blank line before \"Message from the\ncommand line.\" removed, two blank lines from the \"-m\" option\nsqueezed into one, only one blank line to appear before the contents\ntaken from file1, and the trailing blank line removed.\n\nThanks.\n"},{"id":"474425","messageId":"xmqqy1nf8c0f.fsf@gitster.g","threadId":"59295","inReplyTo":"20230329141559.14244-1-tenglong.tl@alibaba-inc.com","subject":"Re: [PATCH v7 3/4] notes.c: introduce '--separator=' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-29T21:48:32Z","receivedAt":"2023-03-29T21:48:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n>> > diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\n>> > index 3288aaec..716192b5 100755\n>> > --- a/t/t3301-notes.sh\n>> > +++ b/t/t3301-notes.sh\n>> > @@ -5,6 +5,7 @@\n>> >\n>> > test_description='Test commit notes'\n>> >\n>> > +TEST_PASSES_SANITIZE_LEAK=true\n>> > . ./test-lib.sh\n>>\n>> This is a strange commit to add this. If the test and the code\n>> involved in the test were leak-sanitizer clean before this commit,\n>> then you should have been able to insert this without any other\n>> change, and we should do it that way in a separate commit. If we\n>> are fixing an existing leak that the sanitizer complains, then that\n>> is a different matter. If that is the case, it makes perfect sense\n>> to have this here, but the proposed commit log message should\n>> explain that it is what is happening.\n>\n> Will fix.\n\nThanks.\n\nJust FYI, it seems that it is indeed premature to declare that this\none to be leak-free. With this topic merged to 'seen', the leak\nchecker CI job seems to choke on t3301 (in addition, it also barfs\non t3307).\n\nhttps://github.com/git/git/actions/runs/4557669485/jobs/8039614419#step:4:1817\n\n"},{"id":"474431","messageId":"xmqqpm8r6w3q.fsf@gitster.g","threadId":"59295","inReplyTo":"9dc123c8f7a1cc2e7a146fb336939d6076369e2e.1680012650.git.dyroneteng@gmail.com","subject":"Re: [PATCH v7 2/4] notes.c: cleanup for \"designated init\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-29T22:17:29Z","receivedAt":"2023-03-29T22:17:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n> From: Teng Long \n>\n> The \"struct note_data d = { 0, 0, NULL, STRBUF_INIT };\" style could be\n> replaced with designated init format.\n\nI think our codebase calls it \"designated initializer\".\n\n notes.c: use designated initializers for clarity\n \n The \"struct note_data d = { 0, 0, NULL, STRBUF_INIT };\" style could be\n replaced with designated initializer for clarity.\n \n Signed-off-by: Teng Long \n Signed-off-by: Junio C Hamano \n"},{"id":"475270","messageId":"20230413093652.75678-1-tenglong.tl@alibaba-inc.com","threadId":"59295","inReplyTo":"xmqqy1nf8c0f.fsf@gitster.g","subject":"Re: [PATCH v7 3/4] notes.c: introduce '--separator=' option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-04-13T09:36:52Z","receivedAt":"2023-04-13T09:37:05Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Junio C Hamano writes:\n\n> Just FYI, it seems that it is indeed premature to declare that this\n> one to be leak-free. With this topic merged to 'seen', the leak\n> checker CI job seems to choke on t3301 (in addition, it also barfs\n> on t3307).\n\nYes, it contains a leak in function \"parse_reuse_arg()\", will fix.\n\nThanks.\n"},{"id":"476021","messageId":"cover.1682429602.git.dyroneteng@gmail.com","threadId":"59295","inReplyTo":"cover.1680012650.git.dyroneteng@gmail.com","subject":"[PATCH 0/6] notes.c: introduce \"--separator\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-04-25T13:34:35Z","receivedAt":"2023-04-25T13:35:28Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nDiff since v7:\n\n1. Fix problems that Junio's replies in last patch, except:\n\n * array of strbufs -> string_list (in [1]): the message may from\n a binary file which may contain '\\0' in middle, string will be\n cut off then make a unexpected result.\n\n2. New commit [3/6] for adding test cases about stripspace behavior.\n3. New commit [6/6] for supporting \"--[no-]stripspace\".\n\n[1] https://public-inbox.org/git/xmqq5yakhoo9.fsf@gitster.g/\n\nThanks.\n\nTeng Long (6):\n notes.c: cleanup 'strbuf_grow' call in 'append_edit'\n notes.c: use designated initializers for clarity\n t3321: add test cases about the notes stripspace behavior\n notes.c: introduce '--separator=' option\n notes.c: append separator instead of insert by pos\n notes.c: introduce \"--[no-]stripspace\" option\n\n Documentation/git-notes.txt | 42 ++-\n builtin/notes.c | 144 +++++++---\n t/t3301-notes.sh | 126 +++++++++\n t/t3321-notes-stripspace.sh | 521 ++++++++++++++++++++++++++++++++++++\n 4 files changed, 791 insertions(+), 42 deletions(-)\n create mode 100755 t/t3321-notes-stripspace.sh\n\nRange-diff against v7:\n1: ea9c199e ! 1: 0634434e notes.c: cleanup 'strbuf_grow' call in 'append_edit'\n @@ builtin/notes.c: static int parse_msg_arg(const struct option *opt, const char *\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);\n + \t\tchar *prev_buf = repo_read_object_file(the_repository, note,\n + \t\t\t\t\t\t &type, &size);\n \n -\t\tstrbuf_grow(&d.buf, size + 1);\n \t\tif (d.buf.len && prev_buf && size)\n2: 9dc123c8 ! 2: 4ad78405 notes.c: cleanup for \"designated init\"\n @@ Metadata\n Author: Teng Long \n \n ## Commit message ##\n - notes.c: cleanup for \"designated init\"\n + notes.c: use designated initializers for clarity\n \n The \"struct note_data d = { 0, 0, NULL, STRBUF_INIT };\" style could be\n - replaced with designated init format.\n + replaced with designated initializer for clarity.\n \n Signed-off-by: Teng Long \n + Signed-off-by: Junio C Hamano \n \n ## builtin/notes.c ##\n @@ builtin/notes.c: static int add(int argc, const char **argv, const char *prefix)\n-: -------- > 3: 6dfb5bf2 t3321: add test cases about the notes stripspace behavior\n3: d1febf86 ! 4: be86f9ca notes.c: introduce '--separator=' option\n @@ Commit message\n \n The default behavour sometimes is not enough, the user may want\n to use a custom delimiter between paragraphs, like when\n - specifiy '-m', '-F', '-C', '-c' options. So this commit\n + specifying '-m', '-F', '-C', '-c' options. So this commit\n introduce a new '--separator' option for 'git notes add' and\n - 'git notes append', for example when execute:\n + 'git notes append', for example when executing:\n \n $ git notes add -m foo -m bar --separator=\"-\"\n $ git notes show HEAD | cat\n @@ Commit message\n -\n bar\n \n - A newline is added to the value given to --separator if it\n - does not end with one already. So execute:\n + a newline is added to the value given to --separator if it\n + does not end with one already. So when executing:\n \n $ git notes add -m foo -m bar --separator=\"-\"\n and\n @@ Commit message\n \"\n $ git notes add -m foo -m bar --separator=\"-$LF\"\n \n - Running A and B produces the same result.\n + Both the two exections produce the same result.\n +\n + The reason we use a \"strbuf\" array to concat but not \"string_list\", is\n + that the binary file content may contain '\\0' in the middle, this will\n + cause the corrupt result if using a string to save.\n \n Signed-off-by: Teng Long \n \n @@ Documentation/git-notes.txt: OPTIONS\n \n ## builtin/notes.c ##\n @@\n - #include \"notes-utils.h\"\n + */\n + \n + #include \"cache.h\"\n ++#include \"alloc.h\"\n + #include \"config.h\"\n + #include \"builtin.h\"\n + #include \"gettext.h\"\n +@@\n #include \"worktree.h\"\n + #include \"write-or-die.h\"\n \n +static char *separator = NULL;\n static const char * const git_notes_usage[] = {\n @@ builtin/notes.c\n \tN_(\"git notes [--ref ] edit [--allow-empty] []\"),\n \tN_(\"git notes [--ref ] show []\"),\n \tN_(\"git notes [--ref ] merge [-v | -q] [-s ] \"),\n -@@ builtin/notes.c: struct note_data {\n +@@ builtin/notes.c: static const char * const git_notes_get_ref_usage[] = {\n + static const char note_template[] =\n + \tN_(\"Write/edit the notes for the following object:\");\n + \n ++struct note_msg {\n ++\tint stripspace;\n ++\tstruct strbuf buf;\n ++};\n ++\n + struct note_data {\n + \tint given;\n \tint use_editor;\n \tchar *edit_path;\n \tstruct strbuf buf;\n -+\tstruct strbuf **messages;\n ++\tstruct note_msg **messages;\n +\tsize_t msg_nr;\n +\tsize_t msg_alloc;\n };\n \n static void free_note_data(struct note_data *d)\n +@@ builtin/notes.c: static void free_note_data(struct note_data *d)\n + \t\tfree(d->edit_path);\n + \t}\n + \tstrbuf_release(&d->buf);\n ++\n ++\twhile (d->msg_nr) {\n ++\t\t--d->msg_nr;\n ++\t\tstrbuf_release(&d->messages[d->msg_nr]->buf);\n ++\t\tfree(d->messages[d->msg_nr]);\n ++\t}\n ++\tfree(d->messages);\n + }\n + \n + static int list_each_note(const struct object_id *object_oid,\n @@ builtin/notes.c: static void write_note_data(struct note_data *d, struct object_id *oid)\n \t}\n }\n @@ builtin/notes.c: static void write_note_data(struct note_data *d, struct object_\n +\t\tstrbuf_insertf(message, pos, \"%s%s\", separator, \"\\n\");\n +}\n +\n -+/* Consume messages and append them into d->buf, then free them */\n +static void concat_messages(struct note_data *d)\n +{\n +\tstruct strbuf msg = STRBUF_INIT;\n @@ builtin/notes.c: static void write_note_data(struct note_data *d, struct object_\n +\tfor (i = 0; i < d->msg_nr ; i++) {\n +\t\tif (d->buf.len)\n +\t\t\tinsert_separator(&d->buf, d->buf.len);\n -+\t\tstrbuf_add(&msg, d->messages[i]->buf, d->messages[i]->len);\n ++\t\tstrbuf_add(&msg, d->messages[i]->buf.buf, d->messages[i]->buf.len);\n +\t\tstrbuf_addbuf(&d->buf, &msg);\n ++\t\tif (d->messages[i]->stripspace)\n ++\t\t\tstrbuf_stripspace(&d->buf, 0);\n +\t\tstrbuf_reset(&msg);\n -+\t\tstrbuf_release(d->messages[i]);\n -+\t\tfree(d->messages[i]);\n +\t}\n +\tstrbuf_release(&msg);\n -+\tfree(d->messages);\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 -+\tstruct strbuf *buf;\n ++\tstruct note_msg *msg = xmalloc(sizeof(*msg));\n \n \tBUG_ON_OPT_NEG(unset);\n \n @@ builtin/notes.c: static void write_note_data(struct note_data *d, struct object_\n -\t\tstrbuf_addch(&d->buf, '\\n');\n -\tstrbuf_addstr(&d->buf, arg);\n -\tstrbuf_stripspace(&d->buf, 0);\n -+\tbuf = xmalloc(sizeof(*buf));\n -+\tstrbuf_init(buf, strlen(arg));\n -+\tstrbuf_addstr(buf, arg);\n -+\tstrbuf_stripspace(buf, 0);\n - \n +-\n -\td->given = 1;\n ++\tstrbuf_init(&msg->buf, strlen(arg));\n ++\tstrbuf_addstr(&msg->buf, arg);\n +\tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n -+\td->messages[d->msg_nr - 1] = buf;\n ++\td->messages[d->msg_nr - 1] = msg;\n ++\tmsg->stripspace = 1;\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 -+\tstruct strbuf *buf;\n ++\tstruct note_msg *msg = xmalloc(sizeof(*msg));\n \n \tBUG_ON_OPT_NEG(unset);\n \n -\tif (d->buf.len)\n -\t\tstrbuf_addch(&d->buf, '\\n');\n -+\tbuf = xmalloc(sizeof(*buf));\n -+\tstrbuf_init(buf, 0);\n -+\n ++\tstrbuf_init(&msg->buf , 0);\n \tif (!strcmp(arg, \"-\")) {\n -\t\tif (strbuf_read(&d->buf, 0, 1024) < 0)\n -+\t\tif (strbuf_read(buf, 0, 1024) < 0)\n ++\t\tif (strbuf_read(&msg->buf, 0, 1024) < 0)\n \t\t\tdie_errno(_(\"cannot read '%s'\"), arg);\n -\t} else if (strbuf_read_file(&d->buf, arg, 1024) < 0)\n -+\t} else if (strbuf_read_file(buf, arg, 1024) < 0)\n ++\t} else if (strbuf_read_file(&msg->buf, arg, 1024) < 0)\n \t\tdie_errno(_(\"could not open or read '%s'\"), arg);\n -\tstrbuf_stripspace(&d->buf, 0);\n -+\tstrbuf_stripspace(buf, 0);\n \n -\td->given = 1;\n -+\t// we will note stripspace the buf here, because the file maybe\n -+\t// is a binary and it maybe contains multiple continuous line breaks.\n +\tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n -+\td->messages[d->msg_nr - 1] = buf;\n ++\td->messages[d->msg_nr - 1] = msg;\n ++\tmsg->stripspace = 1;\n \treturn 0;\n }\n \n @@ builtin/notes.c: static void write_note_data(struct note_data *d, struct object_\n {\n \tstruct note_data *d = opt->value;\n -\tchar *buf;\n -+\tstruct strbuf *buf;\n ++\tstruct note_msg *msg = xmalloc(sizeof(*msg));\n ++\tchar *value;\n \tstruct object_id object;\n \tenum object_type type;\n \tunsigned long len;\n @@ builtin/notes.c: static void write_note_data(struct note_data *d, struct object_\n -\tif (d->buf.len)\n -\t\tstrbuf_addch(&d->buf, '\\n');\n -\n -+\tbuf = xmalloc(sizeof(*buf));\n - \tif (get_oid(arg, &object))\n ++\tstrbuf_init(&msg->buf, 0);\n + \tif (repo_get_oid(the_repository, arg, &object))\n \t\tdie(_(\"failed to resolve '%s' as a valid ref.\"), arg);\n --\tif (!(buf = read_object_file(&object, &type, &len)))\n -+\tif (!(buf->buf = read_object_file(&object, &type, &len)))\n +-\tif (!(buf = repo_read_object_file(the_repository, &object, &type, &len)))\n ++\tif (!(value = repo_read_object_file(the_repository, &object, &type, &len)))\n \t\tdie(_(\"failed to read object '%s'.\"), arg);\n \tif (type != OBJ_BLOB) {\n -+\t\tstrbuf_release(buf);\n - \t\tfree(buf);\n +-\t\tfree(buf);\n ++\t\tstrbuf_release(&msg->buf);\n ++\t\tfree(value);\n ++\t\tfree(msg);\n \t\tdie(_(\"cannot read note data from non-blob object '%s'.\"), arg);\n \t}\n -\tstrbuf_add(&d->buf, buf, len);\n -\tfree(buf);\n \n -\td->given = 1;\n -+\t// The reuse object maybe is a binary content which could\n -+\t// contains '\\0' in the middle, so let's set the correct\n -+\t// lenth of strbuf to concat all of content.\n -+\tbuf->len = len;\n ++\tstrbuf_add(&msg->buf, value, len);\n ++\tfree(value);\n ++\n ++\tmsg->buf.len = len;\n +\tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n -+\td->messages[d->msg_nr - 1] = buf;\n ++\td->messages[d->msg_nr - 1] = msg;\n ++\tmsg->stripspace = 0;\n \treturn 0;\n }\n \n - static int parse_reedit_arg(const struct option *opt, const char *arg, int unset)\n - {\n - \tstruct note_data *d = opt->value;\n -+\n - \tBUG_ON_OPT_NEG(unset);\n - \td->use_editor = 1;\n - \treturn parse_reuse_arg(opt, arg, unset);\n @@ builtin/notes.c: 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 = { .buf = STRBUF_INIT };\n -+\tstruct note_data d = { .buf = STRBUF_INIT, .messages = NULL };\n + \tstruct note_data d = { .buf = STRBUF_INIT };\n +\n \tstruct option options[] = {\n \t\tOPT_CALLBACK_F('m', \"message\", &d, N_(\"message\"),\n @@ builtin/notes.c: static int add(int argc, const char **argv, const char *prefix)\n +\n \tobject_ref = argc > 1 ? argv[1] : \"HEAD\";\n \n - \tif (get_oid(object_ref, &object))\n -@@ builtin/notes.c: 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 = { .buf = STRBUF_INIT };\n -+\tstruct note_data d = { .buf = STRBUF_INIT, .messages = NULL };\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 + \tif (repo_get_oid(the_repository, object_ref, &object))\n @@ builtin/notes.c: 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 @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char\n \t\tfprintf(stderr, _(\"The -m/-F/-c/-C options have been deprecated \"\n \t\t\t\"for the 'edit' subcommand.\\n\"\n @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char *prefix)\n - \t\tchar *prev_buf = read_object_file(note, &type, &size);\n + \t\t\t\t\t\t &type, &size);\n \n \t\tif (d.buf.len && prev_buf && size)\n -\t\t\tstrbuf_insertstr(&d.buf, 0, \"\\n\");\n @@ builtin/notes.c: static int append_edit(int argc, const char **argv, const char\n \t\tfree(prev_buf);\n \n ## t/t3301-notes.sh ##\n -@@\n - \n - test_description='Test commit notes'\n - \n -+TEST_PASSES_SANITIZE_LEAK=true\n - . ./test-lib.sh\n - \n - write_script fake_editor <<\\EOF\n @@ t/t3301-notes.sh: test_expect_success 'do not create empty note with -m \"\"' '\n '\n \n4: b9d12f0c < -: -------- notes.c: don't do stripespace when parse file arg\n-: -------- > 5: ef40e0ef notes.c: append separator instead of insert by pos\n-: -------- > 6: f60f7432 notes.c: introduce \"--[no-]stripspace\" option\n-- \n2.40.0.358.g931d6dc6\n\n"},{"id":"476022","messageId":"0634434e2a3e40ef4af5013349202491d55fe1d7.1682429602.git.dyroneteng@gmail.com","threadId":"59295","inReplyTo":"cover.1682429602.git.dyroneteng@gmail.com","subject":"[PATCH v8 1/6] notes.c: cleanup 'strbuf_grow' call in 'append_edit'","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-04-25T13:34:36Z","receivedAt":"2023-04-25T13:35:30Z","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\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\nSigned-off-by: Teng Long \nHelped-by: Eric Sunshine \nHelped-by: Junio C Hamano \n---\n builtin/notes.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 4ff44f1e..c501c6ee 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -219,7 +219,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@@ -623,7 +622,6 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tchar *prev_buf = repo_read_object_file(the_repository, note,\n \t\t\t\t\t\t &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.40.0.358.g931d6dc6\n\n"},{"id":"476023","messageId":"4ad7840584191d6bbb3fcba72887aa569c797a15.1682429602.git.dyroneteng@gmail.com","threadId":"59295","inReplyTo":"cover.1682429602.git.dyroneteng@gmail.com","subject":"[PATCH v8 2/6] notes.c: use designated initializers for clarity","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-04-25T13:34:37Z","receivedAt":"2023-04-25T13:35:32Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nThe \"struct note_data d = { 0, 0, NULL, STRBUF_INIT };\" style could be\nreplaced with designated initializer for clarity.\n\nSigned-off-by: Teng Long \nSigned-off-by: Junio C Hamano \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 c501c6ee..9d8ca795 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -405,7 +405,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@@ -571,7 +571,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.40.0.358.g931d6dc6\n\n"},{"id":"476024","messageId":"6dfb5bf294dd4038290a4945706c81aececc6d4d.1682429602.git.dyroneteng@gmail.com","threadId":"59295","inReplyTo":"cover.1682429602.git.dyroneteng@gmail.com","subject":"[PATCH v8 3/6] t3321: add test cases about the notes stripspace behavior","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-04-25T13:34:38Z","receivedAt":"2023-04-25T13:35:34Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nSigned-off-by: Teng Long \n---\n t/t3321-notes-stripspace.sh | 291 ++++++++++++++++++++++++++++++++++++\n 1 file changed, 291 insertions(+)\n create mode 100755 t/t3321-notes-stripspace.sh\n\ndiff --git a/t/t3321-notes-stripspace.sh b/t/t3321-notes-stripspace.sh\nnew file mode 100755\nindex 00000000..7c26b184\n--- /dev/null\n+++ b/t/t3321-notes-stripspace.sh\n@@ -0,0 +1,291 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2007 Teng Long\n+#\n+\n+test_description='Test commit notes with stripspace behavior'\n+\n+. ./test-lib.sh\n+\n+MULTI_LF=\"$LF$LF$LF\"\n+write_script fake_editor <<\\EOF\n+echo \"$MSG\" >\"$1\"\n+echo \"$MSG\" >&2\n+EOF\n+GIT_EDITOR=./fake_editor\n+export GIT_EDITOR\n+\n+test_expect_success 'setup the commit' '\n+\ttest_commit 1st\n+'\n+\n+test_expect_success 'add note by editor' '\n+\ttest_when_finished \"git notes remove\" &&\n+\tcat >expect <<-EOF &&\n+\t\tfirst-line\n+\n+\t\tsecond-line\n+\tEOF\n+\n+\tMSG=\"${LF}first-line${MULTI_LF}second-line${LF}\" git notes add &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'add note by specifying single \"-m\"' '\n+\ttest_when_finished \"git notes remove\" &&\n+\tcat >expect <<-EOF &&\n+\t\tfirst-line\n+\n+\t\tsecond-line\n+\tEOF\n+\n+\tgit notes add -m \"${LF}first-line${MULTI_LF}second-line${LF}\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'add note by specifying multiple \"-m\"' '\n+\ttest_when_finished \"git notes remove\" &&\n+\tcat >expect <<-EOF &&\n+\tfirst-line\n+\n+\tsecond-line\n+\tEOF\n+\n+\tgit notes add -m \"${LF}\" \\\n+\t\t -m \"first-line\" \\\n+\t\t -m \"${MULTI_LF}\" \\\n+\t\t -m \"second-line\" \\\n+\t\t -m \"${LF}\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+\n+test_expect_success 'append note by editor' '\n+\ttest_when_finished \"git notes remove\" &&\n+\tcat >expect <<-EOF &&\n+\t\tfirst-line\n+\n+\t\tsecond-line\n+\tEOF\n+\n+\tgit notes add -m \"first-line\" &&\n+\tMSG=\"${MULTI_LF}second-line${LF}\" git notes append &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'append note by specifying single \"-m\"' '\n+\ttest_when_finished \"git notes remove\" &&\n+\tcat >expect <<-EOF &&\n+\t\tfirst-line\n+\n+\t\tsecond-line\n+\tEOF\n+\n+\tgit notes add -m \"${LF}first-line\" &&\n+\tgit notes append -m \"${MULTI_LF}second-line${LF}\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'append note by specifying multiple \"-m\"' '\n+\ttest_when_finished \"git notes remove\" &&\n+\tcat >expect <<-EOF &&\n+\tfirst-line\n+\n+\tsecond-line\n+\tEOF\n+\n+\tgit notes add -m \"${LF}first-line\" &&\n+\tgit notes append -m \"${MULTI_LF}\" \\\n+\t\t -m \"second-line\" \\\n+\t\t -m \"${LF}\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'add note by specifying single \"-F\"' '\n+\ttest_when_finished \"git notes remove\" &&\n+\tcat >expect <<-EOF &&\n+\t\tfirst-line\n+\n+\t\tsecond-line\n+\tEOF\n+\n+\tcat >note-file <<-EOF &&\n+\t\t${LF}\n+\t\tfirst-line\n+\t\t${MULTI_LF}\n+\t\tsecond-line\n+\t\t${LF}\n+\tEOF\n+\n+\tgit notes add -F note-file &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'add notes by specifying multiple \"-F\"' '\n+\ttest_when_finished \"git notes remove\" &&\n+\tcat >expect <<-EOF &&\n+\t\tfile-1-first-line\n+\n+\t\tfile-1-second-line\n+\n+\t\tfile-2-first-line\n+\n+\t\tfile-2-second-line\n+\tEOF\n+\n+\tcat >note-file-1 <<-EOF &&\n+\t\t${LF}\n+\t\tfile-1-first-line\n+\t\t${MULTI_LF}\n+\t\tfile-1-second-line\n+\t\t${LF}\n+\tEOF\n+\n+\tcat >note-file-2 <<-EOF &&\n+\t\t${LF}\n+\t\tfile-2-first-line\n+\t\t${MULTI_LF}\n+\t\tfile-2-second-line\n+\t\t${LF}\n+\tEOF\n+\n+\tgit notes add -F note-file-1 -F note-file-2 &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'append note by specifying single \"-F\"' '\n+\ttest_when_finished \"git notes remove\" &&\n+\tcat >expect <<-EOF &&\n+\t\tinitial-line\n+\n+\t\tfirst-line\n+\n+\t\tsecond-line\n+\tEOF\n+\n+\tcat >note-file <<-EOF &&\n+\t\t${LF}\n+\t\tfirst-line\n+\t\t${MULTI_LF}\n+\t\tsecond-line\n+\t\t${LF}\n+\tEOF\n+\n+\tgit notes add -m \"initial-line\" &&\n+\tgit notes append -F note-file &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'append notes by specifying multiple \"-F\"' '\n+\ttest_when_finished \"git notes remove\" &&\n+\tcat >expect <<-EOF &&\n+\t\tinitial-line\n+\n+\t\tfile-1-first-line\n+\n+\t\tfile-1-second-line\n+\n+\t\tfile-2-first-line\n+\n+\t\tfile-2-second-line\n+\tEOF\n+\n+\tcat >note-file-1 <<-EOF &&\n+\t\t${LF}\n+\t\tfile-1-first-line\n+\t\t${MULTI_LF}\n+\t\tfile-1-second-line\n+\t\t${LF}\n+\tEOF\n+\n+\tcat >note-file-2 <<-EOF &&\n+\t\t${LF}\n+\t\tfile-2-first-line\n+\t\t${MULTI_LF}\n+\t\tfile-2-second-line\n+\t\t${LF}\n+\tEOF\n+\n+\tgit notes add -m \"initial-line\" &&\n+\tgit notes append -F note-file-1 -F note-file-2 &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'add notes with empty messages' '\n+\trev=$(git rev-parse HEAD) &&\n+\tgit notes add -m \"${LF}\" \\\n+\t\t -m \"${MULTI_LF}\" \\\n+\t\t -m \"${LF}\" >actual 2>&1 &&\n+\ttest_i18ngrep \"Removing note for object\" actual\n+'\n+\n+test_expect_success 'add note by specifying \"-C\" , do not stripspace is the default behavior' '\n+\ttest_when_finished \"git notes remove\" &&\n+\tcat >expect <<-EOF &&\n+\t\t${LF}\n+\t\tfirst-line\n+\t\t${MULTI_LF}\n+\t\tsecond-line\n+\t\t${LF}\n+\tEOF\n+\n+\tcat expect | git hash-object -w --stdin >blob &&\n+\tgit notes add -C $(cat blob) &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'add notes with \"-C\" and \"-m\", \"-m\" will stripspace all together' '\n+\ttest_when_finished \"git notes remove\" &&\n+\tcat >data <<-EOF &&\n+\t\t${LF}\n+\t\tfirst-line\n+\t\t${MULTI_LF}\n+\t\tsecond-line\n+\t\t${LF}\n+\tEOF\n+\n+\tcat >expect <<-EOF &&\n+\t\tfirst-line\n+\n+\t\tsecond-line\n+\n+\t\tthird-line\n+\tEOF\n+\n+\tcat data | git hash-object -w --stdin >blob &&\n+\tgit notes add -C $(cat blob) -m \"third-line\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'add notes with \"-m\" and \"-C\", \"-C\" will not stripspace all together' '\n+\ttest_when_finished \"git notes remove\" &&\n+\tcat >data <<-EOF &&\n+\n+\t\tsecond-line\n+\tEOF\n+\n+\tcat >expect <<-EOF &&\n+\t\tfirst-line\n+\t\t${LF}\n+\t\tsecond-line\n+\tEOF\n+\n+\tcat data | git hash-object -w --stdin >blob &&\n+\tgit notes add -m \"first-line\" -C $(cat blob) &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.40.0.358.g931d6dc6\n\n"},{"id":"476025","messageId":"ef40e0efc34ce6b6ee5b9b67bdde3dae02652cec.1682429602.git.dyroneteng@gmail.com","threadId":"59295","inReplyTo":"cover.1682429602.git.dyroneteng@gmail.com","subject":"[PATCH v8 5/6] notes.c: append separator instead of insert by pos","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-04-25T13:34:40Z","receivedAt":"2023-04-25T13:35:36Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nThis commit rename \"insert_separator\" to \"append_separator\" and also\nremove the \"postion\" argument, this serves two purpose:\n\nThe first is that when specifying more than one \"-m\" ( like \"-F\", etc)\nto \"git notes add\" or \"git notes append\", the order of them matters,\nwhich means we need to append the each separator and message in turn,\nso we don't have to make the caller specify the position, the \"append\"\noperation is enough and clear.\n\nThe second is that when we execute the \"git notes append\" subcommand\n, we need to combine the \"prev_note\" and \"current_note\" to get the\nfinal result. Before, we inserted a newline character at the beginning\nof \"current_note\". Now, we will append a newline to the end of\n\"prev_note\" instead, this will give the consisitent results.\n\nSigned-off-by: Teng Long \n---\n builtin/notes.c | 23 +++++++++++++----------\n 1 file changed, 13 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 6ae8b57b..84bc09ed 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -230,14 +230,14 @@ 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+static void append_separator(struct strbuf *message)\n {\n \tif (!separator)\n-\t\tstrbuf_insertstr(message, pos, \"\\n\");\n+\t\tstrbuf_insertstr(message, message->len, \"\\n\");\n \telse if (separator[strlen(separator) - 1] == '\\n')\n-\t\tstrbuf_insertstr(message, pos, separator);\n+\t\tstrbuf_insertstr(message, message->len, separator);\n \telse\n-\t\tstrbuf_insertf(message, pos, \"%s%s\", separator, \"\\n\");\n+\t\tstrbuf_insertf(message, message->len, \"%s%s\", separator, \"\\n\");\n }\n \n static void concat_messages(struct note_data *d)\n@@ -247,7 +247,7 @@ static void concat_messages(struct note_data *d)\n \tsize_t i;\n \tfor (i = 0; i < d->msg_nr ; i++) {\n \t\tif (d->buf.len)\n-\t\t\tinsert_separator(&d->buf, d->buf.len);\n+\t\t\tappend_separator(&d->buf);\n \t\tstrbuf_add(&msg, d->messages[i]->buf.buf, d->messages[i]->buf.len);\n \t\tstrbuf_addbuf(&d->buf, &msg);\n \t\tif (d->messages[i]->stripspace)\n@@ -682,14 +682,17 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\t/* Append buf to previous note contents */\n \t\tunsigned long size;\n \t\tenum object_type type;\n-\t\tchar *prev_buf = repo_read_object_file(the_repository, note,\n-\t\t\t\t\t\t &type, &size);\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tchar *prev_buf = repo_read_object_file(the_repository, note, &type, &size);\n \n-\t\tif (d.buf.len && prev_buf && size)\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\t\tstrbuf_add(&buf, prev_buf, size);\n+\t\tif (d.buf.len && prev_buf && size)\n+\t\t\tappend_separator(&buf);\n+\t\tstrbuf_insert(&d.buf, 0, buf.buf, buf.len);\n+\n \t\tfree(prev_buf);\n+\t\tstrbuf_release(&buf);\n \t}\n \n \tif (d.buf.len || allow_empty) {\n-- \n2.40.0.358.g931d6dc6\n\n"},{"id":"476026","messageId":"be86f9cac3e651ab5aab49f89b6506012b756dab.1682429602.git.dyroneteng@gmail.com","threadId":"59295","inReplyTo":"cover.1682429602.git.dyroneteng@gmail.com","subject":"[PATCH v8 4/6] notes.c: introduce '--separator=' option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-04-25T13:34:39Z","receivedAt":"2023-04-25T13:35:38Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nWhen adding new notes or appending to an existing notes, we will\ninsert a blank line between the paragraphs, like:\n\n $ git notes add -m foo -m bar\n $ git notes show HEAD | cat\n foo\n\n bar\n\nThe default behavour sometimes is not enough, the user may want\nto use a custom delimiter between paragraphs, like when\nspecifying '-m', '-F', '-C', '-c' options. So this commit\nintroduce a new '--separator' option for 'git notes add' and\n'git notes append', for example when executing:\n\n $ git notes add -m foo -m bar --separator=\"-\"\n $ git notes show HEAD | cat\n foo\n -\n bar\n\na newline is added to the value given to --separator if it\ndoes not end with one already. So when executing:\n\n $ git notes add -m foo -m bar --separator=\"-\"\nand\n $ export LF=\"\n \"\n $ git notes add -m foo -m bar --separator=\"-$LF\"\n\nBoth the two exections produce the same result.\n\nThe reason we use a \"strbuf\" array to concat but not \"string_list\", is\nthat the binary file content may contain '\\0' in the middle, this will\ncause the corrupt result if using a string to save.\n\nSigned-off-by: Teng Long \n---\n Documentation/git-notes.txt | 21 ++++--\n builtin/notes.c | 111 ++++++++++++++++++++++++-------\n t/t3301-notes.sh | 126 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 229 insertions(+), 29 deletions(-)\n\ndiff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\nindex efbc10f0..59980b21 100644\n--- a/Documentation/git-notes.txt\n+++ b/Documentation/git-notes.txt\n@@ -9,9 +9,9 @@ SYNOPSIS\n --------\n [verse]\n 'git notes' [list []]\n-'git notes' add [-f] [--allow-empty] [-F | -m | (-c | -C) ] []\n+'git notes' add [-f] [--allow-empty] [--separator=] [-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@@ -65,7 +65,9 @@ add::\n \tHowever, if you're using `add` interactively (using an editor\n \tto supply the notes contents), then - instead of aborting -\n \tthe existing notes will be opened in the editor (like the `edit`\n-\tsubcommand).\n+\tsubcommand). If you specify multiple `-m` and `-F`, a blank\n+\tline will be inserted between the messages. Use the `--separator`\n+\toption to insert other delimiters.\n \n copy::\n \tCopy the notes for the first object onto the second object (defaults to\n@@ -85,8 +87,12 @@ corresponding . (The optional `` is ignored so that\n 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+\tAppend new message(s) given by `-m` or `-F` options to an\n+\texisting note, or add them as a new note if one does not\n+\texist, for the object (defaults to HEAD). When appending to\n+\tan existing note, a blank line is added before each new\n+\tmessage as an inter-paragraph separator. The separator can\n+\tbe customized with the `--separator` option.\n \n edit::\n \tEdit the notes for a given object (defaults to HEAD).\n@@ -159,6 +165,11 @@ 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 a string used as a custom inter-paragraph separator\n+\t(a newline is added at the end as needed). Defaults to a\n+\tblank line.\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 9d8ca795..6ae8b57b 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -8,6 +8,7 @@\n */\n \n #include \"cache.h\"\n+#include \"alloc.h\"\n #include \"config.h\"\n #include \"builtin.h\"\n #include \"gettext.h\"\n@@ -27,11 +28,12 @@\n #include \"worktree.h\"\n #include \"write-or-die.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+\tN_(\"git notes [--ref ] add [-f] [--allow-empty] [--separator=] [-m | -F | (-c | -C) ] []\"),\n \tN_(\"git notes [--ref ] copy [-f] \"),\n-\tN_(\"git notes [--ref ] append [--allow-empty] [-m | -F | (-c | -C) ] []\"),\n+\tN_(\"git notes [--ref ] append [--allow-empty] [--separator=] [-m | -F | (-c | -C) ] []\"),\n \tN_(\"git notes [--ref ] edit [--allow-empty] []\"),\n \tN_(\"git notes [--ref ] show []\"),\n \tN_(\"git notes [--ref ] merge [-v | -q] [-s ] \"),\n@@ -99,11 +101,19 @@ static const char * const git_notes_get_ref_usage[] = {\n static const char note_template[] =\n \tN_(\"Write/edit the notes for the following object:\");\n \n+struct note_msg {\n+\tint stripspace;\n+\tstruct strbuf buf;\n+};\n+\n struct note_data {\n \tint given;\n \tint use_editor;\n \tchar *edit_path;\n \tstruct strbuf buf;\n+\tstruct note_msg **messages;\n+\tsize_t msg_nr;\n+\tsize_t msg_alloc;\n };\n \n static void free_note_data(struct note_data *d)\n@@ -113,6 +123,13 @@ static void free_note_data(struct note_data *d)\n \t\tfree(d->edit_path);\n \t}\n \tstrbuf_release(&d->buf);\n+\n+\twhile (d->msg_nr) {\n+\t\t--d->msg_nr;\n+\t\tstrbuf_release(&d->messages[d->msg_nr]->buf);\n+\t\tfree(d->messages[d->msg_nr]);\n+\t}\n+\tfree(d->messages);\n }\n \n static int list_each_note(const struct object_id *object_oid,\n@@ -213,65 +230,98 @@ 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 void concat_messages(struct note_data *d)\n+{\n+\tstruct strbuf msg = STRBUF_INIT;\n+\n+\tsize_t i;\n+\tfor (i = 0; i < d->msg_nr ; i++) {\n+\t\tif (d->buf.len)\n+\t\t\tinsert_separator(&d->buf, d->buf.len);\n+\t\tstrbuf_add(&msg, d->messages[i]->buf.buf, d->messages[i]->buf.len);\n+\t\tstrbuf_addbuf(&d->buf, &msg);\n+\t\tif (d->messages[i]->stripspace)\n+\t\t\tstrbuf_stripspace(&d->buf, 0);\n+\t\tstrbuf_reset(&msg);\n+\t}\n+\tstrbuf_release(&msg);\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+\tstruct note_msg *msg = xmalloc(sizeof(*msg));\n \n \tBUG_ON_OPT_NEG(unset);\n \n-\tif (d->buf.len)\n-\t\tstrbuf_addch(&d->buf, '\\n');\n-\tstrbuf_addstr(&d->buf, arg);\n-\tstrbuf_stripspace(&d->buf, 0);\n-\n-\td->given = 1;\n+\tstrbuf_init(&msg->buf, strlen(arg));\n+\tstrbuf_addstr(&msg->buf, arg);\n+\tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n+\td->messages[d->msg_nr - 1] = msg;\n+\tmsg->stripspace = 1;\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+\tstruct note_msg *msg = xmalloc(sizeof(*msg));\n \n \tBUG_ON_OPT_NEG(unset);\n \n-\tif (d->buf.len)\n-\t\tstrbuf_addch(&d->buf, '\\n');\n+\tstrbuf_init(&msg->buf , 0);\n \tif (!strcmp(arg, \"-\")) {\n-\t\tif (strbuf_read(&d->buf, 0, 1024) < 0)\n+\t\tif (strbuf_read(&msg->buf, 0, 1024) < 0)\n \t\t\tdie_errno(_(\"cannot read '%s'\"), arg);\n-\t} else if (strbuf_read_file(&d->buf, arg, 1024) < 0)\n+\t} else if (strbuf_read_file(&msg->buf, arg, 1024) < 0)\n \t\tdie_errno(_(\"could not open or read '%s'\"), arg);\n-\tstrbuf_stripspace(&d->buf, 0);\n \n-\td->given = 1;\n+\tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n+\td->messages[d->msg_nr - 1] = msg;\n+\tmsg->stripspace = 1;\n \treturn 0;\n }\n \n static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)\n {\n \tstruct note_data *d = opt->value;\n-\tchar *buf;\n+\tstruct note_msg *msg = xmalloc(sizeof(*msg));\n+\tchar *value;\n \tstruct object_id object;\n \tenum object_type type;\n \tunsigned long len;\n \n \tBUG_ON_OPT_NEG(unset);\n \n-\tif (d->buf.len)\n-\t\tstrbuf_addch(&d->buf, '\\n');\n-\n+\tstrbuf_init(&msg->buf, 0);\n \tif (repo_get_oid(the_repository, arg, &object))\n \t\tdie(_(\"failed to resolve '%s' as a valid ref.\"), arg);\n-\tif (!(buf = repo_read_object_file(the_repository, &object, &type, &len)))\n+\tif (!(value = repo_read_object_file(the_repository, &object, &type, &len)))\n \t\tdie(_(\"failed to read object '%s'.\"), arg);\n \tif (type != OBJ_BLOB) {\n-\t\tfree(buf);\n+\t\tstrbuf_release(&msg->buf);\n+\t\tfree(value);\n+\t\tfree(msg);\n \t\tdie(_(\"cannot read note data from non-blob object '%s'.\"), arg);\n \t}\n-\tstrbuf_add(&d->buf, buf, len);\n-\tfree(buf);\n \n-\td->given = 1;\n+\tstrbuf_add(&msg->buf, value, len);\n+\tfree(value);\n+\n+\tmsg->buf.len = len;\n+\tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n+\td->messages[d->msg_nr - 1] = msg;\n+\tmsg->stripspace = 0;\n \treturn 0;\n }\n \n@@ -406,6 +456,7 @@ static int add(int argc, const char **argv, const char *prefix)\n \tstruct object_id object, new_note;\n \tconst struct object_id *note;\n \tstruct note_data d = { .buf = STRBUF_INIT };\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@@ -422,6 +473,8 @@ static int add(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n \t\t\tN_(\"allow storing empty note\")),\n \t\tOPT__FORCE(&force, N_(\"replace existing notes\"), PARSE_OPT_NOCOMPLETE),\n+\t\tOPT_STRING(0, \"separator\", &separator, N_(\"separator\"),\n+\t\t\tN_(\"insert between paragraphs\")),\n \t\tOPT_END()\n \t};\n \n@@ -433,6 +486,10 @@ static int add(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_notes_add_usage, options);\n \t}\n \n+\tif (d.msg_nr)\n+\t\tconcat_messages(&d);\n+\td.given = !!d.buf.len;\n+\n \tobject_ref = argc > 1 ? argv[1] : \"HEAD\";\n \n \tif (repo_get_oid(the_repository, object_ref, &object))\n@@ -587,6 +644,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 between paragraphs\")),\n \t\tOPT_END()\n \t};\n \tint edit = !strcmp(argv[0], \"edit\");\n@@ -600,6 +659,10 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(usage, options);\n \t}\n \n+\tif (d.msg_nr)\n+\t\tconcat_messages(&d);\n+\td.given = !!d.buf.len;\n+\n \tif (d.given && edit)\n \t\tfprintf(stderr, _(\"The -m/-F/-c/-C options have been deprecated \"\n \t\t\t\"for the 'edit' subcommand.\\n\"\n@@ -623,7 +686,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t\t &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..dbadcf13 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -362,6 +362,7 @@ test_expect_success 'do not create empty note with -m \"\"' '\n '\n \n test_expect_success 'create note with combination of -m and -F' '\n+\ttest_when_finished git notes remove HEAD &&\n \tcat >expect-combine_m_and_F <<-EOF &&\n \t\tfoo\n \n@@ -380,6 +381,25 @@ test_expect_success 'create note with combination of -m and -F' '\n \ttest_cmp expect-combine_m_and_F actual\n '\n \n+test_expect_success 'create note with combination of -m and -F and --separator' '\n+\tcat >expect-combine_m_and_F <<-\\EOF &&\n+\tfoo\n+\t-------\n+\txyzzy\n+\t-------\n+\tbar\n+\t-------\n+\tzyxxy\n+\t-------\n+\tbaz\n+\tEOF\n+\techo \"xyzzy\" >note_a &&\n+\techo \"zyxxy\" >note_b &&\n+\tgit notes add -m \"foo\" -F note_a -m \"bar\" -F note_b -m \"baz\" --separator \"-------\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect-combine_m_and_F actual\n+'\n+\n test_expect_success 'remove note with \"git notes remove\"' '\n \tgit notes remove HEAD^ &&\n \tgit notes remove &&\n@@ -521,6 +541,85 @@ 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 separator 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 note with combination of -m and -F and --separator' '\n+\ttest_when_finished git notes remove HEAD &&\n+\tcat >expect-combine_m_and_F <<-\\EOF &&\n+\tm-notes-1\n+\t-------\n+\tf-notes-1\n+\t-------\n+\tm-notes-2\n+\t-------\n+\tf-notes-2\n+\t-------\n+\tm-notes-3\n+\tEOF\n+\n+\techo \"f-notes-1\" >note_a &&\n+\techo \"f-notes-2\" >note_b &&\n+\tgit notes append -m \"m-notes-1\" -F note_a -m \"m-notes-2\" -F note_b -m \"m-notes-3\" --separator \"-------\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect-combine_m_and_F 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@@ -818,6 +917,33 @@ test_expect_success 'create note from blob with \"git notes add -C\" reuses blob i\n \ttest_cmp blob actual\n '\n \n+test_expect_success 'create note from blob with \"-C\", also specify \"-m\", \"-F\" and \"--separator\"' '\n+\t# 8th will be reuseed in following tests, so rollback when the test is done\n+\ttest_when_finished \"git notes remove && git notes add -C $(cat blob)\" &&\n+\tcommit=$(git rev-parse HEAD) &&\n+\tcat >expect <<-EOF &&\n+\t\tcommit $commit\n+\t\tAuthor: A U Thor \n+\t\tDate: Thu Apr 7 15:20:13 2005 -0700\n+\n+\t\t${indent}8th\n+\n+\t\tNotes:\n+\t\t${indent}This is a blob object\n+\t\t${indent}-------\n+\t\t${indent}This is created by -m\n+\t\t${indent}-------\n+\t\t${indent}This is created by -F\n+\tEOF\n+\n+\tgit notes remove &&\n+\techo \"This is a blob object\" | git hash-object -w --stdin >blob &&\n+\techo \"This is created by -F\" >note_a &&\n+\tgit notes add -C $(cat blob) -m \"This is created by -m\" -F note_a --separator=\"-------\" &&\n+\tgit log -1 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'create note from other note with \"git notes add -c\"' '\n \ttest_commit 9th &&\n \tcommit=$(git rev-parse HEAD) &&\n-- \n2.40.0.358.g931d6dc6\n\n"},{"id":"476027","messageId":"f60f743203d78a489b90df81472e71391b45367d.1682429602.git.dyroneteng@gmail.com","threadId":"59295","inReplyTo":"cover.1682429602.git.dyroneteng@gmail.com","subject":"[PATCH v8 6/6] notes.c: introduce \"--[no-]stripspace\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-04-25T13:34:41Z","receivedAt":"2023-04-25T13:35:53Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nThis commit introduces a new option \"--[no-]stripspace\" to git notes\nappend, git notes edit, and git notes add. This option allows users to\ncontrol whether the note message need to stripped out.\n\nFor the consideration of backward compatibility, let's look at the\nbehavior about \"stripspace\" in \"git notes\" command:\n\n1. \"Edit Message\" case: using the default editor to edit the note\nmessage.\n\n In \"edit\" case, the edited message will always be stripped out, the\n implementation which can be found in the \"prepare_note_data()\". In\n addition, the \"-c\" option supports to reuse an existing blob as a\n note message, then open the editor to make a further edition on it,\n the edited message will be stripped.\n\n This commit doesn't change the default behavior of \"edit\" case by\n using an enum \"notes_stripspace\", only when \"--no-stripspace\" option\n is specified, the note message will not be stripped out. If you do\n not specify the option or you specify \"--stripspace\", clearly, the\n note message will be stripped out.\n\n2. \"Assign Message\" case: using the \"-m\"/\"-F\"/\"-C\" option to specify the\nnote message.\n\n In \"assign\" case, when specify message by \"-m\" or \"-F\", the message\n will be stripped out by default, but when specify message by \"-C\",\n the message will be copied verbatim, in other word, the message will\n not be stripped out. One more thing need to note is \"the order of\n the options matter\", that is, if you specify \"-C\" before \"-m\" or\n \"-F\", the reused message by \"-C\" will be stripped out together,\n because everytime concat \"-m\" or \"-F\" message, the concated message\n will be stripped together. Oppositely, if you specify \"-m\" or \"-F\"\n before \"-C\", the reused message by \"-C\" will not be stripped out.\n\n This commit doesn't change the default behavior of \"assign\" case by\n extending the \"stripspace\" field in \"struct note_msg\", so we can\n distinguish the different behavior of \"-m\"/\"-F\" and \"-C\" options\n when we need to parse and concat the message.\n\nSigned-off-by: Teng Long \n---\n Documentation/git-notes.txt | 25 ++-\n builtin/notes.c | 30 +++-\n t/t3321-notes-stripspace.sh | 296 ++++++++++++++++++++++++++++++++----\n 3 files changed, 304 insertions(+), 47 deletions(-)\n\ndiff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\nindex 59980b21..5f3a9479 100644\n--- a/Documentation/git-notes.txt\n+++ b/Documentation/git-notes.txt\n@@ -9,10 +9,10 @@ SYNOPSIS\n --------\n [verse]\n 'git notes' [list []]\n-'git notes' add [-f] [--allow-empty] [--separator=] [-F | -m | (-c | -C) ] []\n+'git notes' add [-f] [--allow-empty] [--separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] []\n 'git notes' copy [-f] ( --stdin | [] )\n-'git notes' append [--allow-empty] [--separator=] [-F | -m | (-c | -C) ] []\n-'git notes' edit [--allow-empty] []\n+'git notes' append [--allow-empty] [--separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] []\n+'git notes' edit [--allow-empty] [] [--[no-]stripspace]\n 'git notes' show []\n 'git notes' merge [-v | -q] [-s ] \n 'git notes' merge --commit [-v | -q]\n@@ -141,20 +141,26 @@ OPTIONS\n \tIf multiple `-m` options are given, their values\n \tare concatenated as separate paragraphs.\n \tLines starting with `#` and empty lines other than a\n-\tsingle line between paragraphs will be stripped out.\n+\tsingle line between paragraphs will be stripped out,\n+\tif you wish to keep them verbatim, use `--no-stripspace`.\n \n -F ::\n --file=::\n \tTake the note message from the given file. Use '-' to\n \tread the note message from the standard input.\n \tLines starting with `#` and empty lines other than a\n-\tsingle line between paragraphs will be stripped out.\n+\tsingle line between paragraphs will be stripped out,\n+\tif you wish to keep them verbatim, use with\n+\t`--no-stripspace` option.\n \n -C ::\n --reuse-message=::\n \tTake the given blob object (for example, another note) as the\n \tnote message. (Use `git notes copy ` instead to\n-\tcopy notes between objects.)\n+\tcopy notes between objects.). By default, message will be\n+\tcopied verbatim, but if you wish to strip out the lines\n+\tstarting with `#` and empty lines other than a single line\n+\tbetween paragraphs, use with`--stripspace` option.\n \n -c ::\n --reedit-message=::\n@@ -170,6 +176,13 @@ OPTIONS\n \t(a newline is added at the end as needed). Defaults to a\n \tblank line.\n \n+--[no-]stripspace::\n+\tStrip leading and trailing whitespace from the note message.\n+\tAlso strip out empty lines other than a single line between\n+\tparagraphs. For lines starting with `#` will be stripped out\n+\tin non-editor cases like \"-m\", \"-F\" and \"-C\", but not in\n+\teditor case like \"git notes edit\", \"-c\", etc.\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 84bc09ed..b27d82d7 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -101,14 +101,21 @@ static const char * const git_notes_get_ref_usage[] = {\n static const char note_template[] =\n \tN_(\"Write/edit the notes for the following object:\");\n \n+enum notes_stripspace {\n+\tUNSPECIFIED = -1,\n+\tNO_STRIPSPACE = 0,\n+\tSTRIPSPACE = 1,\n+};\n+\n struct note_msg {\n-\tint stripspace;\n+\tenum notes_stripspace stripspace;\n \tstruct strbuf buf;\n };\n \n struct note_data {\n \tint given;\n \tint use_editor;\n+\tint stripspace;\n \tchar *edit_path;\n \tstruct strbuf buf;\n \tstruct note_msg **messages;\n@@ -214,7 +221,8 @@ static void prepare_note_data(const struct object_id *object, struct note_data *\n \t\tif (launch_editor(d->edit_path, &d->buf, NULL)) {\n \t\t\tdie(_(\"please supply the note contents using either -m or -F option\"));\n \t\t}\n-\t\tstrbuf_stripspace(&d->buf, 1);\n+\t\tif (d->stripspace)\n+\t\t\tstrbuf_stripspace(&d->buf, 1);\n \t}\n }\n \n@@ -250,7 +258,9 @@ static void concat_messages(struct note_data *d)\n \t\t\tappend_separator(&d->buf);\n \t\tstrbuf_add(&msg, d->messages[i]->buf.buf, d->messages[i]->buf.len);\n \t\tstrbuf_addbuf(&d->buf, &msg);\n-\t\tif (d->messages[i]->stripspace)\n+\t\tif ((d->stripspace == UNSPECIFIED &&\n+\t\t d->messages[i]->stripspace == STRIPSPACE) ||\n+\t\t d->stripspace == STRIPSPACE)\n \t\t\tstrbuf_stripspace(&d->buf, 0);\n \t\tstrbuf_reset(&msg);\n \t}\n@@ -268,7 +278,7 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n \tstrbuf_addstr(&msg->buf, arg);\n \tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n \td->messages[d->msg_nr - 1] = msg;\n-\tmsg->stripspace = 1;\n+\tmsg->stripspace = STRIPSPACE;\n \treturn 0;\n }\n \n@@ -288,7 +298,7 @@ static int parse_file_arg(const struct option *opt, const char *arg, int unset)\n \n \tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n \td->messages[d->msg_nr - 1] = msg;\n-\tmsg->stripspace = 1;\n+\tmsg->stripspace = STRIPSPACE;\n \treturn 0;\n }\n \n@@ -321,7 +331,7 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)\n \tmsg->buf.len = len;\n \tALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);\n \td->messages[d->msg_nr - 1] = msg;\n-\tmsg->stripspace = 0;\n+\tmsg->stripspace = NO_STRIPSPACE;\n \treturn 0;\n }\n \n@@ -455,7 +465,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 = { .buf = STRBUF_INIT };\n+\tstruct note_data d = { .buf = STRBUF_INIT, .stripspace = UNSPECIFIED };\n \n \tstruct option options[] = {\n \t\tOPT_CALLBACK_F('m', \"message\", &d, N_(\"message\"),\n@@ -475,6 +485,8 @@ static int add(int argc, const char **argv, const char *prefix)\n \t\tOPT__FORCE(&force, N_(\"replace existing notes\"), PARSE_OPT_NOCOMPLETE),\n \t\tOPT_STRING(0, \"separator\", &separator, N_(\"separator\"),\n \t\t\tN_(\"insert between paragraphs\")),\n+\t\tOPT_BOOL(0, \"stripspace\", &d.stripspace,\n+\t\t\tN_(\"remove unnecessary whitespace\")),\n \t\tOPT_END()\n \t};\n \n@@ -628,7 +640,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 = { .buf = STRBUF_INIT };\n+\tstruct note_data d = { .buf = STRBUF_INIT, .stripspace = UNSPECIFIED };\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@@ -646,6 +658,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"allow storing empty note\")),\n \t\tOPT_STRING(0, \"separator\", &separator, N_(\"separator\"),\n \t\t\tN_(\"insert between paragraphs\")),\n+\t\tOPT_BOOL(0, \"stripspace\", &d.stripspace,\n+\t\t\tN_(\"remove unnecessary whitespace\")),\n \t\tOPT_END()\n \t};\n \tint edit = !strcmp(argv[0], \"edit\");\ndiff --git a/t/t3321-notes-stripspace.sh b/t/t3321-notes-stripspace.sh\nindex 7c26b184..8e921157 100755\n--- a/t/t3321-notes-stripspace.sh\n+++ b/t/t3321-notes-stripspace.sh\n@@ -32,7 +32,7 @@ test_expect_success 'add note by editor' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'add note by specifying single \"-m\"' '\n+test_expect_success 'add note by specifying single \"-m\", \"--stripspace\" is the default behavior' '\n \ttest_when_finished \"git notes remove\" &&\n \tcat >expect <<-EOF &&\n \t\tfirst-line\n@@ -42,10 +42,26 @@ test_expect_success 'add note by specifying single \"-m\"' '\n \n \tgit notes add -m \"${LF}first-line${MULTI_LF}second-line${LF}\" &&\n \tgit notes show >actual &&\n+\ttest_cmp expect actual &&\n+\tgit notes remove &&\n+\tgit notes add --stripspace -m \"${LF}first-line${MULTI_LF}second-line${LF}\" &&\n+\tgit notes show >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'add note by specifying multiple \"-m\"' '\n+test_expect_success 'add note by specifying single \"-m\" and \"--no-stripspace\" ' '\n+\ttest_when_finished \"git notes remove\" &&\n+\tcat >expect <<-EOF &&\n+\t\t${LF}first-line${MULTI_LF}second-line\n+\tEOF\n+\n+\tgit notes add --no-stripspace \\\n+\t\t -m \"${LF}first-line${MULTI_LF}second-line${LF}\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'add note by specifying multiple \"-m\", \"--stripspace\" is the default behavior' '\n \ttest_when_finished \"git notes remove\" &&\n \tcat >expect <<-EOF &&\n \tfirst-line\n@@ -59,25 +75,37 @@ test_expect_success 'add note by specifying multiple \"-m\"' '\n \t\t -m \"second-line\" \\\n \t\t -m \"${LF}\" &&\n \tgit notes show >actual &&\n+\ttest_cmp expect actual &&\n+\tgit notes remove &&\n+\tgit notes add --stripspace -m \"${LF}\" \\\n+\t\t -m \"first-line\" \\\n+\t\t -m \"${MULTI_LF}\" \\\n+\t\t -m \"second-line\" \\\n+\t\t -m \"${LF}\" &&\n+\tgit notes show >actual &&\n \ttest_cmp expect actual\n '\n \n-\n-test_expect_success 'append note by editor' '\n+test_expect_success 'add notes by specifying multiple \"-m\" and \"--no-stripspace\"' '\n \ttest_when_finished \"git notes remove\" &&\n \tcat >expect <<-EOF &&\n+\t\t${LF}\n \t\tfirst-line\n-\n-\t\tsecond-line\n+\t\t${MULTI_LF}\n+\t\tsecond-line${LF}\n \tEOF\n \n-\tgit notes add -m \"first-line\" &&\n-\tMSG=\"${MULTI_LF}second-line${LF}\" git notes append &&\n+\tgit notes add --no-stripspace \\\n+\t\t -m \"${LF}\" \\\n+\t\t -m \"first-line\" \\\n+\t\t -m \"${MULTI_LF}\" \\\n+\t\t -m \"second-line\" \\\n+\t\t -m \"${LF}\" &&\n \tgit notes show >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'append note by specifying single \"-m\"' '\n+test_expect_success 'add note by specifying single \"-F\", \"--stripspace\" is the default behavior' '\n \ttest_when_finished \"git notes remove\" &&\n \tcat >expect <<-EOF &&\n \t\tfirst-line\n@@ -85,34 +113,30 @@ test_expect_success 'append note by specifying single \"-m\"' '\n \t\tsecond-line\n \tEOF\n \n-\tgit notes add -m \"${LF}first-line\" &&\n-\tgit notes append -m \"${MULTI_LF}second-line${LF}\" &&\n-\tgit notes show >actual &&\n-\ttest_cmp expect actual\n-'\n-\n-test_expect_success 'append note by specifying multiple \"-m\"' '\n-\ttest_when_finished \"git notes remove\" &&\n-\tcat >expect <<-EOF &&\n-\tfirst-line\n-\n-\tsecond-line\n+\tcat >note-file <<-EOF &&\n+\t\t${LF}\n+\t\tfirst-line\n+\t\t${MULTI_LF}\n+\t\tsecond-line\n+\t\t${LF}\n \tEOF\n \n-\tgit notes add -m \"${LF}first-line\" &&\n-\tgit notes append -m \"${MULTI_LF}\" \\\n-\t\t -m \"second-line\" \\\n-\t\t -m \"${LF}\" &&\n+\tgit notes add -F note-file &&\n \tgit notes show >actual &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\tgit notes remove &&\n+\tgit notes add --stripspace -F note-file &&\n+\tgit notes show >actual\n '\n \n-test_expect_success 'add note by specifying single \"-F\"' '\n+test_expect_success 'add note by specifying single \"-F\" and \"--no-stripspace\"' '\n \ttest_when_finished \"git notes remove\" &&\n \tcat >expect <<-EOF &&\n+\t\t${LF}\n \t\tfirst-line\n-\n+\t\t${MULTI_LF}\n \t\tsecond-line\n+\t\t${LF}\n \tEOF\n \n \tcat >note-file <<-EOF &&\n@@ -123,12 +147,12 @@ test_expect_success 'add note by specifying single \"-F\"' '\n \t\t${LF}\n \tEOF\n \n-\tgit notes add -F note-file &&\n+\tgit notes add --no-stripspace -F note-file &&\n \tgit notes show >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'add notes by specifying multiple \"-F\"' '\n+test_expect_success 'add note by specifying multiple \"-F\", \"--stripspace\" is the default behavior' '\n \ttest_when_finished \"git notes remove\" &&\n \tcat >expect <<-EOF &&\n \t\tfile-1-first-line\n@@ -158,6 +182,89 @@ test_expect_success 'add notes by specifying multiple \"-F\"' '\n \n \tgit notes add -F note-file-1 -F note-file-2 &&\n \tgit notes show >actual &&\n+\ttest_cmp expect actual &&\n+\tgit notes remove &&\n+\tgit notes add --stripspace -F note-file-1 -F note-file-2 &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'add note by specifying multiple \"-F\" with \"--no-stripspace\"' '\n+\ttest_when_finished \"git notes remove\" &&\n+\tcat >expect <<-EOF &&\n+\t\t${LF}\n+\t\tfile-1-first-line\n+\t\t${MULTI_LF}\n+\t\tfile-1-second-line\n+\t\t${LF}\n+\n+\t\t${LF}\n+\t\tfile-2-first-line\n+\t\t${MULTI_LF}\n+\t\tfile-2-second-line\n+\t\t${LF}\n+\tEOF\n+\n+\tcat >note-file-1 <<-EOF &&\n+\t\t${LF}\n+\t\tfile-1-first-line\n+\t\t${MULTI_LF}\n+\t\tfile-1-second-line\n+\t\t${LF}\n+\tEOF\n+\n+\tcat >note-file-2 <<-EOF &&\n+\t\t${LF}\n+\t\tfile-2-first-line\n+\t\t${MULTI_LF}\n+\t\tfile-2-second-line\n+\t\t${LF}\n+\tEOF\n+\n+\tgit notes add --no-stripspace -F note-file-1 -F note-file-2 &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'append note by editor' '\n+\ttest_when_finished \"git notes remove\" &&\n+\tcat >expect <<-EOF &&\n+\t\tfirst-line\n+\n+\t\tsecond-line\n+\tEOF\n+\n+\tgit notes add -m \"first-line\" &&\n+\tMSG=\"${MULTI_LF}second-line${LF}\" git notes append &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'append note by specifying single \"-m\"' '\n+\ttest_when_finished \"git notes remove\" &&\n+\tcat >expect <<-EOF &&\n+\t\tfirst-line\n+\n+\t\tsecond-line\n+\tEOF\n+\n+\tgit notes add -m \"${LF}first-line\" &&\n+\tgit notes append -m \"${MULTI_LF}second-line${LF}\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'append note by specifying multiple \"-m\"' '\n+\ttest_when_finished \"git notes remove\" &&\n+\tcat >expect <<-EOF &&\n+\tfirst-line\n+\n+\tsecond-line\n+\tEOF\n+\n+\tgit notes add -m \"${LF}first-line\" &&\n+\tgit notes append -m \"${MULTI_LF}\" -m \"second-line\" -m \"${LF}\" &&\n+\tgit notes show >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -221,6 +328,45 @@ test_expect_success 'append notes by specifying multiple \"-F\"' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'append note by specifying multiple \"-F\" with \"--no-stripspace\"' '\n+\ttest_when_finished \"git notes remove\" &&\n+\tcat >expect <<-EOF &&\n+\t\tinitial-line\n+\t\t${LF}${LF}\n+\t\tfile-1-first-line\n+\t\t${MULTI_LF}\n+\t\tfile-1-second-line\n+\t\t${LF}\n+\n+\t\t${LF}\n+\t\tfile-2-first-line\n+\t\t${MULTI_LF}\n+\t\tfile-2-second-line\n+\t\t${LF}\n+\tEOF\n+\n+\tcat >note-file-1 <<-EOF &&\n+\t\t${LF}\n+\t\tfile-1-first-line\n+\t\t${MULTI_LF}\n+\t\tfile-1-second-line\n+\t\t${LF}\n+\tEOF\n+\n+\tcat >note-file-2 <<-EOF &&\n+\t\t${LF}\n+\t\tfile-2-first-line\n+\t\t${MULTI_LF}\n+\t\tfile-2-second-line\n+\t\t${LF}\n+\tEOF\n+\n+\tgit notes add -m \"initial-line\" &&\n+\tgit notes append --no-stripspace -F note-file-1 -F note-file-2 &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'add notes with empty messages' '\n \trev=$(git rev-parse HEAD) &&\n \tgit notes add -m \"${LF}\" \\\n@@ -229,7 +375,7 @@ test_expect_success 'add notes with empty messages' '\n \ttest_i18ngrep \"Removing note for object\" actual\n '\n \n-test_expect_success 'add note by specifying \"-C\" , do not stripspace is the default behavior' '\n+test_expect_success 'add note by specifying \"-C\", \"--no-stripspace\" is the default behavior' '\n \ttest_when_finished \"git notes remove\" &&\n \tcat >expect <<-EOF &&\n \t\t${LF}\n@@ -242,10 +388,36 @@ test_expect_success 'add note by specifying \"-C\" , do not stripspace is the defa\n \tcat expect | git hash-object -w --stdin >blob &&\n \tgit notes add -C $(cat blob) &&\n \tgit notes show >actual &&\n+\ttest_cmp expect actual &&\n+\tgit notes remove &&\n+\tgit notes add --no-stripspace -C $(cat blob) &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'reuse note by specifying \"-C\" and \"--stripspace\"' '\n+\ttest_when_finished \"git notes remove\" &&\n+\tcat >data <<-EOF &&\n+\t\t${LF}\n+\t\tfirst-line\n+\t\t${MULTI_LF}\n+\t\tsecond-line\n+\t\t${LF}\n+\tEOF\n+\n+\tcat >expect <<-EOF &&\n+\t\tfirst-line\n+\n+\t\tsecond-line\n+\tEOF\n+\n+\tcat data | git hash-object -w --stdin >blob &&\n+\tgit notes add --stripspace -C $(cat blob) &&\n+\tgit notes show >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'add notes with \"-C\" and \"-m\", \"-m\" will stripspace all together' '\n+test_expect_success 'reuse with \"-C\" and add note with \"-m\", \"-m\" will stripspace all together' '\n \ttest_when_finished \"git notes remove\" &&\n \tcat >data <<-EOF &&\n \t\t${LF}\n@@ -269,7 +441,7 @@ test_expect_success 'add notes with \"-C\" and \"-m\", \"-m\" will stripspace all toge\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'add notes with \"-m\" and \"-C\", \"-C\" will not stripspace all together' '\n+test_expect_success 'add note with \"-m\" and reuse note with \"-C\", \"-C\" will not stripspace all together' '\n \ttest_when_finished \"git notes remove\" &&\n \tcat >data <<-EOF &&\n \n@@ -288,4 +460,62 @@ test_expect_success 'add notes with \"-m\" and \"-C\", \"-C\" will not stripspace all\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'add note by specifying \"-c\", \"--stripspace\" is the default behavior' '\n+\ttest_when_finished \"git notes remove\" &&\n+\tcat >expect <<-EOF &&\n+\t\tfirst-line\n+\n+\t\tsecond-line\n+\tEOF\n+\n+\techo \"initial-line\" | git hash-object -w --stdin >blob &&\n+\tMSG=\"${LF}first-line${MULTI_LF}second-line${LF}\" git notes add -c $(cat blob) &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual &&\n+\tgit notes remove &&\n+\tMSG=\"${LF}first-line${MULTI_LF}second-line${LF}\" git notes add --stripspace -c $(cat blob) &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'add note by specifying \"-c\" with \"--no-stripspace\"' '\n+\ttest_when_finished \"git notes remove\" &&\n+\tcat >expect <<-EOF &&\n+\t\t${LF}first-line${MULTI_LF}second-line${LF}\n+\tEOF\n+\n+\techo \"initial-line\" | git hash-object -w --stdin >blob &&\n+\tMSG=\"${LF}first-line${MULTI_LF}second-line${LF}\" git notes add --no-stripspace -c $(cat blob) &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'edit note by specifying \"-c\", \"--stripspace\" is the default behavior' '\n+\ttest_when_finished \"git notes remove\" &&\n+\tcat >expect <<-EOF &&\n+\t\tfirst-line\n+\n+\t\tsecond-line\n+\tEOF\n+\n+\tMSG=\"${LF}first-line${MULTI_LF}second-line${LF}\" git notes edit &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual &&\n+\tgit notes remove &&\n+\tMSG=\"${LF}first-line${MULTI_LF}second-line${LF}\" git notes edit --stripspace &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'edit note by specifying \"-c\" with \"--no-stripspace\"' '\n+\ttest_when_finished \"git notes remove\" &&\n+\tcat >expect <<-EOF &&\n+\t\t${LF}first-line${MULTI_LF}second-line${LF}\n+\tEOF\n+\n+\tMSG=\"${LF}first-line${MULTI_LF}second-line${LF}\" git notes add --no-stripspace &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.40.0.358.g931d6dc6\n\n"},{"id":"476032","messageId":"xmqqjzy03p4w.fsf@gitster.g","threadId":"59295","inReplyTo":"6dfb5bf294dd4038290a4945706c81aececc6d4d.1682429602.git.dyroneteng@gmail.com","subject":"Re: [PATCH v8 3/6] t3321: add test cases about the notes stripspace behavior","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-25T16:25:35Z","receivedAt":"2023-04-25T16:26:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n> From: Teng Long \n>\n> Signed-off-by: Teng Long \n> ---\n> t/t3321-notes-stripspace.sh | 291 ++++++++++++++++++++++++++++++++++++\n> 1 file changed, 291 insertions(+)\n> create mode 100755 t/t3321-notes-stripspace.sh\n\nLooks quite thorough.\n\n> diff --git a/t/t3321-notes-stripspace.sh b/t/t3321-notes-stripspace.sh\n> new file mode 100755\n> index 00000000..7c26b184\n> --- /dev/null\n> +++ b/t/t3321-notes-stripspace.sh\n> @@ -0,0 +1,291 @@\n> +#!/bin/sh\n> +#\n> +# Copyright (c) 2007 Teng Long\n\nIt's 2023 now.\n\n> +test_expect_success 'add note by editor' '\n> +\ttest_when_finished \"git notes remove\" &&\n> +\tcat >expect <<-EOF &&\n> +\t\tfirst-line\n> +\n> +\t\tsecond-line\n> +\tEOF\n> ...\n> +test_expect_success 'append note by specifying multiple \"-m\"' '\n> +\ttest_when_finished \"git notes remove\" &&\n> +\tcat >expect <<-EOF &&\n> +\tfirst-line\n> +\n> +\tsecond-line\n> +\tEOF\n\nInconsistent indentation confuses readers if the author meant\nsomething unexplained by the difference between the two. Stick to\none style (I personally prefer the \"indent to the same level as the\nstarting 'cat' and ending 'EOF'\" but it is OK to pick the other one,\nas long as it is consistent within a single test script).\n\n> +\tcat >note-file <<-EOF &&\n> +\t\t${LF}\n> +\t\tfirst-line\n> +\t\t${MULTI_LF}\n> +\t\tsecond-line\n> +\t\t${LF}\n> +\tEOF\n\nThis is a bit misleading, as there are TWO blank lines before the\n\"first-line\" (one from the here text itself, the other is from\n${LF}). \n\nI do not think it matters too much, because the point of stripspace\nis to remove any number of leading or trailing blank lines, and\nsquash one or more blank lines elsewhere into one, so having two\nblank lines at the beginning or at the end of the file is just as\ngood an example as having a single blank line. I am mentioning it\nprimarily because I had to spend some time thinking about ways to\nmake it less misleading.\n"},{"id":"476042","messageId":"xmqqsfcn27e0.fsf@gitster.g","threadId":"59295","inReplyTo":"be86f9cac3e651ab5aab49f89b6506012b756dab.1682429602.git.dyroneteng@gmail.com","subject":"Re: [PATCH v8 4/6] notes.c: introduce '--separator=' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-25T17:34:15Z","receivedAt":"2023-04-25T17:34:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n> From: Teng Long \n>\n> When adding new notes or appending to an existing notes, we will\n> insert a blank line between the paragraphs, like:\n>\n> $ git notes add -m foo -m bar\n> $ git notes show HEAD | cat\n> foo\n>\n> bar\n>\n> The default behavour sometimes is not enough, the user may want\n> to use a custom delimiter between paragraphs, like when\n> specifying '-m', '-F', '-C', '-c' options. So this commit\n> introduce a new '--separator' option for 'git notes add' and\n> 'git notes append', for example when executing:\n>\n> $ git notes add -m foo -m bar --separator=\"-\"\n> $ git notes show HEAD | cat\n> foo\n> -\n> bar\n>\n> a newline is added to the value given to --separator if it\n> does not end with one already. So when executing:\n>\n> $ git notes add -m foo -m bar --separator=\"-\"\n> and\n> $ export LF=\"\n> \"\n> $ git notes add -m foo -m bar --separator=\"-$LF\"\n>\n> Both the two exections produce the same result.\n>\n> The reason we use a \"strbuf\" array to concat but not \"string_list\", is\n> that the binary file content may contain '\\0' in the middle, this will\n> cause the corrupt result if using a string to save.\n>\n> Signed-off-by: Teng Long \n> ---\n> Documentation/git-notes.txt | 21 ++++--\n> builtin/notes.c | 111 ++++++++++++++++++++++++-------\n> t/t3301-notes.sh | 126 ++++++++++++++++++++++++++++++++++++\n> 3 files changed, 229 insertions(+), 29 deletions(-)\n>\n> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\n> index efbc10f0..59980b21 100644\n> --- a/Documentation/git-notes.txt\n> +++ b/Documentation/git-notes.txt\n> @@ -9,9 +9,9 @@ SYNOPSIS\n> --------\n> [verse]\n> 'git notes' [list []]\n> -'git notes' add [-f] [--allow-empty] [-F | -m | (-c | -C) ] []\n> +'git notes' add [-f] [--allow-empty] [--separator=] [-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> @@ -65,7 +65,9 @@ add::\n> \tHowever, if you're using `add` interactively (using an editor\n> \tto supply the notes contents), then - instead of aborting -\n> \tthe existing notes will be opened in the editor (like the `edit`\n> -\tsubcommand).\n> +\tsubcommand). If you specify multiple `-m` and `-F`, a blank\n> +\tline will be inserted between the messages. Use the `--separator`\n> +\toption to insert other delimiters.\n> \n> copy::\n> \tCopy the notes for the first object onto the second object (defaults to\n> @@ -85,8 +87,12 @@ corresponding . (The optional `` is ignored so that\n> 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> +\tAppend new message(s) given by `-m` or `-F` options to an\n> +\texisting note, or add them as a new note if one does not\n> +\texist, for the object (defaults to HEAD). When appending to\n> +\tan existing note, a blank line is added before each new\n> +\tmessage as an inter-paragraph separator. The separator can\n> +\tbe customized with the `--separator` option.\n> \n> edit::\n> \tEdit the notes for a given object (defaults to HEAD).\n> @@ -159,6 +165,11 @@ 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 a string used as a custom inter-paragraph separator\n> +\t(a newline is added at the end as needed). Defaults to a\n> +\tblank line.\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 9d8ca795..6ae8b57b 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -8,6 +8,7 @@\n> */\n> \n> #include \"cache.h\"\n> +#include \"alloc.h\"\n> #include \"config.h\"\n> #include \"builtin.h\"\n> #include \"gettext.h\"\n> @@ -27,11 +28,12 @@\n> #include \"worktree.h\"\n> #include \"write-or-die.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> +\tN_(\"git notes [--ref ] add [-f] [--allow-empty] [--separator=] [-m | -F | (-c | -C) ] []\"),\n> \tN_(\"git notes [--ref ] copy [-f] \"),\n> -\tN_(\"git notes [--ref ] append [--allow-empty] [-m | -F | (-c | -C) ] []\"),\n> +\tN_(\"git notes [--ref ] append [--allow-empty] [--separator=] [-m | -F | (-c | -C) ] []\"),\n> \tN_(\"git notes [--ref ] edit [--allow-empty] []\"),\n> \tN_(\"git notes [--ref ] show []\"),\n> \tN_(\"git notes [--ref ] merge [-v | -q] [-s ] \"),\n> @@ -99,11 +101,19 @@ static const char * const git_notes_get_ref_usage[] = {\n> static const char note_template[] =\n> \tN_(\"Write/edit the notes for the following object:\");\n> \n> +struct note_msg {\n> +\tint stripspace;\n> +\tstruct strbuf buf;\n> +};\n> +\n> struct note_data {\n> \tint given;\n> \tint use_editor;\n> \tchar *edit_path;\n> \tstruct strbuf buf;\n> +\tstruct note_msg **messages;\n> +\tsize_t msg_nr;\n> +\tsize_t msg_alloc;\n> };\n> \n> static void free_note_data(struct note_data *d)\n> @@ -113,6 +123,13 @@ static void free_note_data(struct note_data *d)\n> \t\tfree(d->edit_path);\n> \t}\n> \tstrbuf_release(&d->buf);\n> +\n> +\twhile (d->msg_nr) {\n> +\t\t--d->msg_nr;\n> +\t\tstrbuf_release(&d->messages[d->msg_nr]->buf);\n> +\t\tfree(d->messages[d->msg_nr]);\n> +\t}\n\n while (d->msg_nr--) {\n strbuf_relesae(...);\n free(d->messages[d->msg_nr]);\n }\n\n> +\tfree(d->messages);\n> }\n\n\n\n> @@ -213,65 +230,98 @@ 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\nIf you massage an unspecified separator beforehand into \"\\n\"\n(e.g. initialize separator to \"\\n\" instead of NULL), you can lose\none of these three at runtime.\n\n> +static void concat_messages(struct note_data *d)\n> +{\n> +\tstruct strbuf msg = STRBUF_INIT;\n> +\n> +\tsize_t i;\n> +\tfor (i = 0; i < d->msg_nr ; i++) {\n> +\t\tif (d->buf.len)\n> +\t\t\tinsert_separator(&d->buf, d->buf.len);\n> +\t\tstrbuf_add(&msg, d->messages[i]->buf.buf, d->messages[i]->buf.len);\n> +\t\tstrbuf_addbuf(&d->buf, &msg);\n> +\t\tif (d->messages[i]->stripspace)\n> +\t\t\tstrbuf_stripspace(&d->buf, 0);\n> +\t\tstrbuf_reset(&msg);\n> +\t}\n> +\tstrbuf_release(&msg);\n> +}\n\nInteresting. \n\nI would have expected that where we add to d->messages[] array a new\nnote_msg structure and set its .stripspace member, we already knew\nif the associated strbuf needs to be stripspace'd and instead of\nmaintaining the .stripspace member for each note_msg, we can just\nhave it contain only a string (not even a strbuf).\n\nThe above loop, and the design to have .stripspace per each\nnote_msg, smell iffy to me for one thing. The .stripspace member is\nused to flag \"after adding this element, the whole thing need to be\ntreated with stripspace\". Is it intended? What it means is that if\nyou have two elements in d->messages[] array, one of them with and\nthe other one without the .stripspace bit set, depending on the\norder of the elements, the result would be vastly different. When\nthe later element has the stripspace bit set, everything gets\ncleansed, but when the earlier element has it, only the earlier\nelement gets cleansed and the later element is added with multiple\nblank lines and trailing whitespaces intact. It does not sound\nquite right.\n\nI wonder if the semantics intended (not implemented) by this series\nis to concatenate if none of the elements want stripspace and cleanse\nthe whitespaces if any of them want stripspace, in which case an\nobvious implementation would be to just concatenate everything in\nthe loop with separators, while seeing if any of them have the\nstripspace bit set, and then after the loop finishes, run stripspace\non the whole thing just once if any of d->messages[] asked for it?\nIf that is the case, an even better design could be to move the\nstripspace bit out of d->messages[] and make it d->stripspace to\napply to the whole thing.\n\nAnother possiblity is to just stripspace the elements taken from the\nsource you want to stripspace (like \"-m\" and \"-F\" but not the ones\ntaken from parse_reuse_arg()) before you even add them to\nd->messages[] array. That way, the only possible source of multiple\nblank lines and trailing whitespaces in the concatenation of\nelements you want stripspace would be from the separator, which is\nunder control by the end user. So your concatenation could just\nignore running stripspace at all---if the user does (or does not)\nwant multiple blank lines or trailing whitespaces on the separator\nline, that's under their control. That sounds like the simplest and\ncleanest design---we strip as we read from each source and make them\ninto simple strings to be kept in d->messages[] without having to\nallocate and keep a strbuf per each source, and we concatenate just\nonce, without worrying about stripspace.\n\nThe simple approach may break when any of the elements ended up to\nbe truly empty, but then we can solve that by refraining to push an\nempty result into d->messages[] array, or something? I dunno.\n"},{"id":"476043","messageId":"xmqqleif27bk.fsf@gitster.g","threadId":"59295","inReplyTo":"be86f9cac3e651ab5aab49f89b6506012b756dab.1682429602.git.dyroneteng@gmail.com","subject":"Re: [PATCH v8 4/6] notes.c: introduce '--separator=' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-25T17:35:43Z","receivedAt":"2023-04-25T17:35:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n> From: Teng Long \n>\n> When adding new notes or appending to an existing notes, we will\n> insert a blank line between the paragraphs, like:\n>\n> $ git notes add -m foo -m bar\n> $ git notes show HEAD | cat\n> foo\n>\n> bar\n\nThat's a pointless piping to \"cat\". Piping to \"cat -n\" might be a\ngood way to illustrate what happens, though, i.e.\n\n $ git notes show HEAD | cat -n\n 1 foo\n 2\n 3 bar\n\nI have no strong preference either way (the other more obvious\nalternative is to just remove \"| cat\").\n"},{"id":"476044","messageId":"xmqqa5yv26ry.fsf@gitster.g","threadId":"59295","inReplyTo":"ef40e0efc34ce6b6ee5b9b67bdde3dae02652cec.1682429602.git.dyroneteng@gmail.com","subject":"Re: [PATCH v8 5/6] notes.c: append separator instead of insert by pos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-25T17:47:29Z","receivedAt":"2023-04-25T17:47:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n> From: Teng Long \n>\n> This commit rename \"insert_separator\" to \"append_separator\" and also\n\n\"This commit rename\" -> \"Rename\".\n\n> remove the \"postion\" argument, this serves two purpose:\n>\n> The first is that when specifying more than one \"-m\" ( like \"-F\", etc)\n> to \"git notes add\" or \"git notes append\", the order of them matters,\n> which means we need to append the each separator and message in turn,\n> so we don't have to make the caller specify the position, the \"append\"\n> operation is enough and clear.\n\nOK.\n\n> The second is that when we execute the \"git notes append\" subcommand\n> , we need to combine the \"prev_note\" and \"current_note\" to get the\n\nFunny placement of comma?\n\n> +static void append_separator(struct strbuf *message)\n> {\n> \tif (!separator)\n> -\t\tstrbuf_insertstr(message, pos, \"\\n\");\n> +\t\tstrbuf_insertstr(message, message->len, \"\\n\");\n> \telse if (separator[strlen(separator) - 1] == '\\n')\n> -\t\tstrbuf_insertstr(message, pos, separator);\n> +\t\tstrbuf_insertstr(message, message->len, separator);\n> \telse\n> -\t\tstrbuf_insertf(message, pos, \"%s%s\", separator, \"\\n\");\n> +\t\tstrbuf_insertf(message, message->len, \"%s%s\", separator, \"\\n\");\n> }\n\nIs it still needed to use strbuf_insert*() variants when the only\nthing being done is to append at the end? Use of strbuf_add*()\nwould let you drop message->len from the arguments.\n"},{"id":"476045","messageId":"xmqq4jp326oj.fsf@gitster.g","threadId":"59295","inReplyTo":"f60f743203d78a489b90df81472e71391b45367d.1682429602.git.dyroneteng@gmail.com","subject":"Re: [PATCH v8 6/6] notes.c: introduce \"--[no-]stripspace\" option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-25T17:49:32Z","receivedAt":"2023-04-25T17:49:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n> From: Teng Long \n>\n> This commit introduces a new option \"--[no-]stripspace\" to git notes\n> append, git notes edit, and git notes add. This option allows users to\n> control whether the note message need to stripped out.\n\nMakes sense.\n\n> ... One more thing need to note is \"the order of\n> the options matter\", that is, if you specify \"-C\" before \"-m\" or\n> \"-F\", the reused message by \"-C\" will be stripped out together,\n> because everytime concat \"-m\" or \"-F\" message, the concated message\n> will be stripped together. Oppositely, if you specify \"-m\" or \"-F\"\n> before \"-C\", the reused message by \"-C\" will not be stripped out.\n\nThis sounds more like a design/implementation mistake that we may\nwant to fix.\n\n"},{"id":"476161","messageId":"20230427034711.27680-1-tenglong.tl@alibaba-inc.com","threadId":"59295","inReplyTo":"xmqqjzy03p4w.fsf@gitster.g","subject":"Re: [PATCH v8 3/6] t3321: add test cases about the notes stripspace behavior","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-04-27T03:47:11Z","receivedAt":"2023-04-27T03:47:22Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Junio C Hamano writes:\n\n>> ---\n>> t/t3321-notes-stripspace.sh | 291 ++++++++++++++++++++++++++++++++++++\n>> 1 file changed, 291 insertions(+)\n>> create mode 100755 t/t3321-notes-stripspace.sh\n>\n>Looks quite thorough.\n\nI'm not sure if I've covered everything, but I've tried to addi\nsome basic test cases to give us a chance to add on top of that later.\n\n>> diff --git a/t/t3321-notes-stripspace.sh b/t/t3321-notes-stripspace.sh\n>> new file mode 100755\n>> index 00000000..7c26b184\n>> --- /dev/null\n>> +++ b/t/t3321-notes-stripspace.sh\n>> @@ -0,0 +1,291 @@\n>> +#!/bin/sh\n>> +#\n>> +# Copyright (c) 2007 Teng Long\n>\n>It's 2023 now.\n\nWill fix.\n\n>> +test_expect_success 'add note by editor' '\n>> +\ttest_when_finished \"git notes remove\" &&\n>> +\tcat >expect <<-EOF &&\n>> +\t\tfirst-line\n>> +\n>> +\t\tsecond-line\n>> +\tEOF\n>> ...\n>> +test_expect_success 'append note by specifying multiple \"-m\"' '\n>> +\ttest_when_finished \"git notes remove\" &&\n>> +\tcat >expect <<-EOF &&\n>> +\tfirst-line\n>> +\n>> +\tsecond-line\n>> +\tEOF\n>\n>Inconsistent indentation confuses readers if the author meant\n>something unexplained by the difference between the two. Stick to\n>one style (I personally prefer the \"indent to the same level as the\n>starting 'cat' and ending 'EOF'\" but it is OK to pick the other one,\n>as long as it is consistent within a single test script).\n\nI will follow as your prefer format, thanks.\n\n>> +\tcat >note-file <<-EOF &&\n>> +\t\t${LF}\n>> +\t\tfirst-line\n>> +\t\t${MULTI_LF}\n>> +\t\tsecond-line\n>> +\t\t${LF}\n>> +\tEOF\n>\n>This is a bit misleading, as there are TWO blank lines before the\n>\"first-line\" (one from the here text itself, the other is from\n>${LF}). \n>\n>I do not think it matters too much, because the point of stripspace\n>is to remove any number of leading or trailing blank lines, and\n>squash one or more blank lines elsewhere into one, so having two\n>blank lines at the beginning or at the end of the file is just as\n>good an example as having a single blank line. I am mentioning it\n>primarily because I had to spend some time thinking about ways to\n>make it less misleading.\n\nYes, a little bit, I don't found a more clealy so far. It works for\nnow but I'm willing to optimize if there's a better way.\n\nThanks.\n"},{"id":"476172","messageId":"20230427072107.29821-1-tenglong.tl@alibaba-inc.com","threadId":"59295","inReplyTo":"xmqqsfcn27e0.fsf@gitster.g","subject":"Re: [PATCH v8 4/6] notes.c: introduce '--separator=' option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-04-27T07:21:07Z","receivedAt":"2023-04-27T07:22:16Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Junio C Hamano writes:\n\n>> +\twhile (d->msg_nr) {\n>> +\t\t--d->msg_nr;\n>> +\t\tstrbuf_release(&d->messages[d->msg_nr]->buf);\n>> +\t\tfree(d->messages[d->msg_nr]);\n>> +\t}\n>\n> while (d->msg_nr--) {\n> strbuf_relesae(...);\n> free(d->messages[d->msg_nr]);\n> }\n\nWill fix or optimize.\n\n>> +static void concat_messages(struct note_data *d)\n>> +{\n>> +\tstruct strbuf msg = STRBUF_INIT;\n>> +\n>> +\tsize_t i;\n>> +\tfor (i = 0; i < d->msg_nr ; i++) {\n>> +\t\tif (d->buf.len)\n>> +\t\t\tinsert_separator(&d->buf, d->buf.len);\n>> +\t\tstrbuf_add(&msg, d->messages[i]->buf.buf, d->messages[i]->buf.len);\n>> +\t\tstrbuf_addbuf(&d->buf, &msg);\n>> +\t\tif (d->messages[i]->stripspace)\n>> +\t\t\tstrbuf_stripspace(&d->buf, 0);\n>> +\t\tstrbuf_reset(&msg);\n>> +\t}\n>> +\tstrbuf_release(&msg);\n>> +}\n>\n>Interesting. \n>\n>I would have expected that where we add to d->messages[] array a new\n>note_msg structure and set its .stripspace member, we already knew\n>if the associated strbuf needs to be stripspace'd and instead of\n>maintaining the .stripspace member for each note_msg, we can just\n>have it contain only a string (not even a strbuf).\n\nThat's because the \"-C\" option, it will not do stripspace for the\nwhole note, but support to use together with \"-m\" or \"-F\"(they will\ndo stripspace). So, for the consistent result with backforward\ncompatibility, we have to get the information about whether to do\nstripspace when concating each message.\n\n>The above loop, and the design to have .stripspace per each\n>note_msg, smell iffy to me for one thing. The .stripspace member is\n>used to flag \"after adding this element, the whole thing need to be\n>treated with stripspace\". Is it intended? What it means is that if\n>you have two elements in d->messages[] array, one of them with and\n>the other one without the .stripspace bit set, depending on the\n>order of the elements, the result would be vastly different. When\n>the later element has the stripspace bit set, everything gets\n>cleansed, but when the earlier element has it, only the earlier\n>element gets cleansed and the later element is added with multiple\n>blank lines and trailing whitespaces intact. It does not sound\n>quite right.\n\nIn the previous patch, I actually followed your suggestion to only\nstripspace message itself but not the whole concatenate message. But\nactually, there's a problem with this, in fact it's also caused by\n\"-C\", \"-C\" doesn't stripspace, means that when mixed with \"-m\" and\n\"-F\", the order of the options matters.\n\nI'm not sure if this is a design flaw or if it's intentional, but\nit's special about the stripspace handling of \"-C\" right now, which\nis why I've added two new test cases in [3/6], just to make sure my\nsubsequent patches don't break them:\n\n * 'add notes with \"-C\" and \"-m\", \"-m\" will stripspace all together'\n\n * 'add notes with \"-m\" and \"-C\", \"-C\" will not stripspace all together'\n\n>I wonder if the semantics intended (not implemented) by this series\n>is to concatenate if none of the elements want stripspace and cleanse\n>the whitespaces if any of them want stripspace, in which case an\n>obvious implementation would be to just concatenate everything in\n>the loop with separators, while seeing if any of them have the\n>stripspace bit set, and then after the loop finishes, run stripspace\n>on the whole thing just once if any of d->messages[] asked for it?\n>If that is the case, an even better design could be to move the\n>stripspace bit out of d->messages[] and make it d->stripspace to\n>apply to the whole thing.\n\nI agree, but this seems to break the effect of \"the order matters\" because of\nthe \"-C\" argument. \n\nIf we think that the \"-C\" particularity is a case that should be optimized or\nfixed(), then I strongly agree with this modification approach, and I think\nthat compared to this particularity, on the one hand, our implementation\nis simple, on the other hand, users may find it easier to understand.\n\n>Another possiblity is to just stripspace the elements taken from the\n>source you want to stripspace (like \"-m\" and \"-F\" but not the ones\n>taken from parse_reuse_arg()) before you even add them to\n>d->messages[] array. That way, the only possible source of multiple\n>blank lines and trailing whitespaces in the concatenation of\n>elements you want stripspace would be from the separator, which is\n>under control by the end user. ...\n\nI think this approach is feasible, and add judgment logic when supporting\nsubsequent --[no-] stripspaces.\n\n>... So your concatenation could just\n>ignore running stripspace at all---if the user does (or does not)\n>want multiple blank lines or trailing whitespaces on the separator\n>line, that's under their control. That sounds like the simplest and\n>cleanest design---we strip as we read from each source and make them\n>into simple strings to be kept in d->messages[] without having to\n>allocate and keep a strbuf per each source, and we concatenate just\n>once, without worrying about stripspace.\n\nThe strbuf can record the \"len\" of the buf, to make sure the string\nwill be cut of if the content contains '\\0' in the middle, like a binary\nfile. Maybe using a string could make that, but I‘m not sure how to solve\nit.\n\n>The simple approach may break when any of the elements ended up to\n>be truly empty, but then we can solve that by refraining to push an\n>empty result into d->messages[] array, or something? I dunno.\n\nI will add some test cases about it and found it out.\n\nThanks.\n"},{"id":"476174","messageId":"20230427075138.62092-1-tenglong.tl@alibaba-inc.com","threadId":"59295","inReplyTo":"xmqqa5yv26ry.fsf@gitster.g","subject":"Re: [PATCH v8 5/6] notes.c: append separator instead of insert by pos","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-04-27T07:51:38Z","receivedAt":"2023-04-27T07:51:48Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Junio C Hamano writes:\n\n>> From: Teng Long \n>>\n>> This commit rename \"insert_separator\" to \"append_separator\" and also\n>\n>\"This commit rename\" -> \"Rename\".\n>\n>...\n>\n>> The second is that when we execute the \"git notes append\" subcommand\n>> , we need to combine the \"prev_note\" and \"current_note\" to get the\n>\n>Funny placement of comma?\n>\n>> +static void append_separator(struct strbuf *message)\n>> {\n>> \tif (!separator)\n>> -\t\tstrbuf_insertstr(message, pos, \"\\n\");\n>> +\t\tstrbuf_insertstr(message, message->len, \"\\n\");\n>> \telse if (separator[strlen(separator) - 1] == '\\n')\n>> -\t\tstrbuf_insertstr(message, pos, separator);\n>> +\t\tstrbuf_insertstr(message, message->len, separator);\n>> \telse\n>> -\t\tstrbuf_insertf(message, pos, \"%s%s\", separator, \"\\n\");\n>> +\t\tstrbuf_insertf(message, message->len, \"%s%s\", separator, \"\\n\");\n>> }\n>\n>Is it still needed to use strbuf_insert*() variants when the only\n>thing being done is to append at the end? Use of strbuf_add*()\n>would let you drop message->len from the arguments.\n\nWill fix.\n\nThanks.\n"},{"id":"476221","messageId":"xmqq8redkwyr.fsf@gitster.g","threadId":"59295","inReplyTo":"20230427072107.29821-1-tenglong.tl@alibaba-inc.com","subject":"Re: [PATCH v8 4/6] notes.c: introduce '--separator=' option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-27T18:21:16Z","receivedAt":"2023-04-27T18:21:21Z","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>>> +\twhile (d->msg_nr) {\n>>> +\t\t--d->msg_nr;\n>>> +\t\tstrbuf_release(&d->messages[d->msg_nr]->buf);\n>>> +\t\tfree(d->messages[d->msg_nr]);\n>>> +\t}\n>>\n>> while (d->msg_nr--) {\n>> strbuf_relesae(...);\n>> free(d->messages[d->msg_nr]);\n>> }\n>\n> Will fix or optimize.\n\nNot a huge thing and I would be very surprised if you saw a\ndifferent machine code output. The original stood out like a sore\nthumb at me, primarily because we rarely if ever do pre-decrement or\npre-increment in the void context in our own code.\n\n>>Interesting. \n>>\n>>I would have expected that where we add to d->messages[] array a new\n>>note_msg structure and set its .stripspace member, we already knew\n>>if the associated strbuf needs to be stripspace'd and instead of\n>>maintaining the .stripspace member for each note_msg, we can just\n>>have it contain only a string (not even a strbuf).\n>\n> That's because the \"-C\" option, it will not do stripspace for the\n> whole note, but support to use together with \"-m\" or \"-F\"(they will\n> do stripspace). So, for the consistent result with backforward\n> compatibility, we have to get the information about whether to do\n> stripspace when concating each message.\n\nOK.\n\nThanks.\n"},{"id":"476255","messageId":"20230428074013.22998-1-tenglong.tl@alibaba-inc.com","threadId":"59295","inReplyTo":"xmqq4jp326oj.fsf@gitster.g","subject":"Re: [PATCH v8 6/6] notes.c: introduce \"--[no-]stripspace\" option","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2023-04-28T07:40:13Z","receivedAt":"2023-04-28T07:41:06Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":">Teng Long writes:\n>\n>> From: Teng Long \n>>\n>> This commit introduces a new option \"--[no-]stripspace\" to git notes\n>> append, git notes edit, and git notes add. This option allows users to\n>> control whether the note message need to stripped out.\n>\n>Makes sense.\n>\n>> ... One more thing need to note is \"the order of\n>> the options matter\", that is, if you specify \"-C\" before \"-m\" or\n>> \"-F\", the reused message by \"-C\" will be stripped out together,\n>> because everytime concat \"-m\" or \"-F\" message, the concated message\n>> will be stripped together. Oppositely, if you specify \"-m\" or \"-F\"\n>> before \"-C\", the reused message by \"-C\" will not be stripped out.\n>\n>This sounds more like a design/implementation mistake that we may\n>want to fix.\n\nI doubted this either, but for compatibility, implementations of\nthis patchset have been made, such as \"note_msg\" recording the\n\"stripspace\" field, to implement these two new options without\nbreaking the old behavior.\n\nThanks.\n"},{"id":"476280","messageId":"xmqqo7n7eukj.fsf@gitster.g","threadId":"59295","inReplyTo":"20230428074013.22998-1-tenglong.tl@alibaba-inc.com","subject":"Re: [PATCH v8 6/6] notes.c: introduce \"--[no-]stripspace\" option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-28T18:21:48Z","receivedAt":"2023-04-28T18:21:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n>>Teng Long writes:\n>>\n>>> From: Teng Long \n>>>\n>>> This commit introduces a new option \"--[no-]stripspace\" to git notes\n>>> append, git notes edit, and git notes add. This option allows users to\n>>> control whether the note message need to stripped out.\n>>\n>>Makes sense.\n>>\n>>> ... One more thing need to note is \"the order of\n>>> the options matter\", that is, if you specify \"-C\" before \"-m\" or\n>>> \"-F\", the reused message by \"-C\" will be stripped out together,\n>>> because everytime concat \"-m\" or \"-F\" message, the concated message\n>>> will be stripped together. Oppositely, if you specify \"-m\" or \"-F\"\n>>> before \"-C\", the reused message by \"-C\" will not be stripped out.\n>>\n>>This sounds more like a design/implementation mistake that we may\n>>want to fix.\n>\n> I doubted this either, but for compatibility, implementations of\n> this patchset have been made, such as \"note_msg\" recording the\n> \"stripspace\" field, to implement these two new options without\n> breaking the old behavior.\n\nThanks, that is a sensible stance to take.\n"}]}