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. > > > 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. -Peff
