From: Karthik Nayak <karthik.188@gmail.com>
Date: Mon, 08 Jun 2015 18:53:10 GMT
Subject: Re: [RFC/PATCH 5/9] ref-filter: implement '--merged' and '--no-merged' options
Message-ID: <5575E496.9030102@gmail.com>
In-Reply-To: <vpqeglmrsut.fsf@anie.imag.fr>

On 06/08/2015 11:21 PM, Matthieu Moy wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +	if(filter->merge_commit) {
>
> space after if.
>
>> @@ -938,7 +991,13 @@ void ref_array_clear(struct ref_array *array)
>>    */
>>   int filter_refs(int (for_each_ref_fn)(each_ref_fn, void *), struct ref_filter_cbdata *data)
>>   {
>> -	return for_each_ref_fn(ref_filter_handler, data);
>> +	int ret;
>> +
>> +	ret = for_each_ref_fn(ref_filter_handler, data);
>> +	if (data->filter.merge_commit)
>> +		do_merge_filter(data);
>
> Reading this, I first wondered why you did not do the merge_filter as
> part of ref_filter_handler. It seems weird to fill-in an array and then
> re-filter it. I think it would make sense to add a few comments, like
>
> /* Simple per-ref filtering */
> ret = for_each_ref_fn(ref_filter_handler, data);
>
> /* Filters that need revision walking */
> if (data->filter.merge_commit)
> ...
>

Thanks! will add the space and comments as suggested.

-- 
Regards,
Karthik