[PATCH] pack-bitmap: clarify comment in filter_bitmap_exclude_type()

[PATCH] pack-bitmap: clarify comment in filter_bitmap_exclude_type()

[Taylor Blau]

The code that eventually became filter_bitmap_exclude_type() was
originally introduced in 4f3bd5606a (pack-bitmap: implement BLOB_NONE
filtering, 2020-02-14) to accelerate BLOB_NONE filters with bitmaps.

In 856e12c18a (pack-bitmap.c: make object filtering functions generic,
2020-05-04), it became filter_bitmap_exclude_type(). But not all of the
comments were updated to be agnostic to the provided type.

Remove the remaining comments which should have been updated in
856e12c18a to reflect the type-agnostic nature of the function.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
Noticed while I was reading some code in this area to see if we had
accelerated the relatively new type filters. This trivial fix hopefully
shouldn't produce conflicts with other in-flight bitmap series.

 pack-bitmap.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index bfc10148f5..c8cb9ef13b 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -809,8 +809,9 @@ static void filter_bitmap_exclude_type(struct bitmap_index *bitmap_git,
 	tips = find_tip_objects(bitmap_git, tip_objects, type);

 	/*
-	 * We can use the blob type-bitmap to work in whole words
-	 * for the objects that are actually in the bitmapped packfile.
+	 * We can use the type-level bitmap for 'type' to work in whole
+	 * words for the objects that are actually in the bitmapped
+	 * packfile.
 	 */
 	for (i = 0, init_type_iterator(&it, bitmap_git, type);
 	     i < to_filter->word_alloc && ewah_iterator_next(&mask, &it);
@@ -821,9 +822,9 @@ static void filter_bitmap_exclude_type(struct bitmap_index *bitmap_git,
 	}

 	/*
-	 * Clear any blobs that weren't in the packfile (and so would not have
-	 * been caught by the loop above. We'll have to check them
-	 * individually.
+	 * Clear any objects that weren't in the packfile (and so would
+	 * not have been caught by the loop above. We'll have to check
+	 * them individually.
 	 */
 	for (i = 0; i < eindex->count; i++) {
 		uint32_t pos = i + bitmap_git->pack->num_objects;
--
2.31.1.163.ga65ce7f831

Re: [PATCH] pack-bitmap: clarify comment in filter_bitmap_exclude_type()

[Jeff King]

On Tue, Jul 20, 2021 at 04:16:22PM -0400, Taylor Blau wrote:

> The code that eventually became filter_bitmap_exclude_type() was
> originally introduced in 4f3bd5606a (pack-bitmap: implement BLOB_NONE
> filtering, 2020-02-14) to accelerate BLOB_NONE filters with bitmaps.
> 
> In 856e12c18a (pack-bitmap.c: make object filtering functions generic,
> 2020-05-04), it became filter_bitmap_exclude_type(). But not all of the
> comments were updated to be agnostic to the provided type.
> 
> Remove the remaining comments which should have been updated in
> 856e12c18a to reflect the type-agnostic nature of the function.
> 
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> Noticed while I was reading some code in this area to see if we had
> accelerated the relatively new type filters. This trivial fix hopefully
> shouldn't produce conflicts with other in-flight bitmap series.

Yep, this looks good. I wrote a near-identical patch while working on
[1], but that topic got stalled on "how is this part of partial clones
even supposed to work?". But there's no reason not to make this trivial
fix in isolation.

-Peff

Re: [PATCH] pack-bitmap: clarify comment in filter_bitmap_exclude_type()

[Taylor Blau]

On Tue, Jul 20, 2021 at 05:02:24PM -0400, Jeff King wrote:
> Yep, this looks good. I wrote a near-identical patch while working on
> [1], but that topic got stalled on "how is this part of partial clones
> even supposed to work?". But there's no reason not to make this trivial
> fix in isolation.

I'm not sure exactly which thread [1] is referring to, but I do recall
the discussion now that you mention it.

I'm happy to add an "Original-patch-by" trailer or similar, although I
suspect that you do not care ;).

Thanks,
Taylor

Re: [PATCH] pack-bitmap: clarify comment in filter_bitmap_exclude_type()

[Jeff King]

On Tue, Jul 20, 2021 at 05:05:43PM -0400, Taylor Blau wrote:

> On Tue, Jul 20, 2021 at 05:02:24PM -0400, Jeff King wrote:
> > Yep, this looks good. I wrote a near-identical patch while working on
> > [1], but that topic got stalled on "how is this part of partial clones
> > even supposed to work?". But there's no reason not to make this trivial
> > fix in isolation.
> 
> I'm not sure exactly which thread [1] is referring to, but I do recall
> the discussion now that you mention it.

Oops. :)

It was the one with: https://lore.kernel.org/git/YJ4mUJ+EEAnudI3G@coredump.intra.peff.net/

> I'm happy to add an "Original-patch-by" trailer or similar, although I
> suspect that you do not care ;).

Nah, mostly just thinking out loud since it seemed so familiar. :) And
hopefully producing the same patch independently is a form of
endorsement.

-Peff