On Fri, Apr 17, 2020 at 02:06:45PM -0400, Jeff King wrote: > On Fri, Apr 17, 2020 at 11:40:30AM -0600, Taylor Blau wrote: > > > > What do you think about something like: > > > > > > [promisorFilter "noBlobs"] > > > type = blob:none > > > uploadpack = true # maybe "allow" could also mean "true" here > > > ... > > > ? > > > > I'm not sure about introducing a layer of indirection here with > > "noBlobs". It's nice that it could perhaps be enabled/disabled for > > different builtins (e.g., by adding 'revList = false', say), but I'm not > > convinced that this is improving all of those cases, either. > > Yeah, I don't like forcing the user to invent a subsection name. My > first thought was to suggest: > > [promisorFilter "blob:none"] > uploadpack = true > > but your tree example shows why that gets awkward: there are more keys > than just "allow this". > > > One thing that I can think of (other than replacing the '.' with another > > delimiting character other than '=') is renaming the key from > > 'uploadPack' to 'uploadPackFilter'. I believe that this was suggested by > > Yeah, that proposal isn't bad. To me the two viable options seem like: > > - uploadpack.filter.<filter>.*: this has the ugly fake multilevel > subsection, but stays under uploadpack.* > > - uploadpackfilter.<filter>.*: more natural subsection, but not grouped > syntactically with other uploadpack stuff > > I am actually leaning towards the second. It should make the parsing > code less confusing, and it's not like there aren't already other config > sections that impact uploadpack. Me too. > > > > For reference, the patch I was thinking of was this: > > > > > > > > https://lore.kernel.org/git/20190830121005.GI8571@xxxxxxxxxx/ > > > > > > Are you using the patches in this series with or without something > > > like the above patch? I am ok to resend this patch series including > > > the above patch (crediting Szeder) if you use something like it. > > > > We're not using them, but without them we suffer from a problem that if > > we can get a SIGPIPE when writing the "sorry, I don't support that > > filter" message back to the client, then they won't receive it. > > > > Szeder's patches help address that issue by catching the SIGPIPE and > > popping off enough from the client buffer so that we can write the > > message out before dying. > > I definitely think we should pursue that patch, but it really can be > done orthogonally. It's an existing bug that affects other instances > where upload-pack returns an error. The tests can work around it with > "test_must_fail ok=sigpipe" in the meantime. Yes, I agree. My main hesitation is that it would be uncouth of me to send a patch that includes 'test_must_fail ok=sigpipe' to the list, but if you (and others) feel that this is an OK intermediate step (given that we can easily remove it once SZEDER's patch lands), then I am OK with it, too. And I see that Christian already posted such a patch to the list. > -Peff Thanks, Taylor
