Re: [PATCH 3/3] shorten_unambiguous_ref(): tighten up pointer arithmetic

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Jan 10, 2014, 14:16 UTC
Message-ID: <52D000CC.1060402@alum.mit.edu>
In-Reply-To: <xmqqtxdc92ub.fsf@gitster.dls.corp.google.com>

On 01/10/2014 12:01 AM, Junio C Hamano wrote:

Show 30 quoted lines

> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> As long as we're being pathologically stingy with mallocs, we might as
>> well do the math right and save 6 (!) bytes.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>> It is left to the reader to show how another 7 bytes could be saved
>> (11 bytes on a 64-bit architecture!)
>>
>> It probably wouldn't kill performance to use a string_list here
>> instead.
>>
>>  refs.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/refs.c b/refs.c
>> index ef9cdea..63b3a71 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -3351,10 +3351,10 @@ char *shorten_unambiguous_ref(const char *refname, int strict)
>>  		size_t total_len = 0;
>>  		size_t offset = 0;
>>  
>> -		/* the rule list is NULL terminated, count them first */
>> +		/* the rule list is NUL terminated, count them first */
> 
> I think this _is_ wrong; it talks about the NULL termination of the
> ref_rev_parse_rules[] array, not each string that is an element of
> the array being NUL terminated.

Yes, you're right. Thanks for catching my sloppiness. Would you mind squashing the fix onto my patch?

Show 16 quoted lines

> Output from "git grep -e refname_match -e ref_rev_parse_rules"
> suggests me that we actually could make ref_rev_parse_rules[] a
> file-scope static to refs.c, remove its NULL termination and convert
> all the iterators of the array to use ARRAY_SIZE() on it, after
> dropping the third parameter to refname_match().  That way, we do
> not have to count them first here.
> 
> But that is obviously a separate topic.
> 
>>  		for (nr_rules = 0; ref_rev_parse_rules[nr_rules]; nr_rules++)
>> -			/* no +1 because strlen("%s") < strlen("%.*s") */
>> -			total_len += strlen(ref_rev_parse_rules[nr_rules]);
>> +			/* -2 for strlen("%.*s") - strlen("%s"); +1 for NUL */
>> +			total_len += strlen(ref_rev_parse_rules[nr_rules]) - 2 + 1;
>>  
>>  		scanf_fmts = xmalloc(nr_rules * sizeof(char *) + total_len);

The way the code is written now (e.g., as long as it is not converted to use a string_list or something) needs this loop not only to count the number of rules but also to compute the total_len of the string into which will be written all of the scanf format strings.

As for removing the third argument of refname_match(): although all callers pass it ref_ref_parse_rules, that array is sometimes passed to the function via the alias "ref_fetch_rules". So I suppose somebody wanted to leave the way open to make these two rule sets diverge (though I don't know how likely that is to occur). If we discard the third argument to refname_match(), then we loose the distinction.

Thanks for your feedback, Michael

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