On Wed, Mar 18, 2020 at 06:18:25AM -0400, Jeff King wrote: > On Tue, Mar 17, 2020 at 02:39:05PM -0600, Taylor Blau wrote: > > > Hi Christian, > > > > Of course, I would be happy to send along our patches. They are included > > in the series below, and correspond roughly to what we are running at > > GitHub. (For us, there have been a few more clean-ups and additional > > patches, but I squashed them into 2/2 below). > > > > The approach is roughly that we have: > > > > - 'uploadpack.filter.allow' -> specifying the default for unspecified > > filter choices, itself defaulting to true in order to maintain > > backwards compatibility, and > > > > - 'uploadpack.filter.<filter>.allow' -> specifying whether or not each > > filter kind is allowed or not. (Originally this was given as 'git > > config uploadpack.filter=blob:none.allow true', but this '=' is > > ambiguous to configuration given over '-c', which itself uses an '=' > > to separate keys from values.) > > 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 we want to declare a real subsection for each filter and not > just "uploadpack.filter.<filter>". That gives us room to expand to other > config options besides "allow" later on if we need to. > > We don't want to claim "uploadpack.allow" and "uploadpack.<filter>.allow"; > that's too generic. > > Likewise "filter.allow" is too generic. I wonder. A multi-valued 'uploadpack.filter.allow' *might* solve some problems, but the more I turn it over in my head, the more that I think that it's creating more headaches for us than it's removing. On the pro's side, is that we could have this be a multi-valued key where each value is the name of an allowed filter. I guess that would solve the subsection-naming problem, but it is admittedly generic, not to mention the fact that we already *use* this key to specify a default value for missing 'uploadpack.filter.<filter>.allow' values. For that reason, it seems like a non-starter to me. > We could do "uploadpackfilter.allow" and "uploadpackfilter.<filter>.allow", > but that's both ugly _and_ separates these options from the rest of > uploadpack.*. > > We could use a character besides ".", which would reduce confusion. But > what? Using colon is kind of ugly, because it's already syntactically > significant in filter names, and you get: > > uploadpack.filter:blob:none.allow > > We tried equals, like: > > uploadpack.filter=blob:none.allow > > but there's an interesting side effect. Doing: > > git -c uploadpack.filter=blob:none.allow=true upload-pack ... > > doesn't work, because the "-c" parser ends the key at the first "=". As > it should, because otherwise we'd get confused by an "=" in a value. > This is a failing of the "-c" syntax; it can't represent values with > "=". Fixing it would be awkward, and I've never seen it come up in > practice outside of this (you _could_ have a branch with a funny name > and try to do "git -c branch.my=funny=branch.remote=origin" or > something, but the lack of bug reports suggests nobody is that > masochistic). Thanks for adding some more detail to this decision. Another thing we could do is just simply use a different character. It may be a little odd, but it keeps the filter-related variables in their own sub-section, allowing us to add more configuration sub-variables in the future. I guess that calling it something like: $ git config uploadpack.filter@blob:none.allow <true|false> is a little strange (i.e., why '@' over '#'? There's certainly no precedent here that I can think of...), but maybe it is slightly less-weird than a pseudo-four-level key. > So...maybe the extra dot is the last bad thing? > > > I noted in the second patch that there is the unfortunate possibility of > > encountering a SIGPIPE when trying to write the ERR sideband back to a > > client who requested a non-supported filter. Peff and I have had some > > discussion off-list about resurrecting SZEDZER's work which makes room > > in the buffer by reading one packet back from the client when the server > > encounters a SIGPIPE. It is for this reason that I am marking the series > > as 'RFC'. > > For reference, the patch I was thinking of was this: > > https://lore.kernel.org/git/20190830121005.GI8571@xxxxxxxxxx/ Thanks. > -Peff Thanks, Taylor
