Re: [WIP/PATCH v4 1/8] for-each-ref: extract helper functions out of grab_single_ref()

From: Junio C Hamano <gitster@pobox.com>
Date: May 31, 2015, 17:34 UTC
Message-ID: <xmqqvbf8abwi.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1433008411-8550-1-git-send-email-karthik.188@gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

Show 7 quoted lines

>  /*
> + * Given a refname, return 1 if the refname matches with one of the patterns
> + * while the pattern is a pathname like 'refs/tags' or 'refs/heads/master'
> + * and so on, else return 0. Supports use of wild characters.
> + */
> +static int match_name_as_path(const char **pattern, const char *refname)
> +{

I wonder why this is not "match_refname", in other words, what the significance of "as path" part of the name? If you later are going to introuce match_name_as_something_else() and that name may not be a refname, then this naming is perfectly sane. If such a function you will later introduce will still deal with names that are only refnames, then match_refname_as_path() would be sensible. Otherwise this name seems overly long (i.e. "as_path" may not be adding value) while not being desctiptive enough (i.e. is meant to be limited to refnames but just says "name").

Just being curious.

Show 16 quoted lines

> @@ -851,40 +888,16 @@ static int grab_single_ref(const char *refname, const unsigned char *sha1, int f
>  		  return 0;
>  	}
>  
> -	if (*cb->grab_pattern) {
> -...
> -	}
> +	if (*cb->grab_pattern && !match_name_as_path(cb->grab_pattern, refname))
> +		return 0;
>  
> -	/*
> -	 * We do not open the object yet; sort may only need refname
> -	 * to do its job and the resulting list may yet to be pruned
> -	 * by maxcount logic.
> -	 */
> -	ref = xcalloc(1, sizeof(*ref));

The comment still is a good reminder for those who will later touch this grab_single_ref() function to make them think twice.

Thanks.