On Fri, Apr 17, 2020 at 7:40 PM Taylor Blau <me@xxxxxxxxxxxx> 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@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 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.
