> On January 8, 2019 at 3:22 PM Jonathan Tan <jonathantanmy@xxxxxxxxxx> wrote:
>
>
> > > -static void filter_trees_update_omits(
> > > +static int filter_trees_update_omits(
> > > struct object *obj,
> > > struct filter_trees_depth_data *filter_data,
> > > int include_it)
> > > {
> > > if (!filter_data->omits)
> > > - return;
> > > + return 1;
> > >
> > > if (include_it)
> > > - oidset_remove(filter_data->omits, &obj->oid);
> > > + return oidset_remove(filter_data->omits, &obj->oid);
> > > else
> > > - oidset_insert(filter_data->omits, &obj->oid);
> > > + return oidset_insert(filter_data->omits, &obj->oid);
> > > }
> >
> > I think this function is getting too magical - if filter_data->omits is
> > not set, we pretend that we have omitted the tree, because we want the
> > same behavior when not needing omits and when the tree is omitted. Could
> > this be done another way?
>
> Giving some more thought to this, since this is a static function, maybe
> documenting it as "Returns 1 if the objects that this object references need to
> be traversed for "omits" updates, and 0 otherwise" (with the appropriate code
> updates) would suffice.
That's not bad. But I sent a correction which is more like "/* Returns 1 if the oid was in the omits set before it was invoked. */" and returns 0 if omits was NULL. I thought it clearer when the function returns a value in terms of its own arguments and logic, rather than what the caller needs to do. The code I save going with your suggestion (vs. the one I just sent) is offset by the necessity of more detailed comments.
