Subject: Re: [PATCH v4 01/10] list-objects-filter: make API easier to use

From: Matthew DeVore <[email-masked]>
Date: Wed, 26 Jun 2019 17:46:35 -0700

On Fri, Jun 21, 2019 at 03:58:38PM -0700, Jonathan Tan wrote:
> So what happens is that filter_fn, filter_free_fn, and filter_data are
> encapsulated into one opaque object, and users will now use filter_fn
> and filter_free_fn through other functions that we expose, allowing us
> to add some conveniences that currently have to be repeated at each call
> site.
> 
> I would prefer the following commit message:
> 
>   list-objects-filter: encapsulate filter components
> 
>   Encapsulate filter_fn, filter_free_fn, and filter_data into its own
>   opaque struct.
> 
>   Due to opaqueness, filter_fn and filter_free_fn can no longer be
>   accessed directly by users. Currently, all usages of filter_fn are
>   guarded by a necessary check:
> 
>     (obj->flags & NOT_USER_GIVEN) && filter_fn
> 
>   Take the opportunity to include this check into the new function
>   list_objects_filter__filter_object(), so that we no longer need to
>   write this check at every caller of the filter function.
> 
>   Also, the init functions in list-objects-filter.c no longer need to
>   confusingly return the filter constituents in various places
>   (filter_fn and filter_free_fn as out parameters, and filter_data as
>   the function's return value); they can just initialize the "struct
>   filter" passed in.
> 

Very nice, applied. I think your commit message is much more helpful than mine
and doesn't use the filter combination feature as an excuse for the change.

> > +	/*
> > +	 * No filter is active or user gave object explicitly. Choose default
> > +	 * behavior based on filter situation.
> > +	 */
> 
> This part is when we do not need to apply the filter (or none exists). I
> think the comment will be better if stated more explicitly:
> 
>   No filter is active or user gave object explicitly. In this case,
>   always show the object (except when LOFS_END_TREE, since this tree had
>   already been shown when LOFS_BEGIN_TREE).
> 

Agreed, this is a little better. Applied.