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 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
-Peff
