Re: [PATCH 5/8] pack-bitmap-write.c: select pseudo-merges even for small bitmaps

From: Jeff King <peff@peff.net>
Date: Aug 17, 2024, 10:34 UTC
Message-ID: <20240817103401.GB551779@coredump.intra.peff.net>
In-Reply-To: <0fea7803d86ca17451af408e1bf93c32690edc44.1723743050.git.me@ttaylorr.com>

On Thu, Aug 15, 2024 at 01:31:12PM -0400, Taylor Blau wrote:

Show 12 quoted lines

> Ordinarily, the pack-bitmap machinery will select some subset of
> reachable commits to receive bitmaps. But when there are fewer than 100
> commits indexed in the first place, they will all receive bitmaps as a
> special case.
> 
> When this happens, pseudo-merges are not generated, making it impossible
> to test pseudo-merge corner cases with fewer than 100 commits.
> 
> Select pseudo-merges even for bitmaps with fewer than 100 commits to
> make such testing easier. In practice, this should not make a difference
> to non-testing bitmaps, as they are unlikely to be used when a
> repository has so few commits to begin with.

I think you could argue that if there are fewer than 100 commits in the history that pseudo-merge bitmaps are overkill, so it does not matter much either way. But I think being consistent with our behavior (i.e., generating them if asked) is important for testing and debugging.

Show 12 quoted lines

> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
> index 346fb29513..923f793cec 100644
> --- a/pack-bitmap-write.c
> +++ b/pack-bitmap-write.c
> @@ -694,6 +694,10 @@ void bitmap_writer_select_commits(struct bitmap_writer *writer,
>  	if (indexed_commits_nr < 100) {
>  		for (i = 0; i < indexed_commits_nr; ++i)
>  			bitmap_writer_push_commit(writer, indexed_commits[i], 0);
> +
> +		select_pseudo_merges(writer, indexed_commits,
> +				     indexed_commits_nr);
> +

I of course just posted a series which drops the latter two arguments from this function. It's a semantic conflict, not a textual one, so we'll have to fix it up when the two are merged (but the compiler will easily notice to remind us).

-Peff