diff mbox series

[v2,1/8] uploadpack.txt: document implication of `uploadpackfilter.allow`

Message ID 270ff80dacff96f441e12954b059a68300157f2d.1615813673.git.ps@pks.im (mailing list archive)
State Superseded
Headers show
Series rev-parse: implement object type filter | expand

Commit Message

Patrick Steinhardt March 15, 2021, 1:14 p.m. UTC
When `uploadpackfilter.allow` is set to `true`, it means that filters
are enabled by default except in the case where a filter is explicitly
disabled via `uploadpackilter.<filter>.allow`. This option will not only
enable the currently supported set of filters, but also any filters
which get added in the future. As such, an admin which wants to have
tight control over which filters are allowed and which aren't probably
shouldn't ever set `uploadpackfilter.allow=true`.

Amend the documentation to make the ramifications more explicit so that
admins are aware of this.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/config/uploadpack.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Jeff King April 6, 2021, 5:17 p.m. UTC | #1
On Mon, Mar 15, 2021 at 02:14:31PM +0100, Patrick Steinhardt wrote:

> When `uploadpackfilter.allow` is set to `true`, it means that filters
> are enabled by default except in the case where a filter is explicitly
> disabled via `uploadpackilter.<filter>.allow`. This option will not only
> enable the currently supported set of filters, but also any filters
> which get added in the future. As such, an admin which wants to have
> tight control over which filters are allowed and which aren't probably
> shouldn't ever set `uploadpackfilter.allow=true`.
> 
> Amend the documentation to make the ramifications more explicit so that
> admins are aware of this.

It might help to guide the admin a bit more here. What are we really
worried about? Probably that an expensive filter would be added that
would make an admin with a public-facing server unhappy.

Maybe we should be more explicit about our recommendations, like:

  This defaults to `true` for historical reasons, but that includes
  expensive-to-compute filters (both existing ones like `sparse`, but
  also future ones). A safer value is to set this to `false` and
  mark individual filters as allowed.

But then of course somebody wonders which set are expensive and which
ones are not. And really, "expensive" here is not that expensive. It is
"do not support bitmaps".

So I wonder if this concern is overblown in the first place. People who
care about using only bitmap-supported filters probably already set this
to "false". And vaguely calling things "expensive" is probably being
overly scary. But in that case, I'm not sure we even need to add a
reminder that future ones will also be enabled (OTOH, I do not mind it
so much; it is encouraging people to set this to false and mark
individual ones as allowed).

-Peff
diff mbox series

Patch

diff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt
index b0d761282c..6729a072ea 100644
--- a/Documentation/config/uploadpack.txt
+++ b/Documentation/config/uploadpack.txt
@@ -59,7 +59,8 @@  uploadpack.allowFilter::
 
 uploadpackfilter.allow::
 	Provides a default value for unspecified object filters (see: the
-	below configuration variable).
+	below configuration variable). If set to `true`, this will also
+	enable all filters which get added in the future.
 	Defaults to `true`.
 
 uploadpackfilter.<filter>.allow::