Re: [PATCH v5 2/3] pack-bitmap: reword comments in test_bitmap_commits()

[Taylor Blau <me@ttaylorr.com>]

On Tue, Jun 03, 2025 at 03:14:03AM +0000, Lidong Yan via GitGitGadget wrote:
> From: Lidong Yan <502024330056@smail.nju.edu.cn>
>
> In pack-bitmap.c:test_bitmap_commits(), it comments
>
>     /*
>      * As this function is only used to print bitmap selected
>      * commits, we don't have to read the commit table.
>      */
>

There is no need to include the original comment here, since it is clear
from the patch below what you're referring to.

I don't think this alone is worth rerolling the series, but others may
feel differently.

> This suggests that we can avoid reading the commit table altogether.
> However, this comment is misleading. The reason we load bitmap entries here
> is because test_bitmap_commits() needs to print the commit IDs from the
> bitmap, and we must read the bitmap entries to obtain those commit IDs.
> So reword this comment.
>
> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
> ---
>  pack-bitmap.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index fd19c2255163..e514c9da239b 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -2839,8 +2839,9 @@ int test_bitmap_commits(struct repository *r)
>  		die(_("failed to load bitmap indexes"));
>
>  	/*
> -	 * As this function is only used to print bitmap selected
> -	 * commits, we don't have to read the commit table.
> +	 * Since this function needs to print bitmap selected

The phrase "bitmap selected commits" is a little awkward. I might have
written either "the bitmapped commits", or "the set of commits which
have bitmaps".

> +	 * commits, bypass the commit lookup table (if one exists)
> +	 * by forcing the bitmap to eagerly load its entries.
>  	 */
>  	if (bitmap_git->table_lookup) {
>  		if (load_bitmap_entries_v1(bitmap_git) < 0)
> --
> gitgitgadget
>
Thanks,
Taylor