Subject: Re: [PATCH v12 13/13] fsmonitor: fix split-index bitmap bounds in tweak_fsmonitor()

From: Johannes Schindelin <[email-masked]>
Date: Sun, 5 Apr 2026 11:26:02 +0200 (CEST)

Hi Paul,

On Sat, 4 Apr 2026, Paul Tarjan wrote:

> Johannes Schindelin <[email-masked]> writes:
> 
> > So the actual bug fix would be to ensure that `write_shared_index()`
> > produces a hash even if `index.skipHash=true`, because that hash is
> > needed to identify the shared index.
> 
> You're right, and I verified this. I reverted my fsmonitor.c patch
> from seen, applied the fix below in write_shared_index(), and ran
> t9210 with GIT_TEST_SPLIT_INDEX=yes on Fedora:
> 
>   Without either fix:     not ok 12, not ok 13 (BUG assertion)
>   With only skipHash fix: passed all 22 test(s)
> 
> The fsmonitor.c patch is not needed. I'll drop it from the next
> version of the series.

Thank you for confirming.

> Happy to submit this as a separate patch or include it in my
> series if that's helpful. Or if someone is already working on the
> skipHash + split-index interaction, I'll stay out of the way.
> 
> --- >8 ---
> 
> diff --git a/read-cache.c b/read-cache.c
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ write_shared_index
>  	move_cache_to_base_index(istate);
>  	convert_to_sparse(istate, 0);
> 
> -	trace2_region_enter_printf("index", "shared/do_write_index",
> -				   the_repository, "%s", get_tempfile_path(*temp));
> -	ret = do_write_index(si->base, *temp, WRITE_NO_EXTENSION, flags);
> -	trace2_region_leave_printf("index", "shared/do_write_index",
> -				   the_repository, "%s", get_tempfile_path(*temp));
> +	/*
> +	 * The shared index is identified by the hash of its contents
> +	 * (sharedindex.<oid>).  If index.skipHash is set, do_write_index()
> +	 * would produce an all-zero hash and the shared index would not
> +	 * be found on re-read (is_null_oid() check in read_index_from()).
> +	 * Temporarily force hashing for the shared index write.
> +	 */
> +	{
> +		struct repository *r = the_repository;
> +		int save_skip_hash;
> +
> +		prepare_repo_settings(r);
> +		save_skip_hash = r->settings.index_skip_hash;
> +		r->settings.index_skip_hash = 0;
> +
> +		trace2_region_enter_printf("index", "shared/do_write_index",
> +					   the_repository, "%s", get_tempfile_path(*temp));
> +		ret = do_write_index(si->base, *temp, WRITE_NO_EXTENSION, flags);
> +		trace2_region_leave_printf("index", "shared/do_write_index",
> +					   the_repository, "%s", get_tempfile_path(*temp));
> +
> +		r->settings.index_skip_hash = save_skip_hash;
> +	}
> 
>  	if (was_full)
>  		ensure_full_index(istate);

This patch essentially tells Git to disobey the `index.skipHash` config,
at least under some circumstances. While that _looks_ like it works around
the observed problem, it is unlikely to be desirable in the long run
because such an inconsistency is prone to cause problems down the line.

The fundamental problem at hand is that the split-index _requires_ the
index' hash to be calculated, while the `index.skipHash` config
specifically _skips_ it. In other words, those two features are
fundamentally incompatible with one another.

Mind you, I could imagine a sort of hacky way to make them work with each
other: rely on the fact that padding the mtime's raw ytes with enough NULs
to fill the OID array would result in a sort of fake OID that is as
unlikely to clash with a real SHA as any other real SHA. In other words,
under `index.skipHash`, instead of accepting a `base_oid` that consists of
all NULs, fake one that won't fall into the `is_null_oid()` trap.

This approach is still fraught with challenges. For one, a written
`.git/index` file will not indicate whether it was written with or without
`index.skipHash` (unlike the split-index, which results in a `link`
extension to be included), unless you count that all-NUL OID as a hint.
Therefore, just changing the OID that is written under `index.skipHash`
won't work, you'd have to introduce some sort of flag into the index file
format _and_ then you'd run into compatibility issues with other Git
implementations (or older Git versions) trying to read the index file
written with that flag.

So the safest approach I can think of really is what I suggested, to force
the `GIT_TEST_SPLIT_INDEX` variable to be unset in `t9210-scalar.sh`.

It would probably also make sense to contribute a separate patch that lets
Git error out if it is asked to read or write a split-index under
`index.skipHash`, but that might very well lead down another rabbit hole,
therefore I won't recommend it.

Ciao,
Johannes

I briefly considered skipping t9210 altogether if that variable is set,
but then all of those tests would be skipped in `linux-TEST-vars`, which
would be counter-productive.

Ciao,
Johannes