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

[Matthew DeVore <matvore@xxxxxxxxxxx>]

Thank you for the review :) See below.

On 2019/01/07 18:00, Jonathan Tan 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?

Yes, returning a manipulative lie when omits is NULL is rather 
confusing. So I changed it to this (interdiff):

+/* Returns 1 if the oid was in the omits set before it was invoked. */
 static int filter_trees_update_omits(
     struct object *obj,
     struct filter_trees_depth_data *filter_data,
     int include_it)
 {
     if (!filter_data->omits)
-        return 1;
+        return 0;

     if (include_it)
         return oidset_remove(filter_data->omits, &obj->oid);
@@ -177,7 +178,7 @@ static enum list_objects_filter_result 
filter_trees_depth(

             if (include_it)
                 filter_res = LOFR_DO_SHOW;
-            else if (!been_omitted)
+            else if (filter_data->omits && !been_omitted)
                 /*
                  * Must update omit information of children
                  * recursively; they have not been omitted yet.