[PATCH 0/3] pack-objects: introduce pack.preferBitmapTips

[PATCH 0/3] pack-objects: introduce pack.preferBitmapTips

[Taylor Blau]

This series adds a new multi-valued configuration, pack.preferBitmapTips
to allow callers to specify a list of reference prefixes which should
mark their tips with the NEEDS_BITMAP flag.

This has a similar effect as setting pack.islandCore: it indicates to
the bitmap selection code that any commit object with that flag should
get bitmap coverage (it doesn't necessarily imply that it *will*, see
the third patch for details of why).

This is more fall-out from the multi-pack bitmaps topic. Since we
generate MIDX bitmaps after a geometric repack, any delta-islands
configuration only takes effect on objects in the newly-created pack.
Any commits which weren't in that pack may not get the same coverage they
otherwise would have if repacking all-into-one with the same
delta-islands settings.

So, this config was designed to suggest which commits should get marked
with NEEDS_BITMAP when the MIDX bitmap code does its own traversal to
figure out which reachable objects are in the MIDX. But it's useful for
single-pack bitmaps, too, if you're using them without delta islands.

(Like all of the MIDX bitmap topics, we have been running this code at
GitHub without any issue for a few weeks.)

Thanks in advance for your review.

Taylor Blau (3):
  pack-bitmap: add 'test_bitmap_commits()' helper
  t/helper/test-bitmap.c: initial commit
  builtin/pack-objects.c: respect 'pack.preferBitmapTips'

 Documentation/config/pack.txt | 15 ++++++++++++++
 Makefile                      |  1 +
 builtin/pack-objects.c        | 34 +++++++++++++++++++++++++++++++
 pack-bitmap.c                 | 24 ++++++++++++++++++++++
 pack-bitmap.h                 |  4 ++++
 t/helper/test-bitmap.c        | 24 ++++++++++++++++++++++
 t/helper/test-tool.c          |  1 +
 t/helper/test-tool.h          |  1 +
 t/t5310-pack-bitmaps.sh       | 38 +++++++++++++++++++++++++++++++++++
 9 files changed, 142 insertions(+)
 create mode 100644 t/helper/test-bitmap.c

-- 
2.31.1.163.ga65ce7f831

[PATCH 1/3] pack-bitmap: add 'test_bitmap_commits()' helper

[Taylor Blau]

The next patch will add a 'bitmap' test-tool which prints the list of
commits that have bitmaps computed.

The test helper could implement this itself, but it would need access to
the 'bitmaps' field of the 'pack_bitmap' struct. To avoid exposing this
private detail, implement the entirety of the helper behind a
test_bitmap_commits() function in pack-bitmap.c.

There is some precedence for this with test_bitmap_walk() which is used
to implement the '--test-bitmap' flag in 'git rev-list' (and is also
implemented in pack-bitmap.c).

A caller will be added in the next patch.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 pack-bitmap.c | 18 ++++++++++++++++++
 pack-bitmap.h |  1 +
 2 files changed, 19 insertions(+)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 1ebe0c8162..7554510b14 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -1350,6 +1350,24 @@ void test_bitmap_walk(struct rev_info *revs)
 	free_bitmap_index(bitmap_git);
 }
 
+int test_bitmap_commits(struct repository *r)
+{
+	struct bitmap_index *bitmap_git = prepare_bitmap_git(r);
+	struct object_id oid;
+	MAYBE_UNUSED void *value;
+
+	if (!bitmap_git)
+		die("failed to load bitmap indexes");
+
+	kh_foreach(bitmap_git->bitmaps, oid, value, {
+		printf("%s\n", oid_to_hex(&oid));
+	});
+
+	free_bitmap_index(bitmap_git);
+
+	return 0;
+}
+
 int rebuild_bitmap(const uint32_t *reposition,
 		   struct ewah_bitmap *source,
 		   struct bitmap *dest)
diff --git a/pack-bitmap.h b/pack-bitmap.h
index 36d99930d8..c3cdd80756 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -49,6 +49,7 @@ void traverse_bitmap_commit_list(struct bitmap_index *,
 				 struct rev_info *revs,
 				 show_reachable_fn show_reachable);
 void test_bitmap_walk(struct rev_info *revs);
+int test_bitmap_commits(struct repository *r);
 struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs,
 					 struct list_objects_filter_options *filter);
 int reuse_partial_packfile_from_bitmap(struct bitmap_index *,
-- 
2.31.1.163.ga65ce7f831

[PATCH 2/3] t/helper/test-bitmap.c: initial commit

[Taylor Blau]

Add a new 'bitmap' test-tool which can be used to list the commits that
have received bitmaps.

In theory, a determined tester could run 'git rev-list --test-bitmap
<commit>' to check if '<commit>' received a bitmap or not, since
'--test-bitmap' exits with a non-zero code when it can't find the
requested commit.

But this is a dubious behavior to rely on, since arguably 'git
rev-list' could continue its object walk outside of which commits are
covered by bitmaps.

This will be used to test the behavior of 'pack.preferBitmapTips', which
will be added in the following patch.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Makefile               |  1 +
 t/helper/test-bitmap.c | 24 ++++++++++++++++++++++++
 t/helper/test-tool.c   |  1 +
 t/helper/test-tool.h   |  1 +
 4 files changed, 27 insertions(+)
 create mode 100644 t/helper/test-bitmap.c

diff --git a/Makefile b/Makefile
index 55c8035fa8..89b30495eb 100644
--- a/Makefile
+++ b/Makefile
@@ -693,6 +693,7 @@ X =
 PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
 
 TEST_BUILTINS_OBJS += test-advise.o
+TEST_BUILTINS_OBJS += test-bitmap.o
 TEST_BUILTINS_OBJS += test-bloom.o
 TEST_BUILTINS_OBJS += test-chmtime.o
 TEST_BUILTINS_OBJS += test-config.o
diff --git a/t/helper/test-bitmap.c b/t/helper/test-bitmap.c
new file mode 100644
index 0000000000..134a1e9d76
--- /dev/null
+++ b/t/helper/test-bitmap.c
@@ -0,0 +1,24 @@
+#include "test-tool.h"
+#include "cache.h"
+#include "pack-bitmap.h"
+
+static int bitmap_list_commits(void)
+{
+	return test_bitmap_commits(the_repository);
+}
+
+int cmd__bitmap(int argc, const char **argv)
+{
+	setup_git_directory();
+
+	if (argc != 2)
+		goto usage;
+
+	if (!strcmp(argv[1], "list-commits"))
+		return bitmap_list_commits();
+
+usage:
+	usage("\ttest-tool bitmap list-commits");
+
+	return -1;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index f97cd9f48a..a48bd44116 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -15,6 +15,7 @@ struct test_cmd {
 
 static struct test_cmd cmds[] = {
 	{ "advise", cmd__advise_if_enabled },
+	{ "bitmap", cmd__bitmap },
 	{ "bloom", cmd__bloom },
 	{ "chmtime", cmd__chmtime },
 	{ "config", cmd__config },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 28072c0ad5..563fe1b030 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -5,6 +5,7 @@
 #include "git-compat-util.h"
 
 int cmd__advise_if_enabled(int argc, const char **argv);
+int cmd__bitmap(int argc, const char **argv);
 int cmd__bloom(int argc, const char **argv);
 int cmd__chmtime(int argc, const char **argv);
 int cmd__config(int argc, const char **argv);
-- 
2.31.1.163.ga65ce7f831

SunCC doesn't compile v2.32.0-rc* anymore (was "Re: [PATCH 2/3] t/helper/test-bitmap.c: initial commit")

[Ævar Arnfjörð Bjarmason]

On Wed, Mar 31 2021, Taylor Blau wrote:

> Add a new 'bitmap' test-tool which can be used to list the commits that
> have received bitmaps.
> [...]
> @@ -0,0 +1,24 @@
> +#include "test-tool.h"
> +#include "cache.h"
> +#include "pack-bitmap.h"

Since this commit SunCC (on Solaris) refuses to compile git, a new bug
in v2.32.0-rc*.

It's because it can't find the oe_get_size_slow symbol, you include
pack-bitmap.h, which in turn includes pack-objects.h. That file
references oe_get_size_slow, but it's only defined in
builtin/pack-objects.c.

That looseness of definitions far pre-dates v2.32.0, but I suspect we
got away with it due to builtins including everything (or something)
anyway, and that this is the first test-tool usage.

Re: SunCC doesn't compile v2.32.0-rc* anymore (was "Re: [PATCH 2/3] t/helper/test-bitmap.c: initial commit")

[Taylor Blau]

On Wed, May 26, 2021 at 08:30:49PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Wed, Mar 31 2021, Taylor Blau wrote:
>
> > Add a new 'bitmap' test-tool which can be used to list the commits that
> > have received bitmaps.
> > [...]
> > @@ -0,0 +1,24 @@
> > +#include "test-tool.h"
> > +#include "cache.h"
> > +#include "pack-bitmap.h"
>
> Since this commit SunCC (on Solaris) refuses to compile git, a new bug
> in v2.32.0-rc*.
>
> It's because it can't find the oe_get_size_slow symbol, you include
> pack-bitmap.h, which in turn includes pack-objects.h. That file
> references oe_get_size_slow, but it's only defined in
> builtin/pack-objects.c.

I'm not sure I understand. pack-objects.h has a declaration of that
method, but the implementation is in builtin/pack-objects.c. That should
be fine, but I don't know about how SunCC works.

What needs to be changed here?

Thanks,
Taylor

Re: SunCC doesn't compile v2.32.0-rc* anymore (was "Re: [PATCH 2/3] t/helper/test-bitmap.c: initial commit")

[Ævar Arnfjörð Bjarmason]

On Wed, May 26 2021, Taylor Blau wrote:

> On Wed, May 26, 2021 at 08:30:49PM +0200, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Wed, Mar 31 2021, Taylor Blau wrote:
>>
>> > Add a new 'bitmap' test-tool which can be used to list the commits that
>> > have received bitmaps.
>> > [...]
>> > @@ -0,0 +1,24 @@
>> > +#include "test-tool.h"
>> > +#include "cache.h"
>> > +#include "pack-bitmap.h"
>>
>> Since this commit SunCC (on Solaris) refuses to compile git, a new bug
>> in v2.32.0-rc*.
>>
>> It's because it can't find the oe_get_size_slow symbol, you include
>> pack-bitmap.h, which in turn includes pack-objects.h. That file
>> references oe_get_size_slow, but it's only defined in
>> builtin/pack-objects.c.
>
> I'm not sure I understand. pack-objects.h has a declaration of that
> method, but the implementation is in builtin/pack-objects.c. That should
> be fine, but I don't know about how SunCC works.
>
> What needs to be changed here?

I think that oe_get_size_slow needs to be in libgit as long as
pack-objects.h defines other (inline) functions that reference it, or
maybe most of that stuff can just be moved to builtin/pack-objects.h?

This obviously not OK patch makes things "ok" again under SunCC:
    
    diff --git a/Makefile b/Makefile
    index c3565fc0f8f..c2b05aeddac 100644
    --- a/Makefile
    +++ b/Makefile
    @@ -1186,7 +1186,7 @@ THIRD_PARTY_SOURCES += compat/regex/%
     THIRD_PARTY_SOURCES += sha1collisiondetection/%
     THIRD_PARTY_SOURCES += sha1dc/%
    
    -GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB)
    +GITLIBS = builtin/pack-objects.o common-main.o $(LIB_FILE) $(XDIFF_LIB)
     EXTLIBS =
    
     GIT_USER_AGENT = git/$(GIT_VERSION)

I.e. the problem is that in the final program we're linking there's
references to this function we can't find:
    
    Undefined                       first referenced
     symbol                             in file
    oe_get_size_slow                    t/helper/test-bitmap.o
    ld: fatal: symbol referencing errors. No output written to t/helper/test-tool

I think e.g. the GNU toolchain and others are probably OK with it
because they do some analysis to figure out that none of those inline
functions that need oe_get_size_slow are themselves needed. SunCC (or
Solaris's) linker seems to be more eager.

It seems to me that the best way to solve this is something like the
below code-move-only patch of just moving this stuff only used by
builtin/pack-objects.c to that file. This also fixes the build on
Solaris.

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 6ded130e45b..70072b07b6b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -37,6 +37,100 @@
 #include "shallow.h"
 #include "promisor-remote.h"
 
+/*
+ * Objects we are going to pack are collected in the `to_pack` structure.
+ * It contains an array (dynamically expanded) of the object data, and a map
+ * that can resolve SHA1s to their position in the array.
+ */
+static struct packing_data to_pack;
+
+/*
+ * Return the size of the object without doing any delta
+ * reconstruction (so non-deltas are true object sizes, but deltas
+ * return the size of the delta data).
+ */
+unsigned long oe_get_size_slow(struct packing_data *pack,
+			       const struct object_entry *e);
+unsigned long oe_get_size_slow(struct packing_data *pack,
+			       const struct object_entry *e)
+{
+	struct packed_git *p;
+	struct pack_window *w_curs;
+	unsigned char *buf;
+	enum object_type type;
+	unsigned long used, avail, size;
+
+	if (e->type_ != OBJ_OFS_DELTA && e->type_ != OBJ_REF_DELTA) {
+		packing_data_lock(&to_pack);
+		if (oid_object_info(the_repository, &e->idx.oid, &size) < 0)
+			die(_("unable to get size of %s"),
+			    oid_to_hex(&e->idx.oid));
+		packing_data_unlock(&to_pack);
+		return size;
+	}
+
+	p = oe_in_pack(pack, e);
+	if (!p)
+		BUG("when e->type is a delta, it must belong to a pack");
+
+	packing_data_lock(&to_pack);
+	w_curs = NULL;
+	buf = use_pack(p, &w_curs, e->in_pack_offset, &avail);
+	used = unpack_object_header_buffer(buf, avail, &type, &size);
+	if (used == 0)
+		die(_("unable to parse object header of %s"),
+		    oid_to_hex(&e->idx.oid));
+
+	unuse_pack(&w_curs);
+	packing_data_unlock(&to_pack);
+	return size;
+}
+
+static inline unsigned long oe_size(struct packing_data *pack,
+				    const struct object_entry *e)
+{
+	if (e->size_valid)
+		return e->size_;
+
+	return oe_get_size_slow(pack, e);
+}
+
+static inline int oe_size_less_than(struct packing_data *pack,
+				    const struct object_entry *lhs,
+				    unsigned long rhs)
+{
+	if (lhs->size_valid)
+		return lhs->size_ < rhs;
+	if (rhs < pack->oe_size_limit) /* rhs < 2^x <= lhs ? */
+		return 0;
+	return oe_get_size_slow(pack, lhs) < rhs;
+}
+
+static inline int oe_size_greater_than(struct packing_data *pack,
+				       const struct object_entry *lhs,
+				       unsigned long rhs)
+{
+	if (lhs->size_valid)
+		return lhs->size_ > rhs;
+	if (rhs < pack->oe_size_limit) /* rhs < 2^x <= lhs ? */
+		return 1;
+	return oe_get_size_slow(pack, lhs) > rhs;
+}
+
+static inline void oe_set_size(struct packing_data *pack,
+			       struct object_entry *e,
+			       unsigned long size)
+{
+	if (size < pack->oe_size_limit) {
+		e->size_ = size;
+		e->size_valid = 1;
+	} else {
+		e->size_valid = 0;
+		if (oe_get_size_slow(pack, e) != size)
+			BUG("'size' is supposed to be the object size!");
+	}
+}
+
 #define IN_PACK(obj) oe_in_pack(&to_pack, obj)
 #define SIZE(obj) oe_size(&to_pack, obj)
 #define SET_SIZE(obj,size) oe_set_size(&to_pack, obj, size)
@@ -56,13 +150,6 @@ static const char *pack_usage[] = {
 	NULL
 };
 
-/*
- * Objects we are going to pack are collected in the `to_pack` structure.
- * It contains an array (dynamically expanded) of the object data, and a map
- * that can resolve SHA1s to their position in the array.
- */
-static struct packing_data to_pack;
-
 static struct pack_idx_entry **written_list;
 static uint32_t nr_result, nr_written, nr_seen;
 static struct bitmap_index *bitmap_git;
@@ -2231,46 +2318,6 @@ static pthread_mutex_t progress_mutex;
  * progress_mutex for protection.
  */
 
-/*
- * Return the size of the object without doing any delta
- * reconstruction (so non-deltas are true object sizes, but deltas
- * return the size of the delta data).
- */
-unsigned long oe_get_size_slow(struct packing_data *pack,
-			       const struct object_entry *e)
-{
-	struct packed_git *p;
-	struct pack_window *w_curs;
-	unsigned char *buf;
-	enum object_type type;
-	unsigned long used, avail, size;
-
-	if (e->type_ != OBJ_OFS_DELTA && e->type_ != OBJ_REF_DELTA) {
-		packing_data_lock(&to_pack);
-		if (oid_object_info(the_repository, &e->idx.oid, &size) < 0)
-			die(_("unable to get size of %s"),
-			    oid_to_hex(&e->idx.oid));
-		packing_data_unlock(&to_pack);
-		return size;
-	}
-
-	p = oe_in_pack(pack, e);
-	if (!p)
-		BUG("when e->type is a delta, it must belong to a pack");
-
-	packing_data_lock(&to_pack);
-	w_curs = NULL;
-	buf = use_pack(p, &w_curs, e->in_pack_offset, &avail);
-	used = unpack_object_header_buffer(buf, avail, &type, &size);
-	if (used == 0)
-		die(_("unable to parse object header of %s"),
-		    oid_to_hex(&e->idx.oid));
-
-	unuse_pack(&w_curs);
-	packing_data_unlock(&to_pack);
-	return size;
-}
-
 static int try_delta(struct unpacked *trg, struct unpacked *src,
 		     unsigned max_depth, unsigned long *mem_usage)
 {
diff --git a/pack-objects.h b/pack-objects.h
index 9d88e3e518f..0c4d5d475f6 100644
--- a/pack-objects.h
+++ b/pack-objects.h
@@ -332,52 +332,6 @@ static inline void oe_set_delta_sibling(struct packing_data *pack,
 		e->delta_sibling_idx = 0;
 }
 
-unsigned long oe_get_size_slow(struct packing_data *pack,
-			       const struct object_entry *e);
-static inline unsigned long oe_size(struct packing_data *pack,
-				    const struct object_entry *e)
-{
-	if (e->size_valid)
-		return e->size_;
-
-	return oe_get_size_slow(pack, e);
-}
-
-static inline int oe_size_less_than(struct packing_data *pack,
-				    const struct object_entry *lhs,
-				    unsigned long rhs)
-{
-	if (lhs->size_valid)
-		return lhs->size_ < rhs;
-	if (rhs < pack->oe_size_limit) /* rhs < 2^x <= lhs ? */
-		return 0;
-	return oe_get_size_slow(pack, lhs) < rhs;
-}
-
-static inline int oe_size_greater_than(struct packing_data *pack,
-				       const struct object_entry *lhs,
-				       unsigned long rhs)
-{
-	if (lhs->size_valid)
-		return lhs->size_ > rhs;
-	if (rhs < pack->oe_size_limit) /* rhs < 2^x <= lhs ? */
-		return 1;
-	return oe_get_size_slow(pack, lhs) > rhs;
-}
-
-static inline void oe_set_size(struct packing_data *pack,
-			       struct object_entry *e,
-			       unsigned long size)
-{
-	if (size < pack->oe_size_limit) {
-		e->size_ = size;
-		e->size_valid = 1;
-	} else {
-		e->size_valid = 0;
-		if (oe_get_size_slow(pack, e) != size)
-			BUG("'size' is supposed to be the object size!");
-	}
-}
 
 static inline unsigned long oe_delta_size(struct packing_data *pack,
 					  const struct object_entry *e)

[PATCH] pack-objects: move builtin-only code to its own header

[Ævar Arnfjörð Bjarmason]

Move the code that's only used in builtin/pack-objects.c to a new
builtin/pack-objects.h header and out of pack-objects.h.

This fixes an issue where Solaris's SunCC hasn't been able to compile
git since 483fa7f42d9 (t/helper/test-bitmap.c: initial commit,
2021-03-31).

The real origin of that issue is that in 898eba5e630 (pack-objects:
refer to delta objects by index instead of pointer, 2018-04-14)
utility functions only needed by builtin/pack-objects.c were added to
pack-objects.h. Since then the header has been used in a few other
places, but 483fa7f42d9 was the first time it was used by test helper.

Since Solaris is stricter about linking and the oe_get_size_slow()
function lives in builtin/pack-objects.c the build started failing
with:

    Undefined                       first referenced
     symbol                             in file
    oe_get_size_slow                    t/helper/test-bitmap.o
    ld: fatal: symbol referencing errors. No output written to t/helper/test-tool

On other platforms this is presumably OK because the compiler and/or
linker detects that the "static inline" functions that reference
oe_get_size_slow() aren't used.

Let's solve this by moving the relevant code to
builtin/pack-objects.c. This is almost entirely a code-only move, but
because of the early macro definitions in that file referencing some
of these inline functions we need to move the definition of "static
struct packing_data to_pack" earlier, and declare these inline
functions above the macros.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/pack-objects.c | 174 +++++++++++++++++++++++++++++++++++++++--
 pack-objects.h         | 159 -------------------------------------
 2 files changed, 167 insertions(+), 166 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 6ded130e45b..de00adbb9e0 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -37,6 +37,134 @@
 #include "shallow.h"
 #include "promisor-remote.h"
 
+/*
+ * Objects we are going to pack are collected in the `to_pack` structure.
+ * It contains an array (dynamically expanded) of the object data, and a map
+ * that can resolve SHA1s to their position in the array.
+ */
+static struct packing_data to_pack;
+
+static inline struct object_entry *oe_delta(
+		const struct packing_data *pack,
+		const struct object_entry *e)
+{
+	if (!e->delta_idx)
+		return NULL;
+	if (e->ext_base)
+		return &pack->ext_bases[e->delta_idx - 1];
+	else
+		return &pack->objects[e->delta_idx - 1];
+}
+
+static inline unsigned long oe_delta_size(struct packing_data *pack,
+					  const struct object_entry *e)
+{
+	if (e->delta_size_valid)
+		return e->delta_size_;
+
+	/*
+	 * pack->delta_size[] can't be NULL because oe_set_delta_size()
+	 * must have been called when a new delta is saved with
+	 * oe_set_delta().
+	 * If oe_delta() returns NULL (i.e. default state, which means
+	 * delta_size_valid is also false), then the caller must never
+	 * call oe_delta_size().
+	 */
+	return pack->delta_size[e - pack->objects];
+}
+
+unsigned long oe_get_size_slow(struct packing_data *pack,
+			       const struct object_entry *e);
+
+static inline unsigned long oe_size(struct packing_data *pack,
+				    const struct object_entry *e)
+{
+	if (e->size_valid)
+		return e->size_;
+
+	return oe_get_size_slow(pack, e);
+}
+
+static inline void oe_set_delta(struct packing_data *pack,
+				struct object_entry *e,
+				struct object_entry *delta)
+{
+	if (delta)
+		e->delta_idx = (delta - pack->objects) + 1;
+	else
+		e->delta_idx = 0;
+}
+
+static inline struct object_entry *oe_delta_sibling(
+		const struct packing_data *pack,
+		const struct object_entry *e)
+{
+	if (e->delta_sibling_idx)
+		return &pack->objects[e->delta_sibling_idx - 1];
+	return NULL;
+}
+
+static inline struct object_entry *oe_delta_child(
+		const struct packing_data *pack,
+		const struct object_entry *e)
+{
+	if (e->delta_child_idx)
+		return &pack->objects[e->delta_child_idx - 1];
+	return NULL;
+}
+
+static inline void oe_set_delta_child(struct packing_data *pack,
+				      struct object_entry *e,
+				      struct object_entry *delta)
+{
+	if (delta)
+		e->delta_child_idx = (delta - pack->objects) + 1;
+	else
+		e->delta_child_idx = 0;
+}
+
+static inline void oe_set_delta_sibling(struct packing_data *pack,
+					struct object_entry *e,
+					struct object_entry *delta)
+{
+	if (delta)
+		e->delta_sibling_idx = (delta - pack->objects) + 1;
+	else
+		e->delta_sibling_idx = 0;
+}
+
+static inline void oe_set_size(struct packing_data *pack,
+			       struct object_entry *e,
+			       unsigned long size)
+{
+	if (size < pack->oe_size_limit) {
+		e->size_ = size;
+		e->size_valid = 1;
+	} else {
+		e->size_valid = 0;
+		if (oe_get_size_slow(pack, e) != size)
+			BUG("'size' is supposed to be the object size!");
+	}
+}
+
+static inline void oe_set_delta_size(struct packing_data *pack,
+				     struct object_entry *e,
+				     unsigned long size)
+{
+	if (size < pack->oe_delta_size_limit) {
+		e->delta_size_ = size;
+		e->delta_size_valid = 1;
+	} else {
+		packing_data_lock(pack);
+		if (!pack->delta_size)
+			ALLOC_ARRAY(pack->delta_size, pack->nr_alloc);
+		packing_data_unlock(pack);
+
+		pack->delta_size[e - pack->objects] = size;
+		e->delta_size_valid = 0;
+	}
+}
+
 #define IN_PACK(obj) oe_in_pack(&to_pack, obj)
 #define SIZE(obj) oe_size(&to_pack, obj)
 #define SET_SIZE(obj,size) oe_set_size(&to_pack, obj, size)
@@ -56,13 +184,6 @@ static const char *pack_usage[] = {
 	NULL
 };
 
-/*
- * Objects we are going to pack are collected in the `to_pack` structure.
- * It contains an array (dynamically expanded) of the object data, and a map
- * that can resolve SHA1s to their position in the array.
- */
-static struct packing_data to_pack;
-
 static struct pack_idx_entry **written_list;
 static uint32_t nr_result, nr_written, nr_seen;
 static struct bitmap_index *bitmap_git;
@@ -301,6 +422,17 @@ static void copy_pack_data(struct hashfile *f,
 	}
 }
 
+static inline int oe_size_greater_than(struct packing_data *pack,
+				       const struct object_entry *lhs,
+				       unsigned long rhs)
+{
+	if (lhs->size_valid)
+		return lhs->size_ > rhs;
+	if (rhs < pack->oe_size_limit) /* rhs < 2^x <= lhs ? */
+		return 1;
+	return oe_get_size_slow(pack, lhs) > rhs;
+}
+
 /* Return 0 if we will bust the pack-size limit */
 static unsigned long write_no_reuse_object(struct hashfile *f, struct object_entry *entry,
 					   unsigned long limit, int usable_delta)
@@ -642,6 +774,14 @@ static int mark_tagged(const char *path, const struct object_id *oid, int flag,
 	return 0;
 }
 
+static inline unsigned char oe_layer(struct packing_data *pack,
+				     struct object_entry *e)
+{
+	if (!pack->layer)
+		return 0;
+	return pack->layer[e - pack->objects];
+}
+
 static inline void add_to_write_order(struct object_entry **wo,
 			       unsigned int *endp,
 			       struct object_entry *e)
@@ -2231,6 +2371,26 @@ static pthread_mutex_t progress_mutex;
  * progress_mutex for protection.
  */
 
+static inline int oe_size_less_than(struct packing_data *pack,
+				    const struct object_entry *lhs,
+				    unsigned long rhs)
+{
+	if (lhs->size_valid)
+		return lhs->size_ < rhs;
+	if (rhs < pack->oe_size_limit) /* rhs < 2^x <= lhs ? */
+		return 0;
+	return oe_get_size_slow(pack, lhs) < rhs;
+}
+
+static inline void oe_set_tree_depth(struct packing_data *pack,
+				     struct object_entry *e,
+				     unsigned int tree_depth)
+{
+	if (!pack->tree_depth)
+		CALLOC_ARRAY(pack->tree_depth, pack->nr_alloc);
+	pack->tree_depth[e - pack->objects] = tree_depth;
+}
+
 /*
  * Return the size of the object without doing any delta
  * reconstruction (so non-deltas are true object sizes, but deltas
diff --git a/pack-objects.h b/pack-objects.h
index 9d88e3e518f..dca2351ef94 100644
--- a/pack-objects.h
+++ b/pack-objects.h
@@ -268,152 +268,10 @@ static inline void oe_set_in_pack(struct packing_data *pack,
 	pack->in_pack[e - pack->objects] = p;
 }
 
-static inline struct object_entry *oe_delta(
-		const struct packing_data *pack,
-		const struct object_entry *e)
-{
-	if (!e->delta_idx)
-		return NULL;
-	if (e->ext_base)
-		return &pack->ext_bases[e->delta_idx - 1];
-	else
-		return &pack->objects[e->delta_idx - 1];
-}
-
-static inline void oe_set_delta(struct packing_data *pack,
-				struct object_entry *e,
-				struct object_entry *delta)
-{
-	if (delta)
-		e->delta_idx = (delta - pack->objects) + 1;
-	else
-		e->delta_idx = 0;
-}
-
 void oe_set_delta_ext(struct packing_data *pack,
 		      struct object_entry *e,
 		      const struct object_id *oid);
 
-static inline struct object_entry *oe_delta_child(
-		const struct packing_data *pack,
-		const struct object_entry *e)
-{
-	if (e->delta_child_idx)
-		return &pack->objects[e->delta_child_idx - 1];
-	return NULL;
-}
-
-static inline void oe_set_delta_child(struct packing_data *pack,
-				      struct object_entry *e,
-				      struct object_entry *delta)
-{
-	if (delta)
-		e->delta_child_idx = (delta - pack->objects) + 1;
-	else
-		e->delta_child_idx = 0;
-}
-
-static inline struct object_entry *oe_delta_sibling(
-		const struct packing_data *pack,
-		const struct object_entry *e)
-{
-	if (e->delta_sibling_idx)
-		return &pack->objects[e->delta_sibling_idx - 1];
-	return NULL;
-}
-
-static inline void oe_set_delta_sibling(struct packing_data *pack,
-					struct object_entry *e,
-					struct object_entry *delta)
-{
-	if (delta)
-		e->delta_sibling_idx = (delta - pack->objects) + 1;
-	else
-		e->delta_sibling_idx = 0;
-}
-
-unsigned long oe_get_size_slow(struct packing_data *pack,
-			       const struct object_entry *e);
-static inline unsigned long oe_size(struct packing_data *pack,
-				    const struct object_entry *e)
-{
-	if (e->size_valid)
-		return e->size_;
-
-	return oe_get_size_slow(pack, e);
-}
-
-static inline int oe_size_less_than(struct packing_data *pack,
-				    const struct object_entry *lhs,
-				    unsigned long rhs)
-{
-	if (lhs->size_valid)
-		return lhs->size_ < rhs;
-	if (rhs < pack->oe_size_limit) /* rhs < 2^x <= lhs ? */
-		return 0;
-	return oe_get_size_slow(pack, lhs) < rhs;
-}
-
-static inline int oe_size_greater_than(struct packing_data *pack,
-				       const struct object_entry *lhs,
-				       unsigned long rhs)
-{
-	if (lhs->size_valid)
-		return lhs->size_ > rhs;
-	if (rhs < pack->oe_size_limit) /* rhs < 2^x <= lhs ? */
-		return 1;
-	return oe_get_size_slow(pack, lhs) > rhs;
-}
-
-static inline void oe_set_size(struct packing_data *pack,
-			       struct object_entry *e,
-			       unsigned long size)
-{
-	if (size < pack->oe_size_limit) {
-		e->size_ = size;
-		e->size_valid = 1;
-	} else {
-		e->size_valid = 0;
-		if (oe_get_size_slow(pack, e) != size)
-			BUG("'size' is supposed to be the object size!");
-	}
-}
-
-static inline unsigned long oe_delta_size(struct packing_data *pack,
-					  const struct object_entry *e)
-{
-	if (e->delta_size_valid)
-		return e->delta_size_;
-
-	/*
-	 * pack->delta_size[] can't be NULL because oe_set_delta_size()
-	 * must have been called when a new delta is saved with
-	 * oe_set_delta().
-	 * If oe_delta() returns NULL (i.e. default state, which means
-	 * delta_size_valid is also false), then the caller must never
-	 * call oe_delta_size().
-	 */
-	return pack->delta_size[e - pack->objects];
-}
-
-static inline void oe_set_delta_size(struct packing_data *pack,
-				     struct object_entry *e,
-				     unsigned long size)
-{
-	if (size < pack->oe_delta_size_limit) {
-		e->delta_size_ = size;
-		e->delta_size_valid = 1;
-	} else {
-		packing_data_lock(pack);
-		if (!pack->delta_size)
-			ALLOC_ARRAY(pack->delta_size, pack->nr_alloc);
-		packing_data_unlock(pack);
-
-		pack->delta_size[e - pack->objects] = size;
-		e->delta_size_valid = 0;
-	}
-}
-
 static inline unsigned int oe_tree_depth(struct packing_data *pack,
 					 struct object_entry *e)
 {
@@ -422,23 +280,6 @@ static inline unsigned int oe_tree_depth(struct packing_data *pack,
 	return pack->tree_depth[e - pack->objects];
 }
 
-static inline void oe_set_tree_depth(struct packing_data *pack,
-				     struct object_entry *e,
-				     unsigned int tree_depth)
-{
-	if (!pack->tree_depth)
-		CALLOC_ARRAY(pack->tree_depth, pack->nr_alloc);
-	pack->tree_depth[e - pack->objects] = tree_depth;
-}
-
-static inline unsigned char oe_layer(struct packing_data *pack,
-				     struct object_entry *e)
-{
-	if (!pack->layer)
-		return 0;
-	return pack->layer[e - pack->objects];
-}
-
 static inline void oe_set_layer(struct packing_data *pack,
 				struct object_entry *e,
 				unsigned char layer)
-- 
2.32.0.rc1.413.g73297d74493

Re: [PATCH] pack-objects: move builtin-only code to its own header

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Move the code that's only used in builtin/pack-objects.c to a new
> builtin/pack-objects.h header and out of pack-objects.h.
>
> This fixes an issue where Solaris's SunCC hasn't been able to compile
> git since 483fa7f42d9 (t/helper/test-bitmap.c: initial commit,
> 2021-03-31).
>
> The real origin of that issue is that in 898eba5e630 (pack-objects:
> refer to delta objects by index instead of pointer, 2018-04-14)
> utility functions only needed by builtin/pack-objects.c were added to
> pack-objects.h. Since then the header has been used in a few other
> places, but 483fa7f42d9 was the first time it was used by test helper.

Hmph.  Its good that removing these functions that should not be
called by other people from *.h and move them to their only user
regardless of the Solaris issue.  Sounds good.

Re: [PATCH] pack-objects: move builtin-only code to its own header

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Move the code that's only used in builtin/pack-objects.c to a new
> builtin/pack-objects.h header and out of pack-objects.h.
> ...
>  builtin/pack-objects.c | 174 +++++++++++++++++++++++++++++++++++++++--
>  pack-objects.h         | 159 -------------------------------------
>  2 files changed, 167 insertions(+), 166 deletions(-)

Neither the title or the description reflects the reality, though.

move builtin-only "static inline" code from the header to its sole
consumer?

Re: [PATCH] pack-objects: move builtin-only code to its own header

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Move the code that's only used in builtin/pack-objects.c to a new
> builtin/pack-objects.h header and out of pack-objects.h.

I've amended the early part of the proposed log message in
preparation for merging it to 'next' and then later down to
'master'.

Here is what the result looks like.

Thanks.


Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date:   Thu May 27 02:52:51 2021 +0200

    pack-objects: move static inline from a header to its sole consumer
    
    Move the code that is only used in builtin/pack-objects.c out of
    pack-objects.h.
    
    This fixes an issue where Solaris's SunCC hasn't been able to compile
    git since 483fa7f42d9 (t/helper/test-bitmap.c: initial commit,
    2021-03-31).
    
    The real origin of that issue is that in 898eba5e630 (pack-objects:
    refer to delta objects by index instead of pointer, 2018-04-14)
    utility functions only needed by builtin/pack-objects.c were added to
    pack-objects.h. Since then the header has been used in a few other
    places, but 483fa7f42d9 was the first time it was used by test helper.
    
    Since Solaris is stricter about linking and the oe_get_size_slow()
    function lives in builtin/pack-objects.c the build started failing
    with:
    
        Undefined                       first referenced
         symbol                             in file
        oe_get_size_slow                    t/helper/test-bitmap.o
        ld: fatal: symbol referencing errors. No output written to t/helper/test-tool
    
    On other platforms this is presumably OK because the compiler and/or
    linker detects that the "static inline" functions that reference
    oe_get_size_slow() aren't used.
    
    Let's solve this by moving the relevant code from pack-objects.h to
    builtin/pack-objects.c. This is almost entirely a code-only move, but
    because of the early macro definitions in that file referencing some
    of these inline functions we need to move the definition of "static
    struct packing_data to_pack" earlier, and declare these inline
    functions above the macros.
    
    Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Re: [PATCH] pack-objects: move builtin-only code to its own header

[Ævar Arnfjörð Bjarmason]

On Thu, May 27 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Move the code that's only used in builtin/pack-objects.c to a new
>> builtin/pack-objects.h header and out of pack-objects.h.
>
> I've amended the early part of the proposed log message in
> preparation for merging it to 'next' and then later down to
> 'master'.
>
> Here is what the result looks like.
>
> Thanks.
>
>
> Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Date:   Thu May 27 02:52:51 2021 +0200
>
>     pack-objects: move static inline from a header to its sole consumer
>     
>     Move the code that is only used in builtin/pack-objects.c out of
>     pack-objects.h.
>     
>     This fixes an issue where Solaris's SunCC hasn't been able to compile
>     git since 483fa7f42d9 (t/helper/test-bitmap.c: initial commit,
>     2021-03-31).
>     
>     The real origin of that issue is that in 898eba5e630 (pack-objects:
>     refer to delta objects by index instead of pointer, 2018-04-14)
>     utility functions only needed by builtin/pack-objects.c were added to
>     pack-objects.h. Since then the header has been used in a few other
>     places, but 483fa7f42d9 was the first time it was used by test helper.
>     
>     Since Solaris is stricter about linking and the oe_get_size_slow()
>     function lives in builtin/pack-objects.c the build started failing
>     with:
>     
>         Undefined                       first referenced
>          symbol                             in file
>         oe_get_size_slow                    t/helper/test-bitmap.o
>         ld: fatal: symbol referencing errors. No output written to t/helper/test-tool
>     
>     On other platforms this is presumably OK because the compiler and/or
>     linker detects that the "static inline" functions that reference
>     oe_get_size_slow() aren't used.
>     
>     Let's solve this by moving the relevant code from pack-objects.h to
>     builtin/pack-objects.c. This is almost entirely a code-only move, but
>     because of the early macro definitions in that file referencing some
>     of these inline functions we need to move the definition of "static
>     struct packing_data to_pack" earlier, and declare these inline
>     functions above the macros.
>     
>     Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>

Looks good to me for what it's worth, and I see you merged this down
already, thanks! Git builds again on the Solaris boxes now.

[PATCH 3/3] builtin/pack-objects.c: respect 'pack.preferBitmapTips'

[Taylor Blau]

When writing a new pack with a bitmap, it is sometimes convenient to
indicate some reference prefixes which should receive priority when
selecting which commits to receive bitmaps.

A truly motivated caller could accomplish this by setting
'pack.islandCore', (since all commits in the core island are similarly
marked as preferred) but this requires callers to opt into using delta
islands, which they may or may not want to do.

Introduce a new multi-valued configuration, 'pack.preferBitmapTips' to
allow callers to specify a list of reference prefixes. All references
which have a prefix contained in 'pack.preferBitmapTips' will mark their
tips as "preferred" in the same way as commits are marked as preferred
for selection by 'pack.islandCore'.

The choice of the verb "prefer" is intentional: marking the NEEDS_BITMAP
flag on an object does *not* guarantee that that object will receive a
bitmap. It merely guarantees that that commit will receive a bitmap over
any *other* commit in the same window by bitmap_writer_select_commits().

The test this patch adds reflects this quirk, too. It only tests that
a commit (which didn't receive bitmaps by default) is selected for
bitmaps after changing the value of 'pack.preferBitmapTips' to include
it. Other commits may lose their bitmaps as a byproduct of how the
selection process works (bitmap_writer_select_commits() ignores the
remainder of a window after seeing a commit with the NEEDS_BITMAP flag).

This configuration will aide in selecting important references for
multi-pack bitmaps, since they do not respect the same pack.islandCore
configuration. (They could, but doing so may be confusing, since it is
packs--not bitmaps--which are influenced by the delta-islands
configuration).

In a fork network repository (one which lists all forks of a given
repository as remotes), for example, it is useful to set
pack.preferBitmapTips to 'refs/remotes/<root>/heads' and
'refs/remotes/<root>/tags', where '<root>' is an opaque identifier
referring to the repository which is at the base of the fork chain.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/config/pack.txt | 15 ++++++++++++++
 builtin/pack-objects.c        | 34 +++++++++++++++++++++++++++++++
 pack-bitmap.c                 |  6 ++++++
 pack-bitmap.h                 |  3 +++
 t/t5310-pack-bitmaps.sh       | 38 +++++++++++++++++++++++++++++++++++
 5 files changed, 96 insertions(+)

diff --git a/Documentation/config/pack.txt b/Documentation/config/pack.txt
index 3da4ea98e2..c0844d8d8e 100644
--- a/Documentation/config/pack.txt
+++ b/Documentation/config/pack.txt
@@ -122,6 +122,21 @@ pack.useSparse::
 	commits contain certain types of direct renames. Default is
 	`true`.
 
+pack.preferBitmapTips::
+	When selecting which commits will receive bitmaps, prefer a
+	commit at the tip of any reference that is a suffix of any value
+	of this configuration over any other commits in the "selection
+	window".
++
+Note that setting this configuration to `refs/foo` does not mean that
+the commits at the tips of `refs/foo/bar` and `refs/foo/baz` will
+necessarily be selected. This is because commits are selected for
+bitmaps from within a series of windows of variable length.
++
+If a commit at the tip of any reference which is a suffix of any value
+of this configuration is seen in a window, it is immediately given
+preference over any other commit in that window.
+
 pack.writeBitmaps (deprecated)::
 	This is a deprecated synonym for `repack.writeBitmaps`.
 
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 525c2d8552..a1e33d7507 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3547,6 +3547,37 @@ static void record_recent_commit(struct commit *commit, void *data)
 	oid_array_append(&recent_objects, &commit->object.oid);
 }
 
+static int mark_bitmap_preferred_tip(const char *refname,
+				     const struct object_id *oid, int flags,
+				     void *_data)
+{
+	struct object_id peeled;
+	struct object *object;
+
+	if (!peel_iterated_oid(oid, &peeled))
+		oid = &peeled;
+
+	object = parse_object_or_die(oid, refname);
+	if (object->type == OBJ_COMMIT)
+		object->flags |= NEEDS_BITMAP;
+
+	return 0;
+}
+
+static void mark_bitmap_preferred_tips(void)
+{
+	struct string_list_item *item;
+	const struct string_list *preferred_tips;
+
+	preferred_tips = bitmap_preferred_tips(the_repository);
+	if (!preferred_tips)
+		return;
+
+	for_each_string_list_item(item, preferred_tips) {
+		for_each_ref_in(item->string, mark_bitmap_preferred_tip, NULL);
+	}
+}
+
 static void get_object_list(int ac, const char **av)
 {
 	struct rev_info revs;
@@ -3601,6 +3632,9 @@ static void get_object_list(int ac, const char **av)
 	if (use_delta_islands)
 		load_delta_islands(the_repository, progress);
 
+	if (write_bitmap_index)
+		mark_bitmap_preferred_tips();
+
 	if (prepare_revision_walk(&revs))
 		die(_("revision walk setup failed"));
 	mark_edges_uninteresting(&revs, show_edge, sparse);
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 7554510b14..bfe2943a9b 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -13,6 +13,7 @@
 #include "repository.h"
 #include "object-store.h"
 #include "list-objects-filter-options.h"
+#include "config.h"
 
 /*
  * An entry on the bitmap index, representing the bitmap for a given
@@ -1529,3 +1530,8 @@ off_t get_disk_usage_from_bitmap(struct bitmap_index *bitmap_git,
 
 	return total;
 }
+
+const struct string_list *bitmap_preferred_tips(struct repository *r)
+{
+	return repo_config_get_value_multi(r, "pack.preferbitmaptips");
+}
diff --git a/pack-bitmap.h b/pack-bitmap.h
index c3cdd80756..78f2b3ff79 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -5,6 +5,7 @@
 #include "khash.h"
 #include "pack.h"
 #include "pack-objects.h"
+#include "string-list.h"
 
 struct commit;
 struct repository;
@@ -91,4 +92,6 @@ void bitmap_writer_finish(struct pack_idx_entry **index,
 			  const char *filename,
 			  uint16_t options);
 
+const struct string_list *bitmap_preferred_tips(struct repository *r);
+
 #endif
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 40b9f63244..f53efc8229 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -554,4 +554,42 @@ test_expect_success 'fetch with bitmaps can reuse old base' '
 	)
 '
 
+test_expect_success 'pack.preferBitmapTips' '
+	git init repo &&
+	test_when_finished "rm -fr repo" &&
+	(
+		cd repo &&
+
+		# create enough commits that not all are receive bitmap
+		# coverage even if they are all at the tip of some reference.
+		test_commit_bulk --message="%s" 103 &&
+
+		git rev-list HEAD >commits.raw &&
+		sort <commits.raw >commits &&
+
+		git log --format="create refs/tags/%s %H" HEAD >refs &&
+		git update-ref --stdin <refs &&
+
+		git repack -adb &&
+		test-tool bitmap list-commits | sort >bitmaps &&
+
+		# remember which commits did not receive bitmaps
+		comm -13 bitmaps commits >before &&
+		test_file_not_empty before &&
+
+		# mark the commits which did not receive bitmaps as preferred,
+		# and generate the bitmap again
+		perl -pe "s{^}{create refs/tags/include/$. }" <before |
+			git update-ref --stdin &&
+		git -c pack.preferBitmapTips=refs/tags/include repack -adb &&
+
+		# finally, check that the commit(s) without bitmap coverage
+		# are not the same ones as before
+		test-tool bitmap list-commits | sort >bitmaps &&
+		comm -13 bitmaps commits >after &&
+
+		! test_cmp before after
+	)
+'
+
 test_done
-- 
2.31.1.163.ga65ce7f831

Re: [PATCH 3/3] builtin/pack-objects.c: respect 'pack.preferBitmapTips'

[Derrick Stolee]

On 3/31/2021 9:32 PM, Taylor Blau wrote:
> When writing a new pack with a bitmap, it is sometimes convenient to
> indicate some reference prefixes which should receive priority when
> selecting which commits to receive bitmaps.
> 
> A truly motivated caller could accomplish this by setting
> 'pack.islandCore', (since all commits in the core island are similarly
> marked as preferred) but this requires callers to opt into using delta
> islands, which they may or may not want to do.
> 
> Introduce a new multi-valued configuration, 'pack.preferBitmapTips' to
> allow callers to specify a list of reference prefixes. All references
> which have a prefix contained in 'pack.preferBitmapTips' will mark their
> tips as "preferred" in the same way as commits are marked as preferred
> for selection by 'pack.islandCore'.

I almost hit send on an email saying "this doesn't seem to work with
prefixes" but I have discovered the subtle reason why. I'll point it
out below.

> +static int mark_bitmap_preferred_tip(const char *refname,
> +				     const struct object_id *oid, int flags,
> +				     void *_data)
> +{
> +	struct object_id peeled;
> +	struct object *object;
> +
> +	if (!peel_iterated_oid(oid, &peeled))
> +		oid = &peeled;
> +
> +	object = parse_object_or_die(oid, refname);
> +	if (object->type == OBJ_COMMIT)
> +		object->flags |= NEEDS_BITMAP;
> +
> +	return 0;
> +}

This takes a refname and peels it (in case of a tag) but doesn't
do any prefix matching.

> +static void mark_bitmap_preferred_tips(void)
> +{
> +	struct string_list_item *item;
> +	const struct string_list *preferred_tips;
> +
> +	preferred_tips = bitmap_preferred_tips(the_repository);
> +	if (!preferred_tips)
> +		return;
> +
> +	for_each_string_list_item(item, preferred_tips) {
> +		for_each_ref_in(item->string, mark_bitmap_preferred_tip, NULL);
> +	}
> +}

This iterates on the list returned by bitmap_preferred_tips(), but
specifically for_each_ref_in() does the prefix matching for us. I
missed that point on my first read.

> +const struct string_list *bitmap_preferred_tips(struct repository *r)
> +{
> +	return repo_config_get_value_multi(r, "pack.preferbitmaptips");
> +}

And this just returns the config values, which are fed into
for_each_ref_in().

> +test_expect_success 'pack.preferBitmapTips' '
> +	git init repo &&
> +	test_when_finished "rm -fr repo" &&
> +	(
> +		cd repo &&
> +
> +		# create enough commits that not all are receive bitmap
> +		# coverage even if they are all at the tip of some reference.
> +		test_commit_bulk --message="%s" 103 &&
> +
> +		git rev-list HEAD >commits.raw &&
> +		sort <commits.raw >commits &&
> +
> +		git log --format="create refs/tags/%s %H" HEAD >refs &&
> +		git update-ref --stdin <refs &&
> +
> +		git repack -adb &&
> +		test-tool bitmap list-commits | sort >bitmaps &&
> +
> +		# remember which commits did not receive bitmaps
> +		comm -13 bitmaps commits >before &&
> +		test_file_not_empty before &&
> +
> +		# mark the commits which did not receive bitmaps as preferred,
> +		# and generate the bitmap again
> +		perl -pe "s{^}{create refs/tags/include/$. }" <before |
> +			git update-ref --stdin &&
> +		git -c pack.preferBitmapTips=refs/tags/include repack -adb &&

And this test is a clever way of creating a new ref category to
check the prefix matching. Since you created refs pointing to
every commit earlier, this verifies that the config is the reason
they are picked now. Good.

Sorry for all of my thinking out loud. These are good patches and
I have no recommended changes.

Thanks,
-Stolee