Re: [RFC PATCH] list-objects-filter: disable 'sparse:path' filters

From: Matthew DeVore <matvore@comcast.net>
Date: May 24, 2019, 17:07 UTC
Message-ID: <20190524170758.GB7160@comcast.net>
In-Reply-To: <20190524120318.4851-1-chriscool@tuxfamily.org>

On Fri, May 24, 2019 at 02:03:18PM +0200, Christian Couder wrote:

> For now though, let's just disable 'sparse:path' filters.

This is probably the right thing to do. I did jump through a lot of hoops to support escaping sub-filters in my pending filter combination patchset, since sparse spec path names can have arbitrary characters. After this patch we only support a handful of characters in filterspecs, so a lot of that escaping logic can be dropped, at least for now. Anyway, this is not a complaint, just an observation.

The alternative is to hide sparse:path= support behind a flag which is disabled by default, but I don't recommend doing that just to have an excuse to include the URL-encoding logic.

Thank you for cleaning up.

Show 8 quoted lines

>  	} else if (skip_prefix(arg, "sparse:path=", &v0)) {
> -		filter_options->choice = LOFC_SPARSE_PATH;
> -		filter_options->sparse_path_value = strdup(v0);
> -		return 0;
> +		if (errbuf) {
> +			strbuf_addstr(
> +				errbuf,
> +				_("sparse:path filters are now disabled"));

This wording may leave room for misunderstanding, since it sounds a little like the filter can be re-enabled somehow. Maybe you can say "sparse:path filters support has been dropped [optional: 'for security reasons' etc.]"

Show 5 quoted lines

> +		}
> +		return 1;
>  	}
>  	/*
>  	 * Please update _git_fetch() in git-completion.bash when you

As the comment states, don't forget to update git-completion.bash :)