Re: [PATCH 1/2] partial-clone: set default filter with --partial
- From
Jeff King <peff@peff.net>
- Date
- Mar 22, 2020, 09:46 UTC
- Message-ID
- <20200322094659.GA635598@coredump.intra.peff.net>
- In-Reply-To
- <2a75325b-ea50-a6f6-87dc-12184e706ac2@gmail.com>
On Fri, Mar 20, 2020 at 04:38:27PM -0400, Derrick Stolee wrote:
Show 9 quoted lines
> > Specifically, I find it unsatisifying to see the "0" optimization at > > this level. Shouldn't it be done in parse_list_objects_filter() to > > parse "blob:limit=<num>" and after realizing <num> is zero, pretend > > as if it got "blob:none" to optimize? That way, people can even say > > "--partial=0k" and get it interpreted as "--filter=blob:none", right? > > I suppose it would be worth checking the recent server-side improvements > to see if they translate a limit=0k to a "size 0" and then ignore the > size check and simply remove all blobs.
The new bitmap code doesn't do anything special there. It does rely on the normal filter parsing, though. If we rewrote it at that level, perhaps like this (completely untested):
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c index 256bcfbdfe..0225b61912 100644 --- a/list-objects-filter-options.c +++ b/list-objects-filter-options.c @@ -49,7 +49,10 @@ static int gently_parse_list_objects_filter( } else if (skip_prefix(arg, "blob:limit=", &v0)) { if (git_parse_ulong(v0, &filter_options->blob_limit_value)) { - filter_options->choice = LOFC_BLOB_LIMIT; + if (filter_options->blob_limit_value) + filter_options->choice = LOFC_BLOB_LIMIT; + else + filter_options->choice = LOFC_BLOB_NONE; return 0; } then it would just work for regular and bitmapped filters. One interesting user-visible effect would be in the patches we've been discussing to limit which filters are allowed. If you allowed, say, blob:none but not blob:limit, this would quietly allow blob:limit=0 (because it really _is_ blob:none under the hood). I'm not sure if that would be confusing or convenient. I doubt anybody cares much for the blob filters (you'd either enable neither or both, because they cost about the same). But in another thread, I mentioned that doing "tree:depth=0" _would_ be cheap to do with bitmaps, but tree:depth=1" wouldn't. If we quietly rewrote the former to tree:none (which doesn't yet exist, but could), that would let you distinguish between the two (of course if tree:none existed, perhaps nobody would have any reason to write tree:depth=0 in the first place). -Peff