Re: [PATCH/WIP v3 2/4] for-each-ref: introduce new structures for better organisation

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

Karthik Nayak <karthik.188@gmail.com> 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/