Re: Be more careful about updating refs

Junio C Hamano [off-list ref] wrote:

"Shawn O. Pearce" [off-list ref] writes:

quoted

I think the problem is nobody has tested fast-import updating an
existing ref while using NO_MMAP.  Or if they did, they didn't report
the problem as they didn't figure they needed fast-import that badly.

Updating an existing ref is not a common operation, but the test
suite does test for it.  So it must be the NO_MMAP configuration
is simply not being tested well enough.

Now a more important question is how we would properly fix this
issue?

I suspect that fast-import is the only one that opens windows
into an unfinalized pack, and if that is the case, it would be
the only program that may be hit by the issue of mmap emulation
getting stale data.

Yes, it is the only program that is foolish enough to access a
partial packfile.  index-pack uses pread(), and only after it has
the entire packfile downloaded to the local system.  The only other
reader of a partial packfile is unpack-objects, and obviously that
doesn't care about seeking backwards within the packfile.

So its probably the only user who suffers from the mmap emulation.
 

I do not think the patch I posted was correct at all.

Especially, I am not sure if the issue only exists at the
end_packfile() boundary.  Don't we have the same issue reading
from the packfile being built, and isn't the only reason my hack
works it around is because access patterns of the testsuite
happens to not trigger it?

Yes, that's my take on it as well (see my other email).  The
testsuite must just be really lucky that its not hitting the
boundary condition.

I almost said gfi_unpack_entry() was immune from this bug, but
I went back and read the code again and determined that it does
in fact suffer from this under NO_MMAP, and we're just really
damn lucky nobody has caused it.

I'll try to work up a patch this evening.

-- 
Shawn.