Subject: Re: RFC on packfile URIs and .gitmodules check

From: Jonathan Tan <[email-masked]>
Date: Tue, 19 Jan 2021 11:13:06 -0800

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

My initial reaction was that stdout is already used to report the hash
part of the generated name and that stderr is already used for whatever
warnings there are, but looking at the documentation, index-pack
--fsck-objects is "[for] internal use only", so it might be fine to
extend the output format in this case and report the problematic OIDs
after the hash. I'll take a look.