From: Jeff King <peff@peff.net>
Date: Thu, 15 Oct 2026 06:57:54 GMT
Subject: Re: [PATCH v2 2/2] odb: ignore refs of shallow and grafted alternates
Message-ID: <20261015065754.GB1979109@coredump.intra.peff.net>
In-Reply-To: <20261014181134.56220-3-iwiedler@gitlab.com>

On Wed, Oct 14, 2026 at 08:11:34PM +0200, Igor Wiedler wrote:

> That assumption does not hold for a shallow repository, whose refs
> point at commits with no history behind them below its shallow
> boundary, nor for a grafted one. A repository borrowing objects from
> such an alternate does not see the alternate's shallow file, so the
> history that is missing there is missing here as well. The fetch then
> succeeds, the connectivity check stops at the alternate's refs, and
> a later "git log" fails with "Could not read <oid>".

In addition to grafts there are replace refs. I think we generally
insisted on having full connectivity of the original objects, though
(e.g., fsck will ignore replace refs). I thought that was true of
grafts, too, though (except for ones grafted by the shallow mechanism,
which is special).

> Partial clones have the same problem for the objects they lack, but
> detecting one requires reading the alternate's config, so this
> patch does not handle them.

Yeah, I'm sure there are some corner cases there. In theory you can grab
any of the missing partial-clone objects at will, though probably we
have no code to realize that it's the alternate which is partial and
through which the request should be routed. I guess you could argue the
same is true for shallow alternates (i.e., we _could_ get those objects
if we needed them, we just don't have a way to do so).

These are such weird setups that I think punting and saying "just ignore
their alternate refs" is totally fine. The use of the alternate refs is
an optimization, so falling back to the simpler behavior is good.

> --- a/odb.c
> +++ b/odb.c
> @@ -458,6 +458,17 @@ static int refs_from_alternate_cb(struct odb_source *alternate,
>  		goto out;
>  	strbuf_setlen(&path, base_len);
>  
> +	/* A shallow or grafted repository may lack the history behind its refs. */
> +	strbuf_addstr(&path, "/shallow");
> +	if (!access(path.buf, F_OK))
> +		goto out;
> +	strbuf_setlen(&path, base_len);
> +
> +	strbuf_addstr(&path, "/info/grafts");
> +	if (!access(path.buf, F_OK))
> +		goto out;
> +	strbuf_setlen(&path, base_len);

OK. It's a little gross that we have to check these magic paths, but
there's no real alternative here. We do not have a real repository
object or anything like it representing the alternate.

The thread has been talking about check_connected(), but the same
problem applies to advertising alternates for transfer. But your fix
here takes care of both, because it works at the lowest level. Good.

> diff --git a/t/t5618-alternate-refs.sh b/t/t5618-alternate-refs.sh
> index 4cc39af0f0..9c3862709a 100755
> --- a/t/t5618-alternate-refs.sh
> +++ b/t/t5618-alternate-refs.sh
> @@ -70,4 +70,17 @@ test_expect_success 'connectivity check of a deepening fetch excludes alternate
>  		--alternate-refs --quiet <deepen.trace
>  '
>  
> +test_expect_success 'refs of a shallow alternate are ignored' '
> +	git clone --depth=1 "file://$TRASH_DIRECTORY" "$TRASH_DIRECTORY/shallow" &&
> +	test_commit -C "$TRASH_DIRECTORY/shallow" shallow-only &&
> +	git -C "$TRASH_DIRECTORY/shallow" rev-parse HEAD >shallow-tip &&
> +	cp .git/objects/info/alternates alternates.bak &&
> +	test_when_finished "mv alternates.bak .git/objects/info/alternates" &&
> +	echo "$TRASH_DIRECTORY/shallow/.git/objects" >>.git/objects/info/alternates &&
> +	git cat-file -e $(cat shallow-tip) &&
> +	git rev-list --remotes=origin >expect &&
> +	git rev-list --alternate-refs >actual &&
> +	test_cmp expect actual
> +'

This is a direct test of --alternate-refs, which is probably reasonable
and the simplest way of doing it. I think you could also construct a
case where a fetch or push causes the recipient repo to be corrupted (it
claimed to have object X because of the alternate, so the other side
didn't send it, but since _we're_ not shallow, now fsck, etc, fails).
But I don't think there's a particular need to construct that full case
for the tests.

-Peff