Re: [PATCH v2 2/3] builtin/log: prefetch necessary blobs for `git cherry`
From: Elijah Newren <hidden>
Date: 2026-05-13 23:17:41
Hi, Sorry for the long delay. Lots of firefighting of incidents kept me away for a bit... On Mon, Apr 27, 2026 at 6:17 AM Derrick Stolee [off-list ref] wrote:
On 4/17/2026 8:32 PM, Elijah Newren via GitGitGadget wrote:quoted
From: Elijah Newren <redacted>(I'm sorry that I'm reviewing out of order. This reply includes my feelings about patch 3 after reading both.)
Thanks for taking a look! And I have no problems with reviewing out of order (unless the comments on later patches don't make sense due to the reviewer being unaware of previous patches, which isn't the case here).
quoted
+/* + * Enumerate blob OIDs from a single commit's diff, inserting them into blobs. + * Skips files whose userdiff driver explicitly declares binary status + * (drv->binary > 0), since patch-ID uses oid_to_hex() for those and + * never reads blob content. Use userdiff_find_by_path() since + * diff_filespec_load_driver() is static in diff.c. + * + * Clean up with diff_queue_clear() (from diffcore.h). + */ +static void collect_diff_blob_oids(struct commit *commit, + struct diff_options *opts, + struct oidset *blobs)I think that this is generally a good idea, though I worry that having this hidden in builtin/log.c may not be the right long- term home. I expect that we'll find more and more examples where we want to prefetch blobs in different operations, those that exist now and those that may be created in the future. It would be preferred if they could automatically take advantage of the logic already in diff_queued_diff_prefetch() within diffcore_std() in diff.c. Ultimately, _this_ patch cares about a diff.
I read this patch a bit differently -- could you say more about what you have in mind? The body of collect_diff_blob_oids() really is just diff_tree_oid() + diffcore_std() + process each pair, so at the per-commit level I am already leaning on the diff library. One of the things this patch adds is accumulation across many commits: the containing loop (in prefetch_cherry_blobs) is over a commit range, not over a single diff. Concretely, the motivating case was a patch touching a few files where upstream had tens of thousands of commits in <limit>..<head>, several hundred of which modified the same set of files. A per-diff prefetch like diff.c uses would turn that into hundreds of small fetches of 1-3 blobs each; what this series gives you is one fetch. So the win really does live above the diff library, not inside it. There are two further wrinkles in cherry that are filters layered on top of the cross-commit accumulation, and they're cherry-specific in a way that I don't think belongs in the diff library: 1. For most commits in <limit>..<head>, cherry doesn't care about the diff at all -- if the list of files modified doesn't exactly match the commit of interest, the commit is skipped before patch-id is even computed. Prefetching for those would be wasted. 2. We skip prefetching content for binary files (because patch-id uses oid_to_hex() for such files instead of the diff contents).
Could we compute a "diff prep" computation using the core diff library instead of inventing a second queue of results for diffing?
To check this concretely I looked at each of the existing promisor_remote_get_direct() callsites for a similar producer. The closest cousin of collect_diff_blob_oids() (the only part of this patch that looks like it might be close to the right shape to put in a core diff library) is diff.c's diff_queued_diff_prefetch() -- but it operates on the already-populated global diff_queued_diff and fetches immediately, rather than setting up the diff itself and returning an oidset for the caller to accumulate. Reshaping it to match cherry's needs would either break its current caller in diffcore_std() or introduce a parallel function whose only consumer is cherry. None of the other sites (path-walk in backfill, index walk in read-cache, three-way state in merge-ort, etc.) do anything resembling "diff two trees and harvest oids." And even if we did factor a helper out, cherry's filter is patch-id-specific: commit_patch_id() substitutes oid_to_hex() for files marked binary by their userdiff driver, so we deliberately skip prefetching those. That isn't a generic "diff prep" consideration -- it only makes sense because the caller is patch-id. We could express it as a predicate parameter, but with one caller that would feel to me like it's just pushing cherry's policy across an API boundary for no gain.
Patch 3 cares about a "scan prep" which cares about loading all blobs for a given tree with respect to a pathspec. This is very similar to what a checkout would do, though it ultimately uses a form of diff to find out what change should be applied to the working directory. Perhaps 'git archive' is a better matching example.
Agreed that archive is the closer analog -- both grep and archive do a pathspec-filtered single-tree walk, whereas checkout's prefetch is tied to the index and optimizes to the subset of paths that are different since the previous version checked out. Retrofitting that to grep would mean materializing an index for the target revision just to throw it away, which feels like more machinery to bridge the abstractions than the walk itself would take.
By implementing things in a common location, then we can have later integrations add to the confidence in the feature through tests covering each user-facing use.
Sounds great...but what common user-facing uses exist? Looking at the existing 11 callsites of promisor_remote_get_direct() after this series [1], each has pretty specialized data needs -- index-driven (read-cache), index-pack & pack-objects internals, path-walk batches (backfill), merge-ort's three-way logic, diffcore-rename's two independent rename-detection paths, plain old diffs, collection across a subset of commits (cherry), pathspec-filtered tree walk (grep), and on-demand-single-blob-at-a-time (odb.c) -- so I don't see a natural shared layer above the primitive itself (which is already promisor_remote_get_direct). archive, if it had prefetch logic, would be the first match. But it's not clear where the shared logic between grep and archive would live, if archive even had any prefetch logic to share. So I'm inclined to leave both new producers local to their builtins for now, and factor a tree-walk helper when archive (or a third caller) actually wants one. But I'm happy to be told I've missed the boat. Thanks, Elijah [1] builtin/backfill.c, builtin/grep.c, builtin/index-pack.c, builtin/log.c, builtin/pack-objects.c, diff.c, diffcore-rename.c (two callsites), merge-ort.c, odb.c, read-cache.c