{"thread":{"id":"60853","subject":"[PATCH 0/4] Speed up git-notes show","startedAt":"2024-02-05T20:49:43Z","lastAt":"2024-02-20T11:40:36Z","messageCount":36,"participants":["Maarten Bosmans","Eric Sunshine","Junio C Hamano","Kristoffer Haugsbakk","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"487951","messageId":"20240205204932.16653-1-maarten.bosmans@vortech.nl","threadId":"60853","inReplyTo":null,"subject":"[PATCH 0/4] Speed up git-notes show","fromName":"Maarten Bosmans","fromEmail":"mkbosmans@gmail.com","sentAt":"2024-02-05T20:49:28Z","receivedAt":"2024-02-05T20:49:43Z","isPatch":true,"sender":{"key":"mkbosmans@gmail.com","avatar":null},"body":"First time contributor here, trying my first git.git patch series.\n\nBACKGROUND\n We have a script that runs a range of build tests for all new commits in the\n repository and adds a line to the commit note with the result from the test.\n Something along the lines of:\n occa-build-jit-gnu-cuda-develop: PASSED (, 2024-01-01 00:00:00+01:00)\n Pretty useful to quickly check that all commits at least build, not only for\n master, but also in progress feature branches. (a passing test suite is\n generally only required at the merge point)\n \nPROBLEM\n The bash script loops over all remote refs and lists the commits newer than\n days ago. For each commit its note is read and grep'ed for an existing\n test name to see whether the build test needs to run again. The `git note show`\n command that is in this loop nest only takes 14ms to execute, but as it is in\n a loop, those times add up.\n\nANALYSIS\n When asked to show a note for a specific commit, git looks up the blob hash\n for the note and executes `git show` with that hash. That of course adds\n the child process overhead, but also causes the initialization of a lot of\n log related configuration, such as for decorations or the mailmap. Simply\n outputting the blob directly in the main process reduces the run time by\n almost halve.\n\nWhen looking through the git show implementation for useful stuff that command\ndoes that should also be done when showing a note, I could only find the\n`setup_pager()` call. All other git show functionality was related to showing\ncommits or other non-blob objects.\n\nThe only thing I was not 100% sure of was the textconv_object stuff. From what\nI could deduce was that this is only ever used on blobs that represent files in\na tree, not on blobs that represent note objects. So I did not include any\ntextconv calls in the git notes code.\n\nThe first commit is the main one fixing performance. The other three are just\neliminating some overhead I noticed when going through the git notes code.\n\n\nMaarten Bosmans (4):\n notes: print note blob to stdout directly\n notes: use exisisting function stream_blob_to_fd\n notes: do not clean up right before calling die()\n notes: use strbuf_attach to take ownership of the object contents\n\n builtin/notes.c | 37 ++++++++++---------------------------\n 1 file changed, 10 insertions(+), 27 deletions(-)\n\n-- \n2.35.3\n\n"},{"id":"487952","messageId":"20240205204932.16653-3-maarten.bosmans@vortech.nl","threadId":"60853","inReplyTo":"20240205204932.16653-1-maarten.bosmans@vortech.nl","subject":"[PATCH 2/4] notes: use exisisting function stream_blob_to_fd","fromName":"Maarten Bosmans","fromEmail":"mkbosmans@gmail.com","sentAt":"2024-02-05T20:49:30Z","receivedAt":"2024-02-05T20:49:44Z","isPatch":true,"sender":{"key":"mkbosmans@gmail.com","avatar":null},"body":"From: Maarten Bosmans \n\nFrom: Maarten Bosmans \n\nUse functionality from streaming.c and remove the copy_obj_to_fd()\nfunction local to notes.c.\n\nStreaming the blob to stdout instead of copying through an\nintermediate buffer might also be more efficient, but at the\nsize a typical note is, this is unlikely to matter a lot.\n\nSigned-off-by: Maarten Bosmans \n---\n builtin/notes.c | 14 +-------------\n 1 file changed, 1 insertion(+), 13 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 8efe9809b2..048b8c559c 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -149,18 +149,6 @@ static int list_each_note(const struct object_id *object_oid,\n \treturn 0;\n }\n \n-static void copy_obj_to_fd(int fd, const struct object_id *oid)\n-{\n-\tunsigned long size;\n-\tenum object_type type;\n-\tchar *buf = repo_read_object_file(the_repository, oid, &type, &size);\n-\tif (buf) {\n-\t\tif (size)\n-\t\t\twrite_or_die(fd, buf, size);\n-\t\tfree(buf);\n-\t}\n-}\n-\n static void write_commented_object(int fd, const struct object_id *object)\n {\n \tstruct child_process show = CHILD_PROCESS_INIT;\n@@ -205,7 +193,7 @@ static void prepare_note_data(const struct object_id *object, struct note_data *\n \t\tif (d->given)\n \t\t\twrite_or_die(fd, d->buf.buf, d->buf.len);\n \t\telse if (old_note)\n-\t\t\tcopy_obj_to_fd(fd, old_note);\n+\t\t\tstream_blob_to_fd(fd, old_note, NULL, 0);\n \n \t\tstrbuf_addch(&buf, '\\n');\n \t\tstrbuf_add_commented_lines(&buf, \"\\n\", strlen(\"\\n\"), comment_line_char);\n-- \n2.35.3\n\n"},{"id":"487953","messageId":"20240205204932.16653-2-maarten.bosmans@vortech.nl","threadId":"60853","inReplyTo":"20240205204932.16653-1-maarten.bosmans@vortech.nl","subject":"[PATCH 1/4] notes: print note blob to stdout directly","fromName":"Maarten Bosmans","fromEmail":"mkbosmans@gmail.com","sentAt":"2024-02-05T20:49:29Z","receivedAt":"2024-02-05T20:49:44Z","isPatch":true,"sender":{"key":"mkbosmans@gmail.com","avatar":null},"body":"From: Maarten Bosmans \n\nFrom: Maarten Bosmans \n\nAvoid the need to launch a subprocess by calling stream_blob_to_fd\ndirectly. This does not only get rid of the overhead of a separate\nchild process, but also avoids the initalization of the whole log\nmachinery that `git show` does. That is needed for example to show\ndecorated commits and applying the mailmap. For simply displaying\na blob however, the only useful thing show does is enabling the pager.\n\nSigned-off-by: Maarten Bosmans \n---\n builtin/notes.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex e65cae0bcf..8efe9809b2 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -20,7 +20,8 @@\n #include \"repository.h\"\n #include \"pretty.h\"\n #include \"refs.h\"\n-#include \"exec-cmd.h\"\n+#include \"pager.h\"\n+#include \"streaming.h\"\n #include \"run-command.h\"\n #include \"parse-options.h\"\n #include \"string-list.h\"\n@@ -751,7 +752,7 @@ static int show(int argc, const char **argv, const char *prefix)\n \tstruct notes_tree *t;\n \tstruct object_id object;\n \tconst struct object_id *note;\n-\tint retval;\n+\tint retval = 0;\n \tstruct option options[] = {\n \t\tOPT_END()\n \t};\n@@ -776,8 +777,9 @@ static int show(int argc, const char **argv, const char *prefix)\n \t\tretval = error(_(\"no note found for object %s.\"),\n \t\t\t oid_to_hex(&object));\n \telse {\n-\t\tconst char *show_args[3] = {\"show\", oid_to_hex(note), NULL};\n-\t\tretval = execv_git_cmd(show_args);\n+\t\tsetup_pager();\n+\t\tif (stream_blob_to_fd(1, note, NULL, 0))\n+\t\t\tdie(_(\"object %s is not a blob\"), oid_to_hex(note));\n \t}\n \tfree_notes(t);\n \treturn retval;\n-- \n2.35.3\n\n"},{"id":"487954","messageId":"20240205204932.16653-4-maarten.bosmans@vortech.nl","threadId":"60853","inReplyTo":"20240205204932.16653-1-maarten.bosmans@vortech.nl","subject":"[PATCH 3/4] notes: do not clean up right before calling die()","fromName":"Maarten Bosmans","fromEmail":"mkbosmans@gmail.com","sentAt":"2024-02-05T20:49:31Z","receivedAt":"2024-02-05T20:49:44Z","isPatch":true,"sender":{"key":"mkbosmans@gmail.com","avatar":null},"body":"From: Maarten Bosmans \n\nFrom: Maarten Bosmans \n\nFor consistency with the other uses of die() in the same function.\n\nSigned-off-by: Maarten Bosmans \n---\n builtin/notes.c | 6 +-----\n 1 file changed, 1 insertion(+), 5 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 048b8c559c..0e98a820dd 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -310,12 +310,8 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)\n \t\tdie(_(\"failed to resolve '%s' as a valid ref.\"), arg);\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(&msg->buf);\n-\t\tfree(value);\n-\t\tfree(msg);\n+\tif (type != OBJ_BLOB)\n \t\tdie(_(\"cannot read note data from non-blob object '%s'.\"), arg);\n-\t}\n \n \tstrbuf_add(&msg->buf, value, len);\n \tfree(value);\n-- \n2.35.3\n\n"},{"id":"487955","messageId":"20240205204932.16653-5-maarten.bosmans@vortech.nl","threadId":"60853","inReplyTo":"20240205204932.16653-1-maarten.bosmans@vortech.nl","subject":"[PATCH 4/4] notes: use strbuf_attach to take ownership of the object contents","fromName":"Maarten Bosmans","fromEmail":"mkbosmans@gmail.com","sentAt":"2024-02-05T20:49:32Z","receivedAt":"2024-02-05T20:49:44Z","isPatch":true,"sender":{"key":"mkbosmans@gmail.com","avatar":null},"body":"From: Maarten Bosmans \n\nFrom: Maarten Bosmans \n\nAvoid an extra allocation in the strbuf when pushing the string into it.\n\nSigned-off-by: Maarten Bosmans \n---\n builtin/notes.c | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 0e98a820dd..71f36667f3 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -313,10 +313,8 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)\n \tif (type != OBJ_BLOB)\n \t\tdie(_(\"cannot read note data from non-blob object '%s'.\"), arg);\n \n-\tstrbuf_add(&msg->buf, value, len);\n-\tfree(value);\n+\tstrbuf_attach(&msg->buf, value, len, len + 1);\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 = NO_STRIPSPACE;\n@@ -702,12 +700,11 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tchar *prev_buf = repo_read_object_file(the_repository, note, &type, &size);\n \n \t\tif (prev_buf && size)\n-\t\t\tstrbuf_add(&buf, prev_buf, size);\n+\t\t\tstrbuf_attach(&buf, prev_buf, size, size + 1);\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-- \n2.35.3\n\n"},{"id":"487956","messageId":"CAPig+cRVqnp8YTeDvxuM12B_bt7OoJJZc9_v=KtG_By0NJ9ZMA@mail.gmail.com","threadId":"60853","inReplyTo":"20240205204932.16653-3-maarten.bosmans@vortech.nl","subject":"Re: [PATCH 2/4] notes: use exisisting function stream_blob_to_fd","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-02-05T22:00:28Z","receivedAt":"2024-02-05T22:00:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Feb 5, 2024 at 4:53 PM Maarten Bosmans wrote:\n> notes: use exisisting function stream_blob_to_fd\n\ns/exisisting/existing/\n\n> Use functionality from streaming.c and remove the copy_obj_to_fd()\n> function local to notes.c.\n>\n> Streaming the blob to stdout instead of copying through an\n> intermediate buffer might also be more efficient, but at the\n> size a typical note is, this is unlikely to matter a lot.\n>\n> Signed-off-by: Maarten Bosmans \n"},{"id":"487989","messageId":"xmqqil32l0i6.fsf@gitster.g","threadId":"60853","inReplyTo":"20240205204932.16653-2-maarten.bosmans@vortech.nl","subject":"Re: [PATCH 1/4] notes: print note blob to stdout directly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-06T03:44:49Z","receivedAt":"2024-02-06T03:44:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Maarten Bosmans writes:\n\n> From: Maarten Bosmans \n>\n> From: Maarten Bosmans \n\nWhich one of you are you? Please make up your mind and use only one\n;-) IOW, the first one is unneeded, as the latter matches what you\nhave on the S-o-b line.\n\n> Avoid the need to launch a subprocess by calling stream_blob_to_fd\n> directly. This does not only get rid of the overhead of a separate\n> child process, but also avoids the initalization of the whole log\n> machinery that `git show` does. That is needed for example to show\n> decorated commits and applying the mailmap. For simply displaying\n> a blob however, the only useful thing show does is enabling the pager.\n>\n> Signed-off-by: Maarten Bosmans \n> ---\n> builtin/notes.c | 10 ++++++----\n> 1 file changed, 6 insertions(+), 4 deletions(-)\n\nI am not sure if we want to accept an approach that feels somewhat\nnarrow/short sighted, like this patch. When \"git show\" learns an\nimproved way to show blob objects, who will update the code this\npatch touches to teach it to use the same improved way to show the\nnotes?\n\nI actually was hoping, after seeing the use case description in the\ncover letter, that the series would be introducing a batch mode\ninterface to allow callers to ask notes for many objects and have\nthe command respond notes for these objects in a way that which\npiece of output corresponds to which object in the request, reducing\nthe process overhead amortised over many objects.\n\n"},{"id":"488042","messageId":"fa98146f-9308-4844-bec9-4605575b9cd9@app.fastmail.com","threadId":"60853","inReplyTo":"20240205204932.16653-1-maarten.bosmans@vortech.nl","subject":"Re: [PATCH 0/4] Speed up git-notes show","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-06T07:08:24Z","receivedAt":"2024-02-06T07:08:46Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Mon, Feb 5, 2024, at 21:49, Maarten Bosmans wrote:\n> BACKGROUND\n> We have a script that runs a range of build tests for all new\n> commits in the repository and adds a line to the commit note with\n> the result from the test.\n\nNice. I’ve seen a few tools that do that:\n\n• https://github.com/spotify/git-test\n• https://github.com/mhagger/git-test\n\nThanks for improving this use-case.\n\n-- \nKristoffer Haugsbakk\n"},{"id":"488054","messageId":"CA+CvcKRkj9HZ4jtN4R_KZo9U_0AjtAFhyLPzaLNzgow2ag=Esg@mail.gmail.com","threadId":"60853","inReplyTo":"fa98146f-9308-4844-bec9-4605575b9cd9@app.fastmail.com","subject":"Re: [PATCH 0/4] Speed up git-notes show","fromName":"Maarten Bosmans","fromEmail":"mkbosmans@gmail.com","sentAt":"2024-02-06T08:51:21Z","receivedAt":"2024-02-06T08:51:33Z","isPatch":true,"sender":{"key":"mkbosmans@gmail.com","avatar":null},"body":"Op di 6 feb 2024 om 08:08 schreef Kristoffer Haugsbakk :\n> Nice. I’ve seen a few tools that do that:\n>\n> • https://github.com/spotify/git-test\n> • https://github.com/mhagger/git-test\n\nCool to see others are doing something similar.\nThe first git-test stores the test results in a local cache though,\nnot in git notes.\nThe second git-test stores the results in a note attached to the tree\ninstead of the commit. This is a clever idea, as you avoid running\ntests twice on the same tree (e.g. the merge commit of a `git merge\n--no-ff` merged branch that was already rebased to be current). Down\nside is that git log and can display commit notes after the message,\nbut cannot display tree notes.\n\nIn our setup there's one CI server running these tests and pushing the\nnotes to the repo. This way everybody (developer, reviewer) can see\nthe state of in-progress work without having to run all build test\nvariations themselves.\nAnyway, good to see wider usage of the git notes feature as\ninspiration for new possibilities.\n\nMaarten\n"},{"id":"488062","messageId":"CA+CvcKTtcHCCKucQ0h1dnaDAMNfErJ+a1CXEVi=ZE5dv57Tb3A@mail.gmail.com","threadId":"60853","inReplyTo":"xmqqil32l0i6.fsf@gitster.g","subject":"Re: [PATCH 1/4] notes: print note blob to stdout directly","fromName":"Maarten Bosmans","fromEmail":"mkbosmans@gmail.com","sentAt":"2024-02-06T09:55:04Z","receivedAt":"2024-02-06T09:55:17Z","isPatch":true,"sender":{"key":"mkbosmans@gmail.com","avatar":null},"body":"Op di 6 feb 2024 om 04:44 schreef Junio C Hamano :\n>\n> Maarten Bosmans writes:\n> > Avoid the need to launch a subprocess by calling stream_blob_to_fd\n> > directly. This does not only get rid of the overhead of a separate\n> > child process, but also avoids the initalization of the whole log\n> > machinery that `git show` does. That is needed for example to show\n> > decorated commits and applying the mailmap. For simply displaying\n> > a blob however, the only useful thing show does is enabling the pager.\n> >\n> > Signed-off-by: Maarten Bosmans \n> > ---\n> > builtin/notes.c | 10 ++++++----\n> > 1 file changed, 6 insertions(+), 4 deletions(-)\n>\n> I am not sure if we want to accept an approach that feels somewhat\n> narrow/short sighted, like this patch. When \"git show\" learns an\n> improved way to show blob objects, who will update the code this\n> patch touches to teach it to use the same improved way to show the\n> notes?\n\nYes, you would loose some of the flexibility by just calling out to\nanother git command. But whether that is actually an issue depends on\nthe way git show would be extended. As I mentioned in the cover\nletter, there is some handling of textconv\nbeing done in git show for the blob case. I though it would not be\napplicable to note blobs, but am not entirely sure.\n\nTo address your concern and the textconv case, instead of calling\nstream_blob_to_fd() directly, show_blob_object() could be used. Right\nnow that is a static function in building/log.c I guess that needs to\nbe moved then to log.c (or show.c)?\n\nI'm still not sure that is a good approach though, as the notes\nedit/append subcommands use repo_read_object_file() directly to fill\nthe current note contents. So anything fancy that git show would do to\nblob output is not reflected when editing a note.\n\n> I actually was hoping, after seeing the use case description in the\n> cover letter, that the series would be introducing a batch mode\n> interface to allow callers to ask notes for many objects and have\n> the command respond notes for these objects in a way that which\n> piece of output corresponds to which object in the request, reducing\n> the process overhead amortised over many objects.\n\nThat is also a cool idea. That would probably use the functionality of\nthe cat-file batch mode, right? In that case you loose any future\nchanges to git show anyway. At least the current behavior of git show\nwhen fed multiple blob hashes is to simply output them directly after\nanother, without any way of identifying the parts.\n\nMy concern would be that this additional functionality would not get\nused much, as it requires a bit more than just a quick-n-dirty bash\nscript with some loops and command substitutions. If that approach is\nreasonably fast (the object of this patch series), then there is not\nmuch to be gained in a practical sense by a batch mode. To me it seems\nthe git notes code is already a bit undermaintained currently, so I'd\nrather keep it generic, instead of customizing it to more specific use\ncases.\n\nMaarten\n"},{"id":"488071","messageId":"45dd15d9-cdb3-41da-a7dd-cc0da3c85112@app.fastmail.com","threadId":"60853","inReplyTo":"xmqqil32l0i6.fsf@gitster.g","subject":"Re: [PATCH 1/4] notes: print note blob to stdout directly","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-06T13:55:23Z","receivedAt":"2024-02-06T13:55:46Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Tue, Feb 6, 2024, at 04:44, Junio C Hamano wrote:\n> I actually was hoping, after seeing the use case description in the\n> cover letter, that the series would be introducing a batch mode\n> interface to allow callers to ask notes for many objects and have\n> the command respond notes for these objects in a way that which\n> piece of output corresponds to which object in the request, reducing\n> the process overhead amortised over many objects.\n\nThat sounds great. I imagine that would be very useful.\n\n-- \nKristoffer Haugsbakk\n"},{"id":"488078","messageId":"xmqqy1bxiiop.fsf@gitster.g","threadId":"60853","inReplyTo":"CA+CvcKTtcHCCKucQ0h1dnaDAMNfErJ+a1CXEVi=ZE5dv57Tb3A@mail.gmail.com","subject":"Re: [PATCH 1/4] notes: print note blob to stdout directly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-06T17:52:38Z","receivedAt":"2024-02-06T17:52:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Maarten Bosmans writes:\n\n>> I am not sure if we want to accept an approach that feels somewhat\n>> narrow/short sighted, like this patch. When \"git show\" learns an\n>> improved way to show blob objects, who will update the code this\n>> patch touches to teach it to use the same improved way to show the\n>> notes?\n\nAnother thing I forgot to mention was that I suspected the use of\n\"git show\" was so that we can deal with notes trees whose leaves are\n*not* blob objects. As our \"git notes\" machinery, including how the\ninitial contents are populated with \"git notes add\" and how existing\nnotes on the same object are merged, all assume and depend on that\nthe leaves are blobs, we can say \"we'll just dump to stdout with\nwrite_in_full()\", but who knows what kind of custom crap random\nthird-party tools and IDEs try to use notes trees to store.\n\nEven if we limit ourselves to blobs, they may not be suitable to be\ndumped to tty (or a pager, for that matter), and I can see how\ntextconv could be used as a way out (\"detect that the notes payload\nis an image and spawn display\" kind of hack in a repository full of\nimages and notes used to store thumbnails, perhaps).\n\n> That is also a cool idea. That would probably use the functionality of\n> the cat-file batch mode, right?\n\nNot really. I was hoping that \"git show\" that can take multiple\nobjects from its command line would directly be used, or with a new\noption that gives a separator between these objects.\n\nPerhaps we want a new option, e.g., \"git notes show --text\" that\npasses the contents of the leaf blob object to write_in_full(),\nbypassing all the things \"git show\" does, while in a rare case when\nthe leaf we find is not a blob to invoke \"git show\". That might be\na safe approach to move forward, if we wanted to do this.\n"},{"id":"488505","messageId":"20240213080014.GB2225494@coredump.intra.peff.net","threadId":"60853","inReplyTo":"xmqqy1bxiiop.fsf@gitster.g","subject":"Re: [PATCH 1/4] notes: print note blob to stdout directly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-13T08:00:14Z","receivedAt":"2024-02-13T08:00:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 06, 2024 at 09:52:38AM -0800, Junio C Hamano wrote:\n\n> > That is also a cool idea. That would probably use the functionality of\n> > the cat-file batch mode, right?\n> \n> Not really. I was hoping that \"git show\" that can take multiple\n> objects from its command line would directly be used, or with a new\n> option that gives a separator between these objects.\n\nHow about:\n\n cat some_commit_ids |\n git show --stdin -s -z --format='%H%n%N'\n\n?\n\n-Peff\n"},{"id":"488550","messageId":"xmqq5xys6zdr.fsf@gitster.g","threadId":"60853","inReplyTo":"20240213080014.GB2225494@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] notes: print note blob to stdout directly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-13T17:35:28Z","receivedAt":"2024-02-13T17:35:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Feb 06, 2024 at 09:52:38AM -0800, Junio C Hamano wrote:\n>\n>> > That is also a cool idea. That would probably use the functionality of\n>> > the cat-file batch mode, right?\n>> \n>> Not really. I was hoping that \"git show\" that can take multiple\n>> objects from its command line would directly be used, or with a new\n>> option that gives a separator between these objects.\n>\n> How about:\n>\n> cat some_commit_ids |\n> git show --stdin -s -z --format='%H%n%N'\n\nYeah, that is more in line with what I was hoping to see, instead of\ninvoking \"git show %s\" for a note object one by one.\n\nThanks.\n\nRemind me if you can if we (1) had plans to allow non-blob objects\nas notes, or (2) actively support to have non-text blob objects as\nnotes. I _think_ we do assume people may try to add non-text blob\nas notes (while they accept that they cannot merge two such notes on\nthe same object), but I do not recall if we planned to allow them to\nstore trees and commits.\n"},{"id":"488681","messageId":"20240215052640.GC2821179@coredump.intra.peff.net","threadId":"60853","inReplyTo":"xmqq5xys6zdr.fsf@gitster.g","subject":"Re: [PATCH 1/4] notes: print note blob to stdout directly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-15T05:26:40Z","receivedAt":"2024-02-15T05:26:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 13, 2024 at 09:35:28AM -0800, Junio C Hamano wrote:\n\n> Remind me if you can if we (1) had plans to allow non-blob objects\n> as notes, or (2) actively support to have non-text blob objects as\n> notes. I _think_ we do assume people may try to add non-text blob\n> as notes (while they accept that they cannot merge two such notes on\n> the same object), but I do not recall if we planned to allow them to\n> store trees and commits.\n\nShort answer: no for (1) and yes for (2).\n\nIn my head non-blob notes were always something we'd eventually allow.\nBut I don't think the \"git notes\" tool really helps you at all (it\ninsists on being fed message content and makes the blob itself, rather\nthan letting you specify an oid).\n\nI wondered what the display side might do here (like, say, segfault).\nBut it looks like the notes code does not even consider a subtree like\nthis to be a note. It ends up in the \"non_note\" list of the notes struct\n(and format_note() on the display side explicitly ignores non-blobs\nanyway).\n\nSo it looks like it did not ever work, and nobody can even be using it\naccidentally (of course you can put whatever garbage you like under\nrefs/notes/*, but if neither git-notes nor the display machinery\nworks with it, I think we can discount that entirely).\n\nNon-text blob objects, on the other hand, should be easy to do with \"git\nnotes add -F\". Interestingly, the display code (in format_note() again)\nconverts embedded NULs into newlines. I think this is an accidental\nbehavior due to the use of strchrnul().\n\nOf course it's reasonable to also store notes that aren't meant to be\ndisplayed via git-log, etc, at all. The textconv-caching machinery\nstores its results in a separate notes ref. Those should generally be\ntext (since the point is to come up with something diff-able). But I\nthink it would be perfectly reasonable for another mechanism to make use\nof notes in the same way and store binary goo.\n\n(And of course we can store notes on any object, not just commits, as\nthe textconv cache demonstrates. But I think that is orthogonal to what\nyou're asking).\n\n-Peff\n"},{"id":"488690","messageId":"CA+CvcKSpjB6Js3H49yvjNves53STj0keW3BvoREhdnDHzZ7VjQ@mail.gmail.com","threadId":"60853","inReplyTo":"xmqqy1bxiiop.fsf@gitster.g","subject":"Re: [PATCH 1/4] notes: print note blob to stdout directly","fromName":"Maarten Bosmans","fromEmail":"mkbosmans@gmail.com","sentAt":"2024-02-15T07:41:22Z","receivedAt":"2024-02-15T07:41:40Z","isPatch":true,"sender":{"key":"mkbosmans@gmail.com","avatar":null},"body":"Op di 6 feb 2024 om 18:52 schreef Junio C Hamano :\n> Maarten Bosmans writes:\n> > That is also a cool idea. That would probably use the functionality of\n> > the cat-file batch mode, right?\n>\n> Not really. I was hoping that \"git show\" that can take multiple\n> objects from its command line would directly be used, or with a new\n> option that gives a separator between these objects.\n\nOK, I need some guidance here. Should I submit a new version of the\npatch that addresses the comments about the code, or should I this\napproach of making a single `git notes show` faster altogether?\nIn my view it is worthwhile to eliminate the extra `git show`\nsubprocess launch (while still accounting for non-text blob notes, but\nnot non-blob notes) and focus on batching later.\n\nMaarten\n"},{"id":"488692","messageId":"CA+CvcKR9sH=qZB4oZvX9RWd+4H3Bq8WV_qUOiSj_Tsf=Dr_Xvw@mail.gmail.com","threadId":"60853","inReplyTo":"20240213080014.GB2225494@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] notes: print note blob to stdout directly","fromName":"Maarten Bosmans","fromEmail":"mkbosmans@gmail.com","sentAt":"2024-02-15T07:46:02Z","receivedAt":"2024-02-15T07:46:19Z","isPatch":true,"sender":{"key":"mkbosmans@gmail.com","avatar":null},"body":"Op di 13 feb 2024 om 09:00 schreef Jeff King :\n>\n> On Tue, Feb 06, 2024 at 09:52:38AM -0800, Junio C Hamano wrote:\n>\n> > > That is also a cool idea. That would probably use the functionality of\n> > > the cat-file batch mode, right?\n> >\n> > Not really. I was hoping that \"git show\" that can take multiple\n> > objects from its command line would directly be used, or with a new\n> > option that gives a separator between these objects.\n>\n> How about:\n>\n> cat some_commit_ids |\n> git show --stdin -s -z --format='%H%n%N'\n>\n> ?\n>\n> -Peff\n\nWouldn't that fail horribly with non-text blobs?\n\nAre there examples other than cat-file that show how batching is done in git?\nI'm afraid of adding something to git-notes with slightly different\nsyntax from other batch commands. The git cli is already confusing\nenough with all the little inconsistencies.\n\nMaarten\n"},{"id":"488740","messageId":"20240215150430.GA3453@coredump.intra.peff.net","threadId":"60853","inReplyTo":"CA+CvcKR9sH=qZB4oZvX9RWd+4H3Bq8WV_qUOiSj_Tsf=Dr_Xvw@mail.gmail.com","subject":"Re: [PATCH 1/4] notes: print note blob to stdout directly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-15T15:04:30Z","receivedAt":"2024-02-15T15:04:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 15, 2024 at 08:46:02AM +0100, Maarten Bosmans wrote:\n\n> > How about:\n> >\n> > cat some_commit_ids |\n> > git show --stdin -s -z --format='%H%n%N'\n> >\n> Wouldn't that fail horribly with non-text blobs?\n\nYes, if you have NULs embedded in your notes then it won't work. Any\nbatch output format would require byte counts, then. If we wanted to add\na feature to support that, I would suggest one of:\n\n - teach the pretty-print formatter a new placeholder to output the\n number of bytes in an element. Then you could do something like\n \"%H %(size:%N)%n%N\", but it would be generally useful for other\n cases, too.\n\n - teach the pretty-print formatter a variant of %N that outputs only\n the oid of the note, note the note content itself. And then you\n could do something like:\n\n git log --format='%(note:oid) %H' |\n git cat-file --batch='%(objectname) %(objectsize) %(rest)'\n\n to get the usual cat-file output of each note blob, but associated\n with the commit it's attached to (the \"%(rest)\" placeholder for\n cat-file just relays any text found after the object name of each\n line). You might need to do some scripting between the two to handle\n commits with no note.\n\nOf the two, I'd guess that the second one is a lot less work to\nimplement (on the Git side; on the reading side it's a little more\ninvolved, but still should be a constant number of processes).\n\nOne variant of the second one is to use \"git notes list\". For example,\nyou can get all notes via cat-file like this right now:\n\n git notes list |\n git cat-file --batch='%(objectname) %(objectsize) %(rest)'\n\nYou can get individual notes by asking for \"git notes list \",\nbut it will only take one at a time. So another easy patch would be\nsomething like (indentation left funny to make the diff more readable):\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex e65cae0bcf..5fdad5fb8f 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -446,22 +446,22 @@ static int list(int argc, const char **argv, const char *prefix)\n \t\targc = parse_options(argc, argv, prefix, options,\n \t\t\t\t git_notes_list_usage, 0);\n \n-\tif (1 < argc) {\n-\t\terror(_(\"too many arguments\"));\n-\t\tusage_with_options(git_notes_list_usage, options);\n-\t}\n-\n \tt = init_notes_check(\"list\", 0);\n \tif (argc) {\n-\t\tif (repo_get_oid(the_repository, argv[0], &object))\n-\t\t\tdie(_(\"failed to resolve '%s' as a valid ref.\"), argv[0]);\n+\t\tretval = 0;\n+\t\twhile (*++argv) {\n+\t\tif (repo_get_oid(the_repository, *argv, &object))\n+\t\t\tdie(_(\"failed to resolve '%s' as a valid ref.\"), *argv);\n \t\tnote = get_note(t, &object);\n \t\tif (note) {\n-\t\t\tputs(oid_to_hex(note));\n-\t\t\tretval = 0;\n+\t\t\tif (argc > 1)\n+\t\t\t\tprintf(\"%s %s\\n\", oid_to_hex(note), oid_to_hex(&object));\n+\t\t\telse\n+\t\t\t\tputs(oid_to_hex(note));\n \t\t} else\n-\t\t\tretval = error(_(\"no note found for object %s.\"),\n+\t\t\tretval |= error(_(\"no note found for object %s.\"),\n \t\t\t\t oid_to_hex(&object));\n+\t\t}\n \t} else\n \t\tretval = for_each_note(t, 0, list_each_note, NULL);\n \n\nThat would allow:\n\n git rev-list ... |\n xargs git notes list |\n git cat-file --batch='%(objectname) %(objectsize) %(rest)'\n\nWe could even add a \"--stdin\" mode to avoid the use of xargs.\n\n-Peff\n"},{"id":"488783","messageId":"xmqqcysxkjrq.fsf@gitster.g","threadId":"60853","inReplyTo":"20240215052640.GC2821179@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] notes: print note blob to stdout directly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-16T06:25:45Z","receivedAt":"2024-02-16T06:25:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Feb 13, 2024 at 09:35:28AM -0800, Junio C Hamano wrote:\n>\n>> Remind me if you can if we (1) had plans to allow non-blob objects\n>> as notes, or (2) actively support to have non-text blob objects as\n>> notes. I _think_ we do assume people may try to add non-text blob\n>> as notes (while they accept that they cannot merge two such notes on\n>> the same object), but I do not recall if we planned to allow them to\n>> store trees and commits.\n>\n> Short answer: no for (1) and yes for (2).\n>\n> In my head non-blob notes were always something we'd eventually allow.\n> But I don't think the \"git notes\" tool really helps you at all (it\n> insists on being fed message content and makes the blob itself, rather\n> than letting you specify an oid).\n\nOK, that makes my worry about dropping \"git show\" from the codepath\nquite a lot. At least we only have to worry about blobs.\n\n> Non-text blob objects, on the other hand, should be easy to do with \"git\n> notes add -F\". Interestingly, the display code (in format_note() again)\n> converts embedded NULs into newlines. I think this is an accidental\n> behavior due to the use of strchrnul().\n>\n> Of course it's reasonable to also store notes that aren't meant to be\n> displayed via git-log, etc, at all. The textconv-caching machinery\n> stores its results in a separate notes ref. Those should generally be\n> text (since the point is to come up with something diff-able). But I\n> think it would be perfectly reasonable for another mechanism to make use\n> of notes in the same way and store binary goo.\n\nYup. \n\nThe question is if our current use of \"git show\" allows creative use\nof such binary data attached as notes. The patch in question will\nbreak such users, as it would become impossible once we start\nbypassing the \"git show\" and emitting the binary data directly to\nthe standard output stream.\n\nBecause the pathname a note blob is stored at is unpredictable and\nnot under end-user control, it would not be practical to define\ndifferent smudge filters per note object using the attribute\nmechanism, but if you limit the types of binary data you store in\nyour notes (e.g., refs/notes/thumbnail may be a note ref whose\ncontents are all jpeg thumbnail images, where your main history is\nyour photo archive), you might be able to convince the \"git show\"\ninvocation we use to show the note object to show that thumbnail\nimage by using something involving \"display\" (from imagemagick---of\ncourse you could use other programs that allows you to view the\nimage on different platforms) in your smudge filter. Bypassing \"git\nshow\" and sending the blob contents directly to the standard output\nwould be a grave regression for such a user.\n\n"},{"id":"488839","messageId":"20240217051650.GB539459@coredump.intra.peff.net","threadId":"60853","inReplyTo":"xmqqcysxkjrq.fsf@gitster.g","subject":"Re: [PATCH 1/4] notes: print note blob to stdout directly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-17T05:16:50Z","receivedAt":"2024-02-17T05:16:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 15, 2024 at 10:25:45PM -0800, Junio C Hamano wrote:\n\n> > Of course it's reasonable to also store notes that aren't meant to be\n> > displayed via git-log, etc, at all. The textconv-caching machinery\n> > stores its results in a separate notes ref. Those should generally be\n> > text (since the point is to come up with something diff-able). But I\n> > think it would be perfectly reasonable for another mechanism to make use\n> > of notes in the same way and store binary goo.\n> \n> Yup. \n> \n> The question is if our current use of \"git show\" allows creative use\n> of such binary data attached as notes. The patch in question will\n> break such users, as it would become impossible once we start\n> bypassing the \"git show\" and emitting the binary data directly to\n> the standard output stream.\n\nHmm, good question. I can't think offhand of a way that the user could\nconvince \"git show \" to do anything different than just dumping the\nliteral contents. It is not even handed a path that could trigger\n.gitattributes or similar.\n\nThe only difference I can think of is that \"git show\" triggers a pager\nby default. Probably \"git notes show\" should be doing the same (and\nhonestly, that would be less confusing to me; the fact that configuring\n\"pager.show\" would affect \"git notes show\" is surprising to me).\n\nDigging in the history, it looks like we use \"git show\" at all because\nthis was adapted from a shell script (though that shell script probably\nought to have been using cat-file in the first place; maybe back then we\nthought we'd support non-blobs ;) ).\n\n> Because the pathname a note blob is stored at is unpredictable and\n> not under end-user control, it would not be practical to define\n> different smudge filters per note object using the attribute\n> mechanism, but if you limit the types of binary data you store in\n> your notes (e.g., refs/notes/thumbnail may be a note ref whose\n> contents are all jpeg thumbnail images, where your main history is\n> your photo archive), you might be able to convince the \"git show\"\n> invocation we use to show the note object to show that thumbnail\n> image by using something involving \"display\" (from imagemagick---of\n> course you could use other programs that allows you to view the\n> image on different platforms) in your smudge filter. Bypassing \"git\n> show\" and sending the blob contents directly to the standard output\n> would be a grave regression for such a user.\n\nLike I said, I do not think there is a way to convince \"git show\" in\nthat way. Unless perhaps something like:\n\n git -c pager.show='f() { cat >tmp.jpg; display tmp; }; f' \\\n notes show --ref thumbnail\n\nBut that's pretty awful. Why would you do that instead of just \"git\nnotes show >tmp; display tmp\" yourself? And again, even if we wanted to\nsupport such a monstrosity, I feel like setting pager.notes.show would\nbe much more intuitive than piggy-backing on \"git show\".\n\nSometimes, of course, we have to support weird stuff anyway because it\nhas existed for a long time and we don't want to break users. But this\nis really pushing my gut-feeling limit of what is reasonable / plausible\nfor somebody to be doing.\n\nOf course I may be missing some other case where \"show\" behaves in a\nuseful way that is different than a straight dump of the blob. But if\nnot, I'd almost say that getting rid of the extra \"show\" call now is a\ngood thing, because it locks in the simple behavior. ;)\n\n-Peff\n"},{"id":"488842","messageId":"xmqqwmr3fxc1.fsf@gitster.g","threadId":"60853","inReplyTo":"20240217051650.GB539459@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] notes: print note blob to stdout directly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-17T05:56:14Z","receivedAt":"2024-02-17T05:56:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Digging in the history, it looks like we use \"git show\" at all because\n> this was adapted from a shell script (though that shell script probably\n> ought to have been using cat-file in the first place; maybe back then we\n> thought we'd support non-blobs ;) ).\n\nYup, that is why I suspected that we planned to store non-blobs.\n\n> Hmm, good question. I can't think offhand of a way that the user could\n> convince \"git show \" to do anything different than just dumping the\n> literal contents. It is not even handed a path that could trigger\n> .gitattributes or similar.\n\nshow_blob_object() directly calls stream_blob_to_fd() without any\nfilter, as the hardcoded invocation of \"git show \" in the note\ncodepath does not allow --textconv at all, so we probably are safe\nto assume that the contents will appear as-is, not even going\nthrough EOL conversion (which is a bit puzzling, to be honest,\nthough). Lack of path I am not worried about, as you can easily\ndeclare with '*' wildcard that everything in the notes tree is of\nthe same type. But if the stream_blob_to_fd() interface does not\nhave anywhere smudge filters or textconv filters can hook into\nwithout some command line option, we do not have to worry about it.\n\n> Sometimes, of course, we have to support weird stuff anyway because it\n> has existed for a long time and we don't want to break users. But this\n> is really pushing my gut-feeling limit of what is reasonable / plausible\n> for somebody to be doing.\n>\n> Of course I may be missing some other case where \"show\" behaves in a\n> useful way that is different than a straight dump of the blob. But if\n> not, I'd almost say that getting rid of the extra \"show\" call now is a\n> good thing, because it locks in the simple behavior. ;)\n\nYes, of course. It would be great if we can just stream the blob\ncontents out in-process.\n\nThanks.\n"},{"id":"488844","messageId":"20240217060931.GF539459@coredump.intra.peff.net","threadId":"60853","inReplyTo":"xmqqwmr3fxc1.fsf@gitster.g","subject":"Re: [PATCH 1/4] notes: print note blob to stdout directly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-17T06:09:31Z","receivedAt":"2024-02-17T06:09:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 16, 2024 at 09:56:14PM -0800, Junio C Hamano wrote:\n\n> > Hmm, good question. I can't think offhand of a way that the user could\n> > convince \"git show \" to do anything different than just dumping the\n> > literal contents. It is not even handed a path that could trigger\n> > .gitattributes or similar.\n> \n> show_blob_object() directly calls stream_blob_to_fd() without any\n> filter, as the hardcoded invocation of \"git show \" in the note\n> codepath does not allow --textconv at all, so we probably are safe\n> to assume that the contents will appear as-is, not even going\n> through EOL conversion (which is a bit puzzling, to be honest,\n> though). Lack of path I am not worried about, as you can easily\n> declare with '*' wildcard that everything in the notes tree is of\n> the same type. But if the stream_blob_to_fd() interface does not\n> have anywhere smudge filters or textconv filters can hook into\n> without some command line option, we do not have to worry about it.\n\nI think we are mostly on the same page, but just to be pedantic: I do\nnot think even adding a \"*\" attribute in the notes tree could ever do\nanything. The \"git show\" invocation is not \"git show notes_ref:\".\nWe resolve the note blob to an oid, and then pass that oid's hex to \"git\nshow\".\n\nSo the \"git show\" command itself has no context at all, and does not\neven know this blob came from a tree in the first place.\n\n-Peff\n"},{"id":"488846","messageId":"CA+CvcKSQCUukfLNnRkmTp=K=aXBRaxQnattfL+QexgOsYX18nA@mail.gmail.com","threadId":"60853","inReplyTo":"20240215150430.GA3453@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] notes: print note blob to stdout directly","fromName":"Maarten Bosmans","fromEmail":"mkbosmans@gmail.com","sentAt":"2024-02-17T12:45:39Z","receivedAt":"2024-02-17T12:45:57Z","isPatch":true,"sender":{"key":"mkbosmans@gmail.com","avatar":null},"body":"Op do 15 feb 2024 om 16:04 schreef Jeff King :\n>\n> On Thu, Feb 15, 2024 at 08:46:02AM +0100, Maarten Bosmans wrote:\n>\n> > > How about:\n> > >\n> > > cat some_commit_ids |\n> > > git show --stdin -s -z --format='%H%n%N'\n> > >\n> > Wouldn't that fail horribly with non-text blobs?\n>\n> Yes, if you have NULs embedded in your notes then it won't work. Any\n> batch output format would require byte counts, then. If we wanted to add\n> a feature to support that, I would suggest one of:\n>\n> - teach the pretty-print formatter a new placeholder to output the\n> number of bytes in an element. Then you could do something like\n> \"%H %(size:%N)%n%N\", but it would be generally useful for other\n> cases, too.\n>\n> - teach the pretty-print formatter a variant of %N that outputs only\n> the oid of the note, note the note content itself. And then you\n> could do something like:\n>\n> git log --format='%(note:oid) %H' |\n> git cat-file --batch='%(objectname) %(objectsize) %(rest)'\n>\n> to get the usual cat-file output of each note blob, but associated\n> with the commit it's attached to (the \"%(rest)\" placeholder for\n> cat-file just relays any text found after the object name of each\n> line). You might need to do some scripting between the two to handle\n> commits with no note.\n>\n> Of the two, I'd guess that the second one is a lot less work to\n> implement (on the Git side; on the reading side it's a little more\n> involved, but still should be a constant number of processes).\n\nThe second one is attractive for another reason than implementation\nsimplicity. While the first one offers more flexibility, the second\nreuses the existing cat-file batch format, so the interface between\ngit and scripts is familiar and consistent.\n\n> One variant of the second one is to use \"git notes list\". For example,\n> you can get all notes via cat-file like this right now:\n>\n> git notes list |\n> git cat-file --batch='%(objectname) %(objectsize) %(rest)'\n\nSo the cat-file batch output is suitable for blobs containing newline\nor NUL characters. But I struggle a bit with what would be an easy way\nof using this format in a shell script. Something with multiple read\nand read -N commands reading from the output, I guess.\nThe git codebase has `extract_batch_output()` in t1006. This uses a\nseparate perl invocation to parse the cat-file output, which confirms\nmy suspicion there isn't a straight-forward way to do this in e.g.\njust a bash script.\n\nThat was why my first steps were to accept that a launching a separate\nprocess per note in a bash loop is a pretty clear and well understood\nidiom in shell scripts and try to make the git part of that a bit more\nefficient.\n\n> You can get individual notes by asking for \"git notes list \",\n> but it will only take one at a time. So another easy patch would be\n> something like (indentation left funny to make the diff more readable):\n>\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index e65cae0bcf..5fdad5fb8f 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -446,22 +446,22 @@ static int list(int argc, const char **argv, const char *prefix)\n> argc = parse_options(argc, argv, prefix, options,\n> git_notes_list_usage, 0);\n>\n> - if (1 < argc) {\n> - error(_(\"too many arguments\"));\n> - usage_with_options(git_notes_list_usage, options);\n> - }\n> -\n> t = init_notes_check(\"list\", 0);\n> if (argc) {\n> - if (repo_get_oid(the_repository, argv[0], &object))\n> - die(_(\"failed to resolve '%s' as a valid ref.\"), argv[0]);\n> + retval = 0;\n> + while (*++argv) {\n> + if (repo_get_oid(the_repository, *argv, &object))\n> + die(_(\"failed to resolve '%s' as a valid ref.\"), *argv);\n> note = get_note(t, &object);\n> if (note) {\n> - puts(oid_to_hex(note));\n> - retval = 0;\n> + if (argc > 1)\n> + printf(\"%s %s\\n\", oid_to_hex(note), oid_to_hex(&object));\n> + else\n> + puts(oid_to_hex(note));\n> } else\n> - retval = error(_(\"no note found for object %s.\"),\n> + retval |= error(_(\"no note found for object %s.\"),\n> oid_to_hex(&object));\n> + }\n> } else\n> retval = for_each_note(t, 0, list_each_note, NULL);\n>\n>\n> That would allow:\n>\n> git rev-list ... |\n> xargs git notes list |\n> git cat-file --batch='%(objectname) %(objectsize) %(rest)'\n>\n> We could even add a \"--stdin\" mode to avoid the use of xargs.\n\nYes, a --stdin mode for `git notes list` would be a useful building\nblock for scripting indeed.\nI'll probably give it a try when this patch series succeeds.\n\nMaarten\n"},{"id":"488883","messageId":"20240218195938.6253-1-maarten.bosmans@vortech.nl","threadId":"60853","inReplyTo":"20240205204932.16653-1-maarten.bosmans@vortech.nl","subject":"[PATCH v2 0/5] Speed up git-notes show","fromName":"Maarten Bosmans","fromEmail":"mkbosmans@gmail.com","sentAt":"2024-02-18T19:59:33Z","receivedAt":"2024-02-18T19:59:41Z","isPatch":true,"sender":{"key":"mkbosmans@gmail.com","avatar":null},"body":"First time contributor here, trying my first git.git patch series.\n\nBACKGROUND\n We have a script that runs a range of build tests for all new commits in the\n repository and adds a line to the commit note with the result from the test.\n Something along the lines of:\n occa-build-jit-gnu-cuda-develop: PASSED (, 2024-01-01 00:00:00+01:00)\n Pretty useful to quickly check that all commits at least build, not only for\n master, but also in progress feature branches. (a passing test suite is\n generally only required at the merge point)\n\nPROBLEM\n The bash script loops over all remote refs and lists the commits newer than\n days ago. For each commit its note is read and grep'ed for an existing\n test name to see whether the build test needs to run again. The `git note show`\n command that is in this loop nest only takes 14ms to execute, but as it is in\n a loop, those times add up.\n\nANALYSIS\n When asked to show a note for a specific commit, git looks up the blob hash\n for the note and executes `git show` with that hash. That of course adds\n the child process overhead, but also causes the initialization of a lot of\n log related configuration, such as for decorations or the mailmap. Simply\n outputting the blob directly in the main process reduces the run time by\n almost halve.\n\nWhen looking through the git show implementation for useful stuff that command\ndoes that should also be done when showing a note, I could only find the\n`setup_pager()` call and some optional textconv stuff.\n\nThe second commit is the main one fixing performance. The others are just\neliminating some overhead I noticed when going through the git notes code.\n\n\nCHANGES WRT V1\n Sharing of the show_blob_object() function from log.c. The intention here is\n to have `git notes show` behave the same as `git show` when in the future the\n latter might be changed to have more sophisticated behaviour for some blobs,\n e.g. launching an image viewer for PNG blobs. Non-blob notes are still not\n handled, just like in V1. Current master does handle that, but that is not\n deemed a case worth handling.\n\n\nMaarten Bosmans (5):\n log: Move show_blob_object() to log.c\n notes: avoid launching a child process to show a note blob\n notes: use existing function stream_blob_to_fd\n notes: do not clean up right before calling die()\n notes: use strbuf_attach to take ownership of the object contents\n\n Makefile | 1 +\n builtin/log.c | 39 +++++----------------------------------\n builtin/notes.c | 38 +++++++++++---------------------------\n log.c | 41 +++++++++++++++++++++++++++++++++++++++++\n log.h | 11 +++++++++++\n 5 files changed, 69 insertions(+), 61 deletions(-)\n create mode 100644 log.c\n create mode 100644 log.h\n\n-- \n2.35.3\n\n"},{"id":"488884","messageId":"20240218195938.6253-2-maarten.bosmans@vortech.nl","threadId":"60853","inReplyTo":"20240218195938.6253-1-maarten.bosmans@vortech.nl","subject":"[PATCH v2 1/5] log: Move show_blob_object() to log.c","fromName":"Maarten Bosmans","fromEmail":"mkbosmans@gmail.com","sentAt":"2024-02-18T19:59:34Z","receivedAt":"2024-02-18T19:59:41Z","isPatch":true,"sender":{"key":"mkbosmans@gmail.com","avatar":null},"body":"From: Maarten Bosmans \n\nFrom: Maarten Bosmans \n\nThis way it can be used outside of builtin/log.c.\nThe next commit will make builtin/notes.c use it.\n\nSigned-off-by: Maarten Bosmans \n---\n Makefile | 1 +\n builtin/log.c | 39 +++++----------------------------------\n log.c | 41 +++++++++++++++++++++++++++++++++++++++++\n log.h | 11 +++++++++++\n 4 files changed, 58 insertions(+), 34 deletions(-)\n create mode 100644 log.c\n create mode 100644 log.h\n\ndiff --git a/Makefile b/Makefile\nindex 78e874099d..1c19d5c0f3 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1059,6 +1059,7 @@ LIB_OBJS += list-objects-filter-options.o\n LIB_OBJS += list-objects-filter.o\n LIB_OBJS += list-objects.o\n LIB_OBJS += lockfile.o\n+LIB_OBJS += log.o\n LIB_OBJS += log-tree.o\n LIB_OBJS += ls-refs.o\n LIB_OBJS += mailinfo.o\ndiff --git a/builtin/log.c b/builtin/log.c\nindex db1808d7c1..587a4c374d 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -32,7 +32,6 @@\n #include \"parse-options.h\"\n #include \"line-log.h\"\n #include \"branch.h\"\n-#include \"streaming.h\"\n #include \"version.h\"\n #include \"mailmap.h\"\n #include \"progress.h\"\n@@ -42,7 +41,7 @@\n #include \"range-diff.h\"\n #include \"tmp-objdir.h\"\n #include \"tree.h\"\n-#include \"write-or-die.h\"\n+#include \"log.h\"\n \n #define MAIL_DEFAULT_WRAP 72\n #define COVER_FROM_AUTO_MAX_SUBJECT_LEN 100\n@@ -653,37 +652,6 @@ static void show_tagger(const char *buf, struct rev_info *rev)\n \tstrbuf_release(&out);\n }\n \n-static int show_blob_object(const struct object_id *oid, struct rev_info *rev, const char *obj_name)\n-{\n-\tstruct object_id oidc;\n-\tstruct object_context obj_context;\n-\tchar *buf;\n-\tunsigned long size;\n-\n-\tfflush(rev->diffopt.file);\n-\tif (!rev->diffopt.flags.textconv_set_via_cmdline ||\n-\t !rev->diffopt.flags.allow_textconv)\n-\t\treturn stream_blob_to_fd(1, oid, NULL, 0);\n-\n-\tif (get_oid_with_context(the_repository, obj_name,\n-\t\t\t\t GET_OID_RECORD_PATH,\n-\t\t\t\t &oidc, &obj_context))\n-\t\tdie(_(\"not a valid object name %s\"), obj_name);\n-\tif (!obj_context.path ||\n-\t !textconv_object(the_repository, obj_context.path,\n-\t\t\t obj_context.mode, &oidc, 1, &buf, &size)) {\n-\t\tfree(obj_context.path);\n-\t\treturn stream_blob_to_fd(1, oid, NULL, 0);\n-\t}\n-\n-\tif (!buf)\n-\t\tdie(_(\"git show %s: bad file\"), obj_name);\n-\n-\twrite_or_die(1, buf, size);\n-\tfree(obj_context.path);\n-\treturn 0;\n-}\n-\n static int show_tag_object(const struct object_id *oid, struct rev_info *rev)\n {\n \tunsigned long size;\n@@ -770,7 +738,10 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \t\tconst char *name = rev.pending.objects[i].name;\n \t\tswitch (o->type) {\n \t\tcase OBJ_BLOB:\n-\t\t\tret = show_blob_object(&o->oid, &rev, name);\n+\t\t\tfflush(rev.diffopt.file);\n+\t\t\tbool do_textconv = rev.diffopt.flags.textconv_set_via_cmdline &&\n+\t\t\t\trev.diffopt.flags.allow_textconv;\n+\t\t\tret = show_blob_object(&o->oid, name, do_textconv);\n \t\t\tbreak;\n \t\tcase OBJ_TAG: {\n \t\t\tstruct tag *t = (struct tag *)o;\ndiff --git a/log.c b/log.c\nnew file mode 100644\nindex 0000000000..5c77707385\n--- /dev/null\n+++ b/log.c\n@@ -0,0 +1,41 @@\n+#include \"git-compat-util.h\"\n+#include \"gettext.h\"\n+#include \"diff.h\"\n+#include \"log.h\"\n+#include \"notes.h\"\n+#include \"object-name.h\"\n+#include \"repository.h\"\n+#include \"streaming.h\"\n+#include \"write-or-die.h\"\n+\n+/*\n+ * Print blob contents to stdout.\n+ */\n+int show_blob_object(const struct object_id *oid, const char *obj_name, bool do_textconv)\n+{\n+\tstruct object_id oidc;\n+\tstruct object_context obj_context;\n+\tchar *buf;\n+\tunsigned long size;\n+\n+\tif (!do_textconv)\n+\t\treturn stream_blob_to_fd(1, oid, NULL, 0);\n+\n+\tif (get_oid_with_context(the_repository, obj_name,\n+\t\t\t\t GET_OID_RECORD_PATH,\n+\t\t\t\t &oidc, &obj_context))\n+\t\tdie(_(\"not a valid object name %s\"), obj_name);\n+\tif (!obj_context.path ||\n+\t !textconv_object(the_repository, obj_context.path,\n+\t\t\t obj_context.mode, &oidc, 1, &buf, &size)) {\n+\t\tfree(obj_context.path);\n+\t\treturn stream_blob_to_fd(1, oid, NULL, 0);\n+\t}\n+\n+\tif (!buf)\n+\t\tdie(_(\"git show %s: bad file\"), obj_name);\n+\n+\twrite_or_die(1, buf, size);\n+\tfree(obj_context.path);\n+\treturn 0;\n+}\ndiff --git a/log.h b/log.h\nnew file mode 100644\nindex 0000000000..464cca52ff\n--- /dev/null\n+++ b/log.h\n@@ -0,0 +1,11 @@\n+#ifndef LOG_H\n+#define LOG_H\n+\n+struct object_id;\n+\n+/*\n+ * Print blob contents to stdout.\n+ */\n+int show_blob_object(const struct object_id *oid, const char *obj_name, bool do_textconv);\n+\n+#endif\n-- \n2.35.3\n\n"},{"id":"488885","messageId":"20240218195938.6253-4-maarten.bosmans@vortech.nl","threadId":"60853","inReplyTo":"20240218195938.6253-1-maarten.bosmans@vortech.nl","subject":"[PATCH v2 3/5] notes: use existing function stream_blob_to_fd","fromName":"Maarten Bosmans","fromEmail":"mkbosmans@gmail.com","sentAt":"2024-02-18T19:59:36Z","receivedAt":"2024-02-18T19:59:41Z","isPatch":true,"sender":{"key":"mkbosmans@gmail.com","avatar":null},"body":"From: Maarten Bosmans \n\nFrom: Maarten Bosmans \n\nUse functionality from streaming.c and remove the copy_obj_to_fd()\nfunction local to notes.c.\n\nStreaming the blob to stdout instead of copying through an\nintermediate buffer might also be more efficient, but at the\nsize a typical note is, this is unlikely to matter a lot.\n\nSigned-off-by: Maarten Bosmans \n---\n builtin/notes.c | 15 ++-------------\n 1 file changed, 2 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 2a31da6c97..184a92d0c1 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -22,6 +22,7 @@\n #include \"refs.h\"\n #include \"pager.h\"\n #include \"log.h\"\n+#include \"streaming.h\"\n #include \"run-command.h\"\n #include \"parse-options.h\"\n #include \"string-list.h\"\n@@ -149,18 +150,6 @@ static int list_each_note(const struct object_id *object_oid,\n \treturn 0;\n }\n \n-static void copy_obj_to_fd(int fd, const struct object_id *oid)\n-{\n-\tunsigned long size;\n-\tenum object_type type;\n-\tchar *buf = repo_read_object_file(the_repository, oid, &type, &size);\n-\tif (buf) {\n-\t\tif (size)\n-\t\t\twrite_or_die(fd, buf, size);\n-\t\tfree(buf);\n-\t}\n-}\n-\n static void write_commented_object(int fd, const struct object_id *object)\n {\n \tstruct child_process show = CHILD_PROCESS_INIT;\n@@ -205,7 +194,7 @@ static void prepare_note_data(const struct object_id *object, struct note_data *\n \t\tif (d->given)\n \t\t\twrite_or_die(fd, d->buf.buf, d->buf.len);\n \t\telse if (old_note)\n-\t\t\tcopy_obj_to_fd(fd, old_note);\n+\t\t\tstream_blob_to_fd(fd, old_note, NULL, 0);\n \n \t\tstrbuf_addch(&buf, '\\n');\n \t\tstrbuf_add_commented_lines(&buf, \"\\n\", strlen(\"\\n\"), comment_line_char);\n-- \n2.35.3\n\n"},{"id":"488886","messageId":"20240218195938.6253-3-maarten.bosmans@vortech.nl","threadId":"60853","inReplyTo":"20240218195938.6253-1-maarten.bosmans@vortech.nl","subject":"[PATCH v2 2/5] notes: avoid launching a child process to show a note blob","fromName":"Maarten Bosmans","fromEmail":"mkbosmans@gmail.com","sentAt":"2024-02-18T19:59:35Z","receivedAt":"2024-02-18T19:59:41Z","isPatch":true,"sender":{"key":"mkbosmans@gmail.com","avatar":null},"body":"From: Maarten Bosmans \n\nFrom: Maarten Bosmans \n\nAvoid the need to launch a subprocess by calling stream_blob_to_fd\ndirectly. This does not only get rid of the overhead of a separate\nchild process, but also avoids the initalization of the whole log\nmachinery that `git show` does. That is needed for example to show\ndecorated commits and applying the mailmap. For simply displaying\na blob however, the only useful thing show does is enabling the pager.\n\nThis locks in the expectation that a note oid refers to a blob, and\nnot a tree or something else. To still keep the option open that the\nblob might not be a text blob and in the future we might handle such\nnotes differently, the show_blob_object() function is called in order\nto have the same behaviour as though `git show ` was called.\n\nSigned-off-by: Maarten Bosmans \n---\n builtin/notes.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex caf20fd5bd..2a31da6c97 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -20,7 +20,8 @@\n #include \"repository.h\"\n #include \"pretty.h\"\n #include \"refs.h\"\n-#include \"exec-cmd.h\"\n+#include \"pager.h\"\n+#include \"log.h\"\n #include \"run-command.h\"\n #include \"parse-options.h\"\n #include \"string-list.h\"\n@@ -753,7 +754,7 @@ static int show(int argc, const char **argv, const char *prefix)\n \tstruct notes_tree *t;\n \tstruct object_id object;\n \tconst struct object_id *note;\n-\tint retval;\n+\tint retval = 0;\n \tstruct option options[] = {\n \t\tOPT_END()\n \t};\n@@ -778,8 +779,9 @@ static int show(int argc, const char **argv, const char *prefix)\n \t\tretval = error(_(\"no note found for object %s.\"),\n \t\t\t oid_to_hex(&object));\n \telse {\n-\t\tconst char *show_args[3] = {\"show\", oid_to_hex(note), NULL};\n-\t\tretval = execv_git_cmd(show_args);\n+\t\tsetup_pager();\n+\t\tif (show_blob_object(note, NULL, false))\n+\t\t\tdie(_(\"object %s is not a blob\"), oid_to_hex(note));\n \t}\n \tfree_notes(t);\n \treturn retval;\n-- \n2.35.3\n\n"},{"id":"488887","messageId":"20240218195938.6253-6-maarten.bosmans@vortech.nl","threadId":"60853","inReplyTo":"20240218195938.6253-1-maarten.bosmans@vortech.nl","subject":"[PATCH v2 5/5] notes: use strbuf_attach to take ownership of the object contents","fromName":"Maarten Bosmans","fromEmail":"mkbosmans@gmail.com","sentAt":"2024-02-18T19:59:38Z","receivedAt":"2024-02-18T19:59:42Z","isPatch":true,"sender":{"key":"mkbosmans@gmail.com","avatar":null},"body":"From: Maarten Bosmans \n\nFrom: Maarten Bosmans \n\nAvoid an extra allocation in the strbuf when pushing the string into it.\n\nSigned-off-by: Maarten Bosmans \n---\n builtin/notes.c | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 6863935d03..5be24a9c58 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -314,10 +314,8 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)\n \tif (type != OBJ_BLOB)\n \t\tdie(_(\"cannot read note data from non-blob object '%s'.\"), arg);\n \n-\tstrbuf_add(&msg->buf, value, len);\n-\tfree(value);\n+\tstrbuf_attach(&msg->buf, value, len, len + 1);\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 = NO_STRIPSPACE;\n@@ -705,12 +703,11 @@ static int append_edit(int argc, const char **argv, const char *prefix)\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\t\tstrbuf_attach(&buf, prev_buf, size, size + 1);\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 \n-\t\tfree(prev_buf);\n \t\tstrbuf_release(&buf);\n \t}\n \n-- \n2.35.3\n\n"},{"id":"488888","messageId":"20240218195938.6253-5-maarten.bosmans@vortech.nl","threadId":"60853","inReplyTo":"20240218195938.6253-1-maarten.bosmans@vortech.nl","subject":"[PATCH v2 4/5] notes: do not clean up right before calling die()","fromName":"Maarten Bosmans","fromEmail":"mkbosmans@gmail.com","sentAt":"2024-02-18T19:59:37Z","receivedAt":"2024-02-18T19:59:42Z","isPatch":true,"sender":{"key":"mkbosmans@gmail.com","avatar":null},"body":"From: Maarten Bosmans \n\nFrom: Maarten Bosmans \n\nFor consistency with the other uses of die() in the same function.\n\nSigned-off-by: Maarten Bosmans \n---\n builtin/notes.c | 6 +-----\n 1 file changed, 1 insertion(+), 5 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 184a92d0c1..6863935d03 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -311,12 +311,8 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)\n \t\tdie(_(\"failed to resolve '%s' as a valid ref.\"), arg);\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(&msg->buf);\n-\t\tfree(value);\n-\t\tfree(msg);\n+\tif (type != OBJ_BLOB)\n \t\tdie(_(\"cannot read note data from non-blob object '%s'.\"), arg);\n-\t}\n \n \tstrbuf_add(&msg->buf, value, len);\n \tfree(value);\n-- \n2.35.3\n\n"},{"id":"488947","messageId":"xmqqjzn0x72z.fsf@gitster.g","threadId":"60853","inReplyTo":"20240218195938.6253-2-maarten.bosmans@vortech.nl","subject":"Re: [PATCH v2 1/5] log: Move show_blob_object() to log.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-20T01:22:44Z","receivedAt":"2024-02-20T01:22:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Maarten Bosmans writes:\n\n> Subject: Re: [PATCH v2 1/5] log: Move show_blob_object() to log.c\n\n\"Move\" -> \"move\".\n\n> From: Maarten Bosmans \n>\n> From: Maarten Bosmans \n\nThe earlier one is not needed. Please do not include such a line.\n\n>\n> This way it can be used outside of builtin/log.c.\n> The next commit will make builtin/notes.c use it.\n\nThe resulting file is only about a small part of the implementation\ndetail of \"show\", and has very little to do with \"log\".\n\n\"show.c\" that happens to house show_blob_object(), a \"canonical\" way\nto display a blob object, with an anticipation that somebody may\nwant to expand it in the future to house the \"canonical\" way to\ndisplay a tag, a tree or a commit object, would be OK, though.\n"},{"id":"488949","messageId":"20240220015133.GA2713741@coredump.intra.peff.net","threadId":"60853","inReplyTo":"CA+CvcKSQCUukfLNnRkmTp=K=aXBRaxQnattfL+QexgOsYX18nA@mail.gmail.com","subject":"Re: [PATCH 1/4] notes: print note blob to stdout directly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-20T01:51:33Z","receivedAt":"2024-02-20T01:51:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 17, 2024 at 01:45:39PM +0100, Maarten Bosmans wrote:\n\n> > Of the two, I'd guess that the second one is a lot less work to\n> > implement (on the Git side; on the reading side it's a little more\n> > involved, but still should be a constant number of processes).\n> \n> The second one is attractive for another reason than implementation\n> simplicity. While the first one offers more flexibility, the second\n> reuses the existing cat-file batch format, so the interface between\n> git and scripts is familiar and consistent.\n\nAgreed.\n\n> > One variant of the second one is to use \"git notes list\". For example,\n> > you can get all notes via cat-file like this right now:\n> >\n> > git notes list |\n> > git cat-file --batch='%(objectname) %(objectsize) %(rest)'\n> \n> So the cat-file batch output is suitable for blobs containing newline\n> or NUL characters. But I struggle a bit with what would be an easy way\n> of using this format in a shell script. Something with multiple read\n> and read -N commands reading from the output, I guess.\n> The git codebase has `extract_batch_output()` in t1006. This uses a\n> separate perl invocation to parse the cat-file output, which confirms\n> my suspicion there isn't a straight-forward way to do this in e.g.\n> just a bash script.\n\nYes, you could perhaps do it with \"read -N\". But note that it is a\nbash-ism, and other POSIX shells may not support it. That's one of the\nreasons we do not use it in t1006.\n\nAlso, I suspect that even bash does not retain NUL bytes in shell\nvariables. E.g.:\n\n $ printf '\\0foo\\0bar\\0' | cat -A\n ^@foo^@bar^@\n\n $ printf '\\0foo\\0bar\\0' | (read -r -N 9 var; echo \"var=$var\" | cat -A)\n var=foobar$\n\nSo pure shell is probably a losing battle if you want to be binary\nclean. You might be able to do something like using \"read\" to get the\nheader line, and then another tool like \"head -c\" to read those bytes.\nBut now you're back to one process per object. Plus depending on the\nimplementation of \"head\", it might or might not read more bytes from the\npipe as part of its stdio buffering.\n\nSo really, you are probably better off handling the output with a more\ncapable language (though again, there's not much Git can do here; the\ncomplications are from handling binary data, and not Git's output format\nchoices).\n\n> That was why my first steps were to accept that a launching a separate\n> process per note in a bash loop is a pretty clear and well understood\n> idiom in shell scripts and try to make the git part of that a bit more\n> efficient.\n\nYes, I agree it's a simple idiom. And I certainly don't mind any\nspeedups we can get there, if they don't come with a cost (and your\npatches looked pretty reasonable to me, though I didn't read them too\ncarefully). But I do think anytime you are invoking N+1 processes in a\nshell script, it's kind of a losing battle for performance. :)\n\n-Peff\n"},{"id":"488950","messageId":"20240220015928.GB2713741@coredump.intra.peff.net","threadId":"60853","inReplyTo":"xmqqjzn0x72z.fsf@gitster.g","subject":"Re: [PATCH v2 1/5] log: Move show_blob_object() to log.c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-20T01:59:28Z","receivedAt":"2024-02-20T01:59:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 19, 2024 at 05:22:44PM -0800, Junio C Hamano wrote:\n\n> > This way it can be used outside of builtin/log.c.\n> > The next commit will make builtin/notes.c use it.\n> \n> The resulting file is only about a small part of the implementation\n> detail of \"show\", and has very little to do with \"log\".\n> \n> \"show.c\" that happens to house show_blob_object(), a \"canonical\" way\n> to display a blob object, with an anticipation that somebody may\n> want to expand it in the future to house the \"canonical\" way to\n> display a tag, a tree or a commit object, would be OK, though.\n\nI'm not sure this function (or its siblings in builtin/log.c) counts as\n\"canonical\", since it is deeply connected to \"struct rev_info\". So it is\nappropriate for log, show, etc, but not for other commands.\n\nIt felt a little funny to me to make a new file just for this one\nfunction. The related functions are all in log-tree.c. And as a bonus,\nthe name is _already_ horribly confusing because it says \"tree\" but the\nfunctions are really about showing commits. ;)\n\nAll that said, I'm not sure based on our previous discussion why we\ncan't just call stream_blob_to_fd(). Looking at show_blob_object(), most\nof the logic is about recording the tree-context of the given name and\nusing it for textconv. But since we know we are feeding a bare oid,\nthat would never kick in. So I don't know if there's any value in\nsharing this function more widely in the first place.\n\n-Peff\n"},{"id":"488951","messageId":"20240220021220.GC2713741@coredump.intra.peff.net","threadId":"60853","inReplyTo":"20240218195938.6253-6-maarten.bosmans@vortech.nl","subject":"Re: [PATCH v2 5/5] notes: use strbuf_attach to take ownership of the object contents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-20T02:12:20Z","receivedAt":"2024-02-20T02:12:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 18, 2024 at 08:59:38PM +0100, Maarten Bosmans wrote:\n\n> @@ -705,12 +703,11 @@ static int append_edit(int argc, const char **argv, const char *prefix)\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\t\tstrbuf_attach(&buf, prev_buf, size, size + 1);\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> \n> -\t\tfree(prev_buf);\n> \t\tstrbuf_release(&buf);\n> \t}\n\nIs it possible for \"size\" to be 0, but prev_buf to be non-NULL? I assume\nit is so if the previous note is the empty object (and anyway, we'd have\ndied earlier if prev_buf was NULL). In that case your patch introduces a\nleak (we do not attach prev_buf to buf, but we no longer free prev_buf).\n\nI'm a little skeptical that this is actually increasing the speed of the\ncommand in a measurable way, though. It's one allocation/copy, right\nnext to a big old strbuf_insert() that is going to splice into an\nexisting array.\n\n-Peff\n"},{"id":"488956","messageId":"xmqqy1bfx2f5.fsf@gitster.g","threadId":"60853","inReplyTo":"20240220015928.GB2713741@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/5] log: Move show_blob_object() to log.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-20T03:03:26Z","receivedAt":"2024-02-20T03:03:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> All that said, I'm not sure based on our previous discussion why we\n> can't just call stream_blob_to_fd(). Looking at show_blob_object(), most\n> of the logic is about recording the tree-context of the given name and\n> using it for textconv. But since we know we are feeding a bare oid,\n> that would never kick in. So I don't know if there's any value in\n> sharing this function more widely in the first place.\n\nIt is very nice that we do not need to touch this code move at all.\nThanks, as usual, for a dose of sanity.\n"},{"id":"488964","messageId":"CA+CvcKQyXu5JzhTnfQ7W2uaJNk_aP-2bh0HqP3npsfwUJn0Cew@mail.gmail.com","threadId":"60853","inReplyTo":"20240220021220.GC2713741@coredump.intra.peff.net","subject":"Re: [PATCH v2 5/5] notes: use strbuf_attach to take ownership of the object contents","fromName":"Maarten Bosmans","fromEmail":"mkbosmans@gmail.com","sentAt":"2024-02-20T07:42:13Z","receivedAt":"2024-02-20T07:42:31Z","isPatch":true,"sender":{"key":"mkbosmans@gmail.com","avatar":null},"body":"Op di 20 feb 2024 om 03:12 schreef Jeff King :\n>\n> On Sun, Feb 18, 2024 at 08:59:38PM +0100, Maarten Bosmans wrote:\n>\n> > @@ -705,12 +703,11 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> > if (!prev_buf)\n> > die(_(\"unable to read %s\"), oid_to_hex(note));\n> > if (size)\n> > - strbuf_add(&buf, prev_buf, size);\n> > + strbuf_attach(&buf, prev_buf, size, size + 1);\n> > if (d.buf.len && size)\n> > append_separator(&buf);\n> > strbuf_insert(&d.buf, 0, buf.buf, buf.len);\n> >\n> > - free(prev_buf);\n> > strbuf_release(&buf);\n> > }\n>\n> Is it possible for \"size\" to be 0, but prev_buf to be non-NULL? I assume\n> it is so if the previous note is the empty object (and anyway, we'd have\n> died earlier if prev_buf was NULL). In that case your patch introduces a\n> leak (we do not attach prev_buf to buf, but we no longer free prev_buf).\n\nYou are right. I think the `if (size)` is not needed and removing it\nwould remove the potential for a leak.\n\n> I'm a little skeptical that this is actually increasing the speed of the\n> command in a measurable way, though. It's one allocation/copy, right\n> next to a big old strbuf_insert() that is going to splice into an\n> existing array.\n\nYeah, I was doubting this patch a bit too. The simple idiom of\nstarting with an empty strbuf and appending strings to it seems pretty\nnice and clear, so may be there's value in leaving it at that.\nThe speed increase is not measurable of course. I was simply operating\nin full on lets-eliminate-all-sources-of-overhead mode while profiling\nthe notes show code.\n\nI'll drop the patch, in order to keep focus in this series.\n\nMaarten\n"},{"id":"488986","messageId":"CA+CvcKTFSyC-76=DYDdbLV8=cou0JwexS6iFOa0P0_r5NoBEQg@mail.gmail.com","threadId":"60853","inReplyTo":"20240220015928.GB2713741@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/5] log: Move show_blob_object() to log.c","fromName":"Maarten Bosmans","fromEmail":"mkbosmans@gmail.com","sentAt":"2024-02-20T11:40:18Z","receivedAt":"2024-02-20T11:40:36Z","isPatch":true,"sender":{"key":"mkbosmans@gmail.com","avatar":null},"body":"Op di 20 feb 2024 om 02:59 schreef Jeff King :\n> All that said, I'm not sure based on our previous discussion why we\n> can't just call stream_blob_to_fd(). Looking at show_blob_object(), most\n> of the logic is about recording the tree-context of the given name and\n> using it for textconv. But since we know we are feeding a bare oid,\n> that would never kick in. So I don't know if there's any value in\n> sharing this function more widely in the first place.\n\nIndeed. My original analysis of what `git show` does when invoked by\n`git notes show` led to the conclusion that the only thing worthwhile\nto keep is the `setup_pager()` call. Thanks for confirming this.\n\nI'll reroll in a couple of days with the V1 approach back.\n\nMaarten\n"}]}