On 05/29/2015 01:56 AM, Matthieu Moy wrote:
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.
But the patch alone wouldn't make much sense here, as the whole idea is
the introduction of the new structures and renaming 'refinfo' to
'ref_array_item' was more of a byproduct to go along with the new
structures introduced.
>
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.
I'm just so used to single spacing, Will change 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.
Noted.
-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).
Ok will put that into a separate commit.
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!
Thanks for the effort from your side, will try to split things as much
as possible and make it easier for Reviewers.
--
Regards,
Karthik
--
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