Re: [PATCH v2 2/2] odb: ignore refs of shallow and grafted alternates

From: Jeff King <peff@peff.net>
Date: Oct 15, 2026, 06:57 UTC
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:

Show 7 quoted lines

> 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.

Show 16 quoted lines

> --- 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.

Show 20 quoted lines

> 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