Subject: Re: [PATCH v4 00/25] multi-pack reachability bitmaps

From: Junio C Hamano <[email-masked]>
Date: Sun, 29 Aug 2021 15:42:56 -0700

Jeff King <[email-masked]> writes:

>> The new test, specifically this snippet:
>> 
>>     git init repo &&
>>     test_when_finished "rm -fr repo" &&
>>     (
>>       cd repo &&
>>       test_commit base &&
>>       git repack -d
>>     ) &&
>> 
>>     nongit git multi-pack-index --object-dir=$(pwd)/repo/.git/objects write
>> 
>> will fail with GIT_TEST_DEFAULT_HASH=sha256, since the MIDX internals
>> settle on the hash size via `the_hash_algo` which doesn't respect the
>> hash algorithm used by the target repository.
>
> Yeah, I think this is a good example of the class of things that might
> fail: anything that requires the repo config to behave correctly.
>
> I do think the hash format is somewhat unusual here. Most of the changes
> to the on-disk files are reflected in the files themselves (e.g., pack
> index v2 is chosen by config at _write_ time, but readers can interpret
> the file stand-alone).
>
> There may be other config that could influence the writing of the midx,
> and we'd skip it in this kind of non-repo setup. An example here is
> repack.usedeltabaseoffset, which midx_repack() tries to respect.
> Ignoring that doesn't produce a nonsense result, but it doesn't follow
> what would happen if run from inside the repo.
>
> The other class of problems I'd expect is where part of the midx
> operation needs to look at other parts of the repo. Bitmap generation is
> an obvious one there, since we'd want to look at refs to find the
> reachable tips. Now obviously that's a new feature we're trying to
> introduce here, so it can't be an existing breakage. But it does make me
> wonder what other problems might be lurking.
>
> So I dunno. Even if it mostly works now, I'm not sure it's something
> that I'm all that happy about supporting going forward. It seems like a
> recipe for subtle bugs where the midx code calls into other library code
> that assumes that it can look at the repository struct.

I tend to agree that we should first disallow things that are not
what we know we definitely need, and the non-repo setup is something
we would want to punt on to make sure we have a solid support for
the mainstream usecase.

Thanks.