Re: [PATCH v2 3/6] pack-bitmap-write: learn pack.writeBitmapLookupTable and add tests

[Taylor Blau <me@xxxxxxxxxxxx>]

On Sun, Jun 26, 2022 at 01:10:14PM +0000, Abhradeep Chakraborty via GitGitGadget wrote:
> From: Abhradeep Chakraborty <chakrabortyabhradeep79@xxxxxxxxx>
>
> Teach git to provide a way for users to enable/disable bitmap lookup
> table extension by providing a config option named 'writeBitmapLookupTable'.
> Default is true.
>
> Also add test to verify writting of lookup table.
>
> Co-Authored-by: Taylor Blau <me@xxxxxxxxxxxx>
> Signed-off-by: Abhradeep Chakraborty <chakrabortyabhradeep79@xxxxxxxxx>
> Mentored-by: Taylor Blau <me@xxxxxxxxxxxx>
> Co-Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx>

I think that this was covered earlier in the review of this round, but
in general your Signed-off-by (often abbreviated as "S-o-b") should come
last. The order should be chronological, so I'd probably suggest
something like:

    Mentored-by: Taylor Blau <me@xxxxxxxxxxxx>
    Co-Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx>
    Co-Authored-by: Taylor Blau <me@xxxxxxxxxxxx>
    Signed-off-by: Abhradeep Chakraborty <chakrabortyabhradeep79@xxxxxxxxx>

> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
> index 5edbb7fe86e..3757616f09c 100644
> --- a/builtin/multi-pack-index.c
> +++ b/builtin/multi-pack-index.c
> @@ -87,6 +87,13 @@ static int git_multi_pack_index_write_config(const char *var, const char *value,
>  			opts.flags &= ~MIDX_WRITE_BITMAP_HASH_CACHE;
>  	}
>
> +	if (!strcmp(var, "pack.writebitmaplookuptable")) {
> +		if (git_config_bool(var, value))
> +			opts.flags |= MIDX_WRITE_BITMAP_LOOKUP_TABLE;
> +		else
> +			opts.flags &= ~MIDX_WRITE_BITMAP_LOOKUP_TABLE;
> +	}
> +
>  	/*
>  	 * We should never make a fall-back call to 'git_default_config', since
>  	 * this was already called in 'cmd_multi_pack_index()'.
> @@ -123,6 +130,7 @@ static int cmd_multi_pack_index_write(int argc, const char **argv)
>  	};
>
>  	opts.flags |= MIDX_WRITE_BITMAP_HASH_CACHE;
> +	opts.flags |= MIDX_WRITE_BITMAP_LOOKUP_TABLE;

I wonder if this should respect pack.writeBitmapLookupTable, too.
Probably both of them should take into account their separate
configuration values, but cleaning up the hashcache one can be done
separately outside of this series.

Everything else looks good.

> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
> index 899a4a941e1..79be0cf80e6 100644
> --- a/pack-bitmap-write.c
> +++ b/pack-bitmap-write.c
> @@ -713,6 +713,7 @@ static void write_lookup_table(struct hashfile *f,
>  	for (i = 0; i < writer.selected_nr; i++)
>  		table_inv[table[i]] = i;
>
> +	trace2_region_enter("pack-bitmap-write", "writing_lookup_table", the_repository);
>  	for (i = 0; i < writer.selected_nr; i++) {
>  		struct bitmapped_commit *selected = &writer.selected[table[i]];
>  		uint32_t xor_offset = selected->xor_offset;
> @@ -725,6 +726,7 @@ static void write_lookup_table(struct hashfile *f,
>
>  	free(table);
>  	free(table_inv);
> +	trace2_region_leave("pack-bitmap-write", "writing_lookup_table", the_repository);

This region may make more sense to include in the previous commit,
though I don't have a strong feeling about it.

Thanks,
Taylor