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

[Matthew DeVore <matvore@comcast.net>]

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

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

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