git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Michael Haggerty <mhagger@alum.mit.edu>
Date
Aug 27, 2012, 09:22 UTC
Message-ID
<503B3C5C.1020109@alum.mit.edu>
In-Reply-To
<7vvcg5v9hh.fsf@alter.siamese.dyndns.org>
On 08/26/2012 07:39 PM, Junio C Hamano wrote:
Show 56 quoted lines
> 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/
Previous: Junio C HamanoNext: Jeff King
Message 9 of 40 in “Clean up how fetch_pack() handles the heads list”
  1. 00/17 Clean up how fetch_pack() handles the heads listmhagger@alum.mit.edu, Aug 23, 2012
  2. 01/17 t5500: add tests of error output for missing refsmhagger@alum.mit.edu, Aug 23, 2012
  3. 02/17 Rename static function fetch_pack() to http_fetch_pack()mhagger@alum.mit.edu, Aug 23, 2012
  4. 03/17 Fix formatting.mhagger@alum.mit.edu, Aug 23, 2012
  5. 04/17 Name local variables more consistentlymhagger@alum.mit.edu, Aug 23, 2012
  6. Jeff KingAug 23, 2012
  7. Michael HaggertyAug 24, 2012
  8. Junio C HamanoAug 26, 2012
  9. Michael HaggertyAug 27, 2012
  10. Jeff KingAug 27, 2012
  11. Junio C HamanoAug 27, 2012
  12. Junio C HamanoAug 27, 2012
  13. 05/17 Do not check the same match_pos twicemhagger@alum.mit.edu, Aug 23, 2012
  14. Jeff KingAug 23, 2012
  15. 06/17 Let fetch_pack() inform caller about number of unique headsmhagger@alum.mit.edu, Aug 23, 2012
  16. Jeff KingAug 23, 2012
  17. Michael HaggertyAug 25, 2012
  18. 07/17 Pass nr_heads to do_pack_ref() by referencemhagger@alum.mit.edu, Aug 23, 2012
  19. 08/17 Pass nr_heads to everything_local() by referencemhagger@alum.mit.edu, Aug 23, 2012
  20. 09/17 Pass nr_heads to filter_refs() by referencemhagger@alum.mit.edu, Aug 23, 2012
  21. 10/17 Remove ineffective optimizationmhagger@alum.mit.edu, Aug 23, 2012
  22. 11/17 filter_refs(): do not leave gaps in return_refsmhagger@alum.mit.edu, Aug 23, 2012
  23. 12/17 filter_refs(): compress unmatched refs in heads arraymhagger@alum.mit.edu, Aug 23, 2012
  24. 13/17 cmd_fetch_pack: return early if finish_connect() returns an errormhagger@alum.mit.edu, Aug 23, 2012
  25. 14/17 Report missing refs even if no existing refs were receivedmhagger@alum.mit.edu, Aug 23, 2012
  26. 15/17 cmd_fetch_pack(): simplify computation of return valuemhagger@alum.mit.edu, Aug 23, 2012
  27. 16/17 fetch_pack(): free matching headsmhagger@alum.mit.edu, Aug 23, 2012
  28. Jeff KingAug 23, 2012
  29. 17/17 fetch_refs(): simplify logicmhagger@alum.mit.edu, Aug 23, 2012
  30. Jeff KingAug 23, 2012
  31. Michael HaggertyAug 25, 2012
  32. Jeff KingAug 23, 2012
  33. Philip OakleyAug 23, 2012
  34. Jeff KingAug 23, 2012
  35. Jeff KingAug 23, 2012
  36. Michael HaggertyAug 25, 2012
  37. Michael HaggertySep 2, 2012
  38. Philip OakleyAug 23, 2012
  39. Junio C HamanoAug 24, 2012
  40. Philip OakleyAug 24, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.