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

[PATCH 1/2] pack-objects: Teach --use-bitmap-index codepath to respect --local, --honor-pack-keep and --incremental

From
Kirill Smelkov <kirr@nexedi.com>
Date
Jul 29, 2016, 07:46 UTC
Message-ID
<20160729074656.23813-1-kirr@nexedi.com>
In-Reply-To
<20160729074051.GA5987@teco.navytux.spb.ru>

Since 6b8fda2d (pack-objects: use bitmaps when packing objects) there are two codepaths in pack-objects: with & without using bitmap reachability index.

However add_object_entry_from_bitmap(), despite its non-bitmapped counterpart add_object_entry(), in no way does check for whether --local or --honor-pack-keep or --incremental should be respected. In non-bitmapped codepath this is handled in want_object_in_pack(), but bitmapped codepath has simply no such checking at all.

The bitmapped codepath however was allowing to pass in all those options and with bitmap indices still being used under such conditions - potentially giving wrong output (e.g. including objects from non-local or .keep'ed pack).

We can easily fix this by noting the following: when an object comes to add_object_entry_from_bitmap() it can come for two reasons:

    1. entries coming from main pack covered by bitmap index, and
    2. object coming from, possibly alternate, loose or other packs.

For "2" we always have pack not yet found by bitmap traversal code, and thus we can simply reuse non-bitmapped want_object_in_pack() to find in which pack an object lives and also for taking omitting decision.

For "1" we always have pack already found by bitmap traversal code and we only need to check that pack for same criteria used in want_object_in_pack() for found_pack.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Discussed-with: Jeff King <peff@peff.net>
---
 builtin/pack-objects.c  |  39 +++++++++++++++++++
 t/t5310-pack-bitmaps.sh | 100 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 139 insertions(+)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index a2f8cfd..34b3019 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -987,6 +987,42 @@ static int want_object_in_pack(const unsigned char *sha1,
 	return 1;
 }
 
+/* Like want_object_in_pack() but for objects coming from-under bitmapped traversal */
+static int want_object_in_pack_bitmap(const unsigned char *sha1,
+				      struct packed_git **found_pack,
+				      off_t *found_offset)
+{
+	struct packed_git *p = *found_pack;
+
+	/*
+	 * There are two types of requests coming here:
+	 * 1. entries coming from main pack covered by bitmap index, and
+	 * 2. object coming from, possibly alternate, loose or other packs.
+	 *
+	 * For "1" we always have *found_pack != NULL passed here from
+	 * traverse_bitmap_commit_list(). (*found_pack is bitmap_git.pack
+	 * actually).
+	 *
+	 * For "2" we always have *found_pack == NULL passed here from
+	 * traverse_bitmap_commit_list() - since this is the way bitmap
+	 * traversal passes here "extended" bitmap entries.
+	 */
+
+	/* objects not covered by bitmap */
+	if (!p)
+		return want_object_in_pack(sha1, 0, found_pack, found_offset);
+
+	/* objects covered by bitmap - we only have to check p wrt local and .keep */
+	if (incremental)
+		return 0;
+	if (local && !p->pack_local)
+		return 0;
+	if (ignore_packed_keep && p->pack_local && p->pack_keep)
+		return 0;
+
+	return 1;
+}
+
 static void create_object_entry(const unsigned char *sha1,
 				enum object_type type,
 				uint32_t hash,
@@ -1055,6 +1091,9 @@ static int add_object_entry_from_bitmap(const unsigned char *sha1,
 	if (have_duplicate_entry(sha1, 0, &index_pos))
 		return 0;
 
+	if (!want_object_in_pack_bitmap(sha1, &pack, &offset))
+		return 0;
+
 	create_object_entry(sha1, type, name_hash, 0, 0, index_pos, pack, offset);
 
 	display_progress(progress_state, nr_result);
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 3893afd..a76f6ca 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -118,6 +118,88 @@ test_expect_success 'incremental repack can disable bitmaps' '
 	git repack -d --no-write-bitmap-index
 '
 
+test_expect_success 'pack-objects respects --local (non-local loose)' '
+	mkdir -p alt_objects/pack &&
+	echo $(pwd)/alt_objects > .git/objects/info/alternates &&
+	echo content1 > file1 &&
+	objsha1=$(GIT_OBJECT_DIRECTORY=alt_objects git hash-object -w file1) &&
+	git add file1 &&
+	test_tick &&
+	git commit -m commit_file1 &&
+	echo HEAD | \
+	git pack-objects --local --stdout --revs >1.pack &&
+	git index-pack 1.pack &&
+	git verify-pack -v 1.pack >1.objects &&
+	if egrep "^$objsha1" 1.objects; then
+		echo "Non-local object present in pack generated with --local: $objsha1"
+		return 1
+	fi
+'
+
+test_expect_success 'pack-objects respects --honor-pack-keep (local non-bitmapped pack)' '
+	echo content2 > file2 &&
+	objsha2=$(git hash-object -w file2) &&
+	git add file2 &&
+	test_tick &&
+	git commit -m commit_file2 &&
+	pack2=$(echo $objsha2 | \
+		git pack-objects pack2) &&
+	mv pack2-$pack2.* .git/objects/pack/ &&
+	touch .git/objects/pack/pack2-$pack2.keep &&
+	rm $(objpath $objsha2) &&
+	echo HEAD | \
+	git pack-objects --honor-pack-keep --stdout --revs >2a.pack &&
+	git index-pack 2a.pack &&
+	git verify-pack -v 2a.pack >2a.objects &&
+	if egrep "^$objsha2" 2a.objects; then
+		echo "Object from .keeped pack present in pack generated with --honor-pack-keep: $objsha2"
+		return 1
+	fi
+'
+
+test_expect_success 'pack-objects respects --local (non-local pack)' '
+	mv .git/objects/pack/pack2-$pack2.* alt_objects/pack/ &&
+	echo HEAD | \
+	git pack-objects --local --stdout --revs >2b.pack &&
+	git index-pack 2b.pack &&
+	git verify-pack -v 2b.pack >2b.objects &&
+	if egrep "^$objsha2" 2b.objects; then
+		echo "Non-local object present in pack generated with --local: $objsha2"
+		return 1
+	fi
+'
+
+test_expect_success 'pack-objects respects --honor-pack-keep (local bitmapped pack)' '
+	ls .git/objects/pack/ | grep bitmap >output &&
+	test_line_count = 1 output &&
+	packbitmap=$(basename $(cat output) .bitmap) &&
+	git verify-pack -v .git/objects/pack/$packbitmap.pack >packbitmap.verify &&
+	grep -o "^$_x40" packbitmap.verify |sort >packbitmap.objects &&
+	touch .git/objects/pack/$packbitmap.keep &&
+	echo HEAD | \
+	git pack-objects --honor-pack-keep --stdout --revs >3a.pack &&
+	git index-pack 3a.pack &&
+	git verify-pack -v 3a.pack >3a.objects &&
+	if grep -qFf packbitmap.objects 3a.objects; then
+		echo "Object from .keeped bitmapped pack present in pack generated with --honour-pack-keep"
+		return 1
+	fi &&
+	rm .git/objects/pack/$packbitmap.keep
+'
+
+test_expect_success 'pack-objects respects --local (non-local bitmapped pack)' '
+	mv .git/objects/pack/$packbitmap.* alt_objects/pack/ &&
+	echo HEAD | \
+	git pack-objects --local --stdout --revs >3b.pack &&
+	git index-pack 3b.pack &&
+	git verify-pack -v 3b.pack >3b.objects &&
+	if grep -qFf packbitmap.objects 3b.objects; then
+		echo "Non-local object from bitmapped pack present in pack generated with --local"
+		return 1
+	fi &&
+	mv alt_objects/pack/$packbitmap.* .git/objects/pack/
+'
+
 test_expect_success 'full repack, reusing previous bitmaps' '
 	git repack -ad &&
 	ls .git/objects/pack/ | grep bitmap >output &&
@@ -143,6 +225,24 @@ test_expect_success 'create objects for missing-HAVE tests' '
 	EOF
 '
 
+test_expect_success 'pack-objects respects --incremental' '
+	cat >revs2 <<-EOF &&
+	HEAD
+	$commit
+	EOF
+	git pack-objects --incremental --stdout --revs <revs2 >4.pack &&
+	git index-pack 4.pack &&
+	git verify-pack -v 4.pack >4.verify &&
+	grep -o "^$_x40" 4.verify |sort >4.objects &&
+	test_line_count = 4 4.objects &&
+	git rev-list --objects $commit >revlist &&
+	grep -o "^$_x40" revlist |sort >objects &&
+	if grep -qvFf objects 4.objects; then
+		echo "Expected objects not present in incremental pack"
+		return 1
+	fi
+'
+
 test_expect_success 'pack with missing blob' '
 	rm $(objpath $blob) &&
 	git pack-objects --stdout --revs <revs >/dev/null

-- 
2.9.0.431.g3cb5c84
Previous: Kirill SmelkovNext: Junio C Hamano
Message 17 of 62 in “pack-objects: Use reachability bitmap index when generating non-stdout pack too”
  1. pack-objects: Use reachability bitmap index when generating non-stdout pack tooKirill Smelkov, Jul 7, 2016
  2. Jeff KingJul 7, 2016
  3. Kirill SmelkovJul 8, 2016
  4. Kirill SmelkovJul 12, 2016
  5. Jeff KingJul 13, 2016
  6. Kirill SmelkovJul 13, 2016
  7. Kirill SmelkovJul 17, 2016
  8. Jeff KingJul 19, 2016
  9. Kirill SmelkovJul 19, 2016
  10. Jeff KingJul 25, 2016
  11. Jeff KingJul 25, 2016
  12. Kirill SmelkovJul 27, 2016
  13. Junio C HamanoJul 27, 2016
  14. Kirill SmelkovJul 28, 2016
  15. Junio C HamanoJul 28, 2016
  16. Kirill SmelkovJul 29, 2016
  17. 1/2 pack-objects: Teach --use-bitmap-index codepath to respect --local, --honor-pack-keep and --incrementalKirill Smelkov, Jul 29, 2016
  18. Junio C HamanoAug 1, 2016
  19. Kirill SmelkovAug 8, 2016
  20. Jeff KingAug 8, 2016
  21. Jeff KingAug 8, 2016
  22. Junio C HamanoAug 8, 2016
  23. Kirill SmelkovAug 8, 2016
  24. pack-objects: Teach --use-bitmap-index codepath to respect --local, --honor-pack-keep and --incrementalKirill Smelkov, Aug 8, 2016
  25. Junio C HamanoAug 8, 2016
  26. Kirill SmelkovAug 9, 2016
  27. 1/2 pack-objects: respect --local/--honor-pack-keep/--incremental when bitmap is in useKirill Smelkov, Aug 9, 2016
  28. Junio C HamanoAug 9, 2016
  29. Kirill SmelkovAug 9, 2016
  30. 1/2 pack-objects: respect --local/--honor-pack-keep/--incremental when bitmap is in useKirill Smelkov, Aug 9, 2016
  31. Jeff KingAug 18, 2016
  32. Kirill SmelkovSep 10, 2016
  33. 1/2 pack-objects: respect --local/--honor-pack-keep/--incremental when bitmap is in useKirill Smelkov, Sep 10, 2016
  34. Junio C HamanoSep 13, 2016
  35. Kirill SmelkovSep 13, 2016
  36. t/perf/run: Don't forget to copy config.mak.autogen & friends to build areaKirill Smelkov, Sep 10, 2016
  37. Junio C HamanoSep 12, 2016
  38. Junio C HamanoSep 12, 2016
  39. Junio C HamanoSep 12, 2016
  40. Kirill SmelkovSep 13, 2016
  41. Junio C HamanoSep 12, 2016
  42. 2/2 pack-objects: use reachability bitmap index when generating non-stdout packKirill Smelkov, Aug 9, 2016
  43. Jeff KingAug 18, 2016
  44. Kirill SmelkovSep 10, 2016
  45. 2/2 pack-objects: use reachability bitmap index when generating non-stdout packKirill Smelkov, Sep 10, 2016
  46. Junio C HamanoSep 12, 2016
  47. Junio C HamanoAug 9, 2016
  48. Junio C HamanoAug 8, 2016
  49. Jeff KingAug 8, 2016
  50. Junio C HamanoAug 8, 2016
  51. 2/2 pack-objects: Teach it to use reachability bitmap index when generating non-stdout pack tooKirill Smelkov, Jul 29, 2016
  52. Jeff KingAug 8, 2016
  53. Kirill SmelkovAug 8, 2016
  54. Junio C HamanoAug 8, 2016
  55. Kirill SmelkovAug 8, 2016
  56. Junio C HamanoAug 8, 2016
  57. Kirill SmelkovAug 8, 2016
  58. pack-objects: teach it to use reachability bitmap index when generating non-stdout pack tooKirill Smelkov, Aug 8, 2016
  59. Junio C HamanoAug 8, 2016
  60. Kirill SmelkovAug 9, 2016
  61. 2/2 pack-objects: use reachability bitmap index when generating non-stdout packKirill Smelkov, Aug 9, 2016
  62. Jeff KingJul 13, 2016

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.