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.
