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

Commit Message

Taylor Blau April 1, 2021, 1:32 a.m. UTC

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 <[e-mail]>
---
 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

Comments

Ævar Arnfjörð Bjarmason May 26, 2021, 6:30 p.m. UTC | #1

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.

Taylor Blau May 26, 2021, 6:44 p.m. UTC | #2

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

Ævar Arnfjörð Bjarmason May 26, 2021, 9:10 p.m. UTC | #3

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

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);