Karthik Nayak <karthik.188@xxxxxxxxx> writes:
> Rename 'refinfo' to 'ref_array_item' and intoduce 'ref_filter_cbdata'
The fact that you need to use "and" to describe your changes is a hint
that you can split better.
The patch looks much better, but I think you still need to split more to
make it easier to review.
> - * of properties that we need to extract out of objects. refinfo
> + * of properties that we need to extract out of objects. ref_array_item
Not very important, but two spaces after a period is what one is
supposed to do in English. Not everybody follow the rule, but it seems
backward to change the code to break it.
> if (flag & REF_BAD_NAME) {
> - warning("ignoring ref with broken name %s", refname);
> - return 0;
> + warning("ignoring ref with broken name %s", refname);
> + return 0;
Whitespace fix mixed with actual code.
> -static int cmp_ref_sort(struct ref_sort *s, struct refinfo *a, struct refinfo *b)
> +/* Free all memory allocated for ref_filter_cbdata */
> +void ref_filter_clear_data(struct ref_filter_cbdata *ref_cbdata)
> +{
> + int i;
> +
> + for (i = 0; i < ref_cbdata->array.nr; i++)
> + free(ref_cbdata->array.items[i]);
> + free(ref_cbdata->array.items);
> + ref_cbdata->array.items = NULL;
> + ref_cbdata->array.nr = ref_cbdata->array.alloc = 0;
> +}
And this one is a real behavior change, which would be much better
documented in its own patch with a proper commit message (we had a
memory leak before, we didn't care because it happened right before
exiting, but we can't accept that in a clean library).
Reviewing is not just about having a look and seeing if there's
something stupid. Reviewers are actually taking a lot of time to see if
the patch does not introduce new bugs, or looking for better ways to do
the same thing. Be nice with them!
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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