Re: [PATCH v2 4/6] pack-bitmap: prepare to read lookup table extension

[Taylor Blau <me@ttaylorr.com>]

On Wed, Jun 29, 2022 at 12:55:55AM +0530, Abhradeep Chakraborty wrote:
> > > +			commit_pos = get_be32(triplet);
> > > +			offset_xor = triplet_get_offset(triplet);
> > > +
> > > +			if (nth_bitmap_object_oid(bitmap_git, &xor_oid, commit_pos) < 0) {
> >
> > Should it be an error if we can't look up the object's ID here? I'd
> > think so.
>
> I also am not sure about it. Morally, I think it is better to throw
> An error here.

Yeah.

> > Do we have a good way to make sure that we're testing this code in CI?
> > It *seems* correct to me, but of course, we should have a computer check
> > that this produces OK results, not a human ;).
>
> My current test file changes should test this code. As for now, the lookup
> Table is enabled by default, all the existing tests that include write and
> read bitmaps uses this lookup table. So, all the test case scenarios should
> Pass. So, I think it is being tested in CI. Do you have a good idea to test
> It better?

I think having some indication (maybe via a trace2 region?) that we're
actually executing this code would be good. Although it's going to be
*really* noisy, so probably not a good idea to do that in general.

Stolee runs some coverage tests that show lines that we aren't
exercising via tests. So making sure that this doesn't show up in that
report when you run it locally would be good.

See some information from him about how to run those tests locally here:

    https://lore.kernel.org/git/00a57a1d-0566-8f54-26b2-0f3558bde88d@github.com/

(TL;DR: run `make coverage-test` and make sure that these lines don't
show up ;-)).

> > Hmm. I'm not sure I follow the purpose of tweaking
> > GIT_TEST_READ_COMMIT_TABLE like this with setenv(). Are we trying to
> > avoid reading the lookup table? If so, why? I'd rather avoid
> > manipulating the environment directly like this, and instead have a
> > function we could call to fault in all of the bitmaps (when a lookup
> > table exists, otherwise do nothing).
>
> The problem was that the `test-tool bitmap list-commit` command was
> Not printing any commits (the error that I notified you before). It
> is because of this function. As lookup table is enabled by default,
> `prepare_bitmap_git` function doesn't load each bitmap entries and
> thus the below code in this function doesn't provide the bitmapped
> commit list (because Hashtable didn't generated).
>
>         kh_foreach(bitmap_git->bitmaps, oid, value, {
> 		printf("%s\n", oid_to_hex(&oid));
> 	});
>
> So, the simplest fix I found was this. Should I make a function then
> (Which you suggested here)?

I see. I remember that issue, but I think we should go about fixing it
in a different way. Instead of tricking the code into loading all
bitmaps by pretending the lookup table doesn't exist, we should have a
function that forces loading in all bitmaps from the lookup table, if
one exists. If the lookup table doesn't exist, or we have already loaded
its entries, then that function can be noop.

If we had something like that, we could call that function from within
`test_bitmap_commits()` before reading the keys and values out of
`bitmap_git->bitmaps`.

An alternative approach would be to read the table directly when it
exists, perhaps something like this:

--- 8< ---

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 9e09c5824f..3bda059b9f 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -1921,22 +1921,28 @@ int test_bitmap_commits(struct repository *r)
 {
 	struct bitmap_index *bitmap_git = NULL;
 	struct object_id oid;
-	MAYBE_UNUSED void *value;
-
-	/* As this function is only used to print bitmap selected
-	 * commits, we don't have to read the commit table.
-	 */
-	setenv("GIT_TEST_READ_COMMIT_TABLE", "0", 1);

 	bitmap_git = prepare_bitmap_git(r);
 	if (!bitmap_git)
 		die("failed to load bitmap indexes");

-	kh_foreach(bitmap_git->bitmaps, oid, value, {
-		printf("%s\n", oid_to_hex(&oid));
-	});
+	if (bitmap_git->table_lookup) {
+		uint32_t i, commit_pos;
+		for (i = 0; i < bitmap_git->entry_count; i++) {
+			commit_pos = get_be32(bitmap_get_triplet(bitmap_git, i));
+			if (nth_bitmap_object_oid(bitmap_git, &oid,
+						  commit_pos) < 0)
+				return error("could not find commit at "
+					     "position %"PRIu32, commit_pos);
+			printf("%s\n", oid_to_hex(&oid));
+		}
+	} else {
+		MAYBE_UNUSED void *value;
+		kh_foreach(bitmap_git->bitmaps, oid, value, {
+			printf("%s\n", oid_to_hex(&oid));
+		});
+	}

-	setenv("GIT_TEST_READ_COMMIT_TABLE", "1", 1);
 	free_bitmap_index(bitmap_git);

 	return 0;

--- >8 ---

Thanks,
Taylor