From: Matthew DeVore <matvore@comcast.net>
Date: Wed, 09 Jan 2019 00:29:52 GMT
Subject: Re: [PATCH v2 2/2] tree:<depth>: skip some trees even when collecting omits
Message-ID: <8cb0dd36-4c49-228e-17ad-538fb377ffe4@comcast.net>
In-Reply-To: <20190108020034.23648-1-jonathantanmy@google.com>

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.