Re: [PATCH] list-objects-filter: merge filter data structs

From: Matthew DeVore <matvore@comcast.net>
Date: May 29, 2019, 23:10 UTC
Message-ID: <20190529231013.GD4700@comcast.net>
In-Reply-To: <e9147614-80f9-4c18-b431-539e2376295d@jeffhostetler.com>

Show 6 quoted lines

> > I am hoping that I am not misreading the intention but you do not
> > plan to use the above so that you can say "apply 'tree:depth=4' and
> > 'blobs:limit=1G' at the same time" by filling the fields in a single
> > struct, do you?  For combined filter, you'll still have multiple
> > instances of filter_data struct, strung together in a list that says
> > "all of these must be satisfied" or something like that, right?

It is the latter and not the former. I don't want to populate multiple filters in a single struct. My idea was basically that when the fields got too numerous we could use unions or add NULLable pointers for the bigger filter data structs, so the data would be properly "optional".

On Wed, May 29, 2019 at 04:57:23PM -0400, Jeff Hostetler wrote:

Show 6 quoted lines

> I'm not sure I like the combined structure as proposed.
> But let's think about it.
> 
> I think part of problem with my original version was putting the
> filter_fn and filter_free_fn in the traversal_context rather than
> inside the filter_*_data structure.

Agreed. This cleanup I'm proposing is basically something I was itching to do in the process of bundling up the filter_fn and filter_free_fn pointers in a single pointer, which makes the LOFC_COMBINE-particular filter data more concise.

I can still bundle up the pointers into a single pointer and make this cleanup less aggressive.

Show 9 quoted lines

> I did a simple combined structure for the list_objects_filter_options
> and kind of regretted it because it wasn't obvious which data fields
> were defined or undefined in each filter constructor.  But it was
> convenient when parsing the command line.
> 
> I think having a combined structure with a union enclosing a structure
> for the data fields in each filter type would be worth considering.
> That way you have a somewhat self-documenting sub-structure for each
> filter type that indicates which fields are defined.

I'm OK with the union approach. The drawback is that the __free function now needs a switch block to choose the correct union, but the union is also good for the self-documenting aspect you mention.

Show 7 quoted lines

> 
> I'd also suggest keeping the "oidset omits" inside each of the
> sub-structures, but that's just me.
> 
> 
> BTW, I don't see a free_fn.  That may collapse out with your proposal
> but I wanted to ask.

See the list_objects_filter__free function. It's trivial, but it inherits the leakiness of the sparse filters' free function it subsumes.

Thank you for considering the patch.