Re: Re*: [RFC PATCH 0/2] upload-pack.c: limit allowed filter choices

From: Jeff King <peff@peff.net>
Date: Mar 19, 2020, 17:03 UTC
Message-ID: <20200319170350.GA4075823@coredump.intra.peff.net>
In-Reply-To: <xmqqtv2lfrk7.fsf_-_@gitster.c.googlers.com>

On Wed, Mar 18, 2020 at 11:26:00AM -0700, Junio C Hamano wrote:

Show 16 quoted lines

> > One thing that's a little ugly here is the embedded dot in the
> > subsection (i.e., "filter.<filter>"). It makes it look like a four-level
> > key, but really there is no such thing in Git.  But everything else we
> > tried was even uglier.
> 
> I think this gives us the best arrangement by upfront forcing all
> the configuration handers for "<subcommand>.*.<token>" namespace,
> current and future, to use "<group-prefix>" before the unbounded set
> of user-specifiable values that affects the <subcommand> (which is
> "uploadpack").
> 
> So far, the configuration variables that needs to be grouped by
> unbounded set of user-specifiable values we supported happened to
> have only one sensible such set for each <subcommand>, so we could
> get away without such <group-prefix> and it was perfectly OK to
> have, say "guitool.<name>.cmd".

Yeah. We have often just split those out into a separate hierarchy from <subcommand> E.g., tar.<format>.command, which is really feeding the git-archive command. We could do that here, too, but I wasn't sure of a good name (this really is upload-pack specific, though I guess in theory other commands could grow a need to look at or restrict "remote object filters").

> Syntactically, the convention to always end such <group-prefix> with
> a dot "." may look unusual, or once readers' eyes get used to them,
> may look natural.  One tiny sad thing about it is that it cannot be
> mechanically enforced, but that is minor.

The biggest downside to implying a 4-level key is that the case-sensitivity rules may be different. I.e., you can say:

  UploadPack.filter.blob:none.Allow

but not:

  UploadPack.Filter.blob:none.Allow

Since "filter" is part of the subsection, it's case sensitive. We could match it case-insensitively in upload_pack_config(), but it would crop up in other laces (e.g., "git config --unset" would still care).

Show 7 quoted lines

> > We could do "uploadpackfilter.allow" and "uploadpackfilter.<filter>.allow",
> > but that's both ugly _and_ separates these options from the rest of
> > uploadpack.*.
> 
> There is an existing instance of a configuration that affects
> <subcommand> that uses a different word after <subcommand>, which is
> credentialCache.ignoreSIGHUP, and I tend to agree that it is ugly.

I don't think that's what's going on here. It affects only the credential-cache subcommand, but we avoid hyphens in our key names. So it really is the subcommand; it's just that the name is a superset of another command name. :)

Show 9 quoted lines

> By the way, I noticed the following while I was studying the current
> practice, so before I forget...
> 
> -- >8 --
> Subject: [PATCH] separate tar.* config to its own source file
> 
> Even though there is only one configuration variable in the
> namespace, it is not quite right to have tar.umask described
> among the variables for tag.* namespace.

Yeah, this is definitely an improvement. But I was surprised that tar.<format>.* wasn't covered here. It is documented in git-archive. Probably worth moving or duplicating it in git-config.

-Peff