RE: Troubleshoot clone issue to NFS.

> > In builtin/index-pack.c, replace the line "collision_test_needed =
> > has_sha1_file(sha1);" with "collision_test_needed = 0;". Security is
> > compromised but for this test it should be ok. Then clone again. I
> > hope the new number gets down close to v1.8.4.1.
> 
> Yeah, I think that is a good starting point. I timed a local clone
> before and after 45e8a74; there is a small difference on my system
> (about 5%), but it goes away with your suggestion.

> The problem with has_sha1_file() prior to v1.8.4.2 is that it is racy
> with respect to simultaneous operations; we might claim we do not have
> an object, when in fact we do. As you noted, usually has_sha1_file()
> returns true (i.e., we look up objects that we expect to have), and the
> performance impact is minimal.
> 
> But for code paths where _not_ having the object is normal, the impact
> is much greater. So I think there are two possibilities for improving
> this:
> 
>   1. Find places where we expect the object will not exist (like the
>      collision_test check you pointed out) and use a
>      "has_sha1_file_fast" that accepts that it may very occasionally
>      erroneously return false. In this case it would mean potentially
>      skipping a collision check, but I think that is OK. That could have
>      security implications, but only if an attacker:
> 
>        a. has broken sha1 to generate a colliding object
> 
>        b. can manipulate the victim into repacking in a loop
> 
>        c. can manipulate the victim into fetching (or receiving a push)
>           simultaneously with (b)
> 
>      at which point they can try to race the repack procedure to add
>      their colliding object to the repository. It seems rather unlikely
>      (especially part a).
> 
>   2. Make reprepare_packed_git() cheaper in the common case that nothing
>      has changed. It would probably be enough to stat("objects/pack").
>      We know that packfiles themselves do not change; we may only add or
>      delete them. And since the hierarchy of objects/pack is flat, we
>      know that the mtime on that directory will change if any packs are
>      added or removed.
> 
>      Of course, we are still doing an extra stat() for each has_sha1_file
>      call. Whether that helps for the NFS case depends on whether stat()
>      is significantly cheaper than opendir/readdir/closedir. On my local
>      disk, the hacky patch below did seem to give me back the 5% lost by
>      45e8a74 (I did it directly on master, since that old commit does
>      not have the stat_validity infrastructure).