Re: [PATCH v2 03/25] object_array: factor out slopbuf-freeing logic

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 16, 2014, 17:39 UTC
Message-ID: <xmqqr3y7ud5h.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20141015223419.GC25630@peff.net>

Jeff King <peff@peff.net> writes:

> This is not a lot of code, but it's a logical construct that
> should not need to be repeated (and we are about to add a
> third repetition).

Good, but I have two and a half tangential comments about the context that appears in this patch ;-)

Show 23 quoted lines

>  void object_array_filter(struct object_array *array,
>  			 object_array_each_func_t want, void *cb_data)
>  {
> @@ -367,8 +377,7 @@ void object_array_filter(struct object_array *array,
>  				objects[dst] = objects[src];
>  			dst++;
>  		} else {
> -			if (objects[src].name != object_array_slopbuf)
> -				free(objects[src].name);
> +			object_array_release_entry(&objects[src]);
>  		}
>  	}
>  	array->nr = dst;
> @@ -400,8 +409,7 @@ void object_array_remove_duplicates(struct object_array *array)
>  				objects[array->nr] = objects[src];
>  			array->nr++;
>  		} else {
> -			if (objects[src].name != object_array_slopbuf)
> -				free(objects[src].name);
> +			object_array_release_entry(&objects[src]);
>  		}
>  	}
>  }

 1.  These two functions both remove elements from a given array
     in-place, the former being in a more generic form that takes a
     caller-specified criterion while the latter uses a hardcoded
     condition to decide what to filter.  aeb4a51e (object_array:
     add function object_array_filter(), 2013-05-25) and later
     1506510c (object_array_remove_duplicates(): rewrite to reduce
     copying, 2013-05-25) should have refactored the latter further
     to implement it in terms of the former, perhaps?

 1.5 I would have expected a function to "remove duplicates from an
     array" to remove duplicates from the array by comparing the objects
     contained in the array, not entries that may (or may not) point at
     different objects but happens to share the same name; I think this
     function is misnamed.

 2.  We use object_array_remove_duplicates() to de-dup "git bundle
     create x master master", which came from b2a6d1c6 (bundle:
     allow the same ref to be given more than once, 2009-01-17),
     which is still the sole caller of the function, and I think
     this is bogus.  Comparing .name would not de-dup "git bundle
     create x master refs/heads/master".

I think the right way to fix these two and a half problems is to do the following:

 - object_array_remove_duplicates() (and contains_name() helper it
   uses) should be removed from object.c;

 - create_bundle() in bundle.c should implement a helper that is
   similar to contains_name() but knows about ref dwimming and use
   it to call object_array_filter() to replace its call to
   object_array_remove_duplicates().

I am not doing this myself, and I do not expect either you or Michael to do so, either. I am just writing this down to point out a low hanging fruit to aspiring new contributors (hint, hint).

Thanks.