Re: [PATCH] pack-objects: re-validate data we copy from elsewhere.

Linus Torvalds [off-list ref] writes:

On Fri, 1 Sep 2006, Junio C Hamano wrote:

quoted

But "git repack -a -d", which you now consider almost being
free, in the recent kernel repository counts 300k objects, and
reuses 298k objects or so.  That means we expand and recompress
that many objects, totalling 120MB.

Sure. Do we have data for how expensive that is (ie did you apply the 
patch and time it)?

Quite bad.  For the kernel archive of today (I usually am nearly
fully packed):

$ /usr/bin/time ~/git-master/bin/git-pack-objects p1 </var/tmp/1
Generating pack...
Done counting 301361 objects.
Deltifying 301361 objects.
 100% (301361/301361) done
Writing 301361 objects.
 100% (301361/301361) done
a13dc6646622537d29af92b4cfc6d49b82e77e49
Total 301361, written 301361 (delta 238935), reused 300995 (delta 238663)
3.58user 0.84system 0:04.44elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+62727minor)pagefaults 0swaps

$ /usr/bin/time ../git.junio/git-pack-objects p2 </var/tmp/1
Generating pack...
Done counting 301361 objects.
Deltifying 301361 objects.
 100% (301361/301361) done
Writing 301361 objects.
 100% (301361/301361) done
a13dc6646622537d29af92b4cfc6d49b82e77e49
Total 301361, written 301361 (delta 238935), reused 300995 (delta 238663)
57.84user 3.39system 1:01.36elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+0outputs (0major+1022768minor)pagefaults 0swaps

By the way, the one in "next" has a thinko I just noticed.

-- >8 --
[PATCH] pack-objects: fix thinko in revalidate code

When revalidating an entry from an existing pack entry->size and
entry->type are not necessarily the size of the final object
when the entry is deltified, but for base objects they must
match.

Signed-off-by: Junio C Hamano <redacted>
---
 builtin-pack-objects.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 11cc3c8..5e42387 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c

@@ -247,12 +247,13 @@ static int revalidate_one(struct object_
 			  void *data, char *type, unsigned long size)
 {
 	int err;
-	if (!data)
-		return -1;
-	if (size != entry->size)
-		return -1;
-	err = check_sha1_signature(entry->sha1, data, size,
-				   type_names[entry->type]);
+	if ((!data) ||
+	    ((entry->type != OBJ_DELTA) &&
+	     ( (size != entry->size) ||
+	       strcmp(type_names[entry->type], type))))
+		err = -1;
+	else
+		err = check_sha1_signature(entry->sha1, data, size, type);
 	free(data);
 	return err;
 }

-- 
1.4.2.g99d7d



-- 
VGER BF report: U 0.528006

On Sun, 3 Sep 2006, Junio C Hamano wrote:

Quite bad.  For the kernel archive of today (I usually am nearly
fully packed):

Ok. Is it less painful if it just checks the zlib CRC (and that the SHA1 
_exists_ for a delta - although I guess we check that indirectly by just 
accepting the delta in the first place)? That combination should still be 
a fairly strong check, of course.

Then we could have something like

	[repack]
		check=[none|weak|default|strong]

where the "none" check would be to just copy the data as-is, the "weak" 
(aka "default") would check just the CRC, and the strong one would unpack 
the whole object and check the SHA1..

		Linus

-- 
VGER BF report: U 0.5

On Sun, 3 Sep 2006, Linus Torvalds wrote:

Ok. Is it less painful if it just checks the zlib CRC (and that the SHA1 
_exists_ for a delta - although I guess we check that indirectly by just 
accepting the delta in the first place)? That combination should still be 
a fairly strong check, of course.

Thinking some more about it, doing the stupid "apply full delta chain and 
check the final SHA1" is horribly inefficient, because if you have a repo 
that packs well, you'd expect to have a lot of things with a 10-deep delta 
chain.

And doing it the silly way means that you'll do each object independently, 
ie for a 10-deep chain you'd unpack the base object ten times, and apply 
the first delta 9 times, the second one 8 times etc etc. And each time 
you'd deflate everything, since we don't keep a cache of actual object 
contents.

So I'd expect that with full SHA1 checking, you'd end up doing ~45 
deflates for the ten-object chain, instead of doing just 10.

So it should hopefully be _much_ cheaper to just check the zlib CRC, not 
because the "apply delta" and "calculate sha1" are necessarily all that 
expensive, but because the unoptimized chain-unpacking is doing so much 
unnecessary work.

			Linus

-- 
VGER BF report: U 0.5