From: Jeff King <peff@peff.net>
Date: Thu, 15 Oct 2026 06:41:58 GMT
Subject: Re: [PATCH] connected: do not walk alternate refs in deepening fetches
Message-ID: <20261015064158.GA1979109@coredump.intra.peff.net>
In-Reply-To: <xmqqcxtcte5m.fsf@gitster.g>

On Wed, Oct 14, 2026 at 09:55:49AM -0700, Junio C Hamano wrote:

> > That only works because "--alternate-refs" comes after "--not".
> > cf1e7c0770 (fetch-pack: write shallow, then check connectivity,
> > 2018-07-02) drops "--not --all" for deepening fetches, since those
> > need to verify the whole chain up to the new shallow boundary rather
> > than stopping at refs we already had. For such fetches we therefore
> > run
> >
> >   git --shallow-file ... rev-list --objects --stdin --quiet --alternate-refs
> 
> Ouch!  Thanks for spotting the 8-year-old mistake.  It shows how
> rare the deepening fetches together with the use of alternate refs
> in real life?

Thanks for cc-ing me. I looked at Igor's v1 patch, and it's exactly the
solution I would have expected. It should have been written that way in
39b44ba771 (check_everything_connected: assume alternate ref tips are
valid, 2019-07-01), because the intent was "treat alternate ref tips
just like we treat our own existing ref tips". So wherever we use
"--all", we should use "--alternate-refs", and since --all was moved
into that non-deepening conditional, --alternate-refs should go there,
too.

BTW, if you are curious why 39b44ba771 made such an obvious mistake, it
is exactly because it predated that move! The original patch was written
in 2014 and used internally at GitHub. I rebased it forward to send to
the list in 2019, but I don't even think I got a textual conflict at all
(because the movement of "--not" and "--all" were a few lines away).

So it was a very subtle semantic conflict between the two patches. But
because the rebase rewrote history to put mine on top, it looks like I
just did it wrong from the start. ;)

> But stepping back a bit, doesn't the same argument apply even when
> we are a fully populated repository and making a non-deepening
> fetch?  We can trust "--not --all" to make the traversal to stop at
> tips of our local refs and can be assured that all history behind
> the tips we are fetching will still be connected, but a traversal
> that stops at the tip of a shallow repository will break our
> repository, and in a worse form than the shallow repository we are
> borrowing from, because operations in that repository are guarded by
> its shallow boundaries that we usually do not see, but operations in
> our repository do not have the same shallow boundaries in effect.

It took me a few reads to parse this, but I think you are saying: it is
a problem if we are a non-shallow repository trying to use a shallow
alternate for verification?

That sounds likely, though this is the first time I have looked at
cf1e7c0770 (fetch-pack: write shallow, then check connectivity,
2018-07-02) and the surrounding issues. As I understand the issue, it is
really "are the refs we are going to use to verify coming from a shallow
repo which might not have all objects". But I'm not sure how that
interacts with opt->is_deepening_fetch (and why it isn't just checking
if we are in a shallow repo).

> I do agree that for the purpose of "see if this traversal barfs with
> missing objects to see if the updated tips have all the objects
> behind it", listing the alternate refs behind "--not" is the right
> thing to do and it is broken to have "--alternate-refs" without
> having "--not" like the current code does.  But I am not convinced
> that disabling --alternate-refs only when you are deepening is not
> solving the problems of using alternate-refs introduces.  Shouldn't
> the fix be more like two (or more) steps, i.e.,
> 
>  * [1/2] for check_connected(), use the attached patch instead.

OK, so this moves the "--not" out so that we always do it. It's ignored
in the case that we are deepening and don't have any alternates, but it
shouldn't hurt anything.

>  * [2/2] update odb_for_each_alternate_ref() so that it ignores any
>    and all refs from shallow repositories.

Yeah, this seems reasonable. Alternates don't necessarily even have a
full repo, but in that case they wouldn't have any refs, either (IIRC we
do some magic to guess at the containing repo of the alternate).

-Peff