Message ID | 6f340d9aadf71d394ad320ad162f1d140b632f2c.1584638887.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Slightly simplify partial clone user experience | expand |
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c > index 256bcfbdfe6..a71716ef75e 100644 > --- a/list-objects-filter-options.c > +++ b/list-objects-filter-options.c > @@ -270,6 +270,24 @@ int opt_parse_list_objects_filter(const struct option *opt, > return 0; > } > > +int opt_set_blob_none_filter(const struct option *opt, > + const char *arg, int unset) Isn't the convention to use "opt_parse_" for canned parse-options callbacks? > +{ > + struct strbuf filter_arg = STRBUF_INIT; > + struct list_objects_filter_options *filter_options = opt->value; > + > + if (unset || !arg || !strcmp(arg, "0")) { > + parse_list_objects_filter(filter_options, "blob:none"); > + return 0; If "--no-partial" were allowed, it should be usable to countermand "--partial" earlier on the command line or perhaps configured default. But the above (especially the "unset ||" part) makes "--no-partial" a synonym to "--filter=blob:none", no? > + } > + > + strbuf_addf(&filter_arg, "blob:limit=%s", arg); > + parse_list_objects_filter(filter_options, filter_arg.buf); > + strbuf_release(&filter_arg); I would have expected the body of the function to read more like this: if (unset) { ... clear filter_options stored in opt->value ... } else { struct strbuf filter_arg = STRBUF_INIT; if (!arg) strbuf_addstr(&filter_arg, "blob:none"); else strbuf_addf(&filter_arg, "blob:limit=%s", arg); parse_list_objects_filter(filter_options, filter_arg.buf); strbuf_release(&filter_arg); } 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? > #define OPT_PARSE_LIST_OBJECTS_FILTER(fo) \ > { OPTION_CALLBACK, 0, CL_ARG__FILTER, fo, N_("args"), \ > N_("object filtering"), 0, \ > - opt_parse_list_objects_filter } > + opt_parse_list_objects_filter }, \ > + { OPTION_CALLBACK, 0, CL_ARG__PARTIAL, fo, N_("size"), \ > + N_("partial clone with blob filter"), \ > + PARSE_OPT_OPTARG | PARSE_OPT_NONEG , opt_set_blob_none_filter } PARSE_OPT_NONEG means "--no-partial" is forbidden and the callback won't see unset==1 at all, right? Thanks.
On 3/20/2020 4:26 PM, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c >> index 256bcfbdfe6..a71716ef75e 100644 >> --- a/list-objects-filter-options.c >> +++ b/list-objects-filter-options.c >> @@ -270,6 +270,24 @@ int opt_parse_list_objects_filter(const struct option *opt, >> return 0; >> } >> >> +int opt_set_blob_none_filter(const struct option *opt, >> + const char *arg, int unset) > > Isn't the convention to use "opt_parse_" for canned parse-options > callbacks? I can do that. >> +{ >> + struct strbuf filter_arg = STRBUF_INIT; >> + struct list_objects_filter_options *filter_options = opt->value; >> + >> + if (unset || !arg || !strcmp(arg, "0")) { >> + parse_list_objects_filter(filter_options, "blob:none"); >> + return 0; > > If "--no-partial" were allowed, it should be usable to countermand > "--partial" earlier on the command line or perhaps configured > default. But the above (especially the "unset ||" part) makes > "--no-partial" a synonym to "--filter=blob:none", no? I should have been more careful about the use of "unset" (which would never be true with the current option parsing). >> + } >> + >> + strbuf_addf(&filter_arg, "blob:limit=%s", arg); >> + parse_list_objects_filter(filter_options, filter_arg.buf); >> + strbuf_release(&filter_arg); > > I would have expected the body of the function to read more like > this: > > if (unset) { > ... clear filter_options stored in opt->value ... > } else { > struct strbuf filter_arg = STRBUF_INIT; > if (!arg) > strbuf_addstr(&filter_arg, "blob:none"); > else > strbuf_addf(&filter_arg, "blob:limit=%s", arg); > parse_list_objects_filter(filter_options, filter_arg.buf); > strbuf_release(&filter_arg); > } This is a better organization and I will use it. > 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. >> #define OPT_PARSE_LIST_OBJECTS_FILTER(fo) \ >> { OPTION_CALLBACK, 0, CL_ARG__FILTER, fo, N_("args"), \ >> N_("object filtering"), 0, \ >> - opt_parse_list_objects_filter } >> + opt_parse_list_objects_filter }, \ >> + { OPTION_CALLBACK, 0, CL_ARG__PARTIAL, fo, N_("size"), \ >> + N_("partial clone with blob filter"), \ >> + PARSE_OPT_OPTARG | PARSE_OPT_NONEG , opt_set_blob_none_filter } > > PARSE_OPT_NONEG means "--no-partial" is forbidden and the callback > won't see unset==1 at all, right? You're right. I'm being inconsistent. Your reasons above point to a good reason to have --no-partial available. Thanks, -Stolee
On Fri, Mar 20, 2020 at 04:38:27PM -0400, Derrick Stolee wrote: > > 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
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c index 256bcfbdfe6..a71716ef75e 100644 --- a/list-objects-filter-options.c +++ b/list-objects-filter-options.c @@ -270,6 +270,24 @@ int opt_parse_list_objects_filter(const struct option *opt, return 0; } +int opt_set_blob_none_filter(const struct option *opt, + const char *arg, int unset) +{ + struct strbuf filter_arg = STRBUF_INIT; + struct list_objects_filter_options *filter_options = opt->value; + + if (unset || !arg || !strcmp(arg, "0")) { + parse_list_objects_filter(filter_options, "blob:none"); + return 0; + } + + strbuf_addf(&filter_arg, "blob:limit=%s", arg); + parse_list_objects_filter(filter_options, filter_arg.buf); + strbuf_release(&filter_arg); + + return 0; +} + const char *list_objects_filter_spec(struct list_objects_filter_options *filter) { if (!filter->filter_spec.nr) diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h index 2ffb39222c4..ac38ffcbe86 100644 --- a/list-objects-filter-options.h +++ b/list-objects-filter-options.h @@ -62,6 +62,7 @@ struct list_objects_filter_options { /* Normalized command line arguments */ #define CL_ARG__FILTER "filter" +#define CL_ARG__PARTIAL "partial" void list_objects_filter_die_if_populated( struct list_objects_filter_options *filter_options); @@ -80,11 +81,16 @@ void parse_list_objects_filter( int opt_parse_list_objects_filter(const struct option *opt, const char *arg, int unset); +int opt_set_blob_none_filter(const struct option *opt, + const char *arg, int unset); #define OPT_PARSE_LIST_OBJECTS_FILTER(fo) \ { OPTION_CALLBACK, 0, CL_ARG__FILTER, fo, N_("args"), \ N_("object filtering"), 0, \ - opt_parse_list_objects_filter } + opt_parse_list_objects_filter }, \ + { OPTION_CALLBACK, 0, CL_ARG__PARTIAL, fo, N_("size"), \ + N_("partial clone with blob filter"), \ + PARSE_OPT_OPTARG | PARSE_OPT_NONEG , opt_set_blob_none_filter } /* * Translates abbreviated numbers in the filter's filter_spec into their diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh index 77bb91e9769..c42cef61296 100755 --- a/t/t5616-partial-clone.sh +++ b/t/t5616-partial-clone.sh @@ -33,17 +33,39 @@ test_expect_success 'setup bare clone for server' ' # confirm we are missing all of the known blobs. # confirm partial clone was registered in the local config. test_expect_success 'do partial clone 1' ' - git clone --no-checkout --filter=blob:none "file://$(pwd)/srv.bare" pc1 && - - git -C pc1 rev-list --quiet --objects --missing=print HEAD >revs && - awk -f print_1.awk revs | - sed "s/?//" | - sort >observed.oids && + for option in "--filter=blob:none" "--partial" + do + rm -rf pc1 && + git clone --no-checkout "$option" "file://$(pwd)/srv.bare" pc1 && + + git -C pc1 rev-list --quiet --objects --missing=print HEAD >revs && + awk -f print_1.awk revs | + sed "s/?//" | + sort >observed.oids && + + test_cmp expect_1.oids observed.oids && + test "$(git -C pc1 config --local core.repositoryformatversion)" = "1" && + test "$(git -C pc1 config --local remote.origin.promisor)" = "true" && + test "$(git -C pc1 config --local remote.origin.partialclonefilter)" = "blob:none" + done +' - test_cmp expect_1.oids observed.oids && - test "$(git -C pc1 config --local core.repositoryformatversion)" = "1" && - test "$(git -C pc1 config --local remote.origin.promisor)" = "true" && - test "$(git -C pc1 config --local remote.origin.partialclonefilter)" = "blob:none" +test_expect_success 'do partial clone with size limit' ' + for option in "--filter=blob:limit=1" "--partial=1" + do + rm -rf pc-limit && + git clone --no-checkout "$option" "file://$(pwd)/srv.bare" pc-limit && + + git -C pc-limit rev-list --quiet --objects --missing=print HEAD >revs && + awk -f print_1.awk revs | + sed "s/?//" | + sort >observed.oids && + + test_cmp expect_1.oids observed.oids && + test "$(git -C pc-limit config --local core.repositoryformatversion)" = "1" && + test "$(git -C pc-limit config --local remote.origin.promisor)" = "true" && + test "$(git -C pc-limit config --local remote.origin.partialclonefilter)" = "blob:limit=1" + done ' test_expect_success 'verify that .promisor file contains refs fetched' '