Re: [PATCH/WIP v3 2/4] for-each-ref: introduce new structures for better organisation
- From
Matthieu Moy <matthieu.moy@grenoble-inp.fr>
- Date
- May 28, 2015, 20:26 UTC
- Message-ID
- <vpqegm0o3dx.fsf@anie.imag.fr>
- In-Reply-To
- <1432835025-13291-2-git-send-email-karthik.188@gmail.com>
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.
Show 5 quoted lines
> 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.
Show 12 quoted lines
> -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/