From: Karthik Nayak <karthik.188@gmail.com>
Date: Thu, 27 Aug 2026 10:12:13 GMT
Subject: Re: [PATCH 04/10] builtin/fsck: don't check alternates with "--no-full"
Message-ID: <CAOLa=ZThFKpbmifU-zScXqiT1QSQqCTppHWbgSqmpRft8K+0XQ@mail.gmail.com>
In-Reply-To: <20260825-pks-odb-source-fsck-v1-4-b756de0bf24f@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> According to git-fsck(1), the "--full" option behaves in the following
> way:
>
>   Check not just objects in GIT_OBJECT_DIRECTORY ($GIT_DIR/objects), but
>   also the ones found in alternate object pools listed in
>   GIT_ALTERNATE_OBJECT_DIRECTORIES or $GIT_DIR/objects/info/alternates,
>   and in packed Git archives found in $GIT_DIR/objects/pack and
>   corresponding pack subdirectories in alternate object pools.
>
> So ultimately, it is supposed to control two things: (1) whether we only
> check the main object directory, and (2) whether we check packfiles.
>
> In its current state though, the flag only controls whether we check
> packfiles or not, and if so we verify packfiles of all attached sources.
> But we also have checks for loose objects in git-fsck(1), and here we
> unconditionally check them in all sources.
>

To reiterate,

without '--full': Check local loose + alternate loose. No packed objects

with '--full': Check local loose + alternate loose. local packed +
alternates packed.

And we want to only do local loose in the latter. Makes sense.

>
> The flag is arguably conflating two unrelated concerns with one another,
> and it really should be split up into two flags: one that controls how
> thorough we want to check individual sources, and one that controls
> which sources we want to check in the first place. So ideally, we would
> have:
>
>   - "--include-alternates": check all sources, not only the local one.
>
>   - "--include-optimized-objects": check not only loose objects, but
>     also those that have been packed. Note that we explicitly don't say
>     "--include-packed-objects" here to be more backend-agnostic.
>
>   - "--full": implies both of the above flags.
>
> This feels out of scope for this series though. So for now, simply fix
> the code by honoring locality of the sources for loose objects.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/fsck.c  | 3 ++-
>  t/t1450-fsck.sh | 5 +++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index 5132ff0f15..3f6056535f 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -1047,7 +1047,8 @@ int cmd_fsck(int argc,
>  				    mark_object_for_connectivity, repo, 0);
>  	} else {
>  		for (source = repo->objects->sources; source; source = source->next)
> -			fsck_source(repo, source);
> +			if (check_full || source->local)
> +				fsck_source(repo, source);
>

So we check the local bit and only fsck that source. Looks good.

>  		if (check_full) {
>  			struct packed_git *p;
> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
> index 77cd96de78..1b4074304c 100755
> --- a/t/t1450-fsck.sh
> +++ b/t/t1450-fsck.sh
> @@ -844,6 +844,11 @@ test_expect_success 'alternate objects are correctly blamed' '
>  	echo "../../alt.git/objects" >.git/objects/info/alternates &&
>  	mkdir alt.git/objects/$(dirname $path) &&
>  	>alt.git/objects/$(dirname $path)/$(basename $path) &&
> +
> +	# Without "--full", only the local object source is checked.
> +	git fsck --no-full >out 2>&1 &&
> +	test_must_be_empty out &&
> +
>  	test_must_fail git fsck >out 2>&1 &&
>  	test_grep alt.git out
>  '
>
> --
> 2.55.0.822.g20453c30eb.dirty