Linus Torvalds [off-list ref] 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. We did not do paranoid in git-pull
long after we introduced the .keep thing anyway, so I do not
think the following patch is even needed, but I am throwing it
out just for discussion.
diff --git a/fetch-pack.c b/fetch-pack.c
index 06f4aec..c687f9f 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -522,6 +522,7 @@ static int get_pack(int xd[2])
if (do_keep) {
*av++ = "index-pack";
+ *av++ = "--paranoid";
*av++ = "--stdin";
if (!quiet && !no_progress)
*av++ = "-v";On Tue, 3 Apr 2007, Junio C Hamano wrote:
Linus Torvalds [off-list ref] writes:
quoted
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