| Message ID | cover.1584477196.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | upload-pack.c: limit allowed filter choices | expand |
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. 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). 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@szeder.dev/ -Peff
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@szeder.dev/ Thanks. > -Peff Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: >> 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 >> "=". s/value/key/ I presume ;-)
On Wed, Mar 18, 2020 at 03:28:18PM -0600, Taylor Blau wrote: > 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. IMHO we should avoid multi-valued keys when there's not a compelling reason. There are a lot of corner cases they introduce (e.g., there's no standard way to override them rather than adding to the list). > 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. I guess it's subjective, but the "@" just feels odd because it's associated with so many other meanings. Likewise "#". -Peff
On Wed, Mar 18, 2020 at 03:41:51PM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > >> 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 > >> "=". > > s/value/key/ I presume ;-) Yes. :) -Peff
Hi Taylor and Peff, On Wed, Mar 18, 2020 at 11:18 AM Jeff King <peff@peff.net> wrote: > > On Tue, Mar 17, 2020 at 02:39:05PM -0600, Taylor Blau wrote: > > > 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). Thanks for the patches, and sorry for the delay in responding! > > 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. > > We could do "uploadpackfilter.allow" and "uploadpackfilter.<filter>.allow", > but that's both ugly _and_ separates these options from the rest of > uploadpack.*. What do you think about something like: [promisorFilter "noBlobs"] type = blob:none uploadpack = true # maybe "allow" could also mean "true" here ... ? > > 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@szeder.dev/ 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. Thanks, Christian.
On Fri, Apr 17, 2020 at 11:41:48AM +0200, Christian Couder wrote: > Hi Taylor and Peff, > > On Wed, Mar 18, 2020 at 11:18 AM Jeff King <peff@peff.net> wrote: > > > > On Tue, Mar 17, 2020 at 02:39:05PM -0600, Taylor Blau wrote: > > > > > 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). > > Thanks for the patches, and sorry for the delay in responding! No need to apologize. Clearly these had slipped my mind, too :). > > > 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. > > > > We could do "uploadpackfilter.allow" and "uploadpackfilter.<filter>.allow", > > but that's both ugly _and_ separates these options from the rest of > > uploadpack.*. > > 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. For example, what happens if I have something like: [uploadpack "filter.tree"] maxDepth = 1 allow = true but I want to use a different value of maxDepth for, say, rev-list? I'd rather have two sections (each for the 'tree' filter, but scoped to 'upload-pack' and 'rev-list' separately) than write something like: [promisorFilter "treeDepth"] type = tree uploadpack = true uploadpackMaxDepth = 1 revList = true revListMaxDepth = 0 ... So, yeah, the current system is not great because it has the '.' in the second component. I am definitely eager to hear other suggestions about naming it differently, but I think that the general structure is on track. 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 Juino (?) earlier in the thread. I think that it's a fine resolution to this, but I'm also not opposed to what is currently written in too above patches. > > > 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@szeder.dev/ > > 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 appreciate your offer to resubmit the series on my behalf, but I was already planning on doing this myself and wouldn't want to burden you with another to-do. I'll be happy to take it on myself, probably within a week or so. > Thanks, > Christian. Thanks, Taylor
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@szeder.dev/ > > > > 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
On Fri, Apr 17, 2020 at 7:40 PM Taylor Blau <me@ttaylorr.com> wrote: > > On Fri, Apr 17, 2020 at 11:41:48AM +0200, Christian Couder 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. > > For example, what happens if I have something like: > > [uploadpack "filter.tree"] > maxDepth = 1 > allow = true > > but I want to use a different value of maxDepth for, say, rev-list? I'd > rather have two sections (each for the 'tree' filter, but scoped to > 'upload-pack' and 'rev-list' separately) than write something like: > > [promisorFilter "treeDepth"] > type = tree > uploadpack = true > uploadpackMaxDepth = 1 > revList = true > revListMaxDepth = 0 > ... You can have two sections using: [promisorFilter "treeDepth1"] type = tree uploadpack = true maxDepth = 1 [promisorFilter "treeDepth0"] type = tree revList = true maxDepth = 0 (Of course "treeDepth1" for example could be also spelled "treeDepthOneLevel" or however the user prefers.) > So, yeah, the current system is not great because it has the '.' in the > second component. I am definitely eager to hear other suggestions about > naming it differently, but I think that the general structure is on > track. > > 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 don't like either of those very much. I think an upload-pack filter is not very different than a rev-list filter. They are all promisor (or partial clone) filter, so there is no real reason to differentiate at the top level of the key name hierarchy. I also think that users are likely to want to use the same filters for both upload-pack filters and rev-list filters, so using 'uploadPack' or 'uploadPackFilter' might necessitate duplicating entries with other keys for rev-list filters or other filters. > > > For reference, the patch I was thinking of was this: > > > > > > https://lore.kernel.org/git/20190830121005.GI8571@szeder.dev/ > > > > 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 appreciate your offer to resubmit the series on my behalf, but I was > already planning on doing this myself and wouldn't want to burden you > with another to-do. I'll be happy to take it on myself, probably within > a week or so. Ok, I am happy that you will resubmit then. Thanks, Christian.
On Fri, Apr 17, 2020 at 8:06 PM Jeff King <peff@peff.net> 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". I like your first thought better than something that starts with "uploadPack". And I think if we let people find a subsection name (as what I suggest) they might indeed end up with something like: [promisorFilter "blob:none"] type = blob:none uploadpack = true as they might lack inspiration. As filters are becoming more and more complex though, people might find it much simpler to use the subsection name in commands if we let them do that. For example we already allow: git rev-list --filter=combine:<filter1>+<filter2>+...<filterN> ... which could be simplified to: git rev-list --filter=combinedFilter ... (where "combinedFilter" is defined in the config with "type=combine:<filter1>+<filter2>+...<filterN>".) [...] > > 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. Ok, maybe I will take a look a this one then. Thanks, Christian.
On Tue, Apr 21, 2020 at 02:34:18PM +0200, Christian Couder wrote: > On Fri, Apr 17, 2020 at 8:06 PM Jeff King <peff@peff.net> 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". > > I like your first thought better than something that starts with > "uploadPack". And I think if we let people find a subsection name (as > what I suggest) they might indeed end up with something like: > > [promisorFilter "blob:none"] > type = blob:none > uploadpack = true > > as they might lack inspiration. As filters are becoming more and more > complex though, people might find it much simpler to use the > subsection name in commands if we let them do that. For example we > already allow: > > git rev-list --filter=combine:<filter1>+<filter2>+...<filterN> ... > > which could be simplified to: > > git rev-list --filter=combinedFilter ... > > (where "combinedFilter" is defined in the config with > "type=combine:<filter1>+<filter2>+...<filterN>".) > > [...] I really think that we're getting ahead of ourselves here. For now, I don't think that we have powerful enough filters that it makes sense to put them together with combine and give them meaningful names. At least, no one has asked about such a thing on the list, which I take to mean that people don't have a use for it. I'm also skeptical about relying on named filters when working with a server. If the server defines the filter names (as we at GitHub would do under this proposal), then what use are they to the client? For the server, I'm not at all convinced that this is beneficial: the extra layer of indirection through the configuration makes this brittle and hard-to-follow. Not to mention that the server could just as easily spell out the whole filter. I'm not trying to give you a too-simple proposal, but I think yours introduces additional complexity that is trying to enable use-cases that we don't have in practice. > > > 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. > > Ok, maybe I will take a look a this one then. Thanks. > Thanks, > Christian. Thanks, Taylor
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@szeder.dev/ > > > > > > 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
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.) 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, our configuration at GitHub looks something like: [uploadpack] allowAnySHA1InWant = true allowFilter = true [uploadpack "filter"] allow = false [uploadpack "filter.blob:limit"] allow = true [uploadpack "filter.blob:none"] allow = true with a few irrelevant details elided for the purposes of the list :-). I'd be happy to take in any comments that you or others might have before dropping the 'RFC' status. Taylor Blau (2): list_objects_filter_options: introduce 'list_object_filter_config_name' upload-pack.c: allow banning certain object filter(s) Documentation/config/uploadpack.txt | 12 ++++++ list-objects-filter-options.c | 25 +++++++++++ list-objects-filter-options.h | 6 +++ t/t5616-partial-clone.sh | 23 ++++++++++ upload-pack.c | 67 +++++++++++++++++++++++++++++ 5 files changed, 133 insertions(+) -- 2.26.0.rc2.2.g888d9484cf