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

From: Karthik Nayak <karthik.188@gmail.com>
Date: May 28, 2015, 20:37 UTC
Message-ID: <55677C7F.6080400@gmail.com>
In-Reply-To: <vpqegm0o3dx.fsf@anie.imag.fr>

On 05/29/2015 01:56 AM, Matthieu Moy wrote:

Show 7 quoted lines

> 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.
>

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.

Show 10 quoted lines

> 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.

Show 8 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.
>

Noted.

Show 18 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).
>

Ok will put that into a separate commit.

Show 5 quoted lines

> 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