[PATCH] fix for "index-pack: rationalize delta resolution code"

My bad.  A small detail went through the crack: the real_type of
a delta object is the real_type of its base object.

Without this, the created index will be wrong as the actual object SHA1
won't match the object.

Signed-off-by: Nicolas Pitre <redacted>
---

If you got a corrupted .idx file because of this ('git verify-pack' 
should tell) then just toss it and recreate with a fixed 'git 
index-pack'.

Could anyone having problems fetching from kernel.org with git from the 
next branch confirm that this also fixes that? Thanks.

diff --git a/index-pack.c b/index-pack.c
index 0a917d7..8287ebf 100644
--- a/index-pack.c
+++ b/index-pack.c

@@ -517,7 +517,7 @@ static void resolve_delta(struct object_entry *delta_obj,
 	void *delta_data;
 	unsigned long delta_size;
 
-	delta_obj->real_type = base->obj->type;
+	delta_obj->real_type = base->obj->real_type;
 	delta_data = get_data_from_pack(delta_obj);
 	delta_size = delta_obj->size;
 	result->obj = delta_obj;

On Monday Oktober 20th 2008 at 16:04 Nicolas Pitre wrote:

If you want to make the difference really visible, try with
'git repack -a -f --window=100'.

Impressive, yes. Thanks very much.

marco@sirius:~/src/git (next) $ time git repack -a -f --window=100
Counting objects: 85713, done.
Compressing objects: 100% (84207/84207), done.
Writing objects: 100% (85713/85713), done.
Total 85713 (delta 62371), reused 0 (delta 0)

real    1m2.775s
user    1m1.848s
sys     0m0.176s
marco@sirius:~/src/git (next) $ git config pack.threads 0
marco@sirius:~/src/git (next) $ time git repack -a -f --window=100
Counting objects: 85713, done.
Compressing objects: 100% (84207/84207), done.
Writing objects: 100% (85713/85713), done.
Total 85713 (delta 62363), reused 0 (delta 0)

real    0m21.348s
user    1m2.948s
sys     0m0.432s
marco@sirius:~/src/git (next) $ git config --unset pack.threads
marco@sirius:~/src/git (next) $ time git repack -a -f --window=100
Counting objects: 85713, done.
Compressing objects: 100% (84207/84207), done.
Writing objects: 100% (85713/85713), done.
Total 85713 (delta 62371), reused 0 (delta 0)

real    1m1.904s
user    1m1.476s
sys     0m0.184s

This on Intel(R) Core(TM)2 Quad  CPU   Q9450  @ 2.66GHz.
-- 
Marco Roeland

On Mon, Oct 20, 2008 at 11:37 AM, Harvey Harrison
[off-list ref] wrote:

On Mon, Oct 20, 2008 at 11:12 AM, Nicolas Pitre [off-list ref] wrote:

quoted

My bad.  A small detail went through the crack: the real_type of
a delta object is the real_type of its base object.

Without this, the created index will be wrong as the actual object SHA1
won't match the object.

Signed-off-by: Nicolas Pitre <redacted>

This fixes it for me, thanks for the quick fix.

Tested-by: Harvey Harrison <redacted>

Scratch that, it's back to failing again on my next update.

Harvey

On Mon, Oct 20, 2008 at 11:12 AM, Nicolas Pitre [off-list ref] wrote:

My bad.  A small detail went through the crack: the real_type of
a delta object is the real_type of its base object.

Without this, the created index will be wrong as the actual object SHA1
won't match the object.

Signed-off-by: Nicolas Pitre <redacted>

This fixes it for me, thanks for the quick fix.

Tested-by: Harvey Harrison <redacted>

On Monday Oktober 20th 2008 at 15:27 Nicolas Pitre wrote:

quoted

This is on a quadcore. I recently experimented with "git config
pack.threads 0" but as it didn't seem to speedup anything I removed
it again. Just mention it on the infinitesimal chance it might be
important.

It is not.  And the speedup should be noticeable when you repack, not 
when you fetch.

No offense meant! I tried a few "git gc" and "git repack" and only
watched the Gnome CPU applet; perhaps everything was already nicely
packed. I'm certainly going to retry now. Thanks for all your good work.
-- 
Marco Roeland

On Mon, 20 Oct 2008, Marco Roeland wrote:

On Monday Oktober 20th 2008 at 15:27 Nicolas Pitre wrote:

quoted

quoted

This is on a quadcore. I recently experimented with "git config
pack.threads 0" but as it didn't seem to speedup anything I removed
it again. Just mention it on the infinitesimal chance it might be
important.

It is not.  And the speedup should be noticeable when you repack, not 
when you fetch.

No offense meant! 

Oh certainly not.

I tried a few "git gc" and "git repack" and only
watched the Gnome CPU applet; perhaps everything was already nicely
packed. I'm certainly going to retry now. Thanks for all your good work.

If you want to make the difference really visible, try with
'git repack -a -f --window=100'.


Nicolas

On Monday October 2008 at 14:12 Nicolas Pitre wrote:

My bad.  A small detail went through the crack: the real_type of
a delta object is the real_type of its base object.

Without this, the created index will be wrong as the actual object SHA1
won't match the object.

Signed-off-by: Nicolas Pitre <redacted>
---

If you got a corrupted .idx file because of this ('git verify-pack' 
should tell) then just toss it and recreate with a fixed 'git 
index-pack'.

Could anyone having problems fetching from kernel.org with git from the 
next branch confirm that this also fixes that? Thanks.

I still seem to have the same problem after patching:

$ git pull
remote: Counting objects: 279, done.
remote: Compressing objects: 100% (78/78), done.
remote: Total 177 (delta 136), reused 135 (delta 99)
Receiving objects: 100% (177/177), 66.59 KiB, done.
fatal: pack has bad object at offset 53487: failed to apply delta
fatal: index-pack failed

'git verify-pack' does _not_ report an error for either pack or index.
This is with git from branch next at 8f0e41f379d486dd27766d84d994eb1da5b8319d
trying to pull from git://git.kernel.org/pub/scm/git/git.git

This is on Debian 'sid' with an AMD64 architecture.

I've put the whole ".git" directory (warning: almost 35MB) for
investigation at:

http://www.xs4all.nl/~fiberbit/http://www.xs4all.nl/~fiberbit/git-next-8f0e41f3-bad-index.tgz

I hope I've patched correctly. After applying (cleanly) and rebuilding
simply executing "./git" from the workdirectory still uses the old
version. Only after using "make install" I get the patched version,
which as shown above still gives an error, from the die() at line 528 in
index-pack.c: bad_object(delta_obj->idx.offset, "failed to apply
delta");

Not much more time tonight here, but perhaps it's easier to reproduce
now with the copy of an affected .git directory.
-- 
Marco Roeland

On Mon, 20 Oct 2008, Marco Roeland wrote:

Op maandag 20 oktober 2008 om 21:14 uur schreef Marco Roeland het volgende:

quoted

This is on Debian 'sid' with an AMD64 architecture.

I've put the whole ".git" directory (warning: almost 35MB) for
investigation at:

http://www.xs4all.nl/~fiberbit/http://www.xs4all.nl/~fiberbit/git-next-8f0e41f3-bad-index.tgz

Gah, I can't even copy-and-paste:

http://www.xs4all.nl/~fiberbit/git-next-8f0e41f3-bad-index.tgz

Don't worry -- I figured it out and was able to reproduce the problem 
already.  Thanks a lot!

This is on a quadcore. I recently experimented with "git config
pack.threads 0" but as it didn't seem to speedup anything I removed
it again. Just mention it on the infinitesimal chance it might be
important.

It is not.  And the speedup should be noticeable when you repack, not 
when you fetch.


Nicolas

Op maandag 20 oktober 2008 om 21:14 uur schreef Marco Roeland het volgende:

This is on Debian 'sid' with an AMD64 architecture.

I've put the whole ".git" directory (warning: almost 35MB) for
investigation at:

http://www.xs4all.nl/~fiberbit/http://www.xs4all.nl/~fiberbit/git-next-8f0e41f3-bad-index.tgz

Gah, I can't even copy-and-paste:

http://www.xs4all.nl/~fiberbit/git-next-8f0e41f3-bad-index.tgz

This is on a quadcore. I recently experimented with "git config
pack.threads 0" but as it didn't seem to speedup anything I removed
it again. Just mention it on the infinitesimal chance it might be
important.
-- 
Marco Roeland

On Mon, Oct 20, 2008 at 02:12:04PM -0400, Nicolas Pitre wrote:

Could anyone having problems fetching from kernel.org with git from the 
next branch confirm that this also fixes that? Thanks.

Nope, this does not fix it for me.

-Peff