Re: [PATCH v1 1/5] list-objects-filter: refactor into a context struct

[Matthew DeVore <matvore@comcast.net>]

On Thu, May 23, 2019 at 05:49:38PM -0700, Emily Shaffer wrote:
> This commit message might read more easily on its own if you define
> "this bundle of data" at least once. Since there are things being moved
> from both list-objects-filter.c (filter_blobs_none_data) and
> list-objects-filter.h (list_objects_filter_result and filter_free_fn)
> into the new struct in list-objects-filter.h, it's not immediately clear
> to me from the diff what's going on.
> 

I will take care to define the context struct in the commit message and the
code comments.

I was originally intending to put this in the context struct: data common to all
filter types used when executing a filter. But now I'm leaning toward just
using the struct as a grab-bag for execution data for *all* filter types. This
would be more similar to how the list_objects_filter_options struct works, and
it would save having to define a special free_fn for each (actually, most)
filter types. I'll play around with this idea and probably put it in the next
roll-up (comments welcome, though).

> > -static void *filter_blobs_none__init(
> > -	struct oidset *omitted,
> > +static void filter_blobs_none__init(
> >  	struct list_objects_filter_options *filter_options,
> > -	filter_object_fn *filter_fn,
> > -	filter_free_fn *filter_free_fn)
> > +	struct filter_context *ctx)
> >  {
> > -	struct filter_blobs_none_data *d = xcalloc(1, sizeof(*d));
> > -	d->omits = omitted;
> > -
> > -	*filter_fn = filter_blobs_none;
> > -	*filter_free_fn = free;
> > -	return d;
> > +	ctx->filter_fn = filter_blobs_none;
> 
> I think you want to set ctx->free_fn here too, right? It seems like
> you're not setting ctx->omitted anymore because you'd be reading that
> information in from ctx->omitted (so it's redundant).
> 

Not necessary because the blobs:none filter type doesn't have filter-specific
data for it. blobs:none is unique in that regard.