[PATCH] technical/partial-clone: narrow the one-at-a-time fetch limitation

[Aleksei Sviridkin <f@lex.la>]

The design doc says that dynamic object fetching invokes fetch-pack once
for each item, and that it tends to be slow because objects are fetched
one at a time.  Neither passage says which code paths this applies to.

promisor_remote_get_direct() takes an array of object IDs and hands the
whole array to a single fetch subprocess per promisor remote it tries.
Ten of its eleven call sites pass a collected list: builtin/backfill.c,
builtin/grep.c, builtin/index-pack.c, builtin/log.c,
builtin/pack-objects.c, diff.c, diffcore-rename.c twice, merge-ort.c
and read-cache.c.  The eleventh, in odb.c, passes a single object and is
the fallback taken when a lookup fails with no batch fetch in front of
it.  Through builtin/pack-objects.c, a push out of a blobless clone into
a destination with no shared history issues one fetch carrying every
missing blob.

The bullet describes that fallback, and the commit that added it records
checkout as an exception in the same file.  v2.17.0, the first release
with dynamic fetching, had one batching caller beside the fallback, and
nine more were added over the following releases, the last two in
v2.55.0.  7ca3c0ac37 (promisor-remote: lazy-fetch objects in subprocess,
2020-08-17) is why that bullet still names fetch-pack: it moved the fetch
into a "git fetch" subprocess and rewrote the "Fetching Missing Objects"
section to match, without touching the limitation.

Scope the limitation to the fallback, describe the batch path where the
fallback is introduced, and name the subprocess the code actually forks,
as this file already does where it describes fetching.

Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <f@lex.la>
---

Notes:
    Verified against origin/maint (e9019fcafe, Git 2.55).
    
    Call sites of promisor_remote_get_direct(), with the last argument
    shown, from a grep over *.c with the definition excluded:
    
        builtin/backfill.c:49        ctx->current_batch.nr
        builtin/grep.c:829           to_fetch.nr
        builtin/index-pack.c:1509    to_fetch.nr
        builtin/log.c:2726           to_fetch.nr
        builtin/pack-objects.c:2244  to_fetch.nr
        diff.c:7471                  to_fetch.nr
        diffcore-rename.c:128        to_fetch.nr
        diffcore-rename.c:899        to_fetch.nr
        merge-ort.c:4484             to_fetch.nr
        read-cache.c:3736            to_fetch.nr
        odb.c:601                    1
    
    The batch entry point.  promisor_remote_get_direct() calls
    fetch_objects() once per promisor remote it tries, and fetch_objects()
    spawns one `git fetch --stdin` and writes the whole array to it.  A
    second request happens only when a remote fails, and carries the
    objects still missing.  The function returns void, and when every
    remote has failed it die()s only for oids that is_promisor_object()
    confirms, otherwise returning with nothing fetched -- so a batch
    having asked for an object does not mean the object arrived.
    
    Why the text states no count and names no callers.  The batching call
    sites do not partition by mechanism, so no "N of them do X" sentence
    is true.  diff.c:7471 is reached both directly from diffcore_std
    (diff.c:7502) and as missing_object_cb (diff.c:7362,
    diffcore-break.c:73), the latter firing from diff.c:4615 only after a
    lookup carrying OBJECT_INFO_SKIP_FETCH_OBJECT has failed.
    diffcore-rename.c's two are that same callback mechanism.
    builtin/pack-objects.c:2244 is neither: prefetch_to_pack() is a direct
    call from the miss branch of check_object() (:2392) and collects from
    the pack list rather than from the misses.  builtin/backfill.c:49
    fires from the path walk whenever current_batch.nr reaches
    min_batch_size.  A timing claim fails too: only checkout collects
    before the work starts.  builtin/grep.c:829 fires after
    collect_blob_oids_for_treeish() has walked the treeish,
    merge-ort.c:4537 calls prefetch_for_content_merges() from inside
    process_entries(), and builtin/index-pack.c:1509 sits in
    fix_unresolved_deltas() (reached at :1391 after the pack is parsed)
    and prefetches delta bases rather than the objects being indexed.
    
    Reproduction.  Server: 20 commits x 100 files, every file rewritten
    each commit, 2000 distinct blobs, uploadpack.allowFilter=true.
    Client: git clone --filter=blob:none --no-local --no-checkout, then
    the trace2 event fetch_objects() already emits, so one event is one
    subprocess and its value is the request size:
    
        git checkout main                      1 request   100 objects
        git grep -e unique main -- .           1 request   100 objects
        git log --patch main                  19 requests  200 then 100 x18
        git push <empty dest> main             1 request  2000 objects
        git cat-file --batch < 200 oids      200 requests    1 object each
        git checkout-index --all             100 requests    1 object each
    
    Second server, one file grown by a line per commit over 30 commits, to
    make blame walk the whole history:
    
        git blame main -- grow.txt            30 requests    1 object each
        git log --patch main -- grow.txt      29 requests   28x1 and 1x2
    
    Two things follow.  blame is porcelain and reaches the fallback one
    object at a time, faulting in one object per revision it walks --
    neither blame.c nor builtin/blame.c contains a prefetch,
    fill_origin_blob() goes straight to odb_read_object(), and
    diffcore_std()'s prefetch gate never fires for blame's internal
    diff_opts because it needs one of DIFF_FORMAT_{DIFFSTAT,NUMSTAT,PATCH,
    SHORTSTAT,DIRSTAT} or a pickaxe.  So the remaining limitation is not
    confined to plumbing.  Do not state a total for it: the run above used
    --no-checkout and saw 30, while the same 30-commit history in a
    default `git clone --filter=blob:none` sees 29, because the clone's
    own checkout batch-prefetches the HEAD blob.  The rate is the durable
    fact, not the count -- which is why the commit message asserts
    neither.
    
    And a batching call site is routinely reached with a one-element list:
    pathspec-limited `log --patch` diffs one file per commit, so diff.c's
    to_fetch.nr is 1 almost every time.  The per-request size a batching
    caller achieves depends on how much work the command has in front of
    it, not on the call site, which is why the text keys on whether a
    batch fetch covers the object rather than on how many objects it
    carries.
    
    Release in which each batching call site first shipped:
    
        v2.17.0  unpack-trees (c0c578b33c), today read-cache.c
        v2.22.0  diff.c (7fbbcb21b1)
        v2.23.0  builtin/index-pack.c (8a30a1efd1)
        v2.27.0  diffcore-rename.c inexact_prefetch (95acf11a3d)
        v2.29.0  builtin/pack-objects.c (e00549aa9b)
        v2.33.0  diffcore-rename.c basename prefetch (1aedd03afb)
        v2.33.0  merge-ort.c (2bff554b23)
        v2.49.0  builtin/backfill.c (1e72e889e7)
        v2.55.0  builtin/grep.c (854061ea54)
        v2.55.0  builtin/log.c for `git cherry` (463c1bfc2b)
    
    The design doc commit 637fc4467e shipped in v2.16.0, one release
    before the lazy-fetch machinery itself (88e2f9ed8e, v2.17.0), and
    already recorded checkout as an exception to the bullet it added.
    
    Pre-existing, each wanting its own patch, none touched here.
    
    - The V0 bullet's premise and its consequence are both stale, and the
      consequence is false on two independent grounds.  Premise:
      protocol.c:46 returns protocol_v2 when protocol.version is unset
      (eb049759fb, v2.29.0) and fetch_objects() sets no override.
      Consequence: under protocol_v2, handshake() in transport.c calls
      get_remote_refs() only `if (must_list_refs)`, and
      fetch_refs_via_pack() leaves must_list_refs at 0 when every
      to_fetch entry has exact_oid (transport.c:472-480) -- which the
      lazy fetch always does, because its hex-oid refspecs set exact_sha1
      (refspec.c:105) and so exact_oid (remote.c:2093).  Measured it:
      GIT_TRACE_PACKET on a blame that triggers 30 lazy fetches shows 60
      `command=fetch` packets, zero `command=ls-refs`, and zero refs/
      advertisement lines.  So no ref advertisement happens at all on this
      path, let alone a full one.  One release invalidated both halves of
      that bullet: 7ca3c0ac37 and eb049759fb are both v2.29.0.
    - The Future Work item at line 291, "Investigate pack protocol V2 to
      avoid the info/refs broadcast on each connection", was done by
      eb049759fb.
    - protocol.h:34-35 documents the opposite of the code: "If
      unconfigured, a value of 'protocol_v0' is returned" against
      protocol.c:46 returning protocol_v2.
    - The `rev-list --missing=print` bullet suggests `git log -p A..B` may
      want to prefetch in bulk itself; diffcore_std() has done that since
      7fbbcb21b1 (v2.22.0) whenever the output format needs blob data.
    - prefetch_to_pack() is reached with an index into the wrong array.
      get_object_details() fills sorted_by_offset[i] with to_pack.objects
      + i, QSORTs by pack_offset_sort, then calls check_object(entry, i)
      with the sorted counter, which prefetch_to_pack() uses to index
      to_pack.objects.  The correct index is entry - to_pack.objects.  The
      push measurement above survives this only because pack_offset_sort
      compares IN_PACK() pointers, so objects in no local pack sort first
      and the first miss lands at index 0.  No reproducer built, so read
      it as "the batch can miss objects it meant to cover".
    - "once *for each item*", kept from the base text, is strictly at
      least once per item: a failing remote sends the same single oid on
      to the next remote (promisor-remote.c:280).
    - The push sentence in the commit message is scenario-specific, not a
      statement of the code's condition.  prefetch_to_pack() fires on any
      failed odb_read_object_info_extended() for an object being packed,
      with no shared-history test, and it collects the pack list from the
      trigger index onward rather than "every missing blob".  The upstream
      test 'prefetch objects' in t/t5300-pack-object.sh, added by
      e00549aa9b, is the same scenario with a single-batch assertion and
      would be the firmer anchor.
    
    Declined, with reasons.  Restoring "without having asked for it in
    advance" to the limitation bullet is refuted, not merely declined:
    builtin/pack-objects.c and diffcore-rename.c's two callbacks both
    discover their misses mid-flight and still batch, so that clause is
    the exact false discriminator earlier rounds removed.  Shortening the
    ten-path list in the commit message is declined because a "ten of
    eleven" count without its evidence is the set claim a reviewer asks to
    see verified.  Double-quoting "git fetch" rather than backticking it
    follows the two pre-existing occurrences in this same file, at lines
    176 and 332, under CodingGuidelines' instruction to imitate existing
    conventions.
    
    On the claim that the V0 rot predates 7ca3c0ac37, which is wrong and
    worth recording because of how easy it is to reach.  A `git log -S` on
    a feature name finds where it landed and not where it was reverted, so
    a feature that landed, was reverted and re-landed reads as
    continuously present.  Reading the tagged trees is what shows it.
    684ceae32d did default
    fetch to v2 in v2.26.0, but 11c7f2a30b reverted it in v2.27.0.  Read
    straight off the tagged trees, get_protocol_version_config() returns
    protocol_v2 in v2.26.0, protocol_v0 in v2.27.0 and v2.28.0, and
    protocol_v2 again in v2.29.0 via eb049759fb (2020-09-25).  So when
    7ca3c0ac37 landed on 2020-08-17 the default was v0 and the bullet's
    premise was still true; eb049759fb falsified it five weeks later in
    the same release.  The citation in the commit message attributes only
    the fetch-pack naming to 7ca3c0ac37, never the V0 premise.
    
    Base branch: maint, deliberately.  SubmittingPatches' "Choose a
    starting point" splits on fixing bugs in the released version against
    adding features, not on regression against clarification, and a
    documented statement that misdescribes released behaviour is a bug in
    the released version.  For a maintainer who prefers master,
    retargeting is free: the patch applies cleanly there (git apply
    --check --3way), and master's one newer change to this file,
    508ec9837c (repository: move fetch_if_missing into struct
    repository), touches a single line in a different bullet.
    
    The V0 bullet admits no in-scope fix, and the two states are worth
    recording because reviewers have asked for each in turn.  With the
    bullet reworded, the two sentences left standing become this patch's
    own and both are false, so that state is out.  With it untouched, this
    patch scopes the bullet above it to the fallback while the V0 bullet
    keeps saying "each object is requested via fetch-pack" unscoped -- at
    the merge base both bullets were equally broad, so the asymmetry is
    this patch's doing.  Neither state is clean, and no third state is
    reachable without correcting the protocol version and the info/refs
    sentence, which is a separate claim needing its own justification.
    Chose the untouched state: every sentence this patch asserts is true,
    and what is left is an adjacent pre-existing error rather than a new
    one.
    
    Why the V0 bullet is left exactly as upstream has it.  An earlier
    revision of this patch deleted its "which means that each object is
    requested via fetch-pack" clause, to remove a contradiction with the
    new paragraph above.  That made the two remaining sentences this
    patch's own, and both are false, so the clause was restored: editing a
    sentence makes the author answerable for the sentence left behind.  V0
    is therefore untouched here and stays a pre-existing problem, recorded
    above.
    
    Checks: `git show --check` clean, no commit-message line over 75
    columns, `make -C Documentation lint-docs` exits 0.  No manpage render
    was attempted: asciidoctor, asciidoc, xmlto, docbook2x-texi and
    dblatex are all absent here.

 Documentation/technical/partial-clone.adoc | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/Documentation/technical/partial-clone.adoc b/Documentation/technical/partial-clone.adoc
index e513e391ea..757a92eddf 100644
--- a/Documentation/technical/partial-clone.adoc
+++ b/Documentation/technical/partial-clone.adoc
@@ -141,8 +141,10 @@ remote and then retry the object lookup.  This allows objects to be
 For efficiency reasons, no check as to whether the missing object is
 actually a promisor object is performed.
 +
-Dynamic object fetching tends to be slow as objects are fetched one at
-a time.
+This fallback asks for a single object per request, so it is slow when
+many objects are missing.  Code that can name several missing objects at
+once passes the whole list to promisor_remote_get_direct(), which
+requests the set rather than one object at a time.
 
 - `checkout` (and any other command using `unpack-trees`) has been taught
   to bulk pre-fetch all required missing blobs in a single batch.
@@ -247,11 +249,11 @@ remote in a specific order.
 - Repack essentially treats promisor and non-promisor packfiles as 2
   distinct partitions and does not mix them.
 
-- Dynamic object fetching invokes fetch-pack once *for each item*
-  because most algorithms stumble upon a missing object and need to have
-  it resolved before continuing their work.  This may incur significant
-  overhead -- and multiple authentication requests -- if many objects are
-  needed.
+- Dynamic object fetching invokes a "git fetch" subprocess once *for
+  each item* when it goes through the object lookup fallback, because
+  the object has to be resolved before the work can continue.  This may
+  incur significant overhead -- and multiple authentication requests --
+  if many objects are needed.
 
 - Dynamic object fetching currently uses the existing pack protocol V0
   which means that each object is requested via fetch-pack.  The server

base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
-- 
2.55.0