Re: RFC on packfile URIs and .gitmodules check

[Derrick Stolee <stolee@gmail.com>]

On 1/15/2021 10:22 PM, Taylor Blau wrote:
> On Fri, Jan 15, 2021 at 04:30:07PM -0800, Junio C Hamano wrote:
>> Jonathan Tan <jonathantanmy@google.com> writes:
>>
>>> Someone at $DAYJOB noticed that if a .gitmodules-containing tree and the
>>> .gitmodules blob itself are sent in 2 separate packfiles during a fetch
>>> (which can happen when packfile URIs are used), transfer.fsckobjects
>>> causes the fetch to fail. You can reproduce it as follows (as of the
>>> time of writing):
>>>
>>>   $ git -c fetch.uriprotocols=https -c transfer.fsckobjects=true clone https://chromium.googlesource.com/chromiumos/codesearch
>>>   Cloning into 'codesearch'...
>>>   remote: Total 2242 (delta 0), reused 2242 (delta 0)
>>>   Receiving objects: 100% (2242/2242), 1.77 MiB | 4.62 MiB/s, done.
>>>   error: object 1f155c20935ee1154a813a814f03ef2b3976680f: gitmodulesMissing: unable to read .gitmodules blob
>>>   fatal: fsck error in pack objects
>>>   fatal: index-pack failed

I'm contributing a quick suggestion for just this item:

>>> This happens because the fsck part is currently being done in
>>> index-pack, which operates on one pack at a time. When index-pack sees
>>> the tree, it runs fsck on it (like any other object), and the fsck
>>> subsystem remembers the .gitmodules target (specifically, in
>>> gitmodules_found in fsck.c). Later, index-pack runs fsck_finish() which
>>> checks if the target exists, but it doesn't, so it reports the failure.
>>
>> Is this because the gitmodules blob is contained in the base image
>> served via the pack URI mechansim, and the "dynamic" packfile for
>> the latest part of the history refers to the gitmodules file that is
>> unchanged, hence the latter one lacks it?
> 
> That seems like a likely explanation, although this seems ultimately up
> to what the pack CDN serves.
>> You've listed two possible solutions, i.e.
>>
>>  (1) punt and declare that we assume an missing and uncheckable blob
>>      is OK,
>>
>>  (2) defer the check after transfer completes.
>>
>> Between the two, my gut feeling is that the latter is preferrable.
>> If we assume an missing and uncheckable one is OK, then even if a
>> blob is available to be checked, there is not much point in
>> checking, no?
> 
> I'm going to second this. If this were a more benign check, then I'd
> perhaps feel differently, but .gitmodules fsck checks seem to get
> hardened fairly often during security releases, and so it seems
> important to keep performing them when the user asked for it.

It might be nice to teach 'index-pack' a mode that says certain
errors should be reported as warnings by writing the problematic
OIDs to stdout/stderr. Then, the second check after all packs are
present can focus on those problematic objects instead of
re-scanning everything.

Thanks,
-Stolee