Re: [PATCH v3] promisor-remote: prevent lazy-fetch recursion in child fetch

[Paul Tarjan <paul@paultarjan.com>]

Patrick Steinhardt <ps@pks.im> writes:

> I must be misunderstanding something here, but how is it that a commit
> can be garbage collected if a ref points to it? That shouldn't ever
> happen, as reachable commits should not be pruned.
>
> Or do you mean to say that the commits don't exist on the server side
> anymore?

Sloppy wording on my part — "GC'd" is wrong. These refs pointed at
commits that were promised but never materialized on the partial
clone. The ~77K broken refs looked like:

  error: refs/prefetch/remotes/origin/claude/add-azure-dependencies-EaaDn \
         does not point to a valid object!

They were created by git maintenance --task=prefetch, which runs
git fetch --prefetch --prune and writes refs/prefetch/remotes/origin/<branch>
pointing at the remote tip. On a blob:none partial clone it fetches
commit/tree metadata, but some referenced commits were never
actually downloaded before the upstream branches (ephemeral
CI/automation branches, force-pushed and deleted within days)
disappeared.

This is a red herring for the patch though. The stale prefetch refs
explain why the outer fetch got a thin pack — the client advertised
haves from promised-but-absent commits. But the recursion (depth-1
to depth-2+) is entirely inside fetch_objects() with noop
negotiation, independent of any refs.

I'll fix the commit message wording in a v4 if you'd like.

> That's certainly curious. Do you maybe have multiple remotes attached to
> the repository, or are you dropping/modifying the object filter at some
> point?
>
> All subsequent fetches need to use the same object filter as you've used
> during the initial clone, otherwise you may run into a situation as you
> have described. But in theory, Git knows to continue using the filter.

Nobody intentionally changed the filter. What happened is the
lazy-fetch child kept re-writing it. fetch_objects() hardcodes
--filter=blob:none on the child argv, and the child's
builtin/fetch.c writes the active filter to config.

23547c40 ("fetch: do not override partial clone filter", 2020)
guards this write behind a check for an already-set filter. But I
was unsetting remote.origin.partialclonefilter manually trying to
stop the storm, so the guard passed and the next lazy-fetch child
wrote it right back:

  21:45  (unset)     manual git config --unset
  22:05  blob:none   re-written by a lazy-fetch child
  22:11  (unset)     manual unset again
  23:11  blob:none   re-written again
  23:13  (unset)     manual unset
  23:28  blob:none   re-written (caught live by a config-mtime trap)

The actual mitigation was unsetting remote.origin.promisor too —
with no promisor remote, fix_unresolved_deltas() skips the prefetch
entirely.

This is arguably a separate bug: fetch_objects() should probably
pass -c remote.<name>.partialclonefilter=blob:none to override for
the single invocation, rather than --filter=blob:none which
persists to config. Not in scope for this patch, but I could follow
up separately if there's interest.

Paul