Re: [PATCH 1/4] name-ref: factor out name shortening logic from name_ref()

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 8, 2013, 15:04 UTC
Message-ID: <7v7gh12iuk.fsf@alter.siamese.dyndns.org>
In-Reply-To: <51DA7DBD.7070405@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

Show 32 quoted lines

> On 07/08/2013 12:33 AM, Junio C Hamano wrote:
>> The logic will be used in a new codepath for showing exact matches.
>> 
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>  builtin/name-rev.c | 19 ++++++++++++-------
>>  1 file changed, 12 insertions(+), 7 deletions(-)
>> 
>> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
>> index 87d4854..1234ebb 100644
>> --- a/builtin/name-rev.c
>> +++ b/builtin/name-rev.c
>> @@ -96,6 +96,17 @@ static int subpath_matches(const char *path, const char *filter)
>>  	return -1;
>>  }
>>  
>> +static const char *name_ref_abbrev(const char *refname, int shorten_unambiguous)
>> +{
>> +	if (shorten_unambiguous)
>> +		refname = shorten_unambiguous_ref(refname, 0);
>> +	else if (!prefixcmp(refname, "refs/heads/"))
>> +		refname = refname + 11;
>> +	else if (!prefixcmp(refname, "refs/"))
>> +		refname = refname + 5;
>> +	return refname;
>> +}
>> +
>
> In my opinion this would be a tad clearer if each of the branches of the
> "if" returned the value directly rather than setting refname and relying
> on the "return" statement that follows.  But it's probably a matter of
> taste.

I tend to agree; this is a straight-forward code movement (with the variable name changed to conform to your recent refs.c update to call these things "refname"), and that was the primary reason I kept them like so.

Show 22 quoted lines

>
>>  struct name_ref_data {
>>  	int tags_only;
>>  	int name_only;
>> @@ -134,13 +145,7 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void
>>  	if (o && o->type == OBJ_COMMIT) {
>>  		struct commit *commit = (struct commit *)o;
>>  
>> -		if (can_abbreviate_output)
>> -			path = shorten_unambiguous_ref(path, 0);
>> -		else if (!prefixcmp(path, "refs/heads/"))
>> -			path = path + 11;
>> -		else if (!prefixcmp(path, "refs/"))
>> -			path = path + 5;
>> -
>> +		path = name_ref_abbrev(path, can_abbreviate_output);
>>  		name_rev(commit, xstrdup(path), 0, 0, deref);
>>  	}
>>  	return 0;
>> 
>
> Michael