Jeff King <peff@xxxxxxxx> 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 ;-)
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html