Re: [PATCH v2 2/2] tree:<depth>: skip some trees even when collecting omits

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jan 8, 2019, 23:22 UTC
Message-ID: <20190108232251.37748-1-jonathantanmy@google.com>
In-Reply-To: <20190108020034.23648-1-jonathantanmy@google.com>

Show 22 quoted lines

> > -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.