From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Mon, 08 Jun 2015 17:51:38 GMT
Subject: Re: [RFC/PATCH 5/9] ref-filter: implement '--merged' and '--no-merged' options
Message-ID: <vpqeglmrsut.fsf@anie.imag.fr>
In-Reply-To: <1433621052-5588-5-git-send-email-karthik.188@gmail.com>

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

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/