{"thread":{"id":"56134","subject":"[PATCH] pack-bitmap: clarify comment in filter_bitmap_exclude_type()","startedAt":"2021-07-20T20:43:12Z","lastAt":"2021-07-20T21:35:33Z","messageCount":4,"participants":["Taylor Blau","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"430692","messageId":"0d52628985bf4f499e0d5622ea40cdf499e73ebe.1626812121.git.me@ttaylorr.com","threadId":"56134","inReplyTo":null,"subject":"[PATCH] pack-bitmap: clarify comment in filter_bitmap_exclude_type()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-20T20:16:22Z","receivedAt":"2021-07-20T20:43:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The code that eventually became filter_bitmap_exclude_type() was\noriginally introduced in 4f3bd5606a (pack-bitmap: implement BLOB_NONE\nfiltering, 2020-02-14) to accelerate BLOB_NONE filters with bitmaps.\n\nIn 856e12c18a (pack-bitmap.c: make object filtering functions generic,\n2020-05-04), it became filter_bitmap_exclude_type(). But not all of the\ncomments were updated to be agnostic to the provided type.\n\nRemove the remaining comments which should have been updated in\n856e12c18a to reflect the type-agnostic nature of the function.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\nNoticed while I was reading some code in this area to see if we had\naccelerated the relatively new type filters. This trivial fix hopefully\nshouldn't produce conflicts with other in-flight bitmap series.\n\n pack-bitmap.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex bfc10148f5..c8cb9ef13b 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -809,8 +809,9 @@ static void filter_bitmap_exclude_type(struct bitmap_index *bitmap_git,\n \ttips = find_tip_objects(bitmap_git, tip_objects, type);\n\n \t/*\n-\t * We can use the blob type-bitmap to work in whole words\n-\t * for the objects that are actually in the bitmapped packfile.\n+\t * We can use the type-level bitmap for 'type' to work in whole\n+\t * words for the objects that are actually in the bitmapped\n+\t * packfile.\n \t */\n \tfor (i = 0, init_type_iterator(&it, bitmap_git, type);\n \t     i < to_filter->word_alloc && ewah_iterator_next(&mask, &it);\n@@ -821,9 +822,9 @@ static void filter_bitmap_exclude_type(struct bitmap_index *bitmap_git,\n \t}\n\n \t/*\n-\t * Clear any blobs that weren't in the packfile (and so would not have\n-\t * been caught by the loop above. We'll have to check them\n-\t * individually.\n+\t * Clear any objects that weren't in the packfile (and so would\n+\t * not have been caught by the loop above. We'll have to check\n+\t * them individually.\n \t */\n \tfor (i = 0; i < eindex->count; i++) {\n \t\tuint32_t pos = i + bitmap_git->pack->num_objects;\n--\n2.31.1.163.ga65ce7f831\n"},{"id":"430699","messageId":"YPc54AdUqD6T31ad@coredump.intra.peff.net","threadId":"56134","inReplyTo":"0d52628985bf4f499e0d5622ea40cdf499e73ebe.1626812121.git.me@ttaylorr.com","subject":"Re: [PATCH] pack-bitmap: clarify comment in filter_bitmap_exclude_type()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-20T21:02:24Z","receivedAt":"2021-07-20T21:11:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 20, 2021 at 04:16:22PM -0400, Taylor Blau wrote:\n\n> The code that eventually became filter_bitmap_exclude_type() was\n> originally introduced in 4f3bd5606a (pack-bitmap: implement BLOB_NONE\n> filtering, 2020-02-14) to accelerate BLOB_NONE filters with bitmaps.\n> \n> In 856e12c18a (pack-bitmap.c: make object filtering functions generic,\n> 2020-05-04), it became filter_bitmap_exclude_type(). But not all of the\n> comments were updated to be agnostic to the provided type.\n> \n> Remove the remaining comments which should have been updated in\n> 856e12c18a to reflect the type-agnostic nature of the function.\n> \n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n> Noticed while I was reading some code in this area to see if we had\n> accelerated the relatively new type filters. This trivial fix hopefully\n> shouldn't produce conflicts with other in-flight bitmap series.\n\nYep, this looks good. I wrote a near-identical patch while working on\n[1], but that topic got stalled on \"how is this part of partial clones\neven supposed to work?\". But there's no reason not to make this trivial\nfix in isolation.\n\n-Peff\n"},{"id":"430700","messageId":"YPc6p1lxX2AUggEu@nand.local","threadId":"56134","inReplyTo":"YPc54AdUqD6T31ad@coredump.intra.peff.net","subject":"Re: [PATCH] pack-bitmap: clarify comment in filter_bitmap_exclude_type()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-20T21:05:43Z","receivedAt":"2021-07-20T21:11:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jul 20, 2021 at 05:02:24PM -0400, Jeff King wrote:\n> Yep, this looks good. I wrote a near-identical patch while working on\n> [1], but that topic got stalled on \"how is this part of partial clones\n> even supposed to work?\". But there's no reason not to make this trivial\n> fix in isolation.\n\nI'm not sure exactly which thread [1] is referring to, but I do recall\nthe discussion now that you mention it.\n\nI'm happy to add an \"Original-patch-by\" trailer or similar, although I\nsuspect that you do not care ;).\n\nThanks,\nTaylor\n"},{"id":"430705","messageId":"YPdBUbiusnCfC84q@coredump.intra.peff.net","threadId":"56134","inReplyTo":"YPc6p1lxX2AUggEu@nand.local","subject":"Re: [PATCH] pack-bitmap: clarify comment in filter_bitmap_exclude_type()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-20T21:34:09Z","receivedAt":"2021-07-20T21:35:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 20, 2021 at 05:05:43PM -0400, Taylor Blau wrote:\n\n> On Tue, Jul 20, 2021 at 05:02:24PM -0400, Jeff King wrote:\n> > Yep, this looks good. I wrote a near-identical patch while working on\n> > [1], but that topic got stalled on \"how is this part of partial clones\n> > even supposed to work?\". But there's no reason not to make this trivial\n> > fix in isolation.\n> \n> I'm not sure exactly which thread [1] is referring to, but I do recall\n> the discussion now that you mention it.\n\nOops. :)\n\nIt was the one with: https://lore.kernel.org/git/YJ4mUJ+EEAnudI3G@coredump.intra.peff.net/\n\n> I'm happy to add an \"Original-patch-by\" trailer or similar, although I\n> suspect that you do not care ;).\n\nNah, mostly just thinking out loud since it seemed so familiar. :) And\nhopefully producing the same patch independently is a form of\nendorsement.\n\n-Peff\n"}]}