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.
