Re: [PATCH 2/2] Flag and skip over packfiles known to be invalid.

Junio C Hamano [off-list ref] wrote:

I haven't looked your patch very closely, but two successive
"git-repack -a -d" would produce the packfile under the same
name, but with different parameters (--window or --depth), the
old .idx and new .pack may not match.

Can this be possible (time flows from top to bottom):

	git-repack -a -d		somebody else

	produces a new .pack/.idx

        mv new .pack to objects/pack

					mmap .idx (old)
					finds .pack does not match

	mv new .idx to objects/pack

Gah.  That's sick.  And *so* totally possible.
 	

Is this something we care about?

 
Yes and no.

I've pushed the idea that `git repack -a -d` is totally safe to
run in a live repository.  Its clearly not.  *Especially* if you
repack with the same set of objects (as you do above) or if you
repack in rapid succession faster than readers can make progress.
I think both of these are just outside of the realm of "good ideas
to do to live things".

Its like taking your cat to the zoo play with a lion.  Maybe not
the best thing for the cat...


In this particular case that you highlight above this patch would
mark the pack as invalid and never permit it to be accessed again
by the 'somebody else'.

But Git has always been broken for this case.  prepare_packed_git_one
would refuse to open the new index, as it already has a packed_git.
Prior to this patch we'd try to access the packfile and die(),
as it did not match the index that we have.  Now we'd at least
try to locate some other source, or die() over a missing object,
but only after listing the idx/pack mismatch error too.

The only fix for the case above is to unlink the invalid packed_git
from packed_git, so that if we fail to find the object and wind up
calling reprepare_packed_git() we can reload the new index.  I did
consider doing that over the invalid flag, but flipped a coin and
added invalid.  FWIW, maybe invalid is the wrong way to go.


We still have a problem though, as the above case could still
happen to us during the fallback search in read_sha1_file().
In other words two searches ain't enough.  As Dscho pointed out in
the original thread, we probably need to loop until no new packs
are identified before giving up.

I almost submitted a patch to do that tonight, but I couldn't decide
on behavior: should we scan known packs, then try for loose, then
scan packs again until no object or no new pack is found?  Probably.

-- 
Shawn.