[BUG?] 'git verify-pack -v' on multiple packs fails for all but the first pack

(I don't have time to look into this right now, but will do so later if 
nobody comes up with a solution in the meantime...)

When running 'git verify-pack -v' on multiple packs (.idx files), it 
fails for all packs, except the first, with exit code 128, and the 
following single line:

	fatal: internal error: pack revindex fubar

This does not happen when given only a single pack, or when given 
multiple packs, but without '-v' option.

To reproduce, simply do:

	git verify-pack -v .git/objects/pack/*.idx

in any repo with more than one pack file.

This happens with a fairly current 'next' (1.6.0.96.g2fad1). AFAICS, it 
also happens in v1.6.0.


...Johan


PS: Does someone knows a better way to generate a sorted list of all the 
blob objects (by SHA1 sum) in a packed repo?

-- 
Johan Herland, [off-list ref]
www.herland.net

On Friday 22 August 2008, Johan Herland wrote:

(I don't have time to look into this right now, but will do so later
if nobody comes up with a solution in the meantime...)

When running 'git verify-pack -v' on multiple packs (.idx files), it
fails for all packs, except the first, with exit code 128, and the
following single line:

	fatal: internal error: pack revindex fubar

This does not happen when given only a single pack, or when given
multiple packs, but without '-v' option.

To reproduce, simply do:

	git verify-pack -v .git/objects/pack/*.idx

in any repo with more than one pack file.

This happens with a fairly current 'next' (1.6.0.96.g2fad1). AFAICS,
it also happens in v1.6.0.

Bisection point to this commit:

commit 1f5c74f6cf918d317c73b328dcd4cf6f55c44d8a
Author: Nicolas Pitre [off-list ref]
Date:   Mon Jun 23 21:22:14 2008 -0400

    call init_pack_revindex() lazily

    This makes life much easier for next patch, as well as being more efficient
    when the revindex is actually not used.

    Signed-off-by: Nicolas Pitre [off-list ref]
    Signed-off-by: Junio C Hamano [off-list ref]


-- 
Johan Herland, [off-list ref]
www.herland.net

This is set to expect failure. Please update when fixing the associated bug.

Signed-off-by: Johan Herland <redacted>
---
 t/t5300-pack-object.sh |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 645583f..339375a 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh

@@ -186,6 +186,12 @@ test_expect_success \
 			test-2-${packname_2}.idx \
 			test-3-${packname_3}.idx'
 
+test_expect_failure \
+    'verify pack -v' \
+    'git verify-pack -v	test-1-${packname_1}.idx \
+			test-2-${packname_2}.idx \
+			test-3-${packname_3}.idx'
+
 test_expect_success \
     'verify-pack catches mismatched .idx and .pack files' \
     'cat test-1-${packname_1}.idx >test-3.idx &&

-- 
1.6.0.96.g2fad1

On Fri, 22 Aug 2008, Johan Herland wrote:

On Friday 22 August 2008, Johan Herland wrote:

quoted

(I don't have time to look into this right now, but will do so later
if nobody comes up with a solution in the meantime...)

When running 'git verify-pack -v' on multiple packs (.idx files), it
fails for all packs, except the first, with exit code 128, and the
following single line:

	fatal: internal error: pack revindex fubar

This does not happen when given only a single pack, or when given
multiple packs, but without '-v' option.

To reproduce, simply do:

	git verify-pack -v .git/objects/pack/*.idx

in any repo with more than one pack file.

This happens with a fairly current 'next' (1.6.0.96.g2fad1). AFAICS,
it also happens in v1.6.0.

Bisection point to this commit:

commit 1f5c74f6cf918d317c73b328dcd4cf6f55c44d8a
Author: Nicolas Pitre [off-list ref]
Date:   Mon Jun 23 21:22:14 2008 -0400

    call init_pack_revindex() lazily

I'll have a look later today.


Nicolas

This is needed to fix verify-pack -v with multiple pack arguments.

Also, in theory, revindex data (if any) must be discarded whenever 
reprepare_packed_git() is called. In practice this is hard to trigger 
though.

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

On Fri, 22 Aug 2008, Johan Herland wrote:

On Friday 22 August 2008, Johan Herland wrote:

quoted

(I don't have time to look into this right now, but will do so later
if nobody comes up with a solution in the meantime...)

When running 'git verify-pack -v' on multiple packs (.idx files), it
fails for all packs, except the first, with exit code 128, and the
following single line:

	fatal: internal error: pack revindex fubar

This does not happen when given only a single pack, or when given
multiple packs, but without '-v' option.

To reproduce, simply do:

	git verify-pack -v .git/objects/pack/*.idx

in any repo with more than one pack file.

This happens with a fairly current 'next' (1.6.0.96.g2fad1). AFAICS,
it also happens in v1.6.0.

Bisection point to this commit:

commit 1f5c74f6cf918d317c73b328dcd4cf6f55c44d8a

This patch should fix it.  Lightly tested, but appears to work for me.

I'm leaving for the weekend now so hopefully I've got it right.

diff --git a/builtin-verify-pack.c b/builtin-verify-pack.c
index f4ac595..c88ca18 100644
--- a/builtin-verify-pack.c
+++ b/builtin-verify-pack.c

@@ -129,6 +129,7 @@ int cmd_verify_pack(int argc, const char **argv, const char *prefix)
 		else {
 			if (verify_one_pack(argv[1], verbose))
 				err = 1;
+			discard_revindex();
 			nothing_done = 0;
 		}
 		argc--; argv++;

diff --git a/pack-revindex.c b/pack-revindex.c
index cd300bd..6096b62 100644
--- a/pack-revindex.c
+++ b/pack-revindex.c

@@ -142,3 +142,15 @@ struct revindex_entry *find_pack_revindex(struct packed_git *p, off_t ofs)
 	} while (lo < hi);
 	die("internal error: pack revindex corrupt");
 }
+
+void discard_revindex(void)
+{
+	if (pack_revindex_hashsz) {
+		int i;
+		for (i = 0; i < pack_revindex_hashsz; i++)
+			if (pack_revindex[i].revindex)
+				free(pack_revindex[i].revindex);
+		free(pack_revindex);
+		pack_revindex_hashsz = 0;
+	}
+}

diff --git a/pack-revindex.h b/pack-revindex.h
index 36a514a..8d5027a 100644
--- a/pack-revindex.h
+++ b/pack-revindex.h

@@ -7,5 +7,6 @@ struct revindex_entry {
 };
 
 struct revindex_entry *find_pack_revindex(struct packed_git *p, off_t ofs);
+void discard_revindex(void);
 
 #endif

diff --git a/sha1_file.c b/sha1_file.c
index 2aff59b..9ee1ed1 100644
--- a/sha1_file.c
+++ b/sha1_file.c

@@ -990,6 +990,7 @@ void prepare_packed_git(void)
 
 void reprepare_packed_git(void)
 {
+	discard_revindex();
 	prepare_packed_git_run_once = 0;
 	prepare_packed_git();
 }

On Friday 22 August 2008, Nicolas Pitre wrote:

This is needed to fix verify-pack -v with multiple pack arguments.

Also, in theory, revindex data (if any) must be discarded whenever
reprepare_packed_git() is called. In practice this is hard to trigger
though.

Signed-off-by: Nicolas Pitre <redacted>

Tested-by: Johan Herland <redacted>

Thanks. Works for me :)

Junio: if you added the my testcase, please squash the following into this 
patch before applying:

diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 339375a..83abe5f 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh

@@ -186,7 +186,7 @@ test_expect_success \
 			test-2-${packname_2}.idx \
 			test-3-${packname_3}.idx'
 
-test_expect_failure \
+test_expect_success \
     'verify pack -v' \
     'git verify-pack -v	test-1-${packname_1}.idx \
 			test-2-${packname_2}.idx \

-- 
1.6.0.96.g2fad1


Have fun! :)

...Johan


-- 
Johan Herland, <johan@herland.net>
www.herland.net