From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Fri, 24 Aug 2012 07:05:12 GMT
Subject: Re: [PATCH 04/17] Name local variables more consistently
Message-ID: <503727A8.3040600@alum.mit.edu>
In-Reply-To: <20120823083918.GB6963@sigill.intra.peff.net>

On 08/23/2012 10:39 AM, Jeff King wrote:
> 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.

I was up in the air about this, because match_pos *is* the position at
which a match is attempted.  But since the name also strikes you as
wrong, I will change it in the next version.

Thanks for this and all of your other comments!

Michael

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