From: Taylor Blau <me@ttaylorr.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Taylor Blau <me@ttaylorr.com>,
git@vger.kernel.org, peff@peff.net, chriscool@tuxfamily.org
Subject: Re: [PATCH 3/4] pack-bitmap.c: support 'tree:0' filtering
Date: Tue, 5 May 2020 09:59:51 -0600 [thread overview]
Message-ID: <20200505155951.GA69300@syl.local> (raw)
In-Reply-To: <xmqq7dxrc5r9.fsf@gitster.c.googlers.com>
On Mon, May 04, 2020 at 10:25:46PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > diff --git a/pack-bitmap.c b/pack-bitmap.c
> > index 3693c9e62f..195ee8cad0 100644
> > --- a/pack-bitmap.c
> > +++ b/pack-bitmap.c
> > @@ -749,7 +749,7 @@ static void filter_bitmap_exclude_type(struct bitmap_index *bitmap_git,
> > eword_t mask;
> > uint32_t i;
> >
> > - if (type != OBJ_BLOB)
> > + if (type != OBJ_BLOB && type != OBJ_TREE)
> > BUG("filter_bitmap_exclude_type: unsupported type '%d'", type);
>
> OK. This is the same as the previous step, but why would we even
> need this guard? find_tip_objects() is equipped to find tips of any
> object type, iterating on the bitmap for "type", or flipping the
> bits in the to_filter bitmap, does not have any limitation to the
> blob type in the previous step, and there is no limitation to the
> blob or tree types after this step, either, no?
I think we need some sort of guard here, since we could receive any
value of object_type, but you're right that this isn't the right one. It
should probably be something like:
if (type < OBJ_COMMIT || type > OBJ_TAG)
to pick out the sentinel values like OBJ_BAD and OBJ_NONE, as well as
the pack-specific types, like OBJ_OFS_DELTA and so on.
I fixed this locally, and will resend it along with the rest of v2 in a
day or so. Thanks for a review :).
> > @@ -867,6 +867,20 @@ static void filter_bitmap_blob_limit(struct bitmap_index *bitmap_git,
> > bitmap_free(tips);
> > }
> >
> > +static void filter_bitmap_tree_depth(struct bitmap_index *bitmap_git,
> > + struct object_list *tip_objects,
> > + struct bitmap *to_filter,
> > + unsigned long limit)
> > +{
> > + if (limit)
> > + BUG("filter_bitmap_tree_depth given non-zero limit");
>
> This one does make sense, because the code to exclude all trees and
> all blobs we have below won't be able to cull only trees at a given
> depth or deeper.
>
> > + filter_bitmap_exclude_type(bitmap_git, tip_objects, to_filter,
> > + OBJ_TREE);
> > + filter_bitmap_exclude_type(bitmap_git, tip_objects, to_filter,
> > + OBJ_BLOB);
>
> And these two are quite straight-forward.
>
> > +}
> > +
> > static int filter_bitmap(struct bitmap_index *bitmap_git,
> > struct object_list *tip_objects,
> > struct bitmap *to_filter,
> > @@ -890,6 +904,15 @@ static int filter_bitmap(struct bitmap_index *bitmap_git,
> > return 0;
> > }
> >
> > + if (filter->choice == LOFC_TREE_DEPTH &&
> > + filter->tree_exclude_depth == 0) {
> > + if (bitmap_git)
> > + filter_bitmap_tree_depth(bitmap_git, tip_objects,
> > + to_filter,
> > + filter->tree_exclude_depth);
>
> I briefly wondered if it is cleaner to read if we hardcode 0 as the
> last argument. But if the helper function ever learns how to filter
> by tree with non-zero depth, we can only tweak the if() condition
> without changing the call, so the way you wrote it is the right way.
>
> Thanks.
Thanks,
Taylor
next prev parent reply other threads:[~2020-05-05 16:00 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-04 23:12 [PATCH 0/4] pack-bitmap: use bitmaps for traversals with '--filter=tree:0' Taylor Blau
2020-05-04 23:12 ` [PATCH 1/4] list-objects-filter: treat NULL filter_options as "disabled" Taylor Blau
2020-05-05 5:07 ` Junio C Hamano
2020-05-04 23:12 ` [PATCH 2/4] pack-bitmap.c: make object filtering functions generic Taylor Blau
2020-05-05 5:12 ` Junio C Hamano
2020-05-04 23:12 ` [PATCH 3/4] pack-bitmap.c: support 'tree:0' filtering Taylor Blau
2020-05-05 5:25 ` Junio C Hamano
2020-05-05 15:59 ` Taylor Blau [this message]
2020-05-05 18:20 ` Junio C Hamano
2020-05-04 23:12 ` [PATCH 4/4] pack-bitmap: pass object filter to fill-in traversal Taylor Blau
2020-05-05 5:40 ` Junio C Hamano
2020-05-05 16:00 ` Taylor Blau
-- strict thread matches above, loose matches on Subject: below --
2020-04-22 23:13 [PATCH 0/4] pack-bitmap: use bitmaps for traversals with '--filter=tree:0' Taylor Blau
2020-04-22 23:13 ` [PATCH 3/4] pack-bitmap.c: support 'tree:0' filtering Taylor Blau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200505155951.GA69300@syl.local \
--to=me@ttaylorr.com \
--cc=chriscool@tuxfamily.org \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).