Re: git-index-pack really does suck..

From: Nicolas Pitre <nico@cam.org>
Date: Apr 3, 2007, 20:53 UTC
Message-ID: <alpine.LFD.0.98.0704031639470.28181@xanadu.home>
In-Reply-To: <7vbqi5w62c.fsf@assigned-by-dhcp.cox.net>

On Tue, 3 Apr 2007, Junio C Hamano wrote:

Show 13 quoted lines

> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > That whole "verify no SHA1 hash collision" code is really pretty damn 
> > paranoid. Maybe we shouldn't have it enabled by default.
> >
> > So how about this updated patch? We could certainly make "git pull" imply 
> > "--paranoid" if we want to, but even that is likely pretty unnecessary. 
> > It's not like anybody has ever shown a SHA1 collision, and if the *local* 
> > repository is corrupt (and has an object with the wrong SHA1 - that's what 
> > the testsuite checks for), then it's probably good to get the valid object 
> > from the remote..
> 
> I agree with that reasoning.

For the record, I don't agree.  I stated why in my other email.

> We did not do paranoid in git-pull long after we introduced the .keep 
> thing anyway,

That doesn't make it more "correct".

> so I do not
> think the following patch is even needed, but I am throwing it
> out just for discussion.

1) None of the objects in a pack should exist in the local repo when 
   fetching, meaning that the paranoia code should not be executed 
   normally.

2) Running index-pack on a pack _inside_ a repository is a dubious thing 
   to do with questionable usefulness already.

3) It is unefficient to run pack-objects with --stdout just to feed the 
   result to index-pack afterwards while repack-objects can create the 
   index itself, which is the source of this discussion.
   
4) I invite you to read the commit log for 8685da42561 where the 
   _perception_ of GIT's security is discussed which led to the paranoia 
   check, and sometimes the perception is more valuable than the 
   reality, especially when it is free.

Therefore Linus' patch and this one are working around the wrong issue as described in (3) IMHO.

What could be done instead, if really really needed, is to have the paranoia test be made conditional on index-pack --stdin instead. But please no bogus extra switches pretty please.

Nicolas