On Tue, Jun 03, 2025 at 03:14:03AM +0000, Lidong Yan via GitGitGadget wrote:
> From: Lidong Yan <502024330056@xxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxx>
> ---
> 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
