{"thread":{"id":"65502","subject":"[PATCH 0/3] Batch prefetching","startedAt":"2026-04-16T22:48:17Z","lastAt":"2026-05-18T16:56:06Z","messageCount":29,"participants":["Elijah Newren via GitGitGadget","Junio C Hamano","Elijah Newren","Phillip Wood","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"541781","messageId":"pull.2089.git.1776379694.gitgitgadget@gmail.com","threadId":"65502","inReplyTo":null,"subject":"[PATCH 0/3] Batch prefetching","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-04-16T22:48:11Z","receivedAt":"2026-04-16T22:48:17Z","isPatch":true,"body":"Partial clones provide a trade-off for users: avoid downloading blobs\nupfront, at the expense of needing to download them later as they run other\ncommands. This tradeoff can sometimes incur a more severe cost than\nexpected, particularly if needed blobs are discovered as they are accessed,\nresulting in downloading blobs one at a time. Some commands like checkout,\ndiff, and merge do batch prefetches of necessary blobs, since that can\ndramatically reduce the pain of on-demand loading. Extend this ability to\ntwo more commands: cherry and grep.\n\nThis series was spurred by a report where git cherry jobs were each doing\nhundreds of single-blob fetches, at a cost of 3s each. Batching those\ndownloads should dramatically speed up their jobs. (And I decided to fix up\ngit grep similarly while at it.)\n\nI'll also note that git backfill with revisions and/or pathspecs could also\nimprove things for these users, but since backfill is a manual command users\nwould have to run and requires users to try to figure out which data is\nneeded (a challenge in the case of cherry), it still makes sense to provide\nsmarter behavior for folks who don't choose to manually run backfill.\n\nAlso, correct a documentation typo I noticed in patch-ids.h (related to code\nI was using for the git cherry fixes) as a preparatory fixup.\n\nElijah Newren (3):\n patch-ids.h: add missing trailing parenthesis in documentation comment\n builtin/log: prefetch necessary blobs for `git cherry`\n grep: prefetch necessary blobs\n\n builtin/grep.c | 142 ++++++++++++\n builtin/log.c | 125 +++++++++++\n investigations/cherry-prefetch-design-spec.md | 210 ++++++++++++++++++\n patch-ids.h | 2 +-\n t/t3500-cherry.sh | 18 ++\n t/t7810-grep.sh | 35 +++\n 6 files changed, 531 insertions(+), 1 deletion(-)\n create mode 100644 investigations/cherry-prefetch-design-spec.md\n\n\nbase-commit: 9f223ef1c026d91c7ac68cc0211bde255dda6199\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2089%2Fnewren%2Fbatch-prefetching-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2089/newren/batch-prefetching-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/2089\n-- \ngitgitgadget\n"},{"id":"541782","messageId":"7f5ac5942ebfccf2787d582040185245902056f7.1776379694.git.gitgitgadget@gmail.com","threadId":"65502","inReplyTo":"pull.2089.git.1776379694.gitgitgadget@gmail.com","subject":"[PATCH 1/3] patch-ids.h: add missing trailing parenthesis in documentation comment","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-04-16T22:48:12Z","receivedAt":"2026-04-16T22:48:19Z","isPatch":true,"body":"From: Elijah Newren \n\nSigned-off-by: Elijah Newren \n---\n patch-ids.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/patch-ids.h b/patch-ids.h\nindex 490d739371..57534ee722 100644\n--- a/patch-ids.h\n+++ b/patch-ids.h\n@@ -37,7 +37,7 @@ int has_commit_patch_id(struct commit *commit, struct patch_ids *);\n * struct patch_id *cur;\n * for (cur = patch_id_iter_first(commit, ids);\n * cur;\n- * cur = patch_id_iter_next(cur, ids) {\n+ * cur = patch_id_iter_next(cur, ids)) {\n * ... look at cur->commit\n * }\n */\n-- \ngitgitgadget\n\n"},{"id":"541783","messageId":"610be2a49a17620b2e5cfd3b7d9d38977ef77afb.1776379694.git.gitgitgadget@gmail.com","threadId":"65502","inReplyTo":"pull.2089.git.1776379694.gitgitgadget@gmail.com","subject":"[PATCH 2/3] builtin/log: prefetch necessary blobs for `git cherry`","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-04-16T22:48:13Z","receivedAt":"2026-04-16T22:48:20Z","isPatch":true,"body":"From: Elijah Newren \n\nIn partial clones, `git cherry` fetches necessary blobs on-demand one\nat a time, which can be very slow. We would like to prefetch all\nnecessary blobs upfront. To do so, we need to be able to first figure\nout which blobs are needed.\n\n`git cherry` does its work in a two-phase approach: first computing\nheader-only IDs (based on file paths and modes), then falling back to\nfull content-based IDs only when header-only IDs collide -- or, more\naccurately, whenever the oidhash() of the header-only object_ids\ncollide.\n\npatch-ids.c handles this by creating an ids->patches hashmap that has\nall the data we need, but the problem is that any attempt to query the\nhashmap will invoke the patch_id_neq() function on any colliding objects,\nwhich causes the on-demand fetching.\n\nInsert a new prefetch_cherry_blobs() function before checking for\ncollisions. Use a temporary replacement on the ids->patches.cmpfn\nin order to enumerate the blobs that would be needed without yet\nfetching them, and then fetch them all at once, then restore the old\nids->patches.cmpfn.\n\nSigned-off-by: Elijah Newren \n---\n builtin/log.c | 125 +++++++++++\n investigations/cherry-prefetch-design-spec.md | 210 ++++++++++++++++++\n t/t3500-cherry.sh | 18 ++\n 3 files changed, 353 insertions(+)\n create mode 100644 investigations/cherry-prefetch-design-spec.md\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 8c0939dd42..df19876be6 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -21,10 +21,12 @@\n #include \"color.h\"\n #include \"commit.h\"\n #include \"diff.h\"\n+#include \"diffcore.h\"\n #include \"diff-merges.h\"\n #include \"revision.h\"\n #include \"log-tree.h\"\n #include \"oid-array.h\"\n+#include \"oidset.h\"\n #include \"tag.h\"\n #include \"reflog-walk.h\"\n #include \"patch-ids.h\"\n@@ -43,9 +45,11 @@\n #include \"utf8.h\"\n \n #include \"commit-reach.h\"\n+#include \"promisor-remote.h\"\n #include \"range-diff.h\"\n #include \"tmp-objdir.h\"\n #include \"tree.h\"\n+#include \"userdiff.h\"\n #include \"write-or-die.h\"\n \n #define MAIL_DEFAULT_WRAP 72\n@@ -2602,6 +2606,125 @@ static void print_commit(char sign, struct commit *commit, int verbose,\n \t}\n }\n \n+/*\n+ * Enumerate blob OIDs from a single commit's diff, inserting them into blobs.\n+ * Skips files whose userdiff driver explicitly declares binary status\n+ * (drv->binary > 0), since patch-ID uses oid_to_hex() for those and\n+ * never reads blob content. Use userdiff_find_by_path() since\n+ * diff_filespec_load_driver() is static in diff.c.\n+ *\n+ * Clean up with diff_queue_clear() (from diffcore.h).\n+ */\n+static void collect_diff_blob_oids(struct commit *commit,\n+\t\t\t\t struct diff_options *opts,\n+\t\t\t\t struct oidset *blobs)\n+{\n+\tstruct diff_queue_struct *q;\n+\n+\t/*\n+\t * Merge commits are filtered out by patch_id_defined() in patch-ids.c,\n+\t * so we'll never be called with one.\n+\t */\n+\tassert(!commit->parents || !commit->parents->next);\n+\n+\tif (commit->parents)\n+\t\tdiff_tree_oid(&commit->parents->item->object.oid,\n+\t\t\t &commit->object.oid, \"\", opts);\n+\telse\n+\t\tdiff_root_tree_oid(&commit->object.oid, \"\", opts);\n+\tdiffcore_std(opts);\n+\n+\tq = &diff_queued_diff;\n+\tfor (int i = 0; i < q->nr; i++) {\n+\t\tstruct diff_filepair *p = q->queue[i];\n+\t\tstruct userdiff_driver *drv;\n+\n+\t\t/* Skip binary files */\n+\t\tdrv = userdiff_find_by_path(opts->repo->index, p->one->path);\n+\t\tif (drv && drv->binary > 0)\n+\t\t\tcontinue;\n+\n+\t\tif (DIFF_FILE_VALID(p->one))\n+\t\t\toidset_insert(blobs, &p->one->oid);\n+\t\tif (DIFF_FILE_VALID(p->two))\n+\t\t\toidset_insert(blobs, &p->two->oid);\n+\t}\n+\tdiff_queue_clear(q);\n+}\n+\n+static int always_match(const void *cmp_data UNUSED,\n+\t\t\tconst struct hashmap_entry *entry1 UNUSED,\n+\t\t\tconst struct hashmap_entry *entry2 UNUSED,\n+\t\t\tconst void *keydata UNUSED)\n+{\n+\treturn 0;\n+}\n+\n+/*\n+ * Prefetch blobs for git cherry in partial clones.\n+ *\n+ * Called between the revision walk (which builds the head-side\n+ * commit list) and the has_commit_patch_id() comparison loop.\n+ *\n+ * Uses a cmpfn-swap trick to avoid reading blobs: temporarily\n+ * replaces the hashmap's comparison function with a trivial\n+ * always-match function, so hashmap_get()/hashmap_get_next() match\n+ * any entry with the same oidhash bucket. These are the set of oids\n+ * that would trigger patch_id_neq() during normal lookup and cause\n+ * blobs to be read on demand, and we want to prefetch them all at\n+ * once instead.\n+ */\n+static void prefetch_cherry_blobs(struct repository *repo,\n+\t\t\t\t struct commit_list *list,\n+\t\t\t\t struct patch_ids *ids)\n+{\n+\tstruct oidset blobs = OIDSET_INIT;\n+\thashmap_cmp_fn original_cmpfn;\n+\n+\t/* Exit if we're not in a partial clone */\n+\tif (!repo_has_promisor_remote(repo))\n+\t\treturn;\n+\n+\t/* Save original cmpfn, replace with always_match */\n+\toriginal_cmpfn = ids->patches.cmpfn;\n+\tids->patches.cmpfn = always_match;\n+\n+\t/* Find header-only collisions, gather blobs from those commits */\n+\tfor (struct commit_list *l = list; l; l = l->next) {\n+\t\tstruct commit *c = l->item;\n+\t\tbool match_found = false;\n+\t\tfor (struct patch_id *cur = patch_id_iter_first(c, ids);\n+\t\t cur;\n+\t\t cur = patch_id_iter_next(cur, ids)) {\n+\t\t\tmatch_found = true;\n+\t\t\tcollect_diff_blob_oids(cur->commit, &ids->diffopts,\n+\t\t\t\t\t &blobs);\n+\t\t}\n+\t\tif (match_found)\n+\t\t\tcollect_diff_blob_oids(c, &ids->diffopts, &blobs);\n+\t}\n+\n+\t/* Restore original cmpfn */\n+\tids->patches.cmpfn = original_cmpfn;\n+\n+\t/* If we have any blobs to fetch, fetch them */\n+\tif (oidset_size(&blobs)) {\n+\t\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n+\t\tstruct oidset_iter iter;\n+\t\tconst struct object_id *oid;\n+\n+\t\toidset_iter_init(&blobs, &iter);\n+\t\twhile ((oid = oidset_iter_next(&iter)))\n+\t\t\toid_array_append(&to_fetch, oid);\n+\n+\t\tpromisor_remote_get_direct(repo, to_fetch.oid, to_fetch.nr);\n+\n+\t\toid_array_clear(&to_fetch);\n+\t}\n+\n+\toidset_clear(&blobs);\n+}\n+\n int cmd_cherry(int argc,\n \t const char **argv,\n \t const char *prefix,\n@@ -2673,6 +2796,8 @@ int cmd_cherry(int argc,\n \t\tcommit_list_insert(commit, &list);\n \t}\n \n+\tprefetch_cherry_blobs(the_repository, list, &ids);\n+\n \tfor (struct commit_list *l = list; l; l = l->next) {\n \t\tchar sign = '+';\n \ndiff --git a/investigations/cherry-prefetch-design-spec.md b/investigations/cherry-prefetch-design-spec.md\nnew file mode 100644\nindex 0000000000..a499d9538e\n--- /dev/null\n+++ b/investigations/cherry-prefetch-design-spec.md\n@@ -0,0 +1,210 @@\n+# Design Spec: Batch Blob Prefetch for `git cherry` in Partial Clones\n+\n+## Problem\n+\n+In a partial clone with `--filter=blob:none`, `git cherry` compares\n+commits using patch IDs. Patch IDs are computed in two phases:\n+\n+1. Header-only: hashes file paths and mode changes only (no blob reads)\n+2. Full: hashes actual diff content (requires reading blobs)\n+\n+Phase 2 only runs when two commits have matching header-only IDs\n+(i.e. they modify the same set of files with the same modes). This\n+is common — any two commits touching the same file(s) will collide.\n+\n+When phase 2 needs a blob that isn't local, it triggers an on-demand\n+promisor fetch. Each fetch is a separate network round-trip. With\n+many collisions, this means many sequential fetches.\n+\n+## Solution Overview\n+\n+Add a preparatory pass before the existing comparison loop in\n+`cmd_cherry()` that:\n+\n+1. Identifies which commit pairs will collide on header-only IDs\n+2. Collects all blob OIDs those commits will need\n+3. Batch-prefetches them in one fetch\n+\n+After this pass, the existing comparison loop runs as before, but\n+all needed blobs are already local, so no on-demand fetches occur.\n+\n+## Detailed Design\n+\n+### 1. No struct changes to patch_id\n+\n+The existing `struct patch_id` and `patch_id_neq()` are not\n+modified. `is_null_oid()` remains the sentinel for \"full ID not\n+yet computed\". No `has_full_patch_id` boolean, no extra fields.\n+\n+Key insight: `init_patch_id_entry()` stores only `oidhash()` (the\n+first 4 bytes of the header-only ID) in the hashmap bucket key.\n+The real `patch_id_neq()` comparison function is invoked only when\n+`hashmap_get()` or `hashmap_get_next()` finds entries with a\n+matching oidhash — and that comparison triggers blob reads.\n+\n+The prefetch needs to detect exactly those oidhash collisions\n+*without* triggering blob reads. We achieve this by temporarily\n+swapping the hashmap's comparison function.\n+\n+### 2. The prefetch function (in builtin/log.c)\n+\n+This function takes the repository, the head-side commit list (as\n+built by the existing revision walk in `cmd_cherry()`), and the\n+patch_ids structure (which contains the upstream entries).\n+\n+#### 2.1 Early exit\n+\n+If the repository has no promisor remote, return immediately.\n+Use `repo_has_promisor_remote()` from promisor-remote.h.\n+\n+#### 2.2 Swap in a trivial comparison function\n+\n+Save `ids->patches.cmpfn` (the real `patch_id_neq`) and replace\n+it with a trivial function that always returns 0 (\"equal\").\n+\n+```\n+static int patch_id_match(const void *unused_cmpfn_data,\n+ const struct hashmap_entry *a,\n+ const struct hashmap_entry *b,\n+ const void *unused_keydata)\n+{\n+ return 0;\n+}\n+```\n+\n+With this cmpfn in place, `hashmap_get()` and `hashmap_get_next()`\n+will match every entry in the same oidhash bucket — exactly the\n+same set that would trigger `patch_id_neq()` during normal lookup.\n+No blob reads occur because we never call the real comparison\n+function.\n+\n+#### 2.3 For each head-side commit, probe for collisions\n+\n+For each commit in the head-side list:\n+\n+- Use `patch_id_iter_first(commit, ids)` to probe the upstream\n+ hashmap. This handles `init_patch_id_entry()` + hashmap lookup\n+ internally. With our swapped cmpfn, it returns any upstream\n+ entry whose oidhash matches — i.e. any entry that *would*\n+ trigger `patch_id_neq()` during the real comparison loop.\n+ (Merge commits are already handled — `patch_id_iter_first()`\n+ returns NULL for them via `patch_id_defined()`.)\n+- If there's a match: collect blob OIDs from the head-side commit\n+ (see section 3).\n+- Then walk `patch_id_iter_next()` to find ALL upstream entries\n+ in the same bucket. For each, collect blob OIDs from that\n+ upstream commit too. (Multiple upstream commits can share the\n+ same oidhash bucket.)\n+- Collect blob OIDs from the first upstream match too (from\n+ `patch_id_iter_first()`).\n+\n+We need blobs from BOTH sides because `patch_id_neq()` computes\n+full patch IDs for both the upstream and head-side commit when\n+comparing.\n+\n+#### 2.4 Restore the original comparison function\n+\n+Set `ids->patches.cmpfn` back to the saved value (patch_id_neq).\n+This MUST happen before returning — the subsequent\n+`has_commit_patch_id()` loop needs the real comparison function.\n+\n+#### 2.5 Batch prefetch\n+\n+If the oidset is non-empty, populate an oid_array from it using\n+`oidset_iter_first()`/`oidset_iter_next()`, then call\n+`promisor_remote_get_direct(repo, oid_array.oid, oid_array.nr)`.\n+\n+This is a single network round-trip regardless of how many blobs.\n+\n+#### 2.6 Cleanup\n+\n+Free the oid_array and the oidset.\n+\n+### 3. Collecting blob OIDs from a commit (helper function)\n+\n+Given a commit, enumerate the blobs its diff touches. Takes an\n+oidset to insert into (provides automatic dedup — consecutive\n+commits often share blob OIDs, e.g. B:foo == C^:foo when C's\n+parent is B).\n+\n+- Compute the diff: `diff_tree_oid()` for commits with a parent,\n+ `diff_root_tree_oid()` for root commits. Then `diffcore_std()`.\n+- These populate the global `diff_queued_diff` queue.\n+- For each filepair in the queue:\n+ - Check the userdiff driver for the file path. If the driver\n+ explicitly declares the file as binary (`drv->binary != -1`),\n+ skip it. Reason: patch-ID uses `oid_to_hex()` for binary\n+ files (see diff.c around line 6652) and never reads the blob.\n+ Use `userdiff_find_by_path()` (NOT `diff_filespec_load_driver`\n+ which is static in diff.c).\n+ - For both sides of the filepair (p->one and p->two): if the\n+ side is valid (`DIFF_FILE_VALID`) and has a non-null OID,\n+ check the dedup oidset — `oidset_insert()` handles dedup\n+ automatically (returns 1 if newly inserted, 0 if duplicate).\n+- Clear the diff queue with `diff_queue_clear()` (from diffcore.h,\n+ not diff.h).\n+\n+Note on `drv->binary`: The value -1 means \"not set\" (auto-detect\n+at read time by reading the blob); 0 means explicitly text (will\n+be diffed, blob reads needed); positive means explicitly binary\n+(patch-ID uses `oid_to_hex()`, no blob read needed).\n+\n+The correct skip condition is `drv && drv->binary > 0` — skip\n+only known-binary files. Do NOT use `drv->binary != -1`, which\n+would also skip explicitly-text files that DO need blob reads.\n+(The copilot reference implementation uses `!= -1`, which is\n+technically wrong but harmless in practice since explicit text\n+attributes are rare.)\n+\n+### 4. Call site in cmd_cherry()\n+\n+Insert the call between the revision walk loop (which builds the\n+head-side commit list) and the comparison loop (which calls\n+`has_commit_patch_id()`).\n+\n+### 5. Required includes in builtin/log.c\n+\n+- promisor-remote.h (for repo_has_promisor_remote,\n+ promisor_remote_get_direct)\n+- userdiff.h (for userdiff_find_by_path)\n+- oidset.h (for oidset used in blob OID dedup)\n+- diffcore.h (for diff_queue_clear)\n+\n+## Edge Cases\n+\n+- No promisor remote: early return, zero overhead\n+- No collisions: probes the hashmap for each head-side commit but\n+ finds no bucket matches, no blobs collected, no fetch issued\n+- Merge commits in head-side list: skipped (no patch ID defined)\n+- Root commits (no parent): use diff_root_tree_oid instead of\n+ diff_tree_oid\n+- Binary files (explicit driver): skipped, patch-ID doesn't read\n+ them\n+- The cmpfn swap approach matches at oidhash granularity (4 bytes),\n+ which is exactly what the hashmap itself uses to trigger\n+ patch_id_neq(). This means we prefetch for every case the real\n+ code would trigger, plus rare false-positive oidhash collisions\n+ (harmless: we fetch a few extra blobs that won't end up being\n+ compared). No under-fetching is possible.\n+\n+## Testing\n+\n+See t/t3500-cherry.sh on the copilot-faster-partial-clones branch\n+for two tests:\n+\n+Test 5: \"cherry batch-prefetches blobs in partial clone\"\n+ - Creates server with 3 upstream + 3 head-side commits modifying\n+ the same file (guarantees collisions)\n+ - Clones with --filter=blob:none\n+ - Runs `git cherry` with GIT_TRACE2_PERF\n+ - Asserts exactly 1 fetch (batch) instead of 6 (individual)\n+\n+Test 6: \"cherry prefetch omits blobs for cherry-picked commits\"\n+ - Creates a cherry-pick scenario (divergent branches, shared\n+ commit cherry-picked to head side)\n+ - Verifies `git cherry` correctly identifies the cherry-picked\n+ commit as \"-\" and head-only commits as \"+\"\n+ - Important: the head side must diverge before the cherry-pick\n+ so the cherry-pick creates a distinct commit object (otherwise\n+ the commit hash is identical and it's in the symmetric\n+ difference, not needing patch-ID comparison at all)\ndiff --git a/t/t3500-cherry.sh b/t/t3500-cherry.sh\nindex 78c3eac54b..17507d9a28 100755\n--- a/t/t3500-cherry.sh\n+++ b/t/t3500-cherry.sh\n@@ -78,4 +78,22 @@ test_expect_success 'cherry ignores whitespace' '\n \ttest_cmp expect actual\n '\n \n+# Reuse the expect file from the previous test, in a partial clone\n+test_expect_success 'cherry in partial clone does bulk prefetch' '\n+\ttest_config uploadpack.allowfilter 1 &&\n+\ttest_config uploadpack.allowanysha1inwant 1 &&\n+\ttest_when_finished \"rm -rf copy\" &&\n+\n+\tgit clone --bare --filter=blob:none file://\"$(pwd)\" copy &&\n+\t(\n+\t\tcd copy &&\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace.output\" git cherry upstream-with-space feature-without-space >actual &&\n+\t\ttest_cmp ../expect actual &&\n+\n+\t\tgrep \"child_start.*fetch.negotiationAlgorithm\" trace.output >fetches &&\n+\t\ttest_line_count = 1 fetches &&\n+\t\ttest_trace2_data promisor fetch_count 4 \n\nIn partial clones, `git grep` fetches necessary blobs on-demand one\nat a time, which can be very slow. In partial clones, add an extra\npreliminary walk over the tree similar to grep_tree() which collects\nthe blobs of interest, and then prefetches them.\n\nSigned-off-by: Elijah Newren \n---\n builtin/grep.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++++\n t/t7810-grep.sh | 35 ++++++++++++\n 2 files changed, 177 insertions(+)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex e33285e5e6..d559c48d94 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -28,9 +28,12 @@\n #include \"object-file.h\"\n #include \"object-name.h\"\n #include \"odb.h\"\n+#include \"oid-array.h\"\n+#include \"oidset.h\"\n #include \"packfile.h\"\n #include \"pager.h\"\n #include \"path.h\"\n+#include \"promisor-remote.h\"\n #include \"read-cache-ll.h\"\n #include \"write-or-die.h\"\n \n@@ -692,6 +695,143 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \treturn hit;\n }\n \n+static void collect_blob_oids_for_tree(struct repository *repo,\n+\t\t\t\t const struct pathspec *pathspec,\n+\t\t\t\t struct tree_desc *tree,\n+\t\t\t\t struct strbuf *base,\n+\t\t\t\t int tn_len,\n+\t\t\t\t struct oidset *blob_oids)\n+{\n+\tstruct name_entry entry;\n+\tint old_baselen = base->len;\n+\tstruct strbuf name = STRBUF_INIT;\n+\tenum interesting match = entry_not_interesting;\n+\n+\twhile (tree_entry(tree, &entry)) {\n+\t\tif (match != all_entries_interesting) {\n+\t\t\tstrbuf_addstr(&name, base->buf + tn_len);\n+\t\t\tmatch = tree_entry_interesting(repo->index,\n+\t\t\t\t\t\t &entry, &name,\n+\t\t\t\t\t\t pathspec);\n+\t\t\tstrbuf_reset(&name);\n+\n+\t\t\tif (match == all_entries_not_interesting)\n+\t\t\t\tbreak;\n+\t\t\tif (match == entry_not_interesting)\n+\t\t\t\tcontinue;\n+\t\t}\n+\n+\t\tstrbuf_add(base, entry.path, tree_entry_len(&entry));\n+\n+\t\tif (S_ISREG(entry.mode)) {\n+\t\t\toidset_insert(blob_oids, &entry.oid);\n+\t\t} else if (S_ISDIR(entry.mode)) {\n+\t\t\tenum object_type type;\n+\t\t\tstruct tree_desc sub_tree;\n+\t\t\tvoid *data;\n+\t\t\tunsigned long size;\n+\n+\t\t\tdata = odb_read_object(repo->objects, &entry.oid,\n+\t\t\t\t\t &type, &size);\n+\t\t\tif (!data)\n+\t\t\t\tdie(_(\"unable to read tree (%s)\"),\n+\t\t\t\t oid_to_hex(&entry.oid));\n+\n+\t\t\tstrbuf_addch(base, '/');\n+\t\t\tinit_tree_desc(&sub_tree, &entry.oid, data, size);\n+\t\t\tcollect_blob_oids_for_tree(repo, pathspec, &sub_tree,\n+\t\t\t\t\t\t base, tn_len, blob_oids);\n+\t\t\tfree(data);\n+\t\t}\n+\t\t/*\n+\t\t * ...no else clause for S_ISGITLINK: submodules have their\n+\t\t * own promisor configuration and would need separate fetches\n+\t\t * anyway.\n+\t\t */\n+\n+\t\tstrbuf_setlen(base, old_baselen);\n+\t}\n+\n+\tstrbuf_release(&name);\n+}\n+\n+static void collect_blob_oids_for_treeish(struct grep_opt *opt,\n+\t\t\t\t\t const struct pathspec *pathspec,\n+\t\t\t\t\t const struct object_id *tree_ish_oid,\n+\t\t\t\t\t const char *name,\n+\t\t\t\t\t struct oidset *blob_oids)\n+{\n+\tstruct tree_desc tree;\n+\tvoid *data;\n+\tunsigned long size;\n+\tstruct strbuf base = STRBUF_INIT;\n+\tint len;\n+\n+\tdata = odb_read_object_peeled(opt->repo->objects, tree_ish_oid,\n+\t\t\t\t OBJ_TREE, &size, NULL);\n+\n+\tif (!data)\n+\t\treturn;\n+\n+\tlen = name ? strlen(name) : 0;\n+\tif (len) {\n+\t\tstrbuf_add(&base, name, len);\n+\t\tstrbuf_addch(&base, ':');\n+\t}\n+\tinit_tree_desc(&tree, tree_ish_oid, data, size);\n+\n+\tcollect_blob_oids_for_tree(opt->repo, pathspec, &tree,\n+\t\t\t\t &base, base.len, blob_oids);\n+\n+\tstrbuf_release(&base);\n+\tfree(data);\n+}\n+\n+static void prefetch_grep_blobs(struct grep_opt *opt,\n+\t\t\t\tconst struct pathspec *pathspec,\n+\t\t\t\tconst struct object_array *list)\n+{\n+\tstruct oidset blob_oids = OIDSET_INIT;\n+\n+\t/* Exit if we're not in a partial clone */\n+\tif (!repo_has_promisor_remote(opt->repo))\n+\t\treturn;\n+\n+\t/* For each tree, gather the blobs in it */\n+\tfor (int i = 0; i < list->nr; i++) {\n+\t\tstruct object *real_obj;\n+\n+\t\tobj_read_lock();\n+\t\treal_obj = deref_tag(opt->repo, list->objects[i].item,\n+\t\t\t\t NULL, 0);\n+\t\tobj_read_unlock();\n+\n+\t\tif (real_obj &&\n+\t\t (real_obj->type == OBJ_COMMIT ||\n+\t\t real_obj->type == OBJ_TREE))\n+\t\t\tcollect_blob_oids_for_treeish(opt, pathspec,\n+\t\t\t\t\t\t &real_obj->oid,\n+\t\t\t\t\t\t list->objects[i].name,\n+\t\t\t\t\t\t &blob_oids);\n+\t}\n+\n+\t/* Prefetch the blobs we found */\n+\tif (oidset_size(&blob_oids)) {\n+\t\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n+\t\tstruct oidset_iter iter;\n+\t\tconst struct object_id *oid;\n+\n+\t\toidset_iter_init(&blob_oids, &iter);\n+\t\twhile ((oid = oidset_iter_next(&iter)))\n+\t\t\toid_array_append(&to_fetch, oid);\n+\n+\t\tpromisor_remote_get_direct(opt->repo, to_fetch.oid, to_fetch.nr);\n+\n+\t\toid_array_clear(&to_fetch);\n+\t}\n+\toidset_clear(&blob_oids);\n+}\n+\n static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t struct object *obj, const char *name, const char *path)\n {\n@@ -732,6 +872,8 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,\n \tint hit = 0;\n \tconst unsigned int nr = list->nr;\n \n+\tprefetch_grep_blobs(opt, pathspec, list);\n+\n \tfor (i = 0; i < nr; i++) {\n \t\tstruct object *real_obj;\n \ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 64ac4f04ee..1f484502fe 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -1929,4 +1929,39 @@ test_expect_success 'grep does not report i-t-a and assume unchanged with -L' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'grep of revision in partial clone does bulk prefetch' '\n+\ttest_when_finished \"rm -rf grep-partial-src grep-partial\" &&\n+\n+\tgit init grep-partial-src &&\n+\t(\n+\t\tcd grep-partial-src &&\n+\t\tgit config uploadpack.allowfilter 1 &&\n+\t\tgit config uploadpack.allowanysha1inwant 1 &&\n+\t\techo \"needle in haystack\" >searchme &&\n+\t\techo \"no match here\" >other &&\n+\t\tmkdir subdir &&\n+\t\techo \"needle again\" >subdir/deep &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"initial\"\n+\t) &&\n+\n+\tgit clone --no-checkout --filter=blob:none \\\n+\t\t\"file://$(pwd)/grep-partial-src\" grep-partial &&\n+\n+\t# All blobs should be missing after a blobless clone.\n+\tgit -C grep-partial rev-list --quiet --objects \\\n+\t\t--missing=print HEAD >missing &&\n+\ttest_line_count = 3 missing &&\n+\n+\t# grep HEAD should batch-prefetch all blobs in one request.\n+\tGIT_TRACE2_EVENT=\"$(pwd)/grep-trace\" \\\n+\t\tgit -C grep-partial grep -c \"needle\" HEAD >result &&\n+\n+\t# Should find matches in two files.\n+\ttest_line_count = 2 result &&\n+\n+\t# Should have prefetched all 3 objects at once\n+\ttest_trace2_data promisor fetch_count 3 writes:\n\n> From: Elijah Newren \n>\n> In partial clones, `git cherry` fetches necessary blobs on-demand one\n> at a time, which can be very slow. We would like to prefetch all\n> necessary blobs upfront. To do so, we need to be able to first figure\n> out which blobs are needed.\n>\n> `git cherry` does its work in a two-phase approach: first computing\n> header-only IDs (based on file paths and modes), then falling back to\n> full content-based IDs only when header-only IDs collide -- or, more\n> accurately, whenever the oidhash() of the header-only object_ids\n> collide.\n>\n> patch-ids.c handles this by creating an ids->patches hashmap that has\n> all the data we need, but the problem is that any attempt to query the\n> hashmap will invoke the patch_id_neq() function on any colliding objects,\n> which causes the on-demand fetching.\n>\n> Insert a new prefetch_cherry_blobs() function before checking for\n> collisions. Use a temporary replacement on the ids->patches.cmpfn\n> in order to enumerate the blobs that would be needed without yet\n> fetching them, and then fetch them all at once, then restore the old\n> ids->patches.cmpfn.\n>\n> Signed-off-by: Elijah Newren \n> ---\n> builtin/log.c | 125 +++++++++++\n> investigations/cherry-prefetch-design-spec.md | 210 ++++++++++++++++++\n\nDid you mean to add this file to the project? As a document to\ndescribe how \"git cherry\" works, it is vastly lacking, and once this\nseries lands, I am not sure how others would benefit from being able\nto read it. Many of the materials in there seem to typically be given\nin the log message, but not to this degree of details, so I am not\nsure where it belongs.\n\n> t/t3500-cherry.sh | 18 ++\n> 3 files changed, 353 insertions(+)\n> create mode 100644 investigations/cherry-prefetch-design-spec.md\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 8c0939dd42..df19876be6 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -21,10 +21,12 @@\n> #include \"color.h\"\n> #include \"commit.h\"\n> #include \"diff.h\"\n> +#include \"diffcore.h\"\n> #include \"diff-merges.h\"\n> #include \"revision.h\"\n> #include \"log-tree.h\"\n> #include \"oid-array.h\"\n> +#include \"oidset.h\"\n> #include \"tag.h\"\n> #include \"reflog-walk.h\"\n> #include \"patch-ids.h\"\n> @@ -43,9 +45,11 @@\n> #include \"utf8.h\"\n> \n> #include \"commit-reach.h\"\n> +#include \"promisor-remote.h\"\n> #include \"range-diff.h\"\n> #include \"tmp-objdir.h\"\n> #include \"tree.h\"\n> +#include \"userdiff.h\"\n> #include \"write-or-die.h\"\n> \n> #define MAIL_DEFAULT_WRAP 72\n> @@ -2602,6 +2606,125 @@ static void print_commit(char sign, struct commit *commit, int verbose,\n> \t}\n> }\n> \n> +/*\n> + * Enumerate blob OIDs from a single commit's diff, inserting them into blobs.\n> + * Skips files whose userdiff driver explicitly declares binary status\n> + * (drv->binary > 0), since patch-ID uses oid_to_hex() for those and\n> + * never reads blob content. Use userdiff_find_by_path() since\n> + * diff_filespec_load_driver() is static in diff.c.\n> + *\n> + * Clean up with diff_queue_clear() (from diffcore.h).\n> + */\n> +static void collect_diff_blob_oids(struct commit *commit,\n> +\t\t\t\t struct diff_options *opts,\n> +\t\t\t\t struct oidset *blobs)\n> +{\n> +\tstruct diff_queue_struct *q;\n> +\n> +\t/*\n> +\t * Merge commits are filtered out by patch_id_defined() in patch-ids.c,\n> +\t * so we'll never be called with one.\n> +\t */\n> +\tassert(!commit->parents || !commit->parents->next);\n> +\n> +\tif (commit->parents)\n> +\t\tdiff_tree_oid(&commit->parents->item->object.oid,\n> +\t\t\t &commit->object.oid, \"\", opts);\n> +\telse\n> +\t\tdiff_root_tree_oid(&commit->object.oid, \"\", opts);\n> +\tdiffcore_std(opts);\n> +\n> +\tq = &diff_queued_diff;\n> +\tfor (int i = 0; i < q->nr; i++) {\n> +\t\tstruct diff_filepair *p = q->queue[i];\n> +\t\tstruct userdiff_driver *drv;\n> +\n> +\t\t/* Skip binary files */\n> +\t\tdrv = userdiff_find_by_path(opts->repo->index, p->one->path);\n> +\t\tif (drv && drv->binary > 0)\n> +\t\t\tcontinue;\n> +\n> +\t\tif (DIFF_FILE_VALID(p->one))\n> +\t\t\toidset_insert(blobs, &p->one->oid);\n> +\t\tif (DIFF_FILE_VALID(p->two))\n> +\t\t\toidset_insert(blobs, &p->two->oid);\n> +\t}\n> +\tdiff_queue_clear(q);\n> +}\n> +\n> +static int always_match(const void *cmp_data UNUSED,\n> +\t\t\tconst struct hashmap_entry *entry1 UNUSED,\n> +\t\t\tconst struct hashmap_entry *entry2 UNUSED,\n> +\t\t\tconst void *keydata UNUSED)\n> +{\n> +\treturn 0;\n> +}\n> +\n> +/*\n> + * Prefetch blobs for git cherry in partial clones.\n> + *\n> + * Called between the revision walk (which builds the head-side\n> + * commit list) and the has_commit_patch_id() comparison loop.\n> + *\n> + * Uses a cmpfn-swap trick to avoid reading blobs: temporarily\n> + * replaces the hashmap's comparison function with a trivial\n> + * always-match function, so hashmap_get()/hashmap_get_next() match\n> + * any entry with the same oidhash bucket. These are the set of oids\n> + * that would trigger patch_id_neq() during normal lookup and cause\n> + * blobs to be read on demand, and we want to prefetch them all at\n> + * once instead.\n> + */\n> +static void prefetch_cherry_blobs(struct repository *repo,\n> +\t\t\t\t struct commit_list *list,\n> +\t\t\t\t struct patch_ids *ids)\n> +{\n> +\tstruct oidset blobs = OIDSET_INIT;\n> +\thashmap_cmp_fn original_cmpfn;\n> +\n> +\t/* Exit if we're not in a partial clone */\n> +\tif (!repo_has_promisor_remote(repo))\n> +\t\treturn;\n> +\n> +\t/* Save original cmpfn, replace with always_match */\n> +\toriginal_cmpfn = ids->patches.cmpfn;\n> +\tids->patches.cmpfn = always_match;\n> +\n> +\t/* Find header-only collisions, gather blobs from those commits */\n> +\tfor (struct commit_list *l = list; l; l = l->next) {\n> +\t\tstruct commit *c = l->item;\n> +\t\tbool match_found = false;\n> +\t\tfor (struct patch_id *cur = patch_id_iter_first(c, ids);\n> +\t\t cur;\n> +\t\t cur = patch_id_iter_next(cur, ids)) {\n> +\t\t\tmatch_found = true;\n> +\t\t\tcollect_diff_blob_oids(cur->commit, &ids->diffopts,\n> +\t\t\t\t\t &blobs);\n> +\t\t}\n> +\t\tif (match_found)\n> +\t\t\tcollect_diff_blob_oids(c, &ids->diffopts, &blobs);\n> +\t}\n> +\n> +\t/* Restore original cmpfn */\n> +\tids->patches.cmpfn = original_cmpfn;\n> +\n> +\t/* If we have any blobs to fetch, fetch them */\n> +\tif (oidset_size(&blobs)) {\n> +\t\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n> +\t\tstruct oidset_iter iter;\n> +\t\tconst struct object_id *oid;\n> +\n> +\t\toidset_iter_init(&blobs, &iter);\n> +\t\twhile ((oid = oidset_iter_next(&iter)))\n> +\t\t\toid_array_append(&to_fetch, oid);\n> +\n> +\t\tpromisor_remote_get_direct(repo, to_fetch.oid, to_fetch.nr);\n> +\n> +\t\toid_array_clear(&to_fetch);\n> +\t}\n> +\n> +\toidset_clear(&blobs);\n> +}\n> +\n> int cmd_cherry(int argc,\n> \t const char **argv,\n> \t const char *prefix,\n> @@ -2673,6 +2796,8 @@ int cmd_cherry(int argc,\n> \t\tcommit_list_insert(commit, &list);\n> \t}\n> \n> +\tprefetch_cherry_blobs(the_repository, list, &ids);\n> +\n> \tfor (struct commit_list *l = list; l; l = l->next) {\n> \t\tchar sign = '+';\n> \n> diff --git a/investigations/cherry-prefetch-design-spec.md b/investigations/cherry-prefetch-design-spec.md\n> new file mode 100644\n> index 0000000000..a499d9538e\n> --- /dev/null\n> +++ b/investigations/cherry-prefetch-design-spec.md\n> @@ -0,0 +1,210 @@\n> +# Design Spec: Batch Blob Prefetch for `git cherry` in Partial Clones\n> +\n> +## Problem\n> +\n> +In a partial clone with `--filter=blob:none`, `git cherry` compares\n> +commits using patch IDs. Patch IDs are computed in two phases:\n> +\n> +1. Header-only: hashes file paths and mode changes only (no blob reads)\n> +2. Full: hashes actual diff content (requires reading blobs)\n> +\n> +Phase 2 only runs when two commits have matching header-only IDs\n> +(i.e. they modify the same set of files with the same modes). This\n> +is common — any two commits touching the same file(s) will collide.\n> +\n> +When phase 2 needs a blob that isn't local, it triggers an on-demand\n> +promisor fetch. Each fetch is a separate network round-trip. With\n> +many collisions, this means many sequential fetches.\n> +\n> +## Solution Overview\n> +\n> +Add a preparatory pass before the existing comparison loop in\n> +`cmd_cherry()` that:\n> +\n> +1. Identifies which commit pairs will collide on header-only IDs\n> +2. Collects all blob OIDs those commits will need\n> +3. Batch-prefetches them in one fetch\n> +\n> +After this pass, the existing comparison loop runs as before, but\n> +all needed blobs are already local, so no on-demand fetches occur.\n> +\n> +## Detailed Design\n> +\n> +### 1. No struct changes to patch_id\n> +\n> +The existing `struct patch_id` and `patch_id_neq()` are not\n> +modified. `is_null_oid()` remains the sentinel for \"full ID not\n> +yet computed\". No `has_full_patch_id` boolean, no extra fields.\n> +\n> +Key insight: `init_patch_id_entry()` stores only `oidhash()` (the\n> +first 4 bytes of the header-only ID) in the hashmap bucket key.\n> +The real `patch_id_neq()` comparison function is invoked only when\n> +`hashmap_get()` or `hashmap_get_next()` finds entries with a\n> +matching oidhash — and that comparison triggers blob reads.\n> +\n> +The prefetch needs to detect exactly those oidhash collisions\n> +*without* triggering blob reads. We achieve this by temporarily\n> +swapping the hashmap's comparison function.\n> +\n> +### 2. The prefetch function (in builtin/log.c)\n> +\n> +This function takes the repository, the head-side commit list (as\n> +built by the existing revision walk in `cmd_cherry()`), and the\n> +patch_ids structure (which contains the upstream entries).\n> +\n> +#### 2.1 Early exit\n> +\n> +If the repository has no promisor remote, return immediately.\n> +Use `repo_has_promisor_remote()` from promisor-remote.h.\n> +\n> +#### 2.2 Swap in a trivial comparison function\n> +\n> +Save `ids->patches.cmpfn` (the real `patch_id_neq`) and replace\n> +it with a trivial function that always returns 0 (\"equal\").\n> +\n> +```\n> +static int patch_id_match(const void *unused_cmpfn_data,\n> + const struct hashmap_entry *a,\n> + const struct hashmap_entry *b,\n> + const void *unused_keydata)\n> +{\n> + return 0;\n> +}\n> +```\n> +\n> +With this cmpfn in place, `hashmap_get()` and `hashmap_get_next()`\n> +will match every entry in the same oidhash bucket — exactly the\n> +same set that would trigger `patch_id_neq()` during normal lookup.\n> +No blob reads occur because we never call the real comparison\n> +function.\n> +\n> +#### 2.3 For each head-side commit, probe for collisions\n> +\n> +For each commit in the head-side list:\n> +\n> +- Use `patch_id_iter_first(commit, ids)` to probe the upstream\n> + hashmap. This handles `init_patch_id_entry()` + hashmap lookup\n> + internally. With our swapped cmpfn, it returns any upstream\n> + entry whose oidhash matches — i.e. any entry that *would*\n> + trigger `patch_id_neq()` during the real comparison loop.\n> + (Merge commits are already handled — `patch_id_iter_first()`\n> + returns NULL for them via `patch_id_defined()`.)\n> +- If there's a match: collect blob OIDs from the head-side commit\n> + (see section 3).\n> +- Then walk `patch_id_iter_next()` to find ALL upstream entries\n> + in the same bucket. For each, collect blob OIDs from that\n> + upstream commit too. (Multiple upstream commits can share the\n> + same oidhash bucket.)\n> +- Collect blob OIDs from the first upstream match too (from\n> + `patch_id_iter_first()`).\n> +\n> +We need blobs from BOTH sides because `patch_id_neq()` computes\n> +full patch IDs for both the upstream and head-side commit when\n> +comparing.\n> +\n> +#### 2.4 Restore the original comparison function\n> +\n> +Set `ids->patches.cmpfn` back to the saved value (patch_id_neq).\n> +This MUST happen before returning — the subsequent\n> +`has_commit_patch_id()` loop needs the real comparison function.\n> +\n> +#### 2.5 Batch prefetch\n> +\n> +If the oidset is non-empty, populate an oid_array from it using\n> +`oidset_iter_first()`/`oidset_iter_next()`, then call\n> +`promisor_remote_get_direct(repo, oid_array.oid, oid_array.nr)`.\n> +\n> +This is a single network round-trip regardless of how many blobs.\n> +\n> +#### 2.6 Cleanup\n> +\n> +Free the oid_array and the oidset.\n> +\n> +### 3. Collecting blob OIDs from a commit (helper function)\n> +\n> +Given a commit, enumerate the blobs its diff touches. Takes an\n> +oidset to insert into (provides automatic dedup — consecutive\n> +commits often share blob OIDs, e.g. B:foo == C^:foo when C's\n> +parent is B).\n> +\n> +- Compute the diff: `diff_tree_oid()` for commits with a parent,\n> + `diff_root_tree_oid()` for root commits. Then `diffcore_std()`.\n> +- These populate the global `diff_queued_diff` queue.\n> +- For each filepair in the queue:\n> + - Check the userdiff driver for the file path. If the driver\n> + explicitly declares the file as binary (`drv->binary != -1`),\n> + skip it. Reason: patch-ID uses `oid_to_hex()` for binary\n> + files (see diff.c around line 6652) and never reads the blob.\n> + Use `userdiff_find_by_path()` (NOT `diff_filespec_load_driver`\n> + which is static in diff.c).\n> + - For both sides of the filepair (p->one and p->two): if the\n> + side is valid (`DIFF_FILE_VALID`) and has a non-null OID,\n> + check the dedup oidset — `oidset_insert()` handles dedup\n> + automatically (returns 1 if newly inserted, 0 if duplicate).\n> +- Clear the diff queue with `diff_queue_clear()` (from diffcore.h,\n> + not diff.h).\n> +\n> +Note on `drv->binary`: The value -1 means \"not set\" (auto-detect\n> +at read time by reading the blob); 0 means explicitly text (will\n> +be diffed, blob reads needed); positive means explicitly binary\n> +(patch-ID uses `oid_to_hex()`, no blob read needed).\n> +\n> +The correct skip condition is `drv && drv->binary > 0` — skip\n> +only known-binary files. Do NOT use `drv->binary != -1`, which\n> +would also skip explicitly-text files that DO need blob reads.\n> +(The copilot reference implementation uses `!= -1`, which is\n> +technically wrong but harmless in practice since explicit text\n> +attributes are rare.)\n> +\n> +### 4. Call site in cmd_cherry()\n> +\n> +Insert the call between the revision walk loop (which builds the\n> +head-side commit list) and the comparison loop (which calls\n> +`has_commit_patch_id()`).\n> +\n> +### 5. Required includes in builtin/log.c\n> +\n> +- promisor-remote.h (for repo_has_promisor_remote,\n> + promisor_remote_get_direct)\n> +- userdiff.h (for userdiff_find_by_path)\n> +- oidset.h (for oidset used in blob OID dedup)\n> +- diffcore.h (for diff_queue_clear)\n> +\n> +## Edge Cases\n> +\n> +- No promisor remote: early return, zero overhead\n> +- No collisions: probes the hashmap for each head-side commit but\n> + finds no bucket matches, no blobs collected, no fetch issued\n> +- Merge commits in head-side list: skipped (no patch ID defined)\n> +- Root commits (no parent): use diff_root_tree_oid instead of\n> + diff_tree_oid\n> +- Binary files (explicit driver): skipped, patch-ID doesn't read\n> + them\n> +- The cmpfn swap approach matches at oidhash granularity (4 bytes),\n> + which is exactly what the hashmap itself uses to trigger\n> + patch_id_neq(). This means we prefetch for every case the real\n> + code would trigger, plus rare false-positive oidhash collisions\n> + (harmless: we fetch a few extra blobs that won't end up being\n> + compared). No under-fetching is possible.\n> +\n> +## Testing\n> +\n> +See t/t3500-cherry.sh on the copilot-faster-partial-clones branch\n> +for two tests:\n> +\n> +Test 5: \"cherry batch-prefetches blobs in partial clone\"\n> + - Creates server with 3 upstream + 3 head-side commits modifying\n> + the same file (guarantees collisions)\n> + - Clones with --filter=blob:none\n> + - Runs `git cherry` with GIT_TRACE2_PERF\n> + - Asserts exactly 1 fetch (batch) instead of 6 (individual)\n> +\n> +Test 6: \"cherry prefetch omits blobs for cherry-picked commits\"\n> + - Creates a cherry-pick scenario (divergent branches, shared\n> + commit cherry-picked to head side)\n> + - Verifies `git cherry` correctly identifies the cherry-picked\n> + commit as \"-\" and head-only commits as \"+\"\n> + - Important: the head side must diverge before the cherry-pick\n> + so the cherry-pick creates a distinct commit object (otherwise\n> + the commit hash is identical and it's in the symmetric\n> + difference, not needing patch-ID comparison at all)\n> diff --git a/t/t3500-cherry.sh b/t/t3500-cherry.sh\n> index 78c3eac54b..17507d9a28 100755\n> --- a/t/t3500-cherry.sh\n> +++ b/t/t3500-cherry.sh\n> @@ -78,4 +78,22 @@ test_expect_success 'cherry ignores whitespace' '\n> \ttest_cmp expect actual\n> '\n> \n> +# Reuse the expect file from the previous test, in a partial clone\n> +test_expect_success 'cherry in partial clone does bulk prefetch' '\n> +\ttest_config uploadpack.allowfilter 1 &&\n> +\ttest_config uploadpack.allowanysha1inwant 1 &&\n> +\ttest_when_finished \"rm -rf copy\" &&\n> +\n> +\tgit clone --bare --filter=blob:none file://\"$(pwd)\" copy &&\n> +\t(\n> +\t\tcd copy &&\n> +\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace.output\" git cherry upstream-with-space feature-without-space >actual &&\n> +\t\ttest_cmp ../expect actual &&\n> +\n> +\t\tgrep \"child_start.*fetch.negotiationAlgorithm\" trace.output >fetches &&\n> +\t\ttest_line_count = 1 fetches &&\n> +\t\ttest_trace2_data promisor fetch_count 4 +\t)\n> +'\n> +\n> test_done\n"},{"id":"541842","messageId":"CABPp-BGWGBzc23garD_WyveGX+s+SGmNYfWzENnTx656w6raWQ@mail.gmail.com","threadId":"65502","inReplyTo":"xmqqbjfhw9fd.fsf@gitster.g","subject":"Re: [PATCH 2/3] builtin/log: prefetch necessary blobs for `git cherry`","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-04-17T22:02:32Z","receivedAt":"2026-04-17T22:02:45Z","isPatch":true,"body":"On Fri, Apr 17, 2026 at 2:42 PM Junio C Hamano wrote:\n>\n> > investigations/cherry-prefetch-design-spec.md | 210 ++++++++++++++++++\n>\n> Did you mean to add this file to the project? As a document to\n> describe how \"git cherry\" works, it is vastly lacking, and once this\n> series lands, I am not sure how others would benefit from being able\n> to read it. Many of the materials in there seem to typically be given\n> in the log message, but not to this degree of details, so I am not\n> sure where it belongs.\n\nUgh, no, sorry.\n"},{"id":"541846","messageId":"pull.2089.v2.git.1776472347.gitgitgadget@gmail.com","threadId":"65502","inReplyTo":"pull.2089.git.1776379694.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] Batch prefetching","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-04-18T00:32:24Z","receivedAt":"2026-04-18T00:32:31Z","isPatch":true,"body":"Changes since v1:\n\n * Remove stray file that should have never been added. So embarrassing that\n I didn't catch that before submitting.\n\nPartial clones provide a trade-off for users: avoid downloading blobs\nupfront, at the expense of needing to download them later as they run other\ncommands. This tradeoff can sometimes incur a more severe cost than\nexpected, particularly if needed blobs are discovered as they are accessed,\nresulting in downloading blobs one at a time. Some commands like checkout,\ndiff, and merge do batch prefetches of necessary blobs, since that can\ndramatically reduce the pain of on-demand loading. Extend this ability to\ntwo more commands: cherry and grep.\n\nThis series was spurred by a report where git cherry jobs were each doing\nhundreds of single-blob fetches, at a cost of 3s each. Batching those\ndownloads should dramatically speed up their jobs. (And I decided to fix up\ngit grep similarly while at it.)\n\nI'll also note that git backfill with revisions and/or pathspecs could also\nimprove things for these users, but since backfill is a manual command users\nwould have to run and requires users to try to figure out which data is\nneeded (a challenge in the case of cherry), it still makes sense to provide\nsmarter behavior for folks who don't choose to manually run backfill.\n\nAlso, correct a documentation typo I noticed in patch-ids.h (related to code\nI was using for the git cherry fixes) as a preparatory fixup.\n\nElijah Newren (3):\n patch-ids.h: add missing trailing parenthesis in documentation comment\n builtin/log: prefetch necessary blobs for `git cherry`\n grep: prefetch necessary blobs\n\n builtin/grep.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++\n builtin/log.c | 125 ++++++++++++++++++++++++++++++++++++++++\n patch-ids.h | 2 +-\n t/t3500-cherry.sh | 18 ++++++\n t/t7810-grep.sh | 35 ++++++++++++\n 5 files changed, 321 insertions(+), 1 deletion(-)\n\n\nbase-commit: 9f223ef1c026d91c7ac68cc0211bde255dda6199\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2089%2Fnewren%2Fbatch-prefetching-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2089/newren/batch-prefetching-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/2089\n\nRange-diff vs v1:\n\n 1: 7f5ac5942e = 1: 663816a344 patch-ids.h: add missing trailing parenthesis in documentation comment\n 2: 610be2a49a ! 2: a705852723 builtin/log: prefetch necessary blobs for `git cherry`\n @@ builtin/log.c: int cmd_cherry(int argc,\n \t\tchar sign = '+';\n \n \n - ## investigations/cherry-prefetch-design-spec.md (new) ##\n -@@\n -+# Design Spec: Batch Blob Prefetch for `git cherry` in Partial Clones\n -+\n -+## Problem\n -+\n -+In a partial clone with `--filter=blob:none`, `git cherry` compares\n -+commits using patch IDs. Patch IDs are computed in two phases:\n -+\n -+1. Header-only: hashes file paths and mode changes only (no blob reads)\n -+2. Full: hashes actual diff content (requires reading blobs)\n -+\n -+Phase 2 only runs when two commits have matching header-only IDs\n -+(i.e. they modify the same set of files with the same modes). This\n -+is common — any two commits touching the same file(s) will collide.\n -+\n -+When phase 2 needs a blob that isn't local, it triggers an on-demand\n -+promisor fetch. Each fetch is a separate network round-trip. With\n -+many collisions, this means many sequential fetches.\n -+\n -+## Solution Overview\n -+\n -+Add a preparatory pass before the existing comparison loop in\n -+`cmd_cherry()` that:\n -+\n -+1. Identifies which commit pairs will collide on header-only IDs\n -+2. Collects all blob OIDs those commits will need\n -+3. Batch-prefetches them in one fetch\n -+\n -+After this pass, the existing comparison loop runs as before, but\n -+all needed blobs are already local, so no on-demand fetches occur.\n -+\n -+## Detailed Design\n -+\n -+### 1. No struct changes to patch_id\n -+\n -+The existing `struct patch_id` and `patch_id_neq()` are not\n -+modified. `is_null_oid()` remains the sentinel for \"full ID not\n -+yet computed\". No `has_full_patch_id` boolean, no extra fields.\n -+\n -+Key insight: `init_patch_id_entry()` stores only `oidhash()` (the\n -+first 4 bytes of the header-only ID) in the hashmap bucket key.\n -+The real `patch_id_neq()` comparison function is invoked only when\n -+`hashmap_get()` or `hashmap_get_next()` finds entries with a\n -+matching oidhash — and that comparison triggers blob reads.\n -+\n -+The prefetch needs to detect exactly those oidhash collisions\n -+*without* triggering blob reads. We achieve this by temporarily\n -+swapping the hashmap's comparison function.\n -+\n -+### 2. The prefetch function (in builtin/log.c)\n -+\n -+This function takes the repository, the head-side commit list (as\n -+built by the existing revision walk in `cmd_cherry()`), and the\n -+patch_ids structure (which contains the upstream entries).\n -+\n -+#### 2.1 Early exit\n -+\n -+If the repository has no promisor remote, return immediately.\n -+Use `repo_has_promisor_remote()` from promisor-remote.h.\n -+\n -+#### 2.2 Swap in a trivial comparison function\n -+\n -+Save `ids->patches.cmpfn` (the real `patch_id_neq`) and replace\n -+it with a trivial function that always returns 0 (\"equal\").\n -+\n -+```\n -+static int patch_id_match(const void *unused_cmpfn_data,\n -+ const struct hashmap_entry *a,\n -+ const struct hashmap_entry *b,\n -+ const void *unused_keydata)\n -+{\n -+ return 0;\n -+}\n -+```\n -+\n -+With this cmpfn in place, `hashmap_get()` and `hashmap_get_next()`\n -+will match every entry in the same oidhash bucket — exactly the\n -+same set that would trigger `patch_id_neq()` during normal lookup.\n -+No blob reads occur because we never call the real comparison\n -+function.\n -+\n -+#### 2.3 For each head-side commit, probe for collisions\n -+\n -+For each commit in the head-side list:\n -+\n -+- Use `patch_id_iter_first(commit, ids)` to probe the upstream\n -+ hashmap. This handles `init_patch_id_entry()` + hashmap lookup\n -+ internally. With our swapped cmpfn, it returns any upstream\n -+ entry whose oidhash matches — i.e. any entry that *would*\n -+ trigger `patch_id_neq()` during the real comparison loop.\n -+ (Merge commits are already handled — `patch_id_iter_first()`\n -+ returns NULL for them via `patch_id_defined()`.)\n -+- If there's a match: collect blob OIDs from the head-side commit\n -+ (see section 3).\n -+- Then walk `patch_id_iter_next()` to find ALL upstream entries\n -+ in the same bucket. For each, collect blob OIDs from that\n -+ upstream commit too. (Multiple upstream commits can share the\n -+ same oidhash bucket.)\n -+- Collect blob OIDs from the first upstream match too (from\n -+ `patch_id_iter_first()`).\n -+\n -+We need blobs from BOTH sides because `patch_id_neq()` computes\n -+full patch IDs for both the upstream and head-side commit when\n -+comparing.\n -+\n -+#### 2.4 Restore the original comparison function\n -+\n -+Set `ids->patches.cmpfn` back to the saved value (patch_id_neq).\n -+This MUST happen before returning — the subsequent\n -+`has_commit_patch_id()` loop needs the real comparison function.\n -+\n -+#### 2.5 Batch prefetch\n -+\n -+If the oidset is non-empty, populate an oid_array from it using\n -+`oidset_iter_first()`/`oidset_iter_next()`, then call\n -+`promisor_remote_get_direct(repo, oid_array.oid, oid_array.nr)`.\n -+\n -+This is a single network round-trip regardless of how many blobs.\n -+\n -+#### 2.6 Cleanup\n -+\n -+Free the oid_array and the oidset.\n -+\n -+### 3. Collecting blob OIDs from a commit (helper function)\n -+\n -+Given a commit, enumerate the blobs its diff touches. Takes an\n -+oidset to insert into (provides automatic dedup — consecutive\n -+commits often share blob OIDs, e.g. B:foo == C^:foo when C's\n -+parent is B).\n -+\n -+- Compute the diff: `diff_tree_oid()` for commits with a parent,\n -+ `diff_root_tree_oid()` for root commits. Then `diffcore_std()`.\n -+- These populate the global `diff_queued_diff` queue.\n -+- For each filepair in the queue:\n -+ - Check the userdiff driver for the file path. If the driver\n -+ explicitly declares the file as binary (`drv->binary != -1`),\n -+ skip it. Reason: patch-ID uses `oid_to_hex()` for binary\n -+ files (see diff.c around line 6652) and never reads the blob.\n -+ Use `userdiff_find_by_path()` (NOT `diff_filespec_load_driver`\n -+ which is static in diff.c).\n -+ - For both sides of the filepair (p->one and p->two): if the\n -+ side is valid (`DIFF_FILE_VALID`) and has a non-null OID,\n -+ check the dedup oidset — `oidset_insert()` handles dedup\n -+ automatically (returns 1 if newly inserted, 0 if duplicate).\n -+- Clear the diff queue with `diff_queue_clear()` (from diffcore.h,\n -+ not diff.h).\n -+\n -+Note on `drv->binary`: The value -1 means \"not set\" (auto-detect\n -+at read time by reading the blob); 0 means explicitly text (will\n -+be diffed, blob reads needed); positive means explicitly binary\n -+(patch-ID uses `oid_to_hex()`, no blob read needed).\n -+\n -+The correct skip condition is `drv && drv->binary > 0` — skip\n -+only known-binary files. Do NOT use `drv->binary != -1`, which\n -+would also skip explicitly-text files that DO need blob reads.\n -+(The copilot reference implementation uses `!= -1`, which is\n -+technically wrong but harmless in practice since explicit text\n -+attributes are rare.)\n -+\n -+### 4. Call site in cmd_cherry()\n -+\n -+Insert the call between the revision walk loop (which builds the\n -+head-side commit list) and the comparison loop (which calls\n -+`has_commit_patch_id()`).\n -+\n -+### 5. Required includes in builtin/log.c\n -+\n -+- promisor-remote.h (for repo_has_promisor_remote,\n -+ promisor_remote_get_direct)\n -+- userdiff.h (for userdiff_find_by_path)\n -+- oidset.h (for oidset used in blob OID dedup)\n -+- diffcore.h (for diff_queue_clear)\n -+\n -+## Edge Cases\n -+\n -+- No promisor remote: early return, zero overhead\n -+- No collisions: probes the hashmap for each head-side commit but\n -+ finds no bucket matches, no blobs collected, no fetch issued\n -+- Merge commits in head-side list: skipped (no patch ID defined)\n -+- Root commits (no parent): use diff_root_tree_oid instead of\n -+ diff_tree_oid\n -+- Binary files (explicit driver): skipped, patch-ID doesn't read\n -+ them\n -+- The cmpfn swap approach matches at oidhash granularity (4 bytes),\n -+ which is exactly what the hashmap itself uses to trigger\n -+ patch_id_neq(). This means we prefetch for every case the real\n -+ code would trigger, plus rare false-positive oidhash collisions\n -+ (harmless: we fetch a few extra blobs that won't end up being\n -+ compared). No under-fetching is possible.\n -+\n -+## Testing\n -+\n -+See t/t3500-cherry.sh on the copilot-faster-partial-clones branch\n -+for two tests:\n -+\n -+Test 5: \"cherry batch-prefetches blobs in partial clone\"\n -+ - Creates server with 3 upstream + 3 head-side commits modifying\n -+ the same file (guarantees collisions)\n -+ - Clones with --filter=blob:none\n -+ - Runs `git cherry` with GIT_TRACE2_PERF\n -+ - Asserts exactly 1 fetch (batch) instead of 6 (individual)\n -+\n -+Test 6: \"cherry prefetch omits blobs for cherry-picked commits\"\n -+ - Creates a cherry-pick scenario (divergent branches, shared\n -+ commit cherry-picked to head side)\n -+ - Verifies `git cherry` correctly identifies the cherry-picked\n -+ commit as \"-\" and head-only commits as \"+\"\n -+ - Important: the head side must diverge before the cherry-pick\n -+ so the cherry-pick creates a distinct commit object (otherwise\n -+ the commit hash is identical and it's in the symmetric\n -+ difference, not needing patch-ID comparison at all)\n -\n ## t/t3500-cherry.sh ##\n @@ t/t3500-cherry.sh: test_expect_success 'cherry ignores whitespace' '\n \ttest_cmp expect actual\n 3: 6dbfc7608b = 3: 8fbfe69bc4 grep: prefetch necessary blobs\n\n-- \ngitgitgadget\n"},{"id":"541847","messageId":"663816a34496e4d6bb43b815e8a59bf1934efe62.1776472347.git.gitgitgadget@gmail.com","threadId":"65502","inReplyTo":"pull.2089.v2.git.1776472347.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] patch-ids.h: add missing trailing parenthesis in documentation comment","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-04-18T00:32:25Z","receivedAt":"2026-04-18T00:32:33Z","isPatch":true,"body":"From: Elijah Newren \n\nSigned-off-by: Elijah Newren \n---\n patch-ids.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/patch-ids.h b/patch-ids.h\nindex 490d739371..57534ee722 100644\n--- a/patch-ids.h\n+++ b/patch-ids.h\n@@ -37,7 +37,7 @@ int has_commit_patch_id(struct commit *commit, struct patch_ids *);\n * struct patch_id *cur;\n * for (cur = patch_id_iter_first(commit, ids);\n * cur;\n- * cur = patch_id_iter_next(cur, ids) {\n+ * cur = patch_id_iter_next(cur, ids)) {\n * ... look at cur->commit\n * }\n */\n-- \ngitgitgadget\n\n"},{"id":"541848","messageId":"a705852723fbe88e94ad3de1daba548dbce32211.1776472347.git.gitgitgadget@gmail.com","threadId":"65502","inReplyTo":"pull.2089.v2.git.1776472347.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] builtin/log: prefetch necessary blobs for `git cherry`","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-04-18T00:32:26Z","receivedAt":"2026-04-18T00:32:36Z","isPatch":true,"body":"From: Elijah Newren \n\nIn partial clones, `git cherry` fetches necessary blobs on-demand one\nat a time, which can be very slow. We would like to prefetch all\nnecessary blobs upfront. To do so, we need to be able to first figure\nout which blobs are needed.\n\n`git cherry` does its work in a two-phase approach: first computing\nheader-only IDs (based on file paths and modes), then falling back to\nfull content-based IDs only when header-only IDs collide -- or, more\naccurately, whenever the oidhash() of the header-only object_ids\ncollide.\n\npatch-ids.c handles this by creating an ids->patches hashmap that has\nall the data we need, but the problem is that any attempt to query the\nhashmap will invoke the patch_id_neq() function on any colliding objects,\nwhich causes the on-demand fetching.\n\nInsert a new prefetch_cherry_blobs() function before checking for\ncollisions. Use a temporary replacement on the ids->patches.cmpfn\nin order to enumerate the blobs that would be needed without yet\nfetching them, and then fetch them all at once, then restore the old\nids->patches.cmpfn.\n\nSigned-off-by: Elijah Newren \n---\n builtin/log.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++\n t/t3500-cherry.sh | 18 +++++++\n 2 files changed, 143 insertions(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 8c0939dd42..df19876be6 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -21,10 +21,12 @@\n #include \"color.h\"\n #include \"commit.h\"\n #include \"diff.h\"\n+#include \"diffcore.h\"\n #include \"diff-merges.h\"\n #include \"revision.h\"\n #include \"log-tree.h\"\n #include \"oid-array.h\"\n+#include \"oidset.h\"\n #include \"tag.h\"\n #include \"reflog-walk.h\"\n #include \"patch-ids.h\"\n@@ -43,9 +45,11 @@\n #include \"utf8.h\"\n \n #include \"commit-reach.h\"\n+#include \"promisor-remote.h\"\n #include \"range-diff.h\"\n #include \"tmp-objdir.h\"\n #include \"tree.h\"\n+#include \"userdiff.h\"\n #include \"write-or-die.h\"\n \n #define MAIL_DEFAULT_WRAP 72\n@@ -2602,6 +2606,125 @@ static void print_commit(char sign, struct commit *commit, int verbose,\n \t}\n }\n \n+/*\n+ * Enumerate blob OIDs from a single commit's diff, inserting them into blobs.\n+ * Skips files whose userdiff driver explicitly declares binary status\n+ * (drv->binary > 0), since patch-ID uses oid_to_hex() for those and\n+ * never reads blob content. Use userdiff_find_by_path() since\n+ * diff_filespec_load_driver() is static in diff.c.\n+ *\n+ * Clean up with diff_queue_clear() (from diffcore.h).\n+ */\n+static void collect_diff_blob_oids(struct commit *commit,\n+\t\t\t\t struct diff_options *opts,\n+\t\t\t\t struct oidset *blobs)\n+{\n+\tstruct diff_queue_struct *q;\n+\n+\t/*\n+\t * Merge commits are filtered out by patch_id_defined() in patch-ids.c,\n+\t * so we'll never be called with one.\n+\t */\n+\tassert(!commit->parents || !commit->parents->next);\n+\n+\tif (commit->parents)\n+\t\tdiff_tree_oid(&commit->parents->item->object.oid,\n+\t\t\t &commit->object.oid, \"\", opts);\n+\telse\n+\t\tdiff_root_tree_oid(&commit->object.oid, \"\", opts);\n+\tdiffcore_std(opts);\n+\n+\tq = &diff_queued_diff;\n+\tfor (int i = 0; i < q->nr; i++) {\n+\t\tstruct diff_filepair *p = q->queue[i];\n+\t\tstruct userdiff_driver *drv;\n+\n+\t\t/* Skip binary files */\n+\t\tdrv = userdiff_find_by_path(opts->repo->index, p->one->path);\n+\t\tif (drv && drv->binary > 0)\n+\t\t\tcontinue;\n+\n+\t\tif (DIFF_FILE_VALID(p->one))\n+\t\t\toidset_insert(blobs, &p->one->oid);\n+\t\tif (DIFF_FILE_VALID(p->two))\n+\t\t\toidset_insert(blobs, &p->two->oid);\n+\t}\n+\tdiff_queue_clear(q);\n+}\n+\n+static int always_match(const void *cmp_data UNUSED,\n+\t\t\tconst struct hashmap_entry *entry1 UNUSED,\n+\t\t\tconst struct hashmap_entry *entry2 UNUSED,\n+\t\t\tconst void *keydata UNUSED)\n+{\n+\treturn 0;\n+}\n+\n+/*\n+ * Prefetch blobs for git cherry in partial clones.\n+ *\n+ * Called between the revision walk (which builds the head-side\n+ * commit list) and the has_commit_patch_id() comparison loop.\n+ *\n+ * Uses a cmpfn-swap trick to avoid reading blobs: temporarily\n+ * replaces the hashmap's comparison function with a trivial\n+ * always-match function, so hashmap_get()/hashmap_get_next() match\n+ * any entry with the same oidhash bucket. These are the set of oids\n+ * that would trigger patch_id_neq() during normal lookup and cause\n+ * blobs to be read on demand, and we want to prefetch them all at\n+ * once instead.\n+ */\n+static void prefetch_cherry_blobs(struct repository *repo,\n+\t\t\t\t struct commit_list *list,\n+\t\t\t\t struct patch_ids *ids)\n+{\n+\tstruct oidset blobs = OIDSET_INIT;\n+\thashmap_cmp_fn original_cmpfn;\n+\n+\t/* Exit if we're not in a partial clone */\n+\tif (!repo_has_promisor_remote(repo))\n+\t\treturn;\n+\n+\t/* Save original cmpfn, replace with always_match */\n+\toriginal_cmpfn = ids->patches.cmpfn;\n+\tids->patches.cmpfn = always_match;\n+\n+\t/* Find header-only collisions, gather blobs from those commits */\n+\tfor (struct commit_list *l = list; l; l = l->next) {\n+\t\tstruct commit *c = l->item;\n+\t\tbool match_found = false;\n+\t\tfor (struct patch_id *cur = patch_id_iter_first(c, ids);\n+\t\t cur;\n+\t\t cur = patch_id_iter_next(cur, ids)) {\n+\t\t\tmatch_found = true;\n+\t\t\tcollect_diff_blob_oids(cur->commit, &ids->diffopts,\n+\t\t\t\t\t &blobs);\n+\t\t}\n+\t\tif (match_found)\n+\t\t\tcollect_diff_blob_oids(c, &ids->diffopts, &blobs);\n+\t}\n+\n+\t/* Restore original cmpfn */\n+\tids->patches.cmpfn = original_cmpfn;\n+\n+\t/* If we have any blobs to fetch, fetch them */\n+\tif (oidset_size(&blobs)) {\n+\t\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n+\t\tstruct oidset_iter iter;\n+\t\tconst struct object_id *oid;\n+\n+\t\toidset_iter_init(&blobs, &iter);\n+\t\twhile ((oid = oidset_iter_next(&iter)))\n+\t\t\toid_array_append(&to_fetch, oid);\n+\n+\t\tpromisor_remote_get_direct(repo, to_fetch.oid, to_fetch.nr);\n+\n+\t\toid_array_clear(&to_fetch);\n+\t}\n+\n+\toidset_clear(&blobs);\n+}\n+\n int cmd_cherry(int argc,\n \t const char **argv,\n \t const char *prefix,\n@@ -2673,6 +2796,8 @@ int cmd_cherry(int argc,\n \t\tcommit_list_insert(commit, &list);\n \t}\n \n+\tprefetch_cherry_blobs(the_repository, list, &ids);\n+\n \tfor (struct commit_list *l = list; l; l = l->next) {\n \t\tchar sign = '+';\n \ndiff --git a/t/t3500-cherry.sh b/t/t3500-cherry.sh\nindex 78c3eac54b..17507d9a28 100755\n--- a/t/t3500-cherry.sh\n+++ b/t/t3500-cherry.sh\n@@ -78,4 +78,22 @@ test_expect_success 'cherry ignores whitespace' '\n \ttest_cmp expect actual\n '\n \n+# Reuse the expect file from the previous test, in a partial clone\n+test_expect_success 'cherry in partial clone does bulk prefetch' '\n+\ttest_config uploadpack.allowfilter 1 &&\n+\ttest_config uploadpack.allowanysha1inwant 1 &&\n+\ttest_when_finished \"rm -rf copy\" &&\n+\n+\tgit clone --bare --filter=blob:none file://\"$(pwd)\" copy &&\n+\t(\n+\t\tcd copy &&\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace.output\" git cherry upstream-with-space feature-without-space >actual &&\n+\t\ttest_cmp ../expect actual &&\n+\n+\t\tgrep \"child_start.*fetch.negotiationAlgorithm\" trace.output >fetches &&\n+\t\ttest_line_count = 1 fetches &&\n+\t\ttest_trace2_data promisor fetch_count 4 \n\nIn partial clones, `git grep` fetches necessary blobs on-demand one\nat a time, which can be very slow. In partial clones, add an extra\npreliminary walk over the tree similar to grep_tree() which collects\nthe blobs of interest, and then prefetches them.\n\nSigned-off-by: Elijah Newren \n---\n builtin/grep.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++++\n t/t7810-grep.sh | 35 ++++++++++++\n 2 files changed, 177 insertions(+)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex e33285e5e6..d559c48d94 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -28,9 +28,12 @@\n #include \"object-file.h\"\n #include \"object-name.h\"\n #include \"odb.h\"\n+#include \"oid-array.h\"\n+#include \"oidset.h\"\n #include \"packfile.h\"\n #include \"pager.h\"\n #include \"path.h\"\n+#include \"promisor-remote.h\"\n #include \"read-cache-ll.h\"\n #include \"write-or-die.h\"\n \n@@ -692,6 +695,143 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \treturn hit;\n }\n \n+static void collect_blob_oids_for_tree(struct repository *repo,\n+\t\t\t\t const struct pathspec *pathspec,\n+\t\t\t\t struct tree_desc *tree,\n+\t\t\t\t struct strbuf *base,\n+\t\t\t\t int tn_len,\n+\t\t\t\t struct oidset *blob_oids)\n+{\n+\tstruct name_entry entry;\n+\tint old_baselen = base->len;\n+\tstruct strbuf name = STRBUF_INIT;\n+\tenum interesting match = entry_not_interesting;\n+\n+\twhile (tree_entry(tree, &entry)) {\n+\t\tif (match != all_entries_interesting) {\n+\t\t\tstrbuf_addstr(&name, base->buf + tn_len);\n+\t\t\tmatch = tree_entry_interesting(repo->index,\n+\t\t\t\t\t\t &entry, &name,\n+\t\t\t\t\t\t pathspec);\n+\t\t\tstrbuf_reset(&name);\n+\n+\t\t\tif (match == all_entries_not_interesting)\n+\t\t\t\tbreak;\n+\t\t\tif (match == entry_not_interesting)\n+\t\t\t\tcontinue;\n+\t\t}\n+\n+\t\tstrbuf_add(base, entry.path, tree_entry_len(&entry));\n+\n+\t\tif (S_ISREG(entry.mode)) {\n+\t\t\toidset_insert(blob_oids, &entry.oid);\n+\t\t} else if (S_ISDIR(entry.mode)) {\n+\t\t\tenum object_type type;\n+\t\t\tstruct tree_desc sub_tree;\n+\t\t\tvoid *data;\n+\t\t\tunsigned long size;\n+\n+\t\t\tdata = odb_read_object(repo->objects, &entry.oid,\n+\t\t\t\t\t &type, &size);\n+\t\t\tif (!data)\n+\t\t\t\tdie(_(\"unable to read tree (%s)\"),\n+\t\t\t\t oid_to_hex(&entry.oid));\n+\n+\t\t\tstrbuf_addch(base, '/');\n+\t\t\tinit_tree_desc(&sub_tree, &entry.oid, data, size);\n+\t\t\tcollect_blob_oids_for_tree(repo, pathspec, &sub_tree,\n+\t\t\t\t\t\t base, tn_len, blob_oids);\n+\t\t\tfree(data);\n+\t\t}\n+\t\t/*\n+\t\t * ...no else clause for S_ISGITLINK: submodules have their\n+\t\t * own promisor configuration and would need separate fetches\n+\t\t * anyway.\n+\t\t */\n+\n+\t\tstrbuf_setlen(base, old_baselen);\n+\t}\n+\n+\tstrbuf_release(&name);\n+}\n+\n+static void collect_blob_oids_for_treeish(struct grep_opt *opt,\n+\t\t\t\t\t const struct pathspec *pathspec,\n+\t\t\t\t\t const struct object_id *tree_ish_oid,\n+\t\t\t\t\t const char *name,\n+\t\t\t\t\t struct oidset *blob_oids)\n+{\n+\tstruct tree_desc tree;\n+\tvoid *data;\n+\tunsigned long size;\n+\tstruct strbuf base = STRBUF_INIT;\n+\tint len;\n+\n+\tdata = odb_read_object_peeled(opt->repo->objects, tree_ish_oid,\n+\t\t\t\t OBJ_TREE, &size, NULL);\n+\n+\tif (!data)\n+\t\treturn;\n+\n+\tlen = name ? strlen(name) : 0;\n+\tif (len) {\n+\t\tstrbuf_add(&base, name, len);\n+\t\tstrbuf_addch(&base, ':');\n+\t}\n+\tinit_tree_desc(&tree, tree_ish_oid, data, size);\n+\n+\tcollect_blob_oids_for_tree(opt->repo, pathspec, &tree,\n+\t\t\t\t &base, base.len, blob_oids);\n+\n+\tstrbuf_release(&base);\n+\tfree(data);\n+}\n+\n+static void prefetch_grep_blobs(struct grep_opt *opt,\n+\t\t\t\tconst struct pathspec *pathspec,\n+\t\t\t\tconst struct object_array *list)\n+{\n+\tstruct oidset blob_oids = OIDSET_INIT;\n+\n+\t/* Exit if we're not in a partial clone */\n+\tif (!repo_has_promisor_remote(opt->repo))\n+\t\treturn;\n+\n+\t/* For each tree, gather the blobs in it */\n+\tfor (int i = 0; i < list->nr; i++) {\n+\t\tstruct object *real_obj;\n+\n+\t\tobj_read_lock();\n+\t\treal_obj = deref_tag(opt->repo, list->objects[i].item,\n+\t\t\t\t NULL, 0);\n+\t\tobj_read_unlock();\n+\n+\t\tif (real_obj &&\n+\t\t (real_obj->type == OBJ_COMMIT ||\n+\t\t real_obj->type == OBJ_TREE))\n+\t\t\tcollect_blob_oids_for_treeish(opt, pathspec,\n+\t\t\t\t\t\t &real_obj->oid,\n+\t\t\t\t\t\t list->objects[i].name,\n+\t\t\t\t\t\t &blob_oids);\n+\t}\n+\n+\t/* Prefetch the blobs we found */\n+\tif (oidset_size(&blob_oids)) {\n+\t\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n+\t\tstruct oidset_iter iter;\n+\t\tconst struct object_id *oid;\n+\n+\t\toidset_iter_init(&blob_oids, &iter);\n+\t\twhile ((oid = oidset_iter_next(&iter)))\n+\t\t\toid_array_append(&to_fetch, oid);\n+\n+\t\tpromisor_remote_get_direct(opt->repo, to_fetch.oid, to_fetch.nr);\n+\n+\t\toid_array_clear(&to_fetch);\n+\t}\n+\toidset_clear(&blob_oids);\n+}\n+\n static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t struct object *obj, const char *name, const char *path)\n {\n@@ -732,6 +872,8 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,\n \tint hit = 0;\n \tconst unsigned int nr = list->nr;\n \n+\tprefetch_grep_blobs(opt, pathspec, list);\n+\n \tfor (i = 0; i < nr; i++) {\n \t\tstruct object *real_obj;\n \ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 64ac4f04ee..1f484502fe 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -1929,4 +1929,39 @@ test_expect_success 'grep does not report i-t-a and assume unchanged with -L' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'grep of revision in partial clone does bulk prefetch' '\n+\ttest_when_finished \"rm -rf grep-partial-src grep-partial\" &&\n+\n+\tgit init grep-partial-src &&\n+\t(\n+\t\tcd grep-partial-src &&\n+\t\tgit config uploadpack.allowfilter 1 &&\n+\t\tgit config uploadpack.allowanysha1inwant 1 &&\n+\t\techo \"needle in haystack\" >searchme &&\n+\t\techo \"no match here\" >other &&\n+\t\tmkdir subdir &&\n+\t\techo \"needle again\" >subdir/deep &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"initial\"\n+\t) &&\n+\n+\tgit clone --no-checkout --filter=blob:none \\\n+\t\t\"file://$(pwd)/grep-partial-src\" grep-partial &&\n+\n+\t# All blobs should be missing after a blobless clone.\n+\tgit -C grep-partial rev-list --quiet --objects \\\n+\t\t--missing=print HEAD >missing &&\n+\ttest_line_count = 3 missing &&\n+\n+\t# grep HEAD should batch-prefetch all blobs in one request.\n+\tGIT_TRACE2_EVENT=\"$(pwd)/grep-trace\" \\\n+\t\tgit -C grep-partial grep -c \"needle\" HEAD >result &&\n+\n+\t# Should find matches in two files.\n+\ttest_line_count = 2 result &&\n+\n+\t# Should have prefetched all 3 objects at once\n+\ttest_trace2_data promisor fetch_count 3 From: Elijah Newren \n> \n> In partial clones, `git cherry` fetches necessary blobs on-demand one\n> at a time, which can be very slow. We would like to prefetch all\n> necessary blobs upfront. To do so, we need to be able to first figure\n> out which blobs are needed.\n\n\"git rebase\" without \"--reapply-cherry-picks\" suffers from this problem \nas well as it does the equivalent of \"git log --cherry-pick\". Is there \nany way to share prefetch_cherry_blobs() with the cherry-pick detection \nin revision.c?\n\nThanks\n\nPhillip\n\n> `git cherry` does its work in a two-phase approach: first computing\n> header-only IDs (based on file paths and modes), then falling back to\n> full content-based IDs only when header-only IDs collide -- or, more\n> accurately, whenever the oidhash() of the header-only object_ids\n> collide.\n> \n> patch-ids.c handles this by creating an ids->patches hashmap that has\n> all the data we need, but the problem is that any attempt to query the\n> hashmap will invoke the patch_id_neq() function on any colliding objects,\n> which causes the on-demand fetching.\n> \n> Insert a new prefetch_cherry_blobs() function before checking for\n> collisions. Use a temporary replacement on the ids->patches.cmpfn\n> in order to enumerate the blobs that would be needed without yet\n> fetching them, and then fetch them all at once, then restore the old\n> ids->patches.cmpfn.\n> \n> Signed-off-by: Elijah Newren \n> ---\n> builtin/log.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++\n> t/t3500-cherry.sh | 18 +++++++\n> 2 files changed, 143 insertions(+)\n> \n> diff --git a/builtin/log.c b/builtin/log.c\n> index 8c0939dd42..df19876be6 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -21,10 +21,12 @@\n> #include \"color.h\"\n> #include \"commit.h\"\n> #include \"diff.h\"\n> +#include \"diffcore.h\"\n> #include \"diff-merges.h\"\n> #include \"revision.h\"\n> #include \"log-tree.h\"\n> #include \"oid-array.h\"\n> +#include \"oidset.h\"\n> #include \"tag.h\"\n> #include \"reflog-walk.h\"\n> #include \"patch-ids.h\"\n> @@ -43,9 +45,11 @@\n> #include \"utf8.h\"\n> \n> #include \"commit-reach.h\"\n> +#include \"promisor-remote.h\"\n> #include \"range-diff.h\"\n> #include \"tmp-objdir.h\"\n> #include \"tree.h\"\n> +#include \"userdiff.h\"\n> #include \"write-or-die.h\"\n> \n> #define MAIL_DEFAULT_WRAP 72\n> @@ -2602,6 +2606,125 @@ static void print_commit(char sign, struct commit *commit, int verbose,\n> \t}\n> }\n> \n> +/*\n> + * Enumerate blob OIDs from a single commit's diff, inserting them into blobs.\n> + * Skips files whose userdiff driver explicitly declares binary status\n> + * (drv->binary > 0), since patch-ID uses oid_to_hex() for those and\n> + * never reads blob content. Use userdiff_find_by_path() since\n> + * diff_filespec_load_driver() is static in diff.c.\n> + *\n> + * Clean up with diff_queue_clear() (from diffcore.h).\n> + */\n> +static void collect_diff_blob_oids(struct commit *commit,\n> +\t\t\t\t struct diff_options *opts,\n> +\t\t\t\t struct oidset *blobs)\n> +{\n> +\tstruct diff_queue_struct *q;\n> +\n> +\t/*\n> +\t * Merge commits are filtered out by patch_id_defined() in patch-ids.c,\n> +\t * so we'll never be called with one.\n> +\t */\n> +\tassert(!commit->parents || !commit->parents->next);\n> +\n> +\tif (commit->parents)\n> +\t\tdiff_tree_oid(&commit->parents->item->object.oid,\n> +\t\t\t &commit->object.oid, \"\", opts);\n> +\telse\n> +\t\tdiff_root_tree_oid(&commit->object.oid, \"\", opts);\n> +\tdiffcore_std(opts);\n> +\n> +\tq = &diff_queued_diff;\n> +\tfor (int i = 0; i < q->nr; i++) {\n> +\t\tstruct diff_filepair *p = q->queue[i];\n> +\t\tstruct userdiff_driver *drv;\n> +\n> +\t\t/* Skip binary files */\n> +\t\tdrv = userdiff_find_by_path(opts->repo->index, p->one->path);\n> +\t\tif (drv && drv->binary > 0)\n> +\t\t\tcontinue;\n> +\n> +\t\tif (DIFF_FILE_VALID(p->one))\n> +\t\t\toidset_insert(blobs, &p->one->oid);\n> +\t\tif (DIFF_FILE_VALID(p->two))\n> +\t\t\toidset_insert(blobs, &p->two->oid);\n> +\t}\n> +\tdiff_queue_clear(q);\n> +}\n> +\n> +static int always_match(const void *cmp_data UNUSED,\n> +\t\t\tconst struct hashmap_entry *entry1 UNUSED,\n> +\t\t\tconst struct hashmap_entry *entry2 UNUSED,\n> +\t\t\tconst void *keydata UNUSED)\n> +{\n> +\treturn 0;\n> +}\n> +\n> +/*\n> + * Prefetch blobs for git cherry in partial clones.\n> + *\n> + * Called between the revision walk (which builds the head-side\n> + * commit list) and the has_commit_patch_id() comparison loop.\n> + *\n> + * Uses a cmpfn-swap trick to avoid reading blobs: temporarily\n> + * replaces the hashmap's comparison function with a trivial\n> + * always-match function, so hashmap_get()/hashmap_get_next() match\n> + * any entry with the same oidhash bucket. These are the set of oids\n> + * that would trigger patch_id_neq() during normal lookup and cause\n> + * blobs to be read on demand, and we want to prefetch them all at\n> + * once instead.\n> + */\n> +static void prefetch_cherry_blobs(struct repository *repo,\n> +\t\t\t\t struct commit_list *list,\n> +\t\t\t\t struct patch_ids *ids)\n> +{\n> +\tstruct oidset blobs = OIDSET_INIT;\n> +\thashmap_cmp_fn original_cmpfn;\n> +\n> +\t/* Exit if we're not in a partial clone */\n> +\tif (!repo_has_promisor_remote(repo))\n> +\t\treturn;\n> +\n> +\t/* Save original cmpfn, replace with always_match */\n> +\toriginal_cmpfn = ids->patches.cmpfn;\n> +\tids->patches.cmpfn = always_match;\n> +\n> +\t/* Find header-only collisions, gather blobs from those commits */\n> +\tfor (struct commit_list *l = list; l; l = l->next) {\n> +\t\tstruct commit *c = l->item;\n> +\t\tbool match_found = false;\n> +\t\tfor (struct patch_id *cur = patch_id_iter_first(c, ids);\n> +\t\t cur;\n> +\t\t cur = patch_id_iter_next(cur, ids)) {\n> +\t\t\tmatch_found = true;\n> +\t\t\tcollect_diff_blob_oids(cur->commit, &ids->diffopts,\n> +\t\t\t\t\t &blobs);\n> +\t\t}\n> +\t\tif (match_found)\n> +\t\t\tcollect_diff_blob_oids(c, &ids->diffopts, &blobs);\n> +\t}\n> +\n> +\t/* Restore original cmpfn */\n> +\tids->patches.cmpfn = original_cmpfn;\n> +\n> +\t/* If we have any blobs to fetch, fetch them */\n> +\tif (oidset_size(&blobs)) {\n> +\t\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n> +\t\tstruct oidset_iter iter;\n> +\t\tconst struct object_id *oid;\n> +\n> +\t\toidset_iter_init(&blobs, &iter);\n> +\t\twhile ((oid = oidset_iter_next(&iter)))\n> +\t\t\toid_array_append(&to_fetch, oid);\n> +\n> +\t\tpromisor_remote_get_direct(repo, to_fetch.oid, to_fetch.nr);\n> +\n> +\t\toid_array_clear(&to_fetch);\n> +\t}\n> +\n> +\toidset_clear(&blobs);\n> +}\n> +\n> int cmd_cherry(int argc,\n> \t const char **argv,\n> \t const char *prefix,\n> @@ -2673,6 +2796,8 @@ int cmd_cherry(int argc,\n> \t\tcommit_list_insert(commit, &list);\n> \t}\n> \n> +\tprefetch_cherry_blobs(the_repository, list, &ids);\n> +\n> \tfor (struct commit_list *l = list; l; l = l->next) {\n> \t\tchar sign = '+';\n> \n> diff --git a/t/t3500-cherry.sh b/t/t3500-cherry.sh\n> index 78c3eac54b..17507d9a28 100755\n> --- a/t/t3500-cherry.sh\n> +++ b/t/t3500-cherry.sh\n> @@ -78,4 +78,22 @@ test_expect_success 'cherry ignores whitespace' '\n> \ttest_cmp expect actual\n> '\n> \n> +# Reuse the expect file from the previous test, in a partial clone\n> +test_expect_success 'cherry in partial clone does bulk prefetch' '\n> +\ttest_config uploadpack.allowfilter 1 &&\n> +\ttest_config uploadpack.allowanysha1inwant 1 &&\n> +\ttest_when_finished \"rm -rf copy\" &&\n> +\n> +\tgit clone --bare --filter=blob:none file://\"$(pwd)\" copy &&\n> +\t(\n> +\t\tcd copy &&\n> +\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace.output\" git cherry upstream-with-space feature-without-space >actual &&\n> +\t\ttest_cmp ../expect actual &&\n> +\n> +\t\tgrep \"child_start.*fetch.negotiationAlgorithm\" trace.output >fetches &&\n> +\t\ttest_line_count = 1 fetches &&\n> +\t\ttest_trace2_data promisor fetch_count 4 +\t)\n> +'\n> +\n> test_done\n\n"},{"id":"542086","messageId":"CABPp-BF4woakYQ5RZ32J8SzDs_VpvT2Wv+Y2WaHTnFnM=96Kzg@mail.gmail.com","threadId":"65502","inReplyTo":"a010a4ad-403a-4b6f-9a92-a33323eca0f2@gmail.com","subject":"Re: [PATCH v2 2/3] builtin/log: prefetch necessary blobs for `git cherry`","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-04-21T21:28:29Z","receivedAt":"2026-04-21T21:28:42Z","isPatch":true,"body":"Hi Phillip,\n\nOn Sun, Apr 19, 2026 at 7:04 AM Phillip Wood wrote:\n>\n> Hi Elijah\n>\n> On 18/04/2026 01:32, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n> >\n> > In partial clones, `git cherry` fetches necessary blobs on-demand one\n> > at a time, which can be very slow. We would like to prefetch all\n> > necessary blobs upfront. To do so, we need to be able to first figure\n> > out which blobs are needed.\n>\n> \"git rebase\" without \"--reapply-cherry-picks\" suffers from this problem\n> as well as it does the equivalent of \"git log --cherry-pick\". Is there\n> any way to share prefetch_cherry_blobs() with the cherry-pick detection\n> in revision.c?\n\nYes, you're right; git rebase without --reapply-cherry-picks and git\nlog --cherry-pick both go through cherry_pick_list() in revision.c,\nwhich has exactly the same shape as the patch-ids loop in\ncmd_cherry(): build a hashmap of one side via add_commit_patch_id(),\nthen look up the other side via patch_id_iter_first(). The on-demand\nblob fetches come from the same patch_id_neq() callback.\n\nAfter poking around, I think the approximate scope of the fix would\nbe: Move collect_diff_blob_oids(), always_match(), and\nprefetch_cherry_blobs() from builtin/log.c to patch-ids.c and expose\nthe last one in patch-ids.h. In cherry_pick_list(), between the\nadd_commit_patch_id loop and the comparison loop, build a temporary\nlist of just the lookup-side commits (filtering by\nSYMMETRIC_LEFT/BOUNDARY as the existing loop already does) and call\nprefetch_cherry_blobs() on it.\n\nThat said, I'd rather leave this out of the current series. The bigger\npicture is that I have reservations about expanding partial-clone\nsupport further into this area. git cherry, git log --cherry-pick, and\nthe default cherry-pick detection in git rebase all exist to answer\n\"has this patch already landed upstream?\" -- a question that, in\nrepositories large enough to need partial clones, I feel is rarely\nworth the cost of computing patch-ids across arbitrary amounts of\nhistory. The honest guidance I would probably give for users on a\nlarge repo is \"pass --reapply-cherry-picks (with rebase) and skip this\nentirely\" or to narrow the range under consideration. The omission of\na --no-reapply-cherry-picks option in git-replay wasn't a lack of\neffort or oversight, but a deliberate choice where I'd rather hold off\n(possibly indefinitely) on implementing it. So I'm a bit reluctant to\nmake the performance hazard less visible without also asking whether\nwe should even be doing that piece of the operation.\n\nI only implemented the git cherry fix because of a specific customer\nsituation where the operation was already baked into tooling, and\nprefetching at least makes the worst case tolerable. I don't want to\nhold myself to doing the same for the cherry_pick_list() path, but I'm\nfairly confident the code here can be re-used for those other cases\nand I'd help review a patch from anyone who wants to carry it forward.\n\nAnyway, you are making the right connection, it's just that my\npersonal answer is to let some other interested individual do it.\n"},{"id":"542202","messageId":"2abdc8ba-e361-492c-88b7-0c807ee9fb4d@gmail.com","threadId":"65502","inReplyTo":"CABPp-BF4woakYQ5RZ32J8SzDs_VpvT2Wv+Y2WaHTnFnM=96Kzg@mail.gmail.com","subject":"Re: [PATCH v2 2/3] builtin/log: prefetch necessary blobs for `git cherry`","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-04-23T15:15:51Z","receivedAt":"2026-04-23T15:15:56Z","isPatch":true,"body":"Hi Elijah\n\nOn 21/04/2026 22:28, Elijah Newren wrote:\n> On Sun, Apr 19, 2026 at 7:04 AM Phillip Wood wrote:\n>> On 18/04/2026 01:32, Elijah Newren via GitGitGadget wrote:\n>>> From: Elijah Newren \n>>>\n>>> In partial clones, `git cherry` fetches necessary blobs on-demand one\n>>> at a time, which can be very slow. We would like to prefetch all\n>>> necessary blobs upfront. To do so, we need to be able to first figure\n>>> out which blobs are needed.\n>>\n>> \"git rebase\" without \"--reapply-cherry-picks\" suffers from this problem\n>> as well as it does the equivalent of \"git log --cherry-pick\". Is there\n>> any way to share prefetch_cherry_blobs() with the cherry-pick detection\n>> in revision.c?\n> \n> Yes, you're right; git rebase without --reapply-cherry-picks and git\n> log --cherry-pick both go through cherry_pick_list() in revision.c,\n> which has exactly the same shape as the patch-ids loop in\n> cmd_cherry(): build a hashmap of one side via add_commit_patch_id(),\n> then look up the other side via patch_id_iter_first(). The on-demand\n> blob fetches come from the same patch_id_neq() callback.\n> \n> After poking around, I think the approximate scope of the fix would\n> be: Move collect_diff_blob_oids(), always_match(), and\n> prefetch_cherry_blobs() from builtin/log.c to patch-ids.c and expose\n> the last one in patch-ids.h. In cherry_pick_list(), between the\n> add_commit_patch_id loop and the comparison loop, build a temporary\n> list of just the lookup-side commits (filtering by\n> SYMMETRIC_LEFT/BOUNDARY as the existing loop already does) and call\n> prefetch_cherry_blobs() on it.\n\nThanks for taking a look\n\n> That said, I'd rather leave this out of the current series. The bigger\n> picture is that I have reservations about expanding partial-clone\n> support further into this area. git cherry, git log --cherry-pick, and\n> the default cherry-pick detection in git rebase all exist to answer\n> \"has this patch already landed upstream?\" -- a question that, in\n> repositories large enough to need partial clones, I feel is rarely\n> worth the cost of computing patch-ids across arbitrary amounts of\n> history. The honest guidance I would probably give for users on a\n> large repo is \"pass --reapply-cherry-picks (with rebase) and skip this\n> entirely\" or to narrow the range under consideration.\n\n\"--reapply-cherry-picks --empty=drop\" is certainly more efficient. When \nwe're computing patch ids do we do it for every upstream commit or just \nthe ones that modify the set of paths that are modified in the branch \nwe're rebasing?\n\nIt is a shame that we don't have a config setting for \n\"-reapply-cherry-picks\" as it is easy to forget to pass that option. \nUnfortunately it is not supported by the apply backend which makes such \na setting potentially confusing.\n\n> The omission of\n> a --no-reapply-cherry-picks option in git-replay wasn't a lack of\n> effort or oversight, but a deliberate choice where I'd rather hold off\n> (possibly indefinitely) on implementing it. So I'm a bit reluctant to\n> make the performance hazard less visible without also asking whether\n> we should even be doing that piece of the operation.\n> \n> I only implemented the git cherry fix because of a specific customer\n> situation where the operation was already baked into tooling, and\n> prefetching at least makes the worst case tolerable.\n\nI'm a bit surprised customers aren't complaining about tools that use \n\"git rebase\" being slow.\n\n> I don't want to\n> hold myself to doing the same for the cherry_pick_list() path, but I'm\n> fairly confident the code here can be re-used for those other cases\n> and I'd help review a patch from anyone who wants to carry it forward.\n> \n> Anyway, you are making the right connection, it's just that my\n> personal answer is to let some other interested individual do it.\n\nFair enough\n\nThanks\n\nPhillip\n\n"},{"id":"542224","messageId":"CABPp-BGQkN0ZeDAR4NzuyBakJHLM1AuqkdSGbb0YQfgWh2dWFg@mail.gmail.com","threadId":"65502","inReplyTo":"2abdc8ba-e361-492c-88b7-0c807ee9fb4d@gmail.com","subject":"Re: [PATCH v2 2/3] builtin/log: prefetch necessary blobs for `git cherry`","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-04-23T17:38:09Z","receivedAt":"2026-04-23T17:38:21Z","isPatch":true,"body":"Hi Phillip,\n\nOn Thu, Apr 23, 2026 at 8:15 AM Phillip Wood wrote:\n> On 21/04/2026 22:28, Elijah Newren wrote:\n> > On Sun, Apr 19, 2026 at 7:04 AM Phillip Wood wrote:\n> >> On 18/04/2026 01:32, Elijah Newren via GitGitGadget wrote:\n\n> \"--reapply-cherry-picks --empty=drop\" is certainly more efficient. When\n> we're computing patch ids do we do it for every upstream commit or just\n> the ones that modify the set of paths that are modified in the branch\n> we're rebasing?\n\nYou are correct that the patch id computations won't look at file\ncontents of commits unless they modify the same set of files as one of\nthe commits in our topic branch, but in order to determine the set of\ncommits which modify the same paths as commits in the branch we're\nrebasing, we have to walk the upstream commits and do a tree-diff for\nevery one of them. Yes, commits and trees tend to be much smaller\nthan blobs, but the number of trees/commits we have to look at may be\nfar larger than the number of blobs. The biggest repositories are\nconstantly pushing so many commits that they are at a size where even\na merge-base operation can start to feel expensive.\n\n> It is a shame that we don't have a config setting for\n> \"-reapply-cherry-picks\" as it is easy to forget to pass that option.\n> Unfortunately it is not supported by the apply backend which makes such\n> a setting potentially confusing.\n\nIndeed.\n\n> > The omission of\n> > a --no-reapply-cherry-picks option in git-replay wasn't a lack of\n> > effort or oversight, but a deliberate choice where I'd rather hold off\n> > (possibly indefinitely) on implementing it. So I'm a bit reluctant to\n> > make the performance hazard less visible without also asking whether\n> > we should even be doing that piece of the operation.\n> >\n> > I only implemented the git cherry fix because of a specific customer\n> > situation where the operation was already baked into tooling, and\n> > prefetching at least makes the worst case tolerable.\n>\n> I'm a bit surprised customers aren't complaining about tools that use\n> \"git rebase\" being slow.\n\nAre you sure they aren't complaining?\n\nThe merging parts of a rebase operation do have batch prefetching\nalready (up to 3 batches per commit; done that way to minimize the\nnumber of objects downloaded because sometimes 2 or more of those\nbatches can be skipped entirely and trying to combine them into a\nsingle batch would only be doable by downloading far more than\nneeded). But, as you're alluding to, the --no-reapply-cherry-picks\npart does not.\n\nI'll note that GitHub tends to focus far more on the server side; it's\njust that in this particular case with a special customer, they had me\ndig a little closer to their client side operations. In their case,\nthey were using git-replay rather than git-rebase, so they'd have no\nreason to complain about rebase. git-replay shares the same batch\nprefetching for merge operations that rebase has, and doesn't have a\n--no-reapply-cherry-picks behavior that can even be selected.\nHonestly, I think the main reason this customer was also using\ngit-cherry was because I didn't get the drop-commits-that-become-empty\nlogic in the early versions of git-replay. You added that to\ngit-replay (thanks again!), but after they had already built their\ntooling. This is only a guess on my part; they may have other reasons\nfor actively wanting git-cherry, but I think it might be worthwhile\nfor me to ask them if they can upgrade git versions (to get your fixes\nfor empty commits in replay) and then drop the calls to git-cherry.\nHowever, I didn't want it to sound like I was pushing them to change\ntheir workflows at my convenience, and hence this patch so that things\ncan be fast even if they keep the git-cherry in there.\n\n> > I don't want to\n> > hold myself to doing the same for the cherry_pick_list() path, but I'm\n> > fairly confident the code here can be re-used for those other cases\n> > and I'd help review a patch from anyone who wants to carry it forward.\n> >\n> > Anyway, you are making the right connection, it's just that my\n> > personal answer is to let some other interested individual do it.\n>\n> Fair enough\n\nThanks for taking a look and asking interesting questions.\n\nElijah\n"},{"id":"542383","messageId":"31763514-2602-4d8e-ac25-70590f090947@gmail.com","threadId":"65502","inReplyTo":"8fbfe69bc4d0c6166967986f24861ffa393ed7cf.1776472347.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/3] grep: prefetch necessary blobs","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-04-27T12:59:48Z","receivedAt":"2026-04-27T12:59:51Z","isPatch":true,"body":"On 4/17/2026 8:32 PM, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> In partial clones, `git grep` fetches necessary blobs on-demand one\n> at a time, which can be very slow. In partial clones, add an extra\n> preliminary walk over the tree similar to grep_tree() which collects\n> the blobs of interest, and then prefetches them.\n\nA log of the code is about walking trees to find blobs matching\nthe input pathspec, with this being the core method:\n\n> +static void collect_blob_oids_for_tree(struct repository *repo,\n> +\t\t\t\t const struct pathspec *pathspec,\n> +\t\t\t\t struct tree_desc *tree,\n> +\t\t\t\t struct strbuf *base,\n> +\t\t\t\t int tn_len,\n> +\t\t\t\t struct oidset *blob_oids)\n\nAnd in your test, you set up a repo to have three blobs with\nmatches in two of the files:\n\n> +test_expect_success 'grep of revision in partial clone does bulk prefetch' '\n> +\ttest_when_finished \"rm -rf grep-partial-src grep-partial\" &&\n> +\n> +\tgit init grep-partial-src &&\n> +\t(\n> +\t\tcd grep-partial-src &&\n> +\t\tgit config uploadpack.allowfilter 1 &&\n> +\t\tgit config uploadpack.allowanysha1inwant 1 &&\n> +\t\techo \"needle in haystack\" >searchme &&\n> +\t\techo \"no match here\" >other &&\n> +\t\tmkdir subdir &&\n> +\t\techo \"needle again\" >subdir/deep &&\n> +\t\tgit add . &&\n> +\t\tgit commit -m \"initial\"\n> +\t) &&\n\nBut then the command downloads all of the blobs, not using a\npathspec:\n\n> +\t# grep HEAD should batch-prefetch all blobs in one request.\n> +\tGIT_TRACE2_EVENT=\"$(pwd)/grep-trace\" \\\n> +\t\tgit -C grep-partial grep -c \"needle\" HEAD >result &&\n> +\n> +\t# Should find matches in two files.\n> +\ttest_line_count = 2 result &&\n> +\n> +\t# Should have prefetched all 3 objects at once\n> +\ttest_trace2_data promisor fetch_count 3 +'\nI think your code is correct, but I'd like to see a test\nhere that demonstrates a pathspec filter on the 'grep'\ncommand to help filter out a blob that has a matching string.\n\nPerhaps something like:\n\n* matches.txt (has needle)\n* nomatch.txt (does not have needle)\n* matches.md (has needle)\n\nand then 'git grep -c \"needle\" HEAD -- *.txt' would\ndownload two blobs and find one match. A second run without\nthe pathspec would download one blob and find two matches.\n\nDoes that make sense as a test?\n\nThanks,\n-Stolee\n\n"},{"id":"542385","messageId":"a2fbb23d-0809-4a9d-8bf9-8ac0dc8ee054@gmail.com","threadId":"65502","inReplyTo":"a705852723fbe88e94ad3de1daba548dbce32211.1776472347.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] builtin/log: prefetch necessary blobs for `git cherry`","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-04-27T13:16:59Z","receivedAt":"2026-04-27T13:17:02Z","isPatch":true,"body":"On 4/17/2026 8:32 PM, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n\n(I'm sorry that I'm reviewing out of order. This reply includes my\nfeelings about patch 3 after reading both.)\n\n> +/*\n> + * Enumerate blob OIDs from a single commit's diff, inserting them into blobs.\n> + * Skips files whose userdiff driver explicitly declares binary status\n> + * (drv->binary > 0), since patch-ID uses oid_to_hex() for those and\n> + * never reads blob content. Use userdiff_find_by_path() since\n> + * diff_filespec_load_driver() is static in diff.c.\n> + *\n> + * Clean up with diff_queue_clear() (from diffcore.h).\n> + */\n> +static void collect_diff_blob_oids(struct commit *commit,\n> +\t\t\t\t struct diff_options *opts,\n> +\t\t\t\t struct oidset *blobs)\n\nI think that this is generally a good idea, though I worry that\nhaving this hidden in builtin/log.c may not be the right long-\nterm home.\n\nI expect that we'll find more and more examples where we want to\nprefetch blobs in different operations, those that exist now and\nthose that may be created in the future. It would be preferred if\nthey could automatically take advantage of the logic already in\ndiff_queued_diff_prefetch() within diffcore_std() in diff.c.\n\nUltimately, _this_ patch cares about a diff. Could we compute a\n\"diff prep\" computation using the core diff library instead of\ninventing a second queue of results for diffing?\n\nPatch 3 cares about a \"scan prep\" which cares about loading all\nblobs for a given tree with respect to a pathspec. This is very\nsimilar to what a checkout would do, though it ultimately uses\na form of diff to find out what change should be applied to the\nworking directory. Perhaps 'git archive' is a better matching\nexample.\n\nI don't mean to make your series more complicated. I value what\nyou're doing and can see how your current attention can be used\nto make further improvements later. By implementing things in a\ncommon location, then we can have later integrations add to the\nconfidence in the feature through tests covering each user-facing\nuse.\n\nI'm not sure if it makes sense to attempt to create a universal\nlibrary method that would be used by builtin/log.c _and_ diff.c,\nat least not right now. I'm most interested in having this logic\nbe more reusable in the future without needing to move code\nacross files.\n\nThanks,\n-Stolee\n\n"},{"id":"543002","messageId":"xmqqtsseu09t.fsf@gitster.g","threadId":"65502","inReplyTo":"a2fbb23d-0809-4a9d-8bf9-8ac0dc8ee054@gmail.com","subject":"Re: [PATCH v2 2/3] builtin/log: prefetch necessary blobs for `git cherry`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-05-11T02:51:10Z","receivedAt":"2026-05-11T02:51:12Z","isPatch":true,"body":"Derrick Stolee writes:\n\n> Ultimately, _this_ patch cares about a diff. Could we compute a\n> \"diff prep\" computation using the core diff library instead of\n> inventing a second queue of results for diffing?\n>\n> Patch 3 cares about a \"scan prep\" which cares about loading all\n> blobs for a given tree with respect to a pathspec. This is very\n> similar to what a checkout would do, though it ultimately uses\n> a form of diff to find out what change should be applied to the\n> working directory. Perhaps 'git archive' is a better matching\n> example.\n>\n> I don't mean to make your series more complicated. I value what\n> you're doing and can see how your current attention can be used\n> to make further improvements later. By implementing things in a\n> common location, then we can have later integrations add to the\n> confidence in the feature through tests covering each user-facing\n> use.\n>\n> I'm not sure if it makes sense to attempt to create a universal\n> library method that would be used by builtin/log.c _and_ diff.c,\n> at least not right now. I'm most interested in having this logic\n> be more reusable in the future without needing to move code\n> across files.\n\nThe points raised in the message I am responding here, together with\nthe ones in <31763514-2602-4d8e-ac25-70590f090947@gmail.com>, remain\nunanswered.\n\nShould I still keep these patches in my tree, hoping that responses\nmay come some day? I will mark the topic as \"Expeting review\nresponses\" in the draft \"What's cooking\" report I work from for now,\nbut it has been quite a while since we looked at the patches, so...?\n\nThanks.\n"},{"id":"543067","messageId":"CABPp-BFdYjnjhSrjEBf8kjYYY2jtrQ_=w0jYR+DDWh3szmtvqQ@mail.gmail.com","threadId":"65502","inReplyTo":"xmqqtsseu09t.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] builtin/log: prefetch necessary blobs for `git cherry`","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-05-11T17:45:38Z","receivedAt":"2026-05-11T17:45:50Z","isPatch":true,"body":"On Sun, May 10, 2026 at 7:51 PM Junio C Hamano wrote:\n>\n> Derrick Stolee writes:\n>\n> > Ultimately, _this_ patch cares about a diff. Could we compute a\n> > \"diff prep\" computation using the core diff library instead of\n> > inventing a second queue of results for diffing?\n> >\n> > Patch 3 cares about a \"scan prep\" which cares about loading all\n> > blobs for a given tree with respect to a pathspec. This is very\n> > similar to what a checkout would do, though it ultimately uses\n> > a form of diff to find out what change should be applied to the\n> > working directory. Perhaps 'git archive' is a better matching\n> > example.\n> >\n> > I don't mean to make your series more complicated. I value what\n> > you're doing and can see how your current attention can be used\n> > to make further improvements later. By implementing things in a\n> > common location, then we can have later integrations add to the\n> > confidence in the feature through tests covering each user-facing\n> > use.\n> >\n> > I'm not sure if it makes sense to attempt to create a universal\n> > library method that would be used by builtin/log.c _and_ diff.c,\n> > at least not right now. I'm most interested in having this logic\n> > be more reusable in the future without needing to move code\n> > across files.\n>\n> The points raised in the message I am responding here, together with\n> the ones in <31763514-2602-4d8e-ac25-70590f090947@gmail.com>, remain\n> unanswered.\n>\n> Should I still keep these patches in my tree, hoping that responses\n> may come some day? I will mark the topic as \"Expeting review\n> responses\" in the draft \"What's cooking\" report I work from for now,\n> but it has been quite a while since we looked at the patches, so...?\n\nSorry for not responding sooner. There have been a number of\nincidents at work (including a big problematic one the day Derrick\nsent his email), and I was pulled into both firefighting and\nremediation duties which have sucked up all my time. I owe responses\nto Derrick, Patrick, Johannes, Phillip, and Taylor on a variety of\ntopics.\n\nFor this particular series, maybe mark as expecting a re-roll, since\nStolee suggested adding a test on patch 3?\n"},{"id":"543262","messageId":"CABPp-BHq=rRQSxtiVE1s9jiQQAqqAi_k+fH0OHCVjmfzq07hiA@mail.gmail.com","threadId":"65502","inReplyTo":"31763514-2602-4d8e-ac25-70590f090947@gmail.com","subject":"Re: [PATCH v2 3/3] grep: prefetch necessary blobs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-05-13T19:21:39Z","receivedAt":"2026-05-13T19:21:53Z","isPatch":true,"body":"On Mon, Apr 27, 2026 at 5:59 AM Derrick Stolee wrote:\n>\n> On 4/17/2026 8:32 PM, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n> >\n> > In partial clones, `git grep` fetches necessary blobs on-demand one\n> > at a time, which can be very slow. In partial clones, add an extra\n> > preliminary walk over the tree similar to grep_tree() which collects\n> > the blobs of interest, and then prefetches them.\n>\n> A log of the code is about walking trees to find blobs matching\n> the input pathspec, with this being the core method:\n>\n> > +static void collect_blob_oids_for_tree(struct repository *repo,\n> > + const struct pathspec *pathspec,\n> > + struct tree_desc *tree,\n> > + struct strbuf *base,\n> > + int tn_len,\n> > + struct oidset *blob_oids)\n>\n> And in your test, you set up a repo to have three blobs with\n> matches in two of the files:\n>\n> > +test_expect_success 'grep of revision in partial clone does bulk prefetch' '\n> > + test_when_finished \"rm -rf grep-partial-src grep-partial\" &&\n> > +\n> > + git init grep-partial-src &&\n> > + (\n> > + cd grep-partial-src &&\n> > + git config uploadpack.allowfilter 1 &&\n> > + git config uploadpack.allowanysha1inwant 1 &&\n> > + echo \"needle in haystack\" >searchme &&\n> > + echo \"no match here\" >other &&\n> > + mkdir subdir &&\n> > + echo \"needle again\" >subdir/deep &&\n> > + git add . &&\n> > + git commit -m \"initial\"\n> > + ) &&\n>\n> But then the command downloads all of the blobs, not using a\n> pathspec:\n>\n> > + # grep HEAD should batch-prefetch all blobs in one request.\n> > + GIT_TRACE2_EVENT=\"$(pwd)/grep-trace\" \\\n> > + git -C grep-partial grep -c \"needle\" HEAD >result &&\n> > +\n> > + # Should find matches in two files.\n> > + test_line_count = 2 result &&\n> > +\n> > + # Should have prefetched all 3 objects at once\n> > + test_trace2_data promisor fetch_count 3 > +'\n> I think your code is correct, but I'd like to see a test\n> here that demonstrates a pathspec filter on the 'grep'\n> command to help filter out a blob that has a matching string.\n>\n> Perhaps something like:\n>\n> * matches.txt (has needle)\n> * nomatch.txt (does not have needle)\n> * matches.md (has needle)\n>\n> and then 'git grep -c \"needle\" HEAD -- *.txt' would\n> download two blobs and find one match. A second run without\n> the pathspec would download one blob and find two matches.\n>\n> Does that make sense as a test?\n\nYes, absolutely. And thanks for suggesting it; although I was\nhandling pathspecs correctly, I discovered that I was unconditionally\nrequesting to download whatever objects matched the pathspecs (or all\nblobs in the commit if no pathspec given), even if the blobs were\nalready local. I'll send an updated test in v2, along with the fix.\n"},{"id":"543287","messageId":"CABPp-BGpXgDfJeDEB91U-h092-8L6Q_MLrzSLFg9HotPDZ-m-g@mail.gmail.com","threadId":"65502","inReplyTo":"a2fbb23d-0809-4a9d-8bf9-8ac0dc8ee054@gmail.com","subject":"Re: [PATCH v2 2/3] builtin/log: prefetch necessary blobs for `git cherry`","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-05-13T23:17:28Z","receivedAt":"2026-05-13T23:17:41Z","isPatch":true,"body":"Hi,\n\nSorry for the long delay. Lots of firefighting of incidents kept me\naway for a bit...\n\nOn Mon, Apr 27, 2026 at 6:17 AM Derrick Stolee wrote:\n>\n> On 4/17/2026 8:32 PM, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n>\n> (I'm sorry that I'm reviewing out of order. This reply includes my\n> feelings about patch 3 after reading both.)\n\nThanks for taking a look! And I have no problems with reviewing out\nof order (unless the comments on later patches don't make sense due to\nthe reviewer being unaware of previous patches, which isn't the case\nhere).\n\n> > +/*\n> > + * Enumerate blob OIDs from a single commit's diff, inserting them into blobs.\n> > + * Skips files whose userdiff driver explicitly declares binary status\n> > + * (drv->binary > 0), since patch-ID uses oid_to_hex() for those and\n> > + * never reads blob content. Use userdiff_find_by_path() since\n> > + * diff_filespec_load_driver() is static in diff.c.\n> > + *\n> > + * Clean up with diff_queue_clear() (from diffcore.h).\n> > + */\n> > +static void collect_diff_blob_oids(struct commit *commit,\n> > + struct diff_options *opts,\n> > + struct oidset *blobs)\n>\n> I think that this is generally a good idea, though I worry that\n> having this hidden in builtin/log.c may not be the right long-\n> term home.\n>\n> I expect that we'll find more and more examples where we want to\n> prefetch blobs in different operations, those that exist now and\n> those that may be created in the future. It would be preferred if\n> they could automatically take advantage of the logic already in\n> diff_queued_diff_prefetch() within diffcore_std() in diff.c.\n>\n> Ultimately, _this_ patch cares about a diff.\n\nI read this patch a bit differently -- could you say more about what\nyou have in mind?\n\nThe body of collect_diff_blob_oids() really is just diff_tree_oid() +\ndiffcore_std() + process each pair, so at the per-commit level I am\nalready leaning on the diff library. One of the things this patch\nadds is accumulation across many commits: the containing loop (in\nprefetch_cherry_blobs) is over a commit range, not over a single diff.\n\nConcretely, the motivating case was a patch touching a few files where\nupstream had tens of thousands of commits in .., several\nhundred of which modified the same set of files. A per-diff prefetch\nlike diff.c uses would turn that into hundreds of small fetches of 1-3\nblobs each; what this series gives you is one fetch. So the win\nreally does live above the diff library, not inside it.\n\nThere are two further wrinkles in cherry that are filters layered on\ntop of the cross-commit accumulation, and they're cherry-specific in a\nway that I don't think belongs in the diff library:\n\n 1. For most commits in .., cherry doesn't care about\nthe diff at all -- if the list of files modified doesn't exactly match\nthe commit of interest, the commit is skipped before patch-id is even\ncomputed. Prefetching for those would be wasted.\n\n 2. We skip prefetching content for binary files (because patch-id\nuses oid_to_hex() for such files instead of the diff contents).\n\n> Could we compute a\n> \"diff prep\" computation using the core diff library instead of\n> inventing a second queue of results for diffing?\n\nTo check this concretely I looked at each of the existing\npromisor_remote_get_direct() callsites for a similar producer. The\nclosest cousin of collect_diff_blob_oids() (the only part of this\npatch that looks like it might be close to the right shape to put in a\ncore diff library) is diff.c's diff_queued_diff_prefetch() -- but it\noperates on the already-populated global diff_queued_diff and fetches\nimmediately, rather than setting up the diff itself and returning an\noidset for the caller to accumulate. Reshaping it to match cherry's\nneeds would either break its current caller in diffcore_std() or\nintroduce a parallel function whose only consumer is cherry. None of\nthe other sites (path-walk in backfill, index walk in read-cache,\nthree-way state in merge-ort, etc.) do anything resembling \"diff two\ntrees and harvest oids.\"\n\nAnd even if we did factor a helper out, cherry's filter is\npatch-id-specific: commit_patch_id() substitutes oid_to_hex() for\nfiles marked binary by their userdiff driver, so we deliberately skip\nprefetching those. That isn't a generic \"diff prep\" consideration --\nit only makes sense because the caller is patch-id. We could express\nit as a predicate parameter, but with one caller that would feel to me\nlike it's just pushing cherry's policy across an API boundary for no\ngain.\n\n> Patch 3 cares about a \"scan prep\" which cares about loading all\n> blobs for a given tree with respect to a pathspec. This is very\n> similar to what a checkout would do, though it ultimately uses\n> a form of diff to find out what change should be applied to the\n> working directory. Perhaps 'git archive' is a better matching\n> example.\n\nAgreed that archive is the closer analog -- both grep and archive do a\npathspec-filtered single-tree walk, whereas checkout's prefetch is\ntied to the index and optimizes to the subset of paths that are\ndifferent since the previous version checked out. Retrofitting that\nto grep would mean materializing an index for the target revision just\nto throw it away, which feels like more machinery to bridge the\nabstractions than the walk itself would take.\n\n> By implementing things in a\n> common location, then we can have later integrations add to the\n> confidence in the feature through tests covering each user-facing\n> use.\n\nSounds great...but what common user-facing uses exist?\n\nLooking at the existing 11 callsites of promisor_remote_get_direct()\nafter this series [1], each has pretty specialized data needs --\nindex-driven (read-cache), index-pack & pack-objects internals,\npath-walk batches (backfill), merge-ort's three-way logic,\ndiffcore-rename's two independent rename-detection paths, plain old\ndiffs, collection across a subset of commits (cherry),\npathspec-filtered tree walk (grep), and\non-demand-single-blob-at-a-time (odb.c) -- so I don't see a natural\nshared layer above the primitive itself (which is already\npromisor_remote_get_direct).\n\narchive, if it had prefetch logic, would be the first match. But it's\nnot clear where the shared logic between grep and archive would live,\nif archive even had any prefetch logic to share.\n\nSo I'm inclined to leave both new producers local to their builtins\nfor now, and factor a tree-walk helper when archive (or a third\ncaller) actually wants one. But I'm happy to be told I've missed the\nboat.\n\nThanks,\nElijah\n\n[1] builtin/backfill.c, builtin/grep.c, builtin/index-pack.c,\nbuiltin/log.c, builtin/pack-objects.c, diff.c, diffcore-rename.c (two\ncallsites), merge-ort.c, odb.c, read-cache.c\n"},{"id":"543336","messageId":"pull.2089.v3.git.1778775928.gitgitgadget@gmail.com","threadId":"65502","inReplyTo":"pull.2089.v2.git.1776472347.gitgitgadget@gmail.com","subject":"[PATCH v3 0/4] Batch prefetching","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-05-14T16:25:24Z","receivedAt":"2026-05-14T16:25:31Z","isPatch":true,"body":"Changes since v2:\n\n * Modified the final patch as suggested by Stolee to include pathspec usage\n in the testcase\n * Modified the last two patches to not re-download blobs we already have\n locally, and adjusted the tests to verify\n * Inserted a new first patch, containing a documentation addition that\n would have helped me avoid making the above mistake in the first place.\n\nNote: Stolee also suggest some code sharing or code movement in his review\nof v2 2/3, but possibly based on a misunderstanding of v2 2/3 (that patch\nisn't about a diff) and it's not clear to me what could be shared or moved,\nso that's not part of this round.\n\nChanges since v1:\n\n * Remove stray file that should have never been added. So embarrassing that\n I didn't catch that before submitting.\n\n\nOriginal cover-letter:\n======================\n\nPartial clones provide a trade-off for users: avoid downloading blobs\nupfront, at the expense of needing to download them later as they run other\ncommands. This tradeoff can sometimes incur a more severe cost than\nexpected, particularly if needed blobs are discovered as they are accessed,\nresulting in downloading blobs one at a time. Some commands like checkout,\ndiff, and merge do batch prefetches of necessary blobs, since that can\ndramatically reduce the pain of on-demand loading. Extend this ability to\ntwo more commands: cherry and grep.\n\nThis series was spurred by a report where git cherry jobs were each doing\nhundreds of single-blob fetches, at a cost of 3s each. Batching those\ndownloads should dramatically speed up their jobs. (And I decided to fix up\ngit grep similarly while at it.)\n\nI'll also note that git backfill with revisions and/or pathspecs could also\nimprove things for these users, but since backfill is a manual command users\nwould have to run and requires users to try to figure out which data is\nneeded (a challenge in the case of cherry), it still makes sense to provide\nsmarter behavior for folks who don't choose to manually run backfill.\n\nAlso, correct a documentation typo I noticed in patch-ids.h (related to code\nI was using for the git cherry fixes) as a preparatory fixup.\n\nElijah Newren (4):\n promisor-remote: document caller filtering contract\n patch-ids.h: add missing trailing parenthesis in documentation comment\n builtin/log: prefetch necessary blobs for `git cherry`\n grep: prefetch necessary blobs\n\n builtin/grep.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++\n builtin/log.c | 131 ++++++++++++++++++++++++++++++++++++++++++\n patch-ids.h | 2 +-\n promisor-remote.h | 11 ++++\n t/t3500-cherry.sh | 27 +++++++++\n t/t7810-grep.sh | 58 +++++++++++++++++++\n 6 files changed, 371 insertions(+), 1 deletion(-)\n\n\nbase-commit: 9f223ef1c026d91c7ac68cc0211bde255dda6199\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2089%2Fnewren%2Fbatch-prefetching-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2089/newren/batch-prefetching-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/2089\n\nRange-diff vs v2:\n\n -: ---------- > 1: 6ad11e2c28 promisor-remote: document caller filtering contract\n 1: 663816a344 = 2: 08a2c6517b patch-ids.h: add missing trailing parenthesis in documentation comment\n 2: a705852723 ! 3: c0655e5d41 builtin/log: prefetch necessary blobs for `git cherry`\n @@ builtin/log.c: static void print_commit(char sign, struct commit *commit, int ve\n +\t\tif (drv && drv->binary > 0)\n +\t\t\tcontinue;\n +\n -+\t\tif (DIFF_FILE_VALID(p->one))\n ++\t\tif (DIFF_FILE_VALID(p->one) &&\n ++\t\t odb_read_object_info_extended(opts->repo->objects,\n ++\t\t\t\t\t\t &p->one->oid, NULL,\n ++\t\t\t\t\t\t OBJECT_INFO_FOR_PREFETCH))\n +\t\t\toidset_insert(blobs, &p->one->oid);\n -+\t\tif (DIFF_FILE_VALID(p->two))\n ++\t\tif (DIFF_FILE_VALID(p->two) &&\n ++\t\t odb_read_object_info_extended(opts->repo->objects,\n ++\t\t\t\t\t\t &p->two->oid, NULL,\n ++\t\t\t\t\t\t OBJECT_INFO_FOR_PREFETCH))\n +\t\t\toidset_insert(blobs, &p->two->oid);\n +\t}\n +\tdiff_queue_clear(q);\n @@ t/t3500-cherry.sh: test_expect_success 'cherry ignores whitespace' '\n +\n +\t\tgrep \"child_start.*fetch.negotiationAlgorithm\" trace.output >fetches &&\n +\t\ttest_line_count = 1 fetches &&\n -+\t\ttest_trace2_data promisor fetch_count 4 actual &&\n ++\t\ttest_cmp ../expect actual &&\n ++\n ++\t\t! grep \"child_start.*fetch.negotiationAlgorithm\" trace2.output &&\n ++\t\t! grep \"\\\"key\\\":\\\"fetch_count\\\"\" trace2.output\n +\t)\n +'\n +\n 3: 8fbfe69bc4 ! 4: 75d4ca7cff grep: prefetch necessary blobs\n @@ builtin/grep.c: static int grep_tree(struct grep_opt *opt, const struct pathspec\n +\t\tstrbuf_add(base, entry.path, tree_entry_len(&entry));\n +\n +\t\tif (S_ISREG(entry.mode)) {\n -+\t\t\toidset_insert(blob_oids, &entry.oid);\n ++\t\t\tif (!odb_has_object(repo->objects, &entry.oid, 0))\n ++\t\t\t\toidset_insert(blob_oids, &entry.oid);\n +\t\t} else if (S_ISDIR(entry.mode)) {\n +\t\t\tenum object_type type;\n +\t\t\tstruct tree_desc sub_tree;\n @@ t/t7810-grep.sh: test_expect_success 'grep does not report i-t-a and assume unch\n \ttest_cmp expected actual\n '\n \n -+test_expect_success 'grep of revision in partial clone does bulk prefetch' '\n ++test_expect_success 'grep of revision in partial clone batches prefetch and honors pathspec' '\n +\ttest_when_finished \"rm -rf grep-partial-src grep-partial\" &&\n +\n +\tgit init grep-partial-src &&\n @@ t/t7810-grep.sh: test_expect_success 'grep does not report i-t-a and assume unch\n +\t\tcd grep-partial-src &&\n +\t\tgit config uploadpack.allowfilter 1 &&\n +\t\tgit config uploadpack.allowanysha1inwant 1 &&\n -+\t\techo \"needle in haystack\" >searchme &&\n -+\t\techo \"no match here\" >other &&\n -+\t\tmkdir subdir &&\n -+\t\techo \"needle again\" >subdir/deep &&\n ++\t\tmkdir a b &&\n ++\t\techo \"needle in haystack\" >a/matches.txt &&\n ++\t\techo \"nothing to see here\" >a/nomatch.txt &&\n ++\t\techo \"needle again\" >b/matches.md &&\n +\t\tgit add . &&\n +\t\tgit commit -m \"initial\"\n +\t) &&\n @@ t/t7810-grep.sh: test_expect_success 'grep does not report i-t-a and assume unch\n +\tgit clone --no-checkout --filter=blob:none \\\n +\t\t\"file://$(pwd)/grep-partial-src\" grep-partial &&\n +\n -+\t# All blobs should be missing after a blobless clone.\n ++\t# All three blobs are missing immediately after a blobless clone.\n +\tgit -C grep-partial rev-list --quiet --objects \\\n +\t\t--missing=print HEAD >missing &&\n +\ttest_line_count = 3 missing &&\n +\n -+\t# grep HEAD should batch-prefetch all blobs in one request.\n -+\tGIT_TRACE2_EVENT=\"$(pwd)/grep-trace\" \\\n ++\t# A pathspec-limited grep should prefetch only the two blobs\n ++\t# in a/. It should fetch both blobs in one batched request.\n ++\tGIT_TRACE2_EVENT=\"$(pwd)/grep-trace-pathspec\" \\\n ++\t\tgit -C grep-partial grep -c \"needle\" HEAD -- \"a/*.txt\" >result &&\n ++\n ++\t# Only a/matches.txt contains \"needle\" among the matched paths.\n ++\ttest_line_count = 1 result &&\n ++\n ++\t# Exactly the two a/*.txt blobs should have been requested, and\n ++\t# the server packed those two objects in the response.\n ++\ttest_trace2_data promisor fetch_count 2 missing &&\n ++\ttest_line_count = 1 missing &&\n ++\n ++\t# A second grep without a pathspec must recurse into both\n ++\t# subdirectories, but should request only the still-missing blob\n ++\t# from the promisor.\n ++\tGIT_TRACE2_EVENT=\"$(pwd)/grep-trace-all\" \\\n +\t\tgit -C grep-partial grep -c \"needle\" HEAD >result &&\n +\n -+\t# Should find matches in two files.\n +\ttest_line_count = 2 result &&\n ++\ttest_trace2_data promisor fetch_count 1 missing &&\n ++\ttest_line_count = 0 missing\n +'\n +\n test_done\n\n-- \ngitgitgadget\n"},{"id":"543337","messageId":"6ad11e2c28d9c3b3d5dcb8986b726d2d2db18cc3.1778775928.git.gitgitgadget@gmail.com","threadId":"65502","inReplyTo":"pull.2089.v3.git.1778775928.gitgitgadget@gmail.com","subject":"[PATCH v3 1/4] promisor-remote: document caller filtering contract","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-05-14T16:25:25Z","receivedAt":"2026-05-14T16:25:32Z","isPatch":true,"body":"From: Elijah Newren \n\npromisor_remote_get_direct() does not, on its happy path, filter out\nOIDs that are already present in the local object store: every OID\nthe caller supplies is written to the fetch subprocess's stdin and\nends up in the response pack. The only filtering it performs is in\nremove_fetched_oids(), and that only runs after a fetch failure when\nfalling back to a different configured promisor remote.\n\nAlmost every existing caller already filters locally-present OIDs out\nitself (typically with odb_read_object_info_extended() and\nOBJECT_INFO_FOR_PREFETCH, or odb_has_object() with no fetch flag). But\nthe existing API comment does not state this expectation, so a new\ncaller is easy to write incorrectly (I missed this originally and wrote\ntwo problematic callers). Omitting the filter still \"works\" in the\nsense that the desired objects end up local, but it silently makes the\nfetch request -- and the response pack -- larger than necessary,\ndefeating part of the point of batching.\n\nSpell the contract out so future callers know to filter (and\ndeduplicate) themselves, and point them at the helpers they should\nuse to check local presence without accidentally triggering a lazy\nfetch.\n\nSigned-off-by: Elijah Newren \n---\n promisor-remote.h | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/promisor-remote.h b/promisor-remote.h\nindex 3d4d2de018..301f5ac5cb 100644\n--- a/promisor-remote.h\n+++ b/promisor-remote.h\n@@ -29,6 +29,17 @@ int repo_has_promisor_remote(struct repository *r);\n * Fetches all requested objects from all promisor remotes, trying them one at\n * a time until all objects are fetched.\n *\n+ * Callers are responsible for filtering out OIDs that are already present\n+ * locally before calling this function: every supplied OID is sent in the\n+ * fetch request, even if the object already exists in the local object\n+ * store. (Only after a fetch failure does this function fall back to\n+ * stripping already-present OIDs from the list before trying the next\n+ * configured promisor remote.) Callers should also deduplicate the OIDs.\n+ *\n+ * To test for local presence without triggering a lazy fetch (which would\n+ * defeat the purpose of batching), use odb_has_object(..., 0) or\n+ * odb_read_object_info_extended() with OBJECT_INFO_FOR_PREFETCH.\n+ *\n * If oid_nr is 0, this function returns immediately.\n */\n void promisor_remote_get_direct(struct repository *repo,\n-- \ngitgitgadget\n\n"},{"id":"543338","messageId":"08a2c6517bfc75fd7ee514fa513b6f57659acca7.1778775928.git.gitgitgadget@gmail.com","threadId":"65502","inReplyTo":"pull.2089.v3.git.1778775928.gitgitgadget@gmail.com","subject":"[PATCH v3 2/4] patch-ids.h: add missing trailing parenthesis in documentation comment","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-05-14T16:25:26Z","receivedAt":"2026-05-14T16:25:33Z","isPatch":true,"body":"From: Elijah Newren \n\nSigned-off-by: Elijah Newren \n---\n patch-ids.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/patch-ids.h b/patch-ids.h\nindex 490d739371..57534ee722 100644\n--- a/patch-ids.h\n+++ b/patch-ids.h\n@@ -37,7 +37,7 @@ int has_commit_patch_id(struct commit *commit, struct patch_ids *);\n * struct patch_id *cur;\n * for (cur = patch_id_iter_first(commit, ids);\n * cur;\n- * cur = patch_id_iter_next(cur, ids) {\n+ * cur = patch_id_iter_next(cur, ids)) {\n * ... look at cur->commit\n * }\n */\n-- \ngitgitgadget\n\n"},{"id":"543339","messageId":"c0655e5d41012d6d11caa018d6f4b222426f2c7b.1778775928.git.gitgitgadget@gmail.com","threadId":"65502","inReplyTo":"pull.2089.v3.git.1778775928.gitgitgadget@gmail.com","subject":"[PATCH v3 3/4] builtin/log: prefetch necessary blobs for `git cherry`","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-05-14T16:25:27Z","receivedAt":"2026-05-14T16:25:34Z","isPatch":true,"body":"From: Elijah Newren \n\nIn partial clones, `git cherry` fetches necessary blobs on-demand one\nat a time, which can be very slow. We would like to prefetch all\nnecessary blobs upfront. To do so, we need to be able to first figure\nout which blobs are needed.\n\n`git cherry` does its work in a two-phase approach: first computing\nheader-only IDs (based on file paths and modes), then falling back to\nfull content-based IDs only when header-only IDs collide -- or, more\naccurately, whenever the oidhash() of the header-only object_ids\ncollide.\n\npatch-ids.c handles this by creating an ids->patches hashmap that has\nall the data we need, but the problem is that any attempt to query the\nhashmap will invoke the patch_id_neq() function on any colliding objects,\nwhich causes the on-demand fetching.\n\nInsert a new prefetch_cherry_blobs() function before checking for\ncollisions. Use a temporary replacement on the ids->patches.cmpfn\nin order to enumerate the blobs that would be needed without yet\nfetching them, and then fetch them all at once, then restore the old\nids->patches.cmpfn.\n\nSigned-off-by: Elijah Newren \n---\n builtin/log.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++\n t/t3500-cherry.sh | 27 ++++++++++\n 2 files changed, 158 insertions(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 8c0939dd42..e464b30af4 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -21,10 +21,12 @@\n #include \"color.h\"\n #include \"commit.h\"\n #include \"diff.h\"\n+#include \"diffcore.h\"\n #include \"diff-merges.h\"\n #include \"revision.h\"\n #include \"log-tree.h\"\n #include \"oid-array.h\"\n+#include \"oidset.h\"\n #include \"tag.h\"\n #include \"reflog-walk.h\"\n #include \"patch-ids.h\"\n@@ -43,9 +45,11 @@\n #include \"utf8.h\"\n \n #include \"commit-reach.h\"\n+#include \"promisor-remote.h\"\n #include \"range-diff.h\"\n #include \"tmp-objdir.h\"\n #include \"tree.h\"\n+#include \"userdiff.h\"\n #include \"write-or-die.h\"\n \n #define MAIL_DEFAULT_WRAP 72\n@@ -2602,6 +2606,131 @@ static void print_commit(char sign, struct commit *commit, int verbose,\n \t}\n }\n \n+/*\n+ * Enumerate blob OIDs from a single commit's diff, inserting them into blobs.\n+ * Skips files whose userdiff driver explicitly declares binary status\n+ * (drv->binary > 0), since patch-ID uses oid_to_hex() for those and\n+ * never reads blob content. Use userdiff_find_by_path() since\n+ * diff_filespec_load_driver() is static in diff.c.\n+ *\n+ * Clean up with diff_queue_clear() (from diffcore.h).\n+ */\n+static void collect_diff_blob_oids(struct commit *commit,\n+\t\t\t\t struct diff_options *opts,\n+\t\t\t\t struct oidset *blobs)\n+{\n+\tstruct diff_queue_struct *q;\n+\n+\t/*\n+\t * Merge commits are filtered out by patch_id_defined() in patch-ids.c,\n+\t * so we'll never be called with one.\n+\t */\n+\tassert(!commit->parents || !commit->parents->next);\n+\n+\tif (commit->parents)\n+\t\tdiff_tree_oid(&commit->parents->item->object.oid,\n+\t\t\t &commit->object.oid, \"\", opts);\n+\telse\n+\t\tdiff_root_tree_oid(&commit->object.oid, \"\", opts);\n+\tdiffcore_std(opts);\n+\n+\tq = &diff_queued_diff;\n+\tfor (int i = 0; i < q->nr; i++) {\n+\t\tstruct diff_filepair *p = q->queue[i];\n+\t\tstruct userdiff_driver *drv;\n+\n+\t\t/* Skip binary files */\n+\t\tdrv = userdiff_find_by_path(opts->repo->index, p->one->path);\n+\t\tif (drv && drv->binary > 0)\n+\t\t\tcontinue;\n+\n+\t\tif (DIFF_FILE_VALID(p->one) &&\n+\t\t odb_read_object_info_extended(opts->repo->objects,\n+\t\t\t\t\t\t &p->one->oid, NULL,\n+\t\t\t\t\t\t OBJECT_INFO_FOR_PREFETCH))\n+\t\t\toidset_insert(blobs, &p->one->oid);\n+\t\tif (DIFF_FILE_VALID(p->two) &&\n+\t\t odb_read_object_info_extended(opts->repo->objects,\n+\t\t\t\t\t\t &p->two->oid, NULL,\n+\t\t\t\t\t\t OBJECT_INFO_FOR_PREFETCH))\n+\t\t\toidset_insert(blobs, &p->two->oid);\n+\t}\n+\tdiff_queue_clear(q);\n+}\n+\n+static int always_match(const void *cmp_data UNUSED,\n+\t\t\tconst struct hashmap_entry *entry1 UNUSED,\n+\t\t\tconst struct hashmap_entry *entry2 UNUSED,\n+\t\t\tconst void *keydata UNUSED)\n+{\n+\treturn 0;\n+}\n+\n+/*\n+ * Prefetch blobs for git cherry in partial clones.\n+ *\n+ * Called between the revision walk (which builds the head-side\n+ * commit list) and the has_commit_patch_id() comparison loop.\n+ *\n+ * Uses a cmpfn-swap trick to avoid reading blobs: temporarily\n+ * replaces the hashmap's comparison function with a trivial\n+ * always-match function, so hashmap_get()/hashmap_get_next() match\n+ * any entry with the same oidhash bucket. These are the set of oids\n+ * that would trigger patch_id_neq() during normal lookup and cause\n+ * blobs to be read on demand, and we want to prefetch them all at\n+ * once instead.\n+ */\n+static void prefetch_cherry_blobs(struct repository *repo,\n+\t\t\t\t struct commit_list *list,\n+\t\t\t\t struct patch_ids *ids)\n+{\n+\tstruct oidset blobs = OIDSET_INIT;\n+\thashmap_cmp_fn original_cmpfn;\n+\n+\t/* Exit if we're not in a partial clone */\n+\tif (!repo_has_promisor_remote(repo))\n+\t\treturn;\n+\n+\t/* Save original cmpfn, replace with always_match */\n+\toriginal_cmpfn = ids->patches.cmpfn;\n+\tids->patches.cmpfn = always_match;\n+\n+\t/* Find header-only collisions, gather blobs from those commits */\n+\tfor (struct commit_list *l = list; l; l = l->next) {\n+\t\tstruct commit *c = l->item;\n+\t\tbool match_found = false;\n+\t\tfor (struct patch_id *cur = patch_id_iter_first(c, ids);\n+\t\t cur;\n+\t\t cur = patch_id_iter_next(cur, ids)) {\n+\t\t\tmatch_found = true;\n+\t\t\tcollect_diff_blob_oids(cur->commit, &ids->diffopts,\n+\t\t\t\t\t &blobs);\n+\t\t}\n+\t\tif (match_found)\n+\t\t\tcollect_diff_blob_oids(c, &ids->diffopts, &blobs);\n+\t}\n+\n+\t/* Restore original cmpfn */\n+\tids->patches.cmpfn = original_cmpfn;\n+\n+\t/* If we have any blobs to fetch, fetch them */\n+\tif (oidset_size(&blobs)) {\n+\t\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n+\t\tstruct oidset_iter iter;\n+\t\tconst struct object_id *oid;\n+\n+\t\toidset_iter_init(&blobs, &iter);\n+\t\twhile ((oid = oidset_iter_next(&iter)))\n+\t\t\toid_array_append(&to_fetch, oid);\n+\n+\t\tpromisor_remote_get_direct(repo, to_fetch.oid, to_fetch.nr);\n+\n+\t\toid_array_clear(&to_fetch);\n+\t}\n+\n+\toidset_clear(&blobs);\n+}\n+\n int cmd_cherry(int argc,\n \t const char **argv,\n \t const char *prefix,\n@@ -2673,6 +2802,8 @@ int cmd_cherry(int argc,\n \t\tcommit_list_insert(commit, &list);\n \t}\n \n+\tprefetch_cherry_blobs(the_repository, list, &ids);\n+\n \tfor (struct commit_list *l = list; l; l = l->next) {\n \t\tchar sign = '+';\n \ndiff --git a/t/t3500-cherry.sh b/t/t3500-cherry.sh\nindex 78c3eac54b..3e66827d76 100755\n--- a/t/t3500-cherry.sh\n+++ b/t/t3500-cherry.sh\n@@ -78,4 +78,31 @@ test_expect_success 'cherry ignores whitespace' '\n \ttest_cmp expect actual\n '\n \n+# Reuse the expect file from the previous test, in a partial clone\n+test_expect_success 'cherry in partial clone does bulk prefetch' '\n+\ttest_config uploadpack.allowfilter 1 &&\n+\ttest_config uploadpack.allowanysha1inwant 1 &&\n+\ttest_when_finished \"rm -rf copy\" &&\n+\n+\tgit clone --bare --filter=blob:none file://\"$(pwd)\" copy &&\n+\t(\n+\t\tcd copy &&\n+\t\tGIT_TRACE2_EVENT=\"$(pwd)/trace.output\" git cherry upstream-with-space feature-without-space >actual &&\n+\t\ttest_cmp ../expect actual &&\n+\n+\t\tgrep \"child_start.*fetch.negotiationAlgorithm\" trace.output >fetches &&\n+\t\ttest_line_count = 1 fetches &&\n+\t\ttest_trace2_data promisor fetch_count 4 actual &&\n+\t\ttest_cmp ../expect actual &&\n+\n+\t\t! grep \"child_start.*fetch.negotiationAlgorithm\" trace2.output &&\n+\t\t! grep \"\\\"key\\\":\\\"fetch_count\\\"\" trace2.output\n+\t)\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"543340","messageId":"75d4ca7cff07a14b2f0beef4524623e541e140a8.1778775928.git.gitgitgadget@gmail.com","threadId":"65502","inReplyTo":"pull.2089.v3.git.1778775928.gitgitgadget@gmail.com","subject":"[PATCH v3 4/4] grep: prefetch necessary blobs","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-05-14T16:25:28Z","receivedAt":"2026-05-14T16:25:36Z","isPatch":true,"body":"From: Elijah Newren \n\nIn partial clones, `git grep` fetches necessary blobs on-demand one\nat a time, which can be very slow. In partial clones, add an extra\npreliminary walk over the tree similar to grep_tree() which collects\nthe blobs of interest, and then prefetches them.\n\nSigned-off-by: Elijah Newren \n---\n builtin/grep.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++\n t/t7810-grep.sh | 58 ++++++++++++++++++++\n 2 files changed, 201 insertions(+)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex e33285e5e6..85656d8d3f 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -28,9 +28,12 @@\n #include \"object-file.h\"\n #include \"object-name.h\"\n #include \"odb.h\"\n+#include \"oid-array.h\"\n+#include \"oidset.h\"\n #include \"packfile.h\"\n #include \"pager.h\"\n #include \"path.h\"\n+#include \"promisor-remote.h\"\n #include \"read-cache-ll.h\"\n #include \"write-or-die.h\"\n \n@@ -692,6 +695,144 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \treturn hit;\n }\n \n+static void collect_blob_oids_for_tree(struct repository *repo,\n+\t\t\t\t const struct pathspec *pathspec,\n+\t\t\t\t struct tree_desc *tree,\n+\t\t\t\t struct strbuf *base,\n+\t\t\t\t int tn_len,\n+\t\t\t\t struct oidset *blob_oids)\n+{\n+\tstruct name_entry entry;\n+\tint old_baselen = base->len;\n+\tstruct strbuf name = STRBUF_INIT;\n+\tenum interesting match = entry_not_interesting;\n+\n+\twhile (tree_entry(tree, &entry)) {\n+\t\tif (match != all_entries_interesting) {\n+\t\t\tstrbuf_addstr(&name, base->buf + tn_len);\n+\t\t\tmatch = tree_entry_interesting(repo->index,\n+\t\t\t\t\t\t &entry, &name,\n+\t\t\t\t\t\t pathspec);\n+\t\t\tstrbuf_reset(&name);\n+\n+\t\t\tif (match == all_entries_not_interesting)\n+\t\t\t\tbreak;\n+\t\t\tif (match == entry_not_interesting)\n+\t\t\t\tcontinue;\n+\t\t}\n+\n+\t\tstrbuf_add(base, entry.path, tree_entry_len(&entry));\n+\n+\t\tif (S_ISREG(entry.mode)) {\n+\t\t\tif (!odb_has_object(repo->objects, &entry.oid, 0))\n+\t\t\t\toidset_insert(blob_oids, &entry.oid);\n+\t\t} else if (S_ISDIR(entry.mode)) {\n+\t\t\tenum object_type type;\n+\t\t\tstruct tree_desc sub_tree;\n+\t\t\tvoid *data;\n+\t\t\tunsigned long size;\n+\n+\t\t\tdata = odb_read_object(repo->objects, &entry.oid,\n+\t\t\t\t\t &type, &size);\n+\t\t\tif (!data)\n+\t\t\t\tdie(_(\"unable to read tree (%s)\"),\n+\t\t\t\t oid_to_hex(&entry.oid));\n+\n+\t\t\tstrbuf_addch(base, '/');\n+\t\t\tinit_tree_desc(&sub_tree, &entry.oid, data, size);\n+\t\t\tcollect_blob_oids_for_tree(repo, pathspec, &sub_tree,\n+\t\t\t\t\t\t base, tn_len, blob_oids);\n+\t\t\tfree(data);\n+\t\t}\n+\t\t/*\n+\t\t * ...no else clause for S_ISGITLINK: submodules have their\n+\t\t * own promisor configuration and would need separate fetches\n+\t\t * anyway.\n+\t\t */\n+\n+\t\tstrbuf_setlen(base, old_baselen);\n+\t}\n+\n+\tstrbuf_release(&name);\n+}\n+\n+static void collect_blob_oids_for_treeish(struct grep_opt *opt,\n+\t\t\t\t\t const struct pathspec *pathspec,\n+\t\t\t\t\t const struct object_id *tree_ish_oid,\n+\t\t\t\t\t const char *name,\n+\t\t\t\t\t struct oidset *blob_oids)\n+{\n+\tstruct tree_desc tree;\n+\tvoid *data;\n+\tunsigned long size;\n+\tstruct strbuf base = STRBUF_INIT;\n+\tint len;\n+\n+\tdata = odb_read_object_peeled(opt->repo->objects, tree_ish_oid,\n+\t\t\t\t OBJ_TREE, &size, NULL);\n+\n+\tif (!data)\n+\t\treturn;\n+\n+\tlen = name ? strlen(name) : 0;\n+\tif (len) {\n+\t\tstrbuf_add(&base, name, len);\n+\t\tstrbuf_addch(&base, ':');\n+\t}\n+\tinit_tree_desc(&tree, tree_ish_oid, data, size);\n+\n+\tcollect_blob_oids_for_tree(opt->repo, pathspec, &tree,\n+\t\t\t\t &base, base.len, blob_oids);\n+\n+\tstrbuf_release(&base);\n+\tfree(data);\n+}\n+\n+static void prefetch_grep_blobs(struct grep_opt *opt,\n+\t\t\t\tconst struct pathspec *pathspec,\n+\t\t\t\tconst struct object_array *list)\n+{\n+\tstruct oidset blob_oids = OIDSET_INIT;\n+\n+\t/* Exit if we're not in a partial clone */\n+\tif (!repo_has_promisor_remote(opt->repo))\n+\t\treturn;\n+\n+\t/* For each tree, gather the blobs in it */\n+\tfor (int i = 0; i < list->nr; i++) {\n+\t\tstruct object *real_obj;\n+\n+\t\tobj_read_lock();\n+\t\treal_obj = deref_tag(opt->repo, list->objects[i].item,\n+\t\t\t\t NULL, 0);\n+\t\tobj_read_unlock();\n+\n+\t\tif (real_obj &&\n+\t\t (real_obj->type == OBJ_COMMIT ||\n+\t\t real_obj->type == OBJ_TREE))\n+\t\t\tcollect_blob_oids_for_treeish(opt, pathspec,\n+\t\t\t\t\t\t &real_obj->oid,\n+\t\t\t\t\t\t list->objects[i].name,\n+\t\t\t\t\t\t &blob_oids);\n+\t}\n+\n+\t/* Prefetch the blobs we found */\n+\tif (oidset_size(&blob_oids)) {\n+\t\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n+\t\tstruct oidset_iter iter;\n+\t\tconst struct object_id *oid;\n+\n+\t\toidset_iter_init(&blob_oids, &iter);\n+\t\twhile ((oid = oidset_iter_next(&iter)))\n+\t\t\toid_array_append(&to_fetch, oid);\n+\n+\t\tpromisor_remote_get_direct(opt->repo, to_fetch.oid, to_fetch.nr);\n+\n+\t\toid_array_clear(&to_fetch);\n+\t}\n+\toidset_clear(&blob_oids);\n+}\n+\n static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t struct object *obj, const char *name, const char *path)\n {\n@@ -732,6 +873,8 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,\n \tint hit = 0;\n \tconst unsigned int nr = list->nr;\n \n+\tprefetch_grep_blobs(opt, pathspec, list);\n+\n \tfor (i = 0; i < nr; i++) {\n \t\tstruct object *real_obj;\n \ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 64ac4f04ee..3d08fd2a0c 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -1929,4 +1929,62 @@ test_expect_success 'grep does not report i-t-a and assume unchanged with -L' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'grep of revision in partial clone batches prefetch and honors pathspec' '\n+\ttest_when_finished \"rm -rf grep-partial-src grep-partial\" &&\n+\n+\tgit init grep-partial-src &&\n+\t(\n+\t\tcd grep-partial-src &&\n+\t\tgit config uploadpack.allowfilter 1 &&\n+\t\tgit config uploadpack.allowanysha1inwant 1 &&\n+\t\tmkdir a b &&\n+\t\techo \"needle in haystack\" >a/matches.txt &&\n+\t\techo \"nothing to see here\" >a/nomatch.txt &&\n+\t\techo \"needle again\" >b/matches.md &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"initial\"\n+\t) &&\n+\n+\tgit clone --no-checkout --filter=blob:none \\\n+\t\t\"file://$(pwd)/grep-partial-src\" grep-partial &&\n+\n+\t# All three blobs are missing immediately after a blobless clone.\n+\tgit -C grep-partial rev-list --quiet --objects \\\n+\t\t--missing=print HEAD >missing &&\n+\ttest_line_count = 3 missing &&\n+\n+\t# A pathspec-limited grep should prefetch only the two blobs\n+\t# in a/. It should fetch both blobs in one batched request.\n+\tGIT_TRACE2_EVENT=\"$(pwd)/grep-trace-pathspec\" \\\n+\t\tgit -C grep-partial grep -c \"needle\" HEAD -- \"a/*.txt\" >result &&\n+\n+\t# Only a/matches.txt contains \"needle\" among the matched paths.\n+\ttest_line_count = 1 result &&\n+\n+\t# Exactly the two a/*.txt blobs should have been requested, and\n+\t# the server packed those two objects in the response.\n+\ttest_trace2_data promisor fetch_count 2 missing &&\n+\ttest_line_count = 1 missing &&\n+\n+\t# A second grep without a pathspec must recurse into both\n+\t# subdirectories, but should request only the still-missing blob\n+\t# from the promisor.\n+\tGIT_TRACE2_EVENT=\"$(pwd)/grep-trace-all\" \\\n+\t\tgit -C grep-partial grep -c \"needle\" HEAD >result &&\n+\n+\ttest_line_count = 2 result &&\n+\ttest_trace2_data promisor fetch_count 1 missing &&\n+\ttest_line_count = 0 missing\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"543528","messageId":"9ff558a4-1be8-4a77-999a-b32c1812e4de@gmail.com","threadId":"65502","inReplyTo":"CABPp-BGpXgDfJeDEB91U-h092-8L6Q_MLrzSLFg9HotPDZ-m-g@mail.gmail.com","subject":"Re: [PATCH v2 2/3] builtin/log: prefetch necessary blobs for `git cherry`","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-05-18T12:14:03Z","receivedAt":"2026-05-18T12:14:05Z","isPatch":true,"body":"On 5/13/2026 7:17 PM, Elijah Newren wrote:\n> On Mon, Apr 27, 2026 at 6:17 AM Derrick Stolee wrote:\n>>\n>> On 4/17/2026 8:32 PM, Elijah Newren via GitGitGadget wrote:\n>>> From: Elijah Newren \n>>> +static void collect_diff_blob_oids(struct commit *commit,\n>>> + struct diff_options *opts,\n>>> + struct oidset *blobs)\n>>\n>> I think that this is generally a good idea, though I worry that\n>> having this hidden in builtin/log.c may not be the right long-\n>> term home.\n>>\n>> I expect that we'll find more and more examples where we want to\n>> prefetch blobs in different operations, those that exist now and\n>> those that may be created in the future. It would be preferred if\n>> they could automatically take advantage of the logic already in\n>> diff_queued_diff_prefetch() within diffcore_std() in diff.c.\n>>\n>> Ultimately, _this_ patch cares about a diff.\n> \n> I read this patch a bit differently -- could you say more about what\n> you have in mind?\n> \n> The body of collect_diff_blob_oids() really is just diff_tree_oid() +\n> diffcore_std() + process each pair, so at the per-commit level I am\n> already leaning on the diff library. One of the things this patch\n> adds is accumulation across many commits: the containing loop (in\n> prefetch_cherry_blobs) is over a commit range, not over a single diff.\n> \n> Concretely, the motivating case was a patch touching a few files where\n> upstream had tens of thousands of commits in .., several\n> hundred of which modified the same set of files. A per-diff prefetch\n> like diff.c uses would turn that into hundreds of small fetches of 1-3\n> blobs each; what this series gives you is one fetch. So the win\n> really does live above the diff library, not inside it.\n\nMy initial thought was about finding what we can abstract into the\ndiff API for later reuse. Upon rereading, it's clear that this is\ntied very closely with the --cherry feature and wouldn't make a lot\nof sense in the API layer.\n\n> There are two further wrinkles in cherry that are filters layered on\n> top of the cross-commit accumulation, and they're cherry-specific in a\n> way that I don't think belongs in the diff library:\n> \n> 1. For most commits in .., cherry doesn't care about\n> the diff at all -- if the list of files modified doesn't exactly match\n> the commit of interest, the commit is skipped before patch-id is even\n> computed. Prefetching for those would be wasted.\n> \n> 2. We skip prefetching content for binary files (because patch-id\n> uses oid_to_hex() for such files instead of the diff contents).\n> \n>> Could we compute a\n>> \"diff prep\" computation using the core diff library instead of\n>> inventing a second queue of results for diffing?\n> \n> To check this concretely I looked at each of the existing\n> promisor_remote_get_direct() callsites for a similar producer. The\n> closest cousin of collect_diff_blob_oids() (the only part of this\n> patch that looks like it might be close to the right shape to put in a\n> core diff library) is diff.c's diff_queued_diff_prefetch() -- but it\n> operates on the already-populated global diff_queued_diff and fetches\n> immediately, rather than setting up the diff itself and returning an\n> oidset for the caller to accumulate. Reshaping it to match cherry's\n> needs would either break its current caller in diffcore_std() or\n> introduce a parallel function whose only consumer is cherry. None of\n> the other sites (path-walk in backfill, index walk in read-cache,\n> three-way state in merge-ort, etc.) do anything resembling \"diff two\n> trees and harvest oids.\"\n> \n> And even if we did factor a helper out, cherry's filter is\n> patch-id-specific: commit_patch_id() substitutes oid_to_hex() for\n> files marked binary by their userdiff driver, so we deliberately skip\n> prefetching those. That isn't a generic \"diff prep\" consideration --\n> it only makes sense because the caller is patch-id. We could express\n> it as a predicate parameter, but with one caller that would feel to me\n> like it's just pushing cherry's policy across an API boundary for no\n> gain.\n\nThanks for the additional context. I agree with your assessment.\n\n>> Patch 3 cares about a \"scan prep\" which cares about loading all\n>> blobs for a given tree with respect to a pathspec. This is very\n>> similar to what a checkout would do, though it ultimately uses\n>> a form of diff to find out what change should be applied to the\n>> working directory. Perhaps 'git archive' is a better matching\n>> example.\n> \n> Agreed that archive is the closer analog -- both grep and archive do a\n> pathspec-filtered single-tree walk, whereas checkout's prefetch is\n> tied to the index and optimizes to the subset of paths that are\n> different since the previous version checked out. Retrofitting that\n> to grep would mean materializing an index for the target revision just\n> to throw it away, which feels like more machinery to bridge the\n> abstractions than the walk itself would take.\n\nMakes sense.\n\n>> By implementing things in a\n>> common location, then we can have later integrations add to the\n>> confidence in the feature through tests covering each user-facing\n>> use.\n> \n> Sounds great...but what common user-facing uses exist?\n> \n> Looking at the existing 11 callsites of promisor_remote_get_direct()\n> after this series [1], each has pretty specialized data needs --\n> index-driven (read-cache), index-pack & pack-objects internals,\n> path-walk batches (backfill), merge-ort's three-way logic,\n> diffcore-rename's two independent rename-detection paths, plain old\n> diffs, collection across a subset of commits (cherry),\n> pathspec-filtered tree walk (grep), and\n> on-demand-single-blob-at-a-time (odb.c) -- so I don't see a natural\n> shared layer above the primitive itself (which is already\n> promisor_remote_get_direct).\n> \n> archive, if it had prefetch logic, would be the first match. But it's\n> not clear where the shared logic between grep and archive would live,\n> if archive even had any prefetch logic to share.\n> \n> So I'm inclined to leave both new producers local to their builtins\n> for now, and factor a tree-walk helper when archive (or a third\n> caller) actually wants one. But I'm happy to be told I've missed the\n> boat.\n\nNo, clearly I missed the boat. Thanks for giving me insight to your\ndeep understanding of this area. You executed on a good design based\non the right amount of specialization required for this need.\n\nThanks,\n-Stolee\n\n\n"},{"id":"543529","messageId":"0da4f159-8d4b-49e2-93c1-25aa0bf69371@gmail.com","threadId":"65502","inReplyTo":"pull.2089.v3.git.1778775928.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/4] Batch prefetching","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-05-18T12:17:15Z","receivedAt":"2026-05-18T12:17:17Z","isPatch":true,"body":"On 5/14/2026 12:25 PM, Elijah Newren via GitGitGadget wrote:\n> Changes since v2:\n> \n> * Modified the final patch as suggested by Stolee to include pathspec usage\n> in the testcase\n> * Modified the last two patches to not re-download blobs we already have\n> locally, and adjusted the tests to verify\n> * Inserted a new first patch, containing a documentation addition that\n> would have helped me avoid making the above mistake in the first place.\n\nThank you for these changes. I reviewed the updates and documentation and\nthink this version is good to go. \n> Note: Stolee also suggest some code sharing or code movement in his review\n> of v2 2/3, but possibly based on a misunderstanding of v2 2/3 (that patch\n> isn't about a diff) and it's not clear to me what could be shared or moved,\n> so that's not part of this round.\n\nYour detailed responses in the v2 thread helped me understand that my thought\nwas misguided. Thanks for giving me extra confidence in your approach here.\n\n-Stolee\n\n"},{"id":"543532","messageId":"xmqqecj8c2fk.fsf@gitster.g","threadId":"65502","inReplyTo":"0da4f159-8d4b-49e2-93c1-25aa0bf69371@gmail.com","subject":"Re: [PATCH v3 0/4] Batch prefetching","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-05-18T12:44:31Z","receivedAt":"2026-05-18T12:44:33Z","isPatch":true,"body":"Derrick Stolee writes:\n\n> On 5/14/2026 12:25 PM, Elijah Newren via GitGitGadget wrote:\n>> Changes since v2:\n>> \n>> * Modified the final patch as suggested by Stolee to include pathspec usage\n>> in the testcase\n>> * Modified the last two patches to not re-download blobs we already have\n>> locally, and adjusted the tests to verify\n>> * Inserted a new first patch, containing a documentation addition that\n>> would have helped me avoid making the above mistake in the first place.\n>\n> Thank you for these changes. I reviewed the updates and documentation and\n> think this version is good to go. \n>> Note: Stolee also suggest some code sharing or code movement in his review\n>> of v2 2/3, but possibly based on a misunderstanding of v2 2/3 (that patch\n>> isn't about a diff) and it's not clear to me what could be shared or moved,\n>> so that's not part of this round.\n>\n> Your detailed responses in the v2 thread helped me understand that my thought\n> was misguided. Thanks for giving me extra confidence in your approach here.\n\nThanks, both. I do agree that the series is in a good shape. Let\nme mark the topic for 'next'.\n"},{"id":"543544","messageId":"CABPp-BHzVW9zf6kzfrpcWBny3bW_J0KhkVkg5+RYiQ8ymv+OdA@mail.gmail.com","threadId":"65502","inReplyTo":"0da4f159-8d4b-49e2-93c1-25aa0bf69371@gmail.com","subject":"Re: [PATCH v3 0/4] Batch prefetching","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2026-05-18T16:55:53Z","receivedAt":"2026-05-18T16:56:06Z","isPatch":true,"body":"On Mon, May 18, 2026 at 5:17 AM Derrick Stolee wrote:\n>\n> On 5/14/2026 12:25 PM, Elijah Newren via GitGitGadget wrote:\n> > Changes since v2:\n> >\n> > * Modified the final patch as suggested by Stolee to include pathspec usage\n> > in the testcase\n> > * Modified the last two patches to not re-download blobs we already have\n> > locally, and adjusted the tests to verify\n> > * Inserted a new first patch, containing a documentation addition that\n> > would have helped me avoid making the above mistake in the first place.\n>\n> Thank you for these changes. I reviewed the updates and documentation and\n> think this version is good to go.\n\nAs always, thanks for reviewing! Your comments on patch 3 in\nparticular led me to what would have been a rather annoying bug, so\nthanks for calling out an improvement to the testcase that alerted me\nto that issue.\n\n> > Note: Stolee also suggest some code sharing or code movement in his review\n> > of v2 2/3, but possibly based on a misunderstanding of v2 2/3 (that patch\n> > isn't about a diff) and it's not clear to me what could be shared or moved,\n> > so that's not part of this round.\n>\n> Your detailed responses in the v2 thread helped me understand that my thought\n> was misguided. Thanks for giving me extra confidence in your approach here.\n\nI can totally see where the comments came from; they did seem logical\non the surface. I knew the changes were cherry-specific in a few\nways, but trying to figure out how to explain that and dig further\ninto the details to find a good angle (and try to make sure I wasn't\njust missing something about how part of the logic could be shared)\ntook a bunch of additional time, so I'm happy to hear that others\nconsider it enlightening. That makes it time well spent.\n"}]}