git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 04/12] packfile: check midx coverage with .idx rather than .pack

From
Jeff King <peff@peff.net>
Date
Apr 5, 2019, 13:40 UTC
Message-ID
<20190405134037.GC1460@sigill.intra.peff.net>
In-Reply-To
<20190405120120.GA8796@szeder.dev>
On Fri, Apr 05, 2019 at 02:01:20PM +0200, SZEDER Gábor wrote:
Show 20 quoted lines
> > The tests don't notice because there's nothing about opening those .idx
> > files that would cause us to give incorrect output. It's just a little
> > slower. The new test checks this case by corrupting the covered .idx,
> > and then making sure we don't complain about it.
> 
> When the test suite is run with GIT_TEST_MULTI_PACK_INDEX=1, the
> following tests fail with this patch:
> 
>   t1006-cat-file.sh                                (Wstat: 256 Tests: 111 Failed: 1)
>     Failed test:  87
>     Non-zero exit status: 1
>   t5320-delta-islands.sh                           (Wstat: 256 Tests: 15 Failed: 10)
>     Failed tests:  2-4, 6-10, 12-13
>     Non-zero exit status: 1
>   t5310-pack-bitmaps.sh                            (Wstat: 256 Tests: 46 Failed: 1)
>     Failed test:  44
>     Non-zero exit status: 1
>   t5570-git-daemon.sh                              (Wstat: 256 Tests: 21 Failed: 1)
>     Failed test:  10
>     Non-zero exit status: 1

Thanks. It looks like this is not a bug in my patch per se, but rather that the feature of the midx code it is un-breaking is not actually a good idea.

The problem is with generating the pack revindex. It needs the actual index for each pack to be loaded, and now open_packed_git() will sometimes quietly not do that.

Probably we _could_ be generating a big revindex off of the midx, but in the meantime, the revindex code needs to make sure that the index is loaded before looking at it.

This patch clears up those test failures (I'll re-roll the whole series with it in the proper spot).

---
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 4695aaf6b4..3960ad94c8 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -308,7 +308,8 @@ static int load_pack_bitmap(struct bitmap_index *bitmap_git)
 
 	bitmap_git->bitmaps = kh_init_sha1();
 	bitmap_git->ext_index.positions = kh_init_sha1_pos();
-	load_pack_revindex(bitmap_git->pack);
+	if (load_pack_revindex(bitmap_git->pack))
+		goto failed;
 
 	if (!(bitmap_git->commits = read_bitmap_1(bitmap_git)) ||
 		!(bitmap_git->trees = read_bitmap_1(bitmap_git)) ||
diff --git a/pack-revindex.c b/pack-revindex.c
index 50891f77a2..d28a7e43d0 100644
--- a/pack-revindex.c
+++ b/pack-revindex.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "pack-revindex.h"
 #include "object-store.h"
+#include "packfile.h"
 
 /*
  * Pack index for existing packs give us easy access to the offsets into
@@ -158,10 +159,14 @@ static void create_pack_revindex(struct packed_git *p)
 	sort_revindex(p->revindex, num_ent, p->pack_size);
 }
 
-void load_pack_revindex(struct packed_git *p)
+int load_pack_revindex(struct packed_git *p)
 {
-	if (!p->revindex)
+	if (!p->revindex) {
+		if (open_pack_index(p))
+			return -1;
 		create_pack_revindex(p);
+	}
+	return 0;
 }
 
 int find_revindex_position(struct packed_git *p, off_t ofs)
@@ -188,7 +193,9 @@ struct revindex_entry *find_pack_revindex(struct packed_git *p, off_t ofs)
 {
 	int pos;
 
-	load_pack_revindex(p);
+	if (load_pack_revindex(p))
+		return NULL;
+
 	pos = find_revindex_position(p, ofs);
 
 	if (pos < 0)
diff --git a/pack-revindex.h b/pack-revindex.h
index e262f3efe8..848331d5d6 100644
--- a/pack-revindex.h
+++ b/pack-revindex.h
@@ -8,7 +8,7 @@ struct revindex_entry {
 	unsigned int nr;
 };
 
-void load_pack_revindex(struct packed_git *p);
+int load_pack_revindex(struct packed_git *p);
 int find_revindex_position(struct packed_git *p, off_t ofs);
 
 struct revindex_entry *find_pack_revindex(struct packed_git *p, off_t ofs);
diff --git a/packfile.c b/packfile.c
index e7ca135ed5..ca73a5b12a 100644
--- a/packfile.c
+++ b/packfile.c
@@ -2040,8 +2040,10 @@ int for_each_object_in_pack(struct packed_git *p,
 	uint32_t i;
 	int r = 0;
 
-	if (flags & FOR_EACH_OBJECT_PACK_ORDER)
-		load_pack_revindex(p);
+	if (flags & FOR_EACH_OBJECT_PACK_ORDER) {
+		if (load_pack_revindex(p))
+			return -1;
+	}
 
 	for (i = 0; i < p->num_objects; i++) {
 		uint32_t pos;
Previous: SZEDER GáborNext: Jeff King
Message 13 of 48 in “a rabbit hole of update-server-info fixes”
  1. 0/12 a rabbit hole of update-server-info fixesJeff King, Apr 4, 2019
  2. 01/12 t5319: fix bogus cat-file argumentJeff King, Apr 4, 2019
  3. Ramsay JonesApr 5, 2019
  4. Jeff KingApr 5, 2019
  5. Jeff KingApr 5, 2019
  6. 02/12 t5319: drop useless --buffer from cat-fileJeff King, Apr 4, 2019
  7. 03/12 packfile: factor out .pack to .idx name conversionJeff King, Apr 4, 2019
  8. 04/12 packfile: check midx coverage with .idx rather than .packJeff King, Apr 4, 2019
  9. René ScharfeApr 5, 2019
  10. Jeff KingApr 5, 2019
  11. Jeff KingApr 5, 2019
  12. SZEDER GáborApr 5, 2019
  13. Jeff KingApr 5, 2019
  14. 05/12 http: simplify parsing of remote objects/info/packsJeff King, Apr 4, 2019
  15. René ScharfeApr 5, 2019
  16. Jeff KingApr 5, 2019
  17. René ScharfeApr 5, 2019
  18. 06/12 server-info: fix blind pointer arithmeticJeff King, Apr 4, 2019
  19. 07/12 server-info: simplify cleanup in parse_pack_def()Jeff King, Apr 4, 2019
  20. 08/12 server-info: use strbuf to read old info/packs fileJeff King, Apr 4, 2019
  21. 09/12 server-info: drop nr_alloc struct memberJeff King, Apr 4, 2019
  22. 10/12 packfile.h: drop extern from function declarationsJeff King, Apr 4, 2019
  23. 11/12 server-info: drop objdirlen pointer arithmeticJeff King, Apr 4, 2019
  24. 12/12 update_info_refs(): drop unused force parameterJeff King, Apr 4, 2019
  25. Junio C HamanoApr 5, 2019
  26. 0/13 a rabbit hole of update-server-info (and midx!) fixesJeff King, Apr 5, 2019
  27. 01/13 packfile.h: drop extern from function declarationsJeff King, Apr 5, 2019
  28. Ramsay JonesApr 5, 2019
  29. Jeff KingApr 5, 2019
  30. Junio C HamanoApr 8, 2019
  31. Jeff KingApr 8, 2019
  32. Junio C HamanoApr 9, 2019
  33. Jeff KingApr 9, 2019
  34. 02/13 pack-revindex: open index if necessaryJeff King, Apr 5, 2019
  35. 03/13 t5319: fix bogus cat-file argumentJeff King, Apr 5, 2019
  36. 04/13 t5319: drop useless --buffer from cat-fileJeff King, Apr 5, 2019
  37. 05/13 midx: check both pack and index names for containmentJeff King, Apr 5, 2019
  38. René ScharfeApr 5, 2019
  39. 06/13 packfile: fix pack basename computationJeff King, Apr 5, 2019
  40. 07/13 http: simplify parsing of remote objects/info/packsJeff King, Apr 5, 2019
  41. 08/13 server-info: fix blind pointer arithmeticJeff King, Apr 5, 2019
  42. 09/13 server-info: simplify cleanup in parse_pack_def()Jeff King, Apr 5, 2019
  43. Jeff KingApr 5, 2019
  44. 10/13 server-info: use strbuf to read old info/packs fileJeff King, Apr 5, 2019
  45. 11/13 server-info: drop nr_alloc struct memberJeff King, Apr 5, 2019
  46. 12/13 server-info: drop objdirlen pointer arithmeticJeff King, Apr 5, 2019
  47. 13/13 update_info_refs(): drop unused force parameterJeff King, Apr 5, 2019
  48. Jeff KingApr 5, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.