From: Taylor Blau <firstname.lastname@example.org> To: Jeff King <email@example.com> Cc: Taylor Blau <firstname.lastname@example.org>, email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org Subject: Re: [PATCH v3 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth' Date: Fri, 31 Jul 2020 17:29:05 -0400 [thread overview] Message-ID: <20200731212905.GE3409@syl.lan> (raw) In-Reply-To: <20200731210114.GC1440890@coredump.intra.peff.net> On Fri, Jul 31, 2020 at 05:01:14PM -0400, Jeff King wrote: > On Fri, Jul 31, 2020 at 04:26:39PM -0400, Taylor Blau wrote: > > > +test_expect_success 'upload-pack limits tree depth filters' ' > > + test_config -C srv.bare uploadpackfilter.allow false && > > + test_config -C srv.bare uploadpackfilter.tree.allow true && > > + test_config -C srv.bare uploadpackfilter.tree.maxDepth 0 && > > + test_must_fail ok=sigpipe git clone --no-checkout --filter=tree:1 \ > > + "file://$(pwd)/srv.bare" pc3 2>err && > > + test_i18ngrep "filter '\''tree'\'' not supported (maximum depth: 0, but got: 1)" err > > +' > > Same i18ngrep comment as in the earlier patch (i.e., we can use grep > here). > > > @@ -1029,6 +1040,11 @@ static void die_if_using_banned_filter(struct upload_pack_data *data) > > > > strbuf_addf(&buf, "git upload-pack: filter '%s' not supported", > > list_object_filter_config_name(banned->choice)); > > + if (banned->choice == LOFC_TREE_DEPTH && > > + data->tree_filter_max_depth != ULONG_MAX) > > + strbuf_addf(&buf, _(" (maximum depth: %lu, but got: %lu)"), > > + data->tree_filter_max_depth, > > + banned->tree_exclude_depth); > > Hmm. So I see now why you wanted to go with the strbuf in the earlier > patch. This does still feel awkward, though. You check "is it allowed" > in an earlier function, we get "nope, it's not allowed", and now we have > to reimplement the check here. That seems like a maintenance burden. I'm not sure that I follow. Is the earlier function that you're referring to 'banned_filter'? If so, the only use of that function is within 'die_if_using_banned_filter'. 'banned_filter' exists only insofar as to answer the question "return me the first banned filter, if one exists, or NULL otherwise". Then, dying here is as simple as (1) lookup the banned filter, and (2) check if it's NULL or not. If you're referring to 'allows_filter_choice', I guess I see what you're getting it, but to be honest I'm not sure if I'm buying it. If we were to combine 'allows_filter_choice', 'banned_filter', and 'die_if_using_banned_filter' into one function that traversed the filter tree and 'die()'d as soon as it got to a banned one, that function would have to know how to: 1. Recurse through the tree when it hits a LOFC_COMBINE node. 2. At each node, translate the filter->choice into the appropriate key name, look it up, and then figure out how to interpret its allowed status (including falling back to the default if unspecified). 3. And, it would have to figure out how to format the message at each step. (3) I think is made easier, since we know what message to format based on whether or not we're in the 'opts->choice == LOFC_TREE_DEPTH' arm or not. But, there are two more things that we now have to cram into that same function. Maybe I'm being too strict an adherent to having simpler functions, but I'm failing to see how to combine these in a way that's cleaner than what's written here. > I think a more natural flow would be either: > > - the "is it allowed" functions calls immediately into the function > that sends the error and dies (this might need a conditional if > there's a caller who doesn't want to die; I didn't check) > > or > > - on failure it populates an error buffer itself, which the caller can > then pass along as it sees fit I guess; I'm not a huge fan of the side-effecting nature, but maybe it's cleaner. I dunno. I appreciate your review, but I feel like we're in a bikeshed. > -Peff Thanks, Taylor
next prev parent reply other threads:[~2020-07-31 21:29 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-23 1:48 [PATCH v2 0/4] upload-pack: custom allowed object filters Taylor Blau 2020-07-23 1:48 ` [PATCH v2 1/4] list_objects_filter_options: introduce 'list_object_filter_config_name' Taylor Blau 2020-07-23 1:49 ` [PATCH v2 2/4] upload-pack.c: allow banning certain object filter(s) Taylor Blau 2020-07-23 1:49 ` [PATCH v2 3/4] upload-pack.c: pass 'struct list_objects_filter_options *' Taylor Blau 2020-07-23 1:49 ` [PATCH v2 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth' Taylor Blau 2020-07-23 20:43 ` [PATCH v2 0/4] upload-pack: custom allowed object filters SZEDER Gábor 2020-07-24 16:51 ` Taylor Blau 2020-07-24 19:51 ` Jeff King 2020-07-27 14:25 ` Taylor Blau 2020-07-27 19:34 ` SZEDER Gábor 2020-07-27 19:36 ` Taylor Blau 2020-07-27 19:42 ` Jeff King 2020-07-27 19:59 ` SZEDER Gábor 2020-07-27 20:03 ` Taylor Blau 2020-07-31 20:26 ` [PATCH v3 " Taylor Blau 2020-07-31 20:26 ` [PATCH v3 1/4] list_objects_filter_options: introduce 'list_object_filter_config_name' Taylor Blau 2020-07-31 20:26 ` [PATCH v3 2/4] upload-pack.c: allow banning certain object filter(s) Taylor Blau 2020-07-31 20:54 ` Jeff King 2020-07-31 21:20 ` Taylor Blau 2020-07-31 20:26 ` [PATCH v3 3/4] upload-pack.c: pass 'struct list_objects_filter_options *' Taylor Blau 2020-07-31 20:26 ` [PATCH v3 4/4] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth' Taylor Blau 2020-07-31 21:01 ` Jeff King 2020-07-31 21:22 ` Jeff King 2020-07-31 21:30 ` Taylor Blau 2020-07-31 21:29 ` Taylor Blau [this message] 2020-07-31 21:36 ` Jeff King 2020-07-31 21:43 ` Jeff King 2020-08-03 18:00 ` [PATCH v4 0/3] upload-pack: custom allowed object filters Taylor Blau 2020-08-03 18:00 ` [PATCH v4 2/3] upload-pack.c: allow banning certain object filter(s) Taylor Blau 2020-08-03 18:00 ` [PATCH v4 1/3] list_objects_filter_options: introduce 'list_object_filter_config_name' Taylor Blau 2020-08-03 18:00 ` [PATCH v4 3/3] upload-pack.c: introduce 'uploadpackfilter.tree.maxDepth' Taylor Blau 2020-08-04 0:37 ` [PATCH v4 0/3] upload-pack: custom allowed object filters Jeff King
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=20200731212905.GE3409@syl.lan \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH v3 4/4] upload-pack.c: introduce '\''uploadpackfilter.tree.maxDepth'\''' \ /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
Code repositories for project(s) associated with this 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).