{"thread":{"id":"62400","subject":"[RFC PATCH] notes: add prepend command","startedAt":"2024-10-23T20:16:16Z","lastAt":"2024-10-26T22:35:16Z","messageCount":5,"participants":["Bence Ferdinandy","Taylor Blau","Đoàn Trần Công Danh"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"505971","messageId":"20241023201430.986389-1-bence@ferdinandy.com","threadId":"62400","inReplyTo":null,"subject":"[RFC PATCH] notes: add prepend command","fromName":"Bence Ferdinandy","fromEmail":"bence@ferdinandy.com","sentAt":"2024-10-23T20:14:24Z","receivedAt":"2024-10-23T20:16:16Z","isPatch":true,"sender":{"key":"bence@ferdinandy.com","avatar":"https://avatars.githubusercontent.com/u/6343487?v=4"},"body":"When a note is detailing commit history, it makes sense to keep the\nlatest change on top, but unlike adding things at the bottom with\n\"git notes append\" this can only be done manually. Add a\n\n    git notes prepend\n\ncommand, which works exactly like the append command, except that it\ninserts the text before the current contents of the note instead of\nafter.\n\nSigned-off-by: Bence Ferdinandy <bence@ferdinandy.com>\n---\n\nNotes:\n    RFC v1: Cf.\n        https://lore.kernel.org/git/20241023153736.257733-1-bence@ferdinandy.com/T/#m5b6644827590c2518089ab84f936a970c4e9be0f\n    \n        For that particular series I've used\n        git rev-list HEAD~8..HEAD | xargs -i git notes append {} -m \"v12: no change\"\n        for a quick-start on updating notes, when only 1 note needed to be\n        really edited with meaningful content, and for some of the patches\n        you now need to scroll a bit to actually find that \"no change\" text,\n        instead of seeing it right at the top.\n\n builtin/notes.c | 32 ++++++++++++++++++++++++++------\n 1 file changed, 26 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 8c26e45526..cf158cab1c 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -35,6 +35,7 @@ static const char * const git_notes_usage[] = {\n \tN_(\"git notes [--ref <notes-ref>] add [-f] [--allow-empty] [--[no-]separator|--separator=<paragraph-break>] [--[no-]stripspace] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]\"),\n \tN_(\"git notes [--ref <notes-ref>] copy [-f] <from-object> <to-object>\"),\n \tN_(\"git notes [--ref <notes-ref>] append [--allow-empty] [--[no-]separator|--separator=<paragraph-break>] [--[no-]stripspace] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]\"),\n+\tN_(\"git notes [--ref <notes-ref>] prepend [--allow-empty] [--[no-]separator|--separator=<paragraph-break>] [--[no-]stripspace] [-m <msg> | -F <file> | (-c | -C) <object>] [<object>]\"),\n \tN_(\"git notes [--ref <notes-ref>] edit [--allow-empty] [<object>]\"),\n \tN_(\"git notes [--ref <notes-ref>] show [<object>]\"),\n \tN_(\"git notes [--ref <notes-ref>] merge [-v | -q] [-s <strategy>] <notes-ref>\"),\n@@ -644,7 +645,8 @@ static int copy(int argc, const char **argv, const char *prefix)\n \treturn retval;\n }\n \n-static int append_edit(int argc, const char **argv, const char *prefix)\n+\n+static int append_prepend_edit(int argc, const char **argv, const char *prefix, int prepend)\n {\n \tint allow_empty = 0;\n \tconst char *object_ref;\n@@ -716,11 +718,18 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \n \t\tif (!prev_buf)\n \t\t\tdie(_(\"unable to read %s\"), oid_to_hex(note));\n-\t\tif (size)\n-\t\t\tstrbuf_add(&buf, prev_buf, size);\n-\t\tif (d.buf.len && size)\n-\t\t\tappend_separator(&buf);\n-\t\tstrbuf_insert(&d.buf, 0, buf.buf, buf.len);\n+\t\tif (prepend) {\n+\t\t\tif (d.buf.len && size)\n+\t\t\t\tappend_separator(&buf);\n+\t\t\tif (size)\n+\t\t\t\tstrbuf_add(&buf, prev_buf, size);\n+\t\t} else {\n+\t\t\tif (size)\n+\t\t\t\tstrbuf_add(&buf, prev_buf, size);\n+\t\t\tif (d.buf.len && size)\n+\t\t\t\tappend_separator(&buf);\n+\t\t}\n+\t\tstrbuf_insert(&d.buf, prepend ? d.buf.len : 0, buf.buf, buf.len);\n \n \t\tfree(prev_buf);\n \t\tstrbuf_release(&buf);\n@@ -745,6 +754,16 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static int prepend_edit(int argc, const char **argv, const char *prefix)\n+{\n+\treturn append_prepend_edit(argc, argv, prefix, 1);\n+}\n+\n+static int append_edit(int argc, const char **argv, const char *prefix)\n+{\n+\treturn append_prepend_edit(argc, argv, prefix, 0);\n+}\n+\n static int show(int argc, const char **argv, const char *prefix)\n {\n \tconst char *object_ref;\n@@ -1116,6 +1135,7 @@ int cmd_notes(int argc,\n \t\tOPT_SUBCOMMAND(\"add\", &fn, add),\n \t\tOPT_SUBCOMMAND(\"copy\", &fn, copy),\n \t\tOPT_SUBCOMMAND(\"append\", &fn, append_edit),\n+\t\tOPT_SUBCOMMAND(\"prepend\", &fn, prepend_edit),\n \t\tOPT_SUBCOMMAND(\"edit\", &fn, append_edit),\n \t\tOPT_SUBCOMMAND(\"show\", &fn, show),\n \t\tOPT_SUBCOMMAND(\"merge\", &fn, merge),\n-- \n2.47.0.119.g5b706304f7.dirty\n\n"},{"id":"505974","messageId":"ZxlahJygsRFcxDev@nand.local","threadId":"62400","inReplyTo":"20241023201430.986389-1-bence@ferdinandy.com","subject":"Re: [RFC PATCH] notes: add prepend command","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-23T20:20:20Z","receivedAt":"2024-10-23T20:20:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Oct 23, 2024 at 10:14:24PM +0200, Bence Ferdinandy wrote:\n> When a note is detailing commit history, it makes sense to keep the\n> latest change on top, but unlike adding things at the bottom with\n> \"git notes append\" this can only be done manually. Add a\n>\n>     git notes prepend\n>\n> command, which works exactly like the append command, except that it\n> inserts the text before the current contents of the note instead of\n> after.\n\nHmmm. I am not sure that I see the widespread need for such a tool. If\nthis is specific to your use-case, I think a custom script and\n`$GIT_EDITOR` would do the trick.\n\nThanks,\nTaylor\n"},{"id":"505979","messageId":"D53GZBSWBUW2.36KFBIW6AERF9@ferdinandy.com","threadId":"62400","inReplyTo":"ZxlahJygsRFcxDev@nand.local","subject":"Re: [RFC PATCH] notes: add prepend command","fromName":"Bence Ferdinandy","fromEmail":"bence@ferdinandy.com","sentAt":"2024-10-23T20:32:08Z","receivedAt":"2024-10-23T20:32:47Z","isPatch":true,"sender":{"key":"bence@ferdinandy.com","avatar":"https://avatars.githubusercontent.com/u/6343487?v=4"},"body":"\nOn Wed Oct 23, 2024 at 22:20, Taylor Blau <me@ttaylorr.com> wrote:\n> On Wed, Oct 23, 2024 at 10:14:24PM +0200, Bence Ferdinandy wrote:\n>> When a note is detailing commit history, it makes sense to keep the\n>> latest change on top, but unlike adding things at the bottom with\n>> \"git notes append\" this can only be done manually. Add a\n>>\n>>     git notes prepend\n>>\n>> command, which works exactly like the append command, except that it\n>> inserts the text before the current contents of the note instead of\n>> after.\n>\n> Hmmm. I am not sure that I see the widespread need for such a tool. If\n> this is specific to your use-case, I think a custom script and\n> `$GIT_EDITOR` would do the trick.\n\nCouldn't the same argument be made for append? Imho, it's a missing symmetry.\nOfc it's probably not quite hard to script around this.\n\n"},{"id":"506012","messageId":"ZxotMcKv5rEIMZ8q@danh.dev","threadId":"62400","inReplyTo":"20241023201430.986389-1-bence@ferdinandy.com","subject":"Re: [RFC PATCH] notes: add prepend command","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2024-10-24T11:19:13Z","receivedAt":"2024-10-24T11:19:17Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2024-10-23 22:14:24+0200, Bence Ferdinandy <bence@ferdinandy.com> wrote:\n> -static int append_edit(int argc, const char **argv, const char *prefix)\n> +\n> +static int append_prepend_edit(int argc, const char **argv, const char *prefix, int prepend)\n>  {\n>  \tint allow_empty = 0;\n>  \tconst char *object_ref;\n> @@ -716,11 +718,18 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n>  \n>  \t\tif (!prev_buf)\n>  \t\t\tdie(_(\"unable to read %s\"), oid_to_hex(note));\n> -\t\tif (size)\n> -\t\t\tstrbuf_add(&buf, prev_buf, size);\n> -\t\tif (d.buf.len && size)\n> -\t\t\tappend_separator(&buf);\n> -\t\tstrbuf_insert(&d.buf, 0, buf.buf, buf.len);\n> +\t\tif (prepend) {\n> +\t\t\tif (d.buf.len && size)\n> +\t\t\t\tappend_separator(&buf);\n> +\t\t\tif (size)\n> +\t\t\t\tstrbuf_add(&buf, prev_buf, size);\n> +\t\t} else {\n> +\t\t\tif (size)\n> +\t\t\t\tstrbuf_add(&buf, prev_buf, size);\n> +\t\t\tif (d.buf.len && size)\n> +\t\t\t\tappend_separator(&buf);\n> +\t\t}\n> +\t\tstrbuf_insert(&d.buf, prepend ? d.buf.len : 0, buf.buf, buf.len);\n>  \n>  \t\tfree(prev_buf);\n>  \t\tstrbuf_release(&buf);\n\nWithout prejudice about whether we should take this command.\n(I think we shouldn't, just like we shouldn't top-posting).\n\nI think this diff should be written like this for easier reasoning:\n\n----- 8< -----------------\n@@ -711,19 +713,27 @@ 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\tstruct strbuf buf = STRBUF_INIT;\n \t\tchar *prev_buf = repo_read_object_file(the_repository, note, &type, &size);\n \n \t\tif (!prev_buf)\n \t\t\tdie(_(\"unable to read %s\"), oid_to_hex(note));\n-\t\tif (size)\n+\t\tif (!size) {\n+\t\t\t// no existing notes, use whatever we have here\n+\t\t} else if (prepend) {\n+\t\t\tif (d.buf.len)\n+\t\t\t\tappend_separator(&d.buf);\n+\t\t\tstrbuf_add(&d.buf, prev_buf, size);\n+\t\t} else {\n+\t\t\tstruct strbuf buf = STRBUF_INIT;\n \t\t\tstrbuf_add(&buf, prev_buf, size);\n-\t\tif (d.buf.len && size)\n-\t\t\tappend_separator(&buf);\n-\t\tstrbuf_insert(&d.buf, 0, buf.buf, buf.len);\n+\t\t\tif (d.buf.len)\n+\t\t\t\tappend_separator(&buf);\n+\t\t\tstrbuf_addbuf(&buf, &d.buf);\n+\t\t\tstrbuf_swap(&buf, &d.buf);\n+\t\t\tstrbuf_release(&buf);\n+\t\t}\n \n \t\tfree(prev_buf);\n-\t\tstrbuf_release(&buf);\n \t}\n \n \tif (d.buf.len || allow_empty) {\n-------------- 8< --------------------\n\nEven if we don't take this subcommand, I think we should re-write the\nappend part, so:\n- we can see the append logic better,\n- we can avoid the `strbuf_insert` which will require memmove/memcpy.\n\nWell, the second point is micro-optimisation, so take it with a grain\nof salt.\n\n\nAlso tests.\n-------------- 8< -----------------------\ndiff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\nindex 99137fb235731..5a7ad40fde6a8 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -558,6 +558,20 @@ test_expect_success 'listing non-existing notes fails' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'append: specify a separator with an empty arg' '\n+\ttest_when_finished git notes remove HEAD &&\n+\tcat >expect <<-\\EOF &&\n+\tnotes-2\n+\n+\tnotes-1\n+\tEOF\n+\n+\tgit notes add -m \"notes-1\" &&\n+\tgit notes prepend --separator=\"\" -m \"notes-2\" &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'append: specify a separator with an empty arg' '\n \ttest_when_finished git notes remove HEAD &&\n \tcat >expect <<-\\EOF &&\n----------- >8 --------------\n\n\n-- \nDanh\n"},{"id":"506141","messageId":"D563GBE4H09H.2JENKJVUOLMD6@ferdinandy.com","threadId":"62400","inReplyTo":"ZxotMcKv5rEIMZ8q@danh.dev","subject":"Re: [RFC PATCH] notes: add prepend command","fromName":"Bence Ferdinandy","fromEmail":"bence@ferdinandy.com","sentAt":"2024-10-26T22:34:03Z","receivedAt":"2024-10-26T22:35:16Z","isPatch":true,"sender":{"key":"bence@ferdinandy.com","avatar":"https://avatars.githubusercontent.com/u/6343487?v=4"},"body":"\nOn Thu Oct 24, 2024 at 13:19, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:\n> On 2024-10-23 22:14:24+0200, Bence Ferdinandy <bence@ferdinandy.com> wrote:\n>> -static int append_edit(int argc, const char **argv, const char *prefix)\n>> +\n>> +static int append_prepend_edit(int argc, const char **argv, const char *prefix, int prepend)\n>>  {\n>>  \tint allow_empty = 0;\n>>  \tconst char *object_ref;\n>> @@ -716,11 +718,18 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n>>  \n>>  \t\tif (!prev_buf)\n>>  \t\t\tdie(_(\"unable to read %s\"), oid_to_hex(note));\n>> -\t\tif (size)\n>> -\t\t\tstrbuf_add(&buf, prev_buf, size);\n>> -\t\tif (d.buf.len && size)\n>> -\t\t\tappend_separator(&buf);\n>> -\t\tstrbuf_insert(&d.buf, 0, buf.buf, buf.len);\n>> +\t\tif (prepend) {\n>> +\t\t\tif (d.buf.len && size)\n>> +\t\t\t\tappend_separator(&buf);\n>> +\t\t\tif (size)\n>> +\t\t\t\tstrbuf_add(&buf, prev_buf, size);\n>> +\t\t} else {\n>> +\t\t\tif (size)\n>> +\t\t\t\tstrbuf_add(&buf, prev_buf, size);\n>> +\t\t\tif (d.buf.len && size)\n>> +\t\t\t\tappend_separator(&buf);\n>> +\t\t}\n>> +\t\tstrbuf_insert(&d.buf, prepend ? d.buf.len : 0, buf.buf, buf.len);\n>>  \n>>  \t\tfree(prev_buf);\n>>  \t\tstrbuf_release(&buf);\n>\n> Without prejudice about whether we should take this command.\n> (I think we shouldn't, just like we shouldn't top-posting).\n\nAgain, I do not feel very strongly, about this patch, since it's not that hard\nto do with a script, but I don't think the analogy with top-posting is\nappropriate. It's usually not a discussion going on in the comments, and\nprepending might happen for any reason. The ordering of content in a note may\nnot even be temporal in nature (although to be fair, I have personally never\nused it for anything else than versioning patches).\n\nThe specific use-case came up in patch versioning (pointed out by Kristoffer),\nwhere in a longer series with many iterations, seeing the \"v1024: no change\" at\nthe top would save reviewers from having to scroll an indefinite amount in the\nparticular patch just to find that they actually don't need to look at that\none, since it hasn't changed since the previous iteration they saw. In this\nsense having the newest at the top rather than the bottom would be more\nnatural. I'd think probably even new reviewers jumping in during the middle\nmight not be very interested in the beginning.\n\n>\n> I think this diff should be written like this for easier reasoning:\n>\n> ----- 8< -----------------\n> @@ -711,19 +713,27 @@ 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\tstruct strbuf buf = STRBUF_INIT;\n>  \t\tchar *prev_buf = repo_read_object_file(the_repository, note, &type, &size);\n>  \n>  \t\tif (!prev_buf)\n>  \t\t\tdie(_(\"unable to read %s\"), oid_to_hex(note));\n> -\t\tif (size)\n> +\t\tif (!size) {\n> +\t\t\t// no existing notes, use whatever we have here\n> +\t\t} else if (prepend) {\n> +\t\t\tif (d.buf.len)\n> +\t\t\t\tappend_separator(&d.buf);\n> +\t\t\tstrbuf_add(&d.buf, prev_buf, size);\n> +\t\t} else {\n> +\t\t\tstruct strbuf buf = STRBUF_INIT;\n>  \t\t\tstrbuf_add(&buf, prev_buf, size);\n> -\t\tif (d.buf.len && size)\n> -\t\t\tappend_separator(&buf);\n> -\t\tstrbuf_insert(&d.buf, 0, buf.buf, buf.len);\n> +\t\t\tif (d.buf.len)\n> +\t\t\t\tappend_separator(&buf);\n> +\t\t\tstrbuf_addbuf(&buf, &d.buf);\n> +\t\t\tstrbuf_swap(&buf, &d.buf);\n> +\t\t\tstrbuf_release(&buf);\n> +\t\t}\n>  \n>  \t\tfree(prev_buf);\n> -\t\tstrbuf_release(&buf);\n>  \t}\n>  \n>  \tif (d.buf.len || allow_empty) {\n> -------------- 8< --------------------\n>\n> Even if we don't take this subcommand, I think we should re-write the\n> append part, so:\n> - we can see the append logic better,\n> - we can avoid the `strbuf_insert` which will require memmove/memcpy.\n\nThanks, I do find this a bit more easier to read indeed.\n\n>\n> Well, the second point is micro-optimisation, so take it with a grain\n> of salt.\n>\n>\n> Also tests.\n> -------------- 8< -----------------------\n> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\n> index 99137fb235731..5a7ad40fde6a8 100755\n> --- a/t/t3301-notes.sh\n> +++ b/t/t3301-notes.sh\n> @@ -558,6 +558,20 @@ test_expect_success 'listing non-existing notes fails' '\n>  \ttest_must_be_empty actual\n>  '\n>  \n> +test_expect_success 'append: specify a separator with an empty arg' '\n> +\ttest_when_finished git notes remove HEAD &&\n> +\tcat >expect <<-\\EOF &&\n> +\tnotes-2\n> +\n> +\tnotes-1\n> +\tEOF\n> +\n> +\tgit notes add -m \"notes-1\" &&\n> +\tgit notes prepend --separator=\"\" -m \"notes-2\" &&\n> +\tgit notes show >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_expect_success 'append: specify a separator with an empty arg' '\n>  \ttest_when_finished git notes remove HEAD &&\n>  \tcat >expect <<-\\EOF &&\n> ----------- >8 --------------\n\nThanks! I didn't look at tests (and documentation) before it was clear if the\nidea got a green light or not, but I guess if it does, this would cover tests.\n\nBest,\nBence\n\n-- \nbence.ferdinandy.com\n\n"}]}