On Fri, May 24, 2019 at 05:01:15PM -0400, Jeff Hostetler wrote:
> We are allowing an unlimited number of filters in the composition.
> In the code, the compose filter data has space for a LHS and RHS, so
> I'm assuming we're mapping
>
> --filter=f1 --filter=f2 --filter=f3 --filter=f4
> or --filter=combine:f1+f2+f3+f4
> into basically
> (compose f1 (compose f2 (compose (f3 f4)))
>
> I wonder if it would be easier to understand if we just built an array
> or linked list, but I'll read on.
As I mentioned in earlier messages, I have changed this to use an array. It's
nicer now.
(nit: the filters were left-associative rather than right-associative)
> Should we swap the order of the terms in the || so that we always
> clear the d->sub[i].is_skipping_tree on LOFS_END_TREE ?
>
Done, and added a comment:
/*
* Check should_delegate before oidset_contains so that
* is_skipping_tree gets unset even when the object is marked as seen.
* As of this writing, no filter uses LOFR_MARK_SEEN on trees that also
* uses LOFR_SKIP_TREE, so the ordering is only theoretically
* important. Be cautious if you change the order of the below checks
* and more filters have been added!
*/
>
> > + result[i] = LOFR_ZERO;
> > + continue;
> > + }
> > +
> > + result[i] = d->sub[i].ctx.filter_fn(
> > + r, filter_situation, obj, pathname, filename,
> > + &d->sub[i].ctx);
> > +
> > + if (result[i] & LOFR_MARK_SEEN)
> > + oidset_insert(&d->sub[i].seen, &obj->oid);
>
> So filter[i] has said it never wants to show this object (hard omit).
> And the guard at the top of the loop will prevent future invocations
> from checking it again if the object is revisited.
>
Yes.
> > +
> > + if (result[i] & LOFR_SKIP_TREE) {
> > + d->sub[i].is_skipping_tree = 1;
> > + d->sub[i].skip_tree = obj->oid;
>
> So this marks the tree object at the top of the skip as far as
> filter[i] is concerned.
>
Yes.
> > + }
> > + }
> > +
> > + if ((result[0] & LOFR_DO_SHOW) && (result[1] & LOFR_DO_SHOW))
> > + combined_result |= LOFR_DO_SHOW;
> > + if (d->sub[0].is_skipping_tree && d->sub[1].is_skipping_tree)
> > + combined_result |= LOFR_SKIP_TREE;
>
> Something about the above bothers me, but I can't quite say what
> it is.
>
It looks nicer now that it's array-based. Let me know what you think after I
send the next roll-up.
> Do we need to do:
> if ((result[0] & LOFR_MARK_SEEN) && (result[1] & LOFR_MARK_SEEN))
> combined_result |= LOFR_MARK_SEEN;
This should be a O(1) sort of optimization, since if we don't set it, the top
filter will still be called, but won't delegate to any sub-filters. It doesn't
complicate the code much, so it seems worth it to add. Done.
> I'm out of time now, will pick this up again next week.
Thank you for taking a look and for your patience so far.
