From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS53758 23.128.96.0/24 X-Spam-Status: No, score=-3.8 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_PASS, SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by dcvr.yhbt.net (Postfix) with ESMTP id B9FFB1F4B4 for ; Tue, 6 Apr 2021 17:17:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233676AbhDFRRn (ORCPT ); Tue, 6 Apr 2021 13:17:43 -0400 Received: from cloud.peff.net ([104.130.231.41]:42744 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233060AbhDFRRm (ORCPT ); Tue, 6 Apr 2021 13:17:42 -0400 Received: (qmail 8157 invoked by uid 109); 6 Apr 2021 17:17:33 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Tue, 06 Apr 2021 17:17:33 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 30505 invoked by uid 111); 6 Apr 2021 17:17:33 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Tue, 06 Apr 2021 13:17:33 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 6 Apr 2021 13:17:33 -0400 From: Jeff King To: Patrick Steinhardt Cc: git@vger.kernel.org, Christian Couder , Taylor Blau Subject: Re: [PATCH v2 1/8] uploadpack.txt: document implication of `uploadpackfilter.allow` Message-ID: References: <270ff80dacff96f441e12954b059a68300157f2d.1615813673.git.ps@pks.im> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <270ff80dacff96f441e12954b059a68300157f2d.1615813673.git.ps@pks.im> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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..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