Re: [PATCH 04/17] Name local variables more consistently

[Michael Haggerty <mhagger@alum.mit.edu>]

On 08/26/2012 07:39 PM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>> On Thu, Aug 23, 2012 at 10:10:29AM +0200, mhagger@alum.mit.edu wrote:
>>
>>> From: Michael Haggerty <mhagger@alum.mit.edu>
>>>
>>> Use the names (nr_heads, heads) consistently across functions, instead
>>> of sometimes naming the same values (nr_match, match).
>>
>> I think this is fine, although:
>>
>>> --- a/builtin/fetch-pack.c
>>> +++ b/builtin/fetch-pack.c
>>> @@ -521,7 +521,7 @@ static void mark_recent_complete_commits(unsigned long cutoff)
>>>  	}
>>>  }
>>>  
>>> -static void filter_refs(struct ref **refs, int nr_match, char **match)
>>> +static void filter_refs(struct ref **refs, int nr_heads, char **heads)
>>>  {
>>>  	struct ref **return_refs;
>>>  	struct ref *newlist = NULL;
>>> @@ -530,12 +530,12 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)
>>>  	struct ref *fastarray[32];
>>>  	int match_pos;
>>
>> This match_pos is an index into the "match" array, which becomes "head".
>> Should it become head_pos?
>>
>> And then bits like this:
>>
>>> -			while (match_pos < nr_match) {
>>> -				cmp = strcmp(ref->name, match[match_pos]);
>>> +			while (match_pos < nr_heads) {
>>> +				cmp = strcmp(ref->name, heads[match_pos]);
>>
>> Would be:
>>
>>   while (head_pos < nr_heads)
>>
>> which makes more sense to me.
> 
> Using one name is better, but I wonder "heads" is the better one
> between the two.
> 
> After all, this codepath is not limited to branches these days as
> the word "head" implies.  Rather, <nr_thing, thing> has what we
> asked for, and <refs> has what the other sides have, and we are
> trying to make sure we haven't asked what they do not have in this
> function.
> 
> And the way we do so is to match the "thing"s with what are in
> "refs" to find unmatched ones.
> 
> So between the two, I would have chosen "match" instead of "heads"
> to call the "thing".

When I decided between "heads" and "match", my main consideration was
that "match" sounds like something that has already been matched, not
something that is being matched against.  The word "match" also implies
to me that some nontrivial matching process is going on, like glob
expansion.

But I agree with you that "heads" also has disadvantages.

What about a third option: "refnames"?  This name makes it clear that we
are talking about simple names as opposed to "struct ref" or some kind
of refname glob patterns and also makes it clear that they are not
necessarily all branches.  It would also be distinct from the "refs"
linked list that is often used in the same functions.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/