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

Re: [PATCH v2 8/8] builtin/repack.c: pass `--refs-snapshot` when writing bitmaps

From
Taylor Blau <me@ttaylorr.com>
Date
Oct 1, 2021, 22:38 UTC
Message-ID
<YVeN0mXqYvTHtNB+@nand.local>
In-Reply-To
<20210924182247.2922561-1-jonathantanmy@google.com>
On Fri, Sep 24, 2021 at 11:22:47AM -0700, Jonathan Tan wrote:
Show 29 quoted lines
> > +static void midx_snapshot_refs(struct tempfile *f)
> > +{
> > +	struct midx_snapshot_ref_data data;
> > +	const struct string_list *preferred = bitmap_preferred_tips(the_repository);
> > +
> > +	data.f = f;
> > +	oidset_init(&data.seen, 0);
> > +
> > +	if (!fdopen_tempfile(f, "w"))
> > +		 die(_("could not open tempfile %s for writing"),
> > +		     get_tempfile_path(f));
> > +
> > +	if (preferred) {
> > +		struct string_list_item *item;
> > +
> > +		data.preferred = 1;
> > +		for_each_string_list_item(item, preferred)
> > +			for_each_ref_in(item->string, midx_snapshot_ref_one, &data);
> > +		data.preferred = 0;
> > +	}
> > +
> > +	for_each_ref(midx_snapshot_ref_one, &data);
>
> One small thing here - I think "data.preferred = 0;" needs to be moved
> to right above this for_each_ref line. As it is, if preferred is NULL,
> data.preferred wouldn't be initialized.
>
> Other than that (and the minor changes I've suggested in previous
> patches), this series looks good to me.

Eek. Great catch. I was wondering why this didn't bite us at GitHub by marking all refs as preferred, but it doesn't because we always get back a non-NULL string list from bitmap_preferred_tips(), which causes us to set data.preferred back to 0.

Here's a replacement patch which adds the missing `data.preferred = 0` while initializing data in this function, as well as calls oidset_clear() and adds a test. Unfortunately, the new test in t7700 is basically a smoke test, because of the way that bitmap selection works

If Junio would rather I send a reroll of the topic instead, I'm happy to do that, too.

--- 8< ---
Subject: [PATCH] builtin/repack.c: pass `--refs-snapshot` when writing bitmaps

To prevent the race described in an earlier patch, generate and pass a reference snapshot to the multi-pack bitmap code, if we are writing one from `git repack`.

This patch is mostly limited to creating a temporary file, and then calling for_each_ref(). Except we try to minimize duplicates, since doing so can drastically reduce the size in network-of-forks style repositories. In the kernel's fork network (the repository containing all objects from the kernel and all its forks), deduplicating the references drops the snapshot size from 934 MB to just 12 MB.

But since we're handling duplicates in this way, we have to make sure that we preferred references (those listed in pack.preferBitmapTips) before non-preferred ones (to avoid recording an object which is pointed at by a preferred tip as non-preferred).

We accomplish this by doing separate passes over the references: first visiting each prefix in pack.preferBitmapTips, and then over the rest of the references.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/repack.c  | 82 +++++++++++++++++++++++++++++++++++++++++++++++
 t/t7700-repack.sh | 42 ++++++++++++++++++++++++
 2 files changed, 124 insertions(+)
diff --git a/builtin/repack.c b/builtin/repack.c
index 1577f0d59f..9354b2bd93 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -15,6 +15,8 @@
 #include "promisor-remote.h"
 #include "shallow.h"
 #include "pack.h"
+#include "pack-bitmap.h"
+#include "refs.h"

 static int delta_base_offset = 1;
 static int pack_kept_objects = -1;
@@ -453,6 +455,68 @@ static void clear_pack_geometry(struct pack_geometry *geometry)
 	geometry->split = 0;
 }

+struct midx_snapshot_ref_data {
+	struct tempfile *f;
+	struct oidset seen;
+	int preferred;
+};
+
+static int midx_snapshot_ref_one(const char *refname,
+				 const struct object_id *oid,
+				 int flag, void *_data)
+{
+	struct midx_snapshot_ref_data *data = _data;
+	struct object_id peeled;
+
+	if (!peel_iterated_oid(oid, &peeled))
+		oid = &peeled;
+
+	if (oidset_insert(&data->seen, oid))
+		return 0; /* already seen */
+
+	if (oid_object_info(the_repository, oid, NULL) != OBJ_COMMIT)
+		return 0;
+
+	fprintf(data->f->fp, "%s%s\n", data->preferred ? "+" : "",
+		oid_to_hex(oid));
+
+	return 0;
+}
+
+static void midx_snapshot_refs(struct tempfile *f)
+{
+	struct midx_snapshot_ref_data data;
+	const struct string_list *preferred = bitmap_preferred_tips(the_repository);
+
+	data.f = f;
+	data.preferred = 0;
+	oidset_init(&data.seen, 0);
+
+	if (!fdopen_tempfile(f, "w"))
+		 die(_("could not open tempfile %s for writing"),
+		     get_tempfile_path(f));
+
+	if (preferred) {
+		struct string_list_item *item;
+
+		data.preferred = 1;
+		for_each_string_list_item(item, preferred)
+			for_each_ref_in(item->string, midx_snapshot_ref_one, &data);
+		data.preferred = 0;
+	}
+
+	for_each_ref(midx_snapshot_ref_one, &data);
+
+	if (close_tempfile_gently(f)) {
+		int save_errno = errno;
+		delete_tempfile(&f);
+		errno = save_errno;
+		die_errno(_("could not close refs snapshot tempfile"));
+	}
+
+	oidset_clear(&data.seen);
+}
+
 static void midx_included_packs(struct string_list *include,
 				struct string_list *existing_nonkept_packs,
 				struct string_list *existing_kept_packs,
@@ -488,6 +552,7 @@ static void midx_included_packs(struct string_list *include,

 static int write_midx_included_packs(struct string_list *include,
 				     struct pack_geometry *geometry,
+				     const char *refs_snapshot,
 				     int show_progress, int write_bitmaps)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
@@ -517,6 +582,9 @@ static int write_midx_included_packs(struct string_list *include,
 		strvec_pushf(&cmd.args, "--preferred-pack=%s",
 			     pack_basename(largest));

+	if (refs_snapshot)
+		strvec_pushf(&cmd.args, "--refs-snapshot=%s", refs_snapshot);
+
 	ret = start_command(&cmd);
 	if (ret)
 		return ret;
@@ -539,6 +607,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	struct string_list existing_kept_packs = STRING_LIST_INIT_DUP;
 	struct pack_geometry *geometry = NULL;
 	struct strbuf line = STRBUF_INIT;
+	struct tempfile *refs_snapshot = NULL;
 	int i, ext, ret;
 	FILE *out;
 	int show_progress = isatty(2);
@@ -627,6 +696,18 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	if (write_bitmaps && !(pack_everything & ALL_INTO_ONE) && !write_midx)
 		die(_(incremental_bitmap_conflict_error));

+	if (write_midx && write_bitmaps) {
+		struct strbuf path = STRBUF_INIT;
+
+		strbuf_addf(&path, "%s/%s_XXXXXX", get_object_directory(),
+			    "bitmap-ref-tips");
+
+		refs_snapshot = xmks_tempfile(path.buf);
+		midx_snapshot_refs(refs_snapshot);
+
+		strbuf_release(&path);
+	}
+
 	if (geometric_factor) {
 		if (pack_everything)
 			die(_("--geometric is incompatible with -A, -a"));
@@ -809,6 +890,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 				    &existing_kept_packs, &names, geometry);

 		ret = write_midx_included_packs(&include, geometry,
+						refs_snapshot ? get_tempfile_path(refs_snapshot) : NULL,
 						show_progress, write_bitmaps > 0);

 		string_list_clear(&include, 0);
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 6792531dfd..0260ad6f0e 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -330,4 +330,46 @@ test_expect_success 'cleans up MIDX when appropriate' '
 	)
 '

+test_expect_success '--write-midx with preferred bitmap tips' '
+	git init midx-preferred-tips &&
+	test_when_finished "rm -fr midx-preferred-tips" &&
+	(
+		cd midx-preferred-tips &&
+
+		test_commit_bulk --message="%s" 103 &&
+
+		git log --format="%H" >commits.raw &&
+		sort <commits.raw >commits &&
+
+		git log --format="create refs/tags/%s/%s %H" HEAD >refs &&
+		git update-ref --stdin <refs &&
+
+		git repack --write-midx --write-bitmap-index &&
+		test_path_is_file $midx &&
+		test_path_is_file $midx-$(midx_checksum $objdir).bitmap &&
+
+		test-tool bitmap list-commits | sort >bitmaps &&
+		comm -13 bitmaps commits >before &&
+		test_line_count = 1 before &&
+
+		rm -fr $midx-$(midx_checksum $objdir).bitmap &&
+		rm -fr $midx-$(midx_checksum $objdir).rev &&
+		rm -fr $midx &&
+
+		# instead of constructing the snapshot ourselves (c.f., the test
+		# "write a bitmap with --refs-snapshot (preferred tips)" in
+		# t5326), mark the missing commit as preferred by adding it to
+		# the pack.preferBitmapTips configuration.
+		git for-each-ref --format="%(refname:rstrip=1)" \
+			--points-at="$(cat before)" >missing &&
+		git config pack.preferBitmapTips "$(cat missing)" &&
+		git repack --write-midx --write-bitmap-index &&
+
+		test-tool bitmap list-commits | sort >bitmaps &&
+		comm -13 bitmaps commits >after &&
+
+		! test_cmp before after
+	)
+'
+
 test_done
--
2.33.0.96.g73915697e6
Previous: Jonathan TanNext: Junio C Hamano
Message 59 of 76 in “repack: introduce `--write-midx`”
  1. 0/8 repack: introduce `--write-midx`Taylor Blau, Sep 11, 2021
  2. 1/8 midx: expose 'write_midx_file_only()' publiclyTaylor Blau, Sep 11, 2021
  3. Junio C HamanoSep 11, 2021
  4. Taylor BlauSep 11, 2021
  5. Ævar Arnfjörð BjarmasonSep 11, 2021
  6. Taylor BlauSep 11, 2021
  7. 2/8 builtin/multi-pack-index.c: support --stdin-packs modeTaylor Blau, Sep 11, 2021
  8. Ævar Arnfjörð BjarmasonSep 11, 2021
  9. Taylor BlauSep 11, 2021
  10. Taylor BlauSep 11, 2021
  11. Eric SunshineSep 12, 2021
  12. Taylor BlauSep 12, 2021
  13. Ævar Arnfjörð BjarmasonSep 12, 2021
  14. Junio C HamanoSep 12, 2021
  15. Ævar Arnfjörð BjarmasonSep 12, 2021
  16. Jeff KingSep 14, 2021
  17. Taylor BlauSep 14, 2021
  18. Eric SunshineSep 15, 2021
  19. 4/8 builtin/repack.c: keep track of existing packs unconditionallyTaylor Blau, Sep 11, 2021
  20. 3/8 midx: preliminary support for `--refs-snapshot`Taylor Blau, Sep 11, 2021
  21. 5/8 builtin/repack.c: extract showing progress to a variableTaylor Blau, Sep 11, 2021
  22. 6/8 builtin/repack.c: support writing a MIDX while repackingTaylor Blau, Sep 11, 2021
  23. 7/8 builtin/repack.c: make largest pack preferredTaylor Blau, Sep 11, 2021
  24. Ævar Arnfjörð BjarmasonSep 11, 2021
  25. Taylor BlauSep 11, 2021
  26. 8/8 builtin/repack.c: pass `--refs-snapshot` when writing bitmapsTaylor Blau, Sep 11, 2021
  27. Ævar Arnfjörð BjarmasonSep 11, 2021
  28. Ævar Arnfjörð BjarmasonSep 11, 2021
  29. Taylor BlauSep 11, 2021
  30. Jeff KingSep 14, 2021
  31. Taylor BlauSep 14, 2021
  32. Ævar Arnfjörð BjarmasonSep 14, 2021
  33. Taylor BlauSep 15, 2021
  34. Taylor BlauSep 11, 2021
  35. 0/8 repack: introduce `--write-midx`Taylor Blau, Sep 15, 2021
  36. 1/8 midx: expose `write_midx_file_only()` publiclyTaylor Blau, Sep 15, 2021
  37. Jonathan TanSep 22, 2021
  38. Taylor BlauSep 23, 2021
  39. 2/8 builtin/multi-pack-index.c: support `--stdin-packs` modeTaylor Blau, Sep 15, 2021
  40. Josh SteadmonSep 22, 2021
  41. Taylor BlauSep 23, 2021
  42. Jonathan TanSep 22, 2021
  43. Taylor BlauSep 23, 2021
  44. 3/8 midx: preliminary support for `--refs-snapshot`Taylor Blau, Sep 15, 2021
  45. Josh SteadmonSep 22, 2021
  46. Taylor BlauSep 23, 2021
  47. Jonathan TanSep 22, 2021
  48. Taylor BlauSep 23, 2021
  49. 4/8 builtin/repack.c: keep track of existing packs unconditionallyTaylor Blau, Sep 15, 2021
  50. Jonathan TanSep 22, 2021
  51. Taylor BlauSep 23, 2021
  52. 5/8 builtin/repack.c: extract showing progress to a variableTaylor Blau, Sep 15, 2021
  53. 6/8 builtin/repack.c: support writing a MIDX while repackingTaylor Blau, Sep 15, 2021
  54. Jonathan TanSep 22, 2021
  55. Taylor BlauSep 23, 2021
  56. 7/8 builtin/repack.c: make largest pack preferredTaylor Blau, Sep 15, 2021
  57. 8/8 builtin/repack.c: pass `--refs-snapshot` when writing bitmapsTaylor Blau, Sep 15, 2021
  58. Jonathan TanSep 24, 2021
  59. Taylor BlauOct 1, 2021
  60. Junio C HamanoSep 15, 2021
  61. Junio C HamanoSep 15, 2021
  62. Taylor BlauSep 15, 2021
  63. Junio C HamanoSep 16, 2021
  64. 0/9 repack: introduce `--write-midx`Taylor Blau, Sep 29, 2021
  65. 1/9 midx: expose `write_midx_file_only()` publiclyTaylor Blau, Sep 29, 2021
  66. 2/9 builtin/multi-pack-index.c: support `--stdin-packs` modeTaylor Blau, Sep 29, 2021
  67. 3/9 midx: preliminary support for `--refs-snapshot`Taylor Blau, Sep 29, 2021
  68. 4/9 builtin/repack.c: keep track of existing packs unconditionallyTaylor Blau, Sep 29, 2021
  69. 5/9 builtin/repack.c: rename variables that deal with non-kept packsTaylor Blau, Sep 29, 2021
  70. 6/9 builtin/repack.c: extract showing progress to a variableTaylor Blau, Sep 29, 2021
  71. 7/9 builtin/repack.c: support writing a MIDX while repackingTaylor Blau, Sep 29, 2021
  72. 8/9 builtin/repack.c: make largest pack preferredTaylor Blau, Sep 29, 2021
  73. 9/9 builtin/repack.c: pass `--refs-snapshot` when writing bitmapsTaylor Blau, Sep 29, 2021
  74. Junio C HamanoSep 29, 2021
  75. Jonathan TanOct 1, 2021
  76. Taylor BlauOct 1, 2021

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.