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

From: Derrick Stolee <derrickstolee@github.com>
Date: Jun 27, 2022, 14:43 UTC
Message-ID: <05edd01a-8b6f-b25d-0cd1-b1a46ca7c219@github.com>
In-Reply-To: <7786dc879f006c8316c33dd70e98888ceb50a014.1656249017.git.gitgitgadget@gmail.com>

On 6/26/2022 9:10 AM, Abhradeep Chakraborty via GitGitGadget wrote:

Show 5 quoted lines

> From: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
> 
> 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.

I wonder if it makes sense to have it default to 'false' for now, but to change that default after the feature has been shipped and running in production for a while.

> Also add test to verify writting of lookup table.

s/writting/writing/

> +pack.writeBitmapLookupTable::
> +	When true, git will include a "lookup table" section in the

I think you should either use "Git" when talking about the software generally, OR use "`git repack --write-bitmap-index` will include..."

> +	bitmap index (if one is written). This table is used to defer
> +	loading individual bitmaps as late as possible. This can be
> +	beneficial in repositories which have relatively large bitmap

s/which/that/

(I'm pretty sure that "that" is better. We're trying to restrict the set of repositories we are talking about, not implying that all repositories have this property.)

> +	indexes. Defaults to true.
> +

Show 16 quoted lines

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

These lines seem misplaced. Maybe they were meant for the previous patch?

Thanks, -Stolee