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

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

[Nicolas Pitre]

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 <nico@cam.org>
---

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;

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

[Harvey Harrison]

On Mon, Oct 20, 2008 at 11:12 AM, Nicolas Pitre <nico@cam.org> 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 <nico@cam.org>

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

Tested-by: Harvey Harrison <harvey.harrison@gmail.com>

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

[Harvey Harrison]

On Mon, Oct 20, 2008 at 11:37 AM, Harvey Harrison
<harvey.harrison@gmail.com> wrote:
> On Mon, Oct 20, 2008 at 11:12 AM, Nicolas Pitre <nico@cam.org> 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 <nico@cam.org>
>
> This fixes it for me, thanks for the quick fix.
>
> Tested-by: Harvey Harrison <harvey.harrison@gmail.com>
>

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

Harvey

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

[Marco Roeland]

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 <nico@cam.org>
> ---
> 
> 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

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

[Marco Roeland]

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

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

[Nicolas Pitre]

On Mon, 20 Oct 2008, Marco Roeland wrote:

> 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

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

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

[Marco Roeland]

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

> > 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

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

[Nicolas Pitre]

On Mon, 20 Oct 2008, Marco Roeland wrote:

> On Monday Oktober 20th 2008 at 15:27 Nicolas Pitre wrote:
> 
> > > 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

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

[Jeff King]

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

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

[Marco Roeland]

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