Re: [PATCH 3/3] wrapper: use a loop instead of repetitive statements

[Derrick Stolee <stolee@gmail.com>]

On 9/26/2019 10:45 PM, Jeff King wrote:
> On Tue, Sep 24, 2019 at 08:01:58PM -0600, Alex Henrie wrote:
> 
>> diff --git a/wrapper.c b/wrapper.c
>> index c55d7722d7..c23ac6adcd 100644
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -469,13 +469,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
>>  	filename_template = &pattern[len - 6 - suffix_len];
>>  	for (count = 0; count < TMP_MAX; ++count) {
>>  		uint64_t v = value;
>> +		int i;
>>  		/* Fill in the random bits. */
>> -		filename_template[0] = letters[v % num_letters]; v /= num_letters;
>> -		filename_template[1] = letters[v % num_letters]; v /= num_letters;
>> -		filename_template[2] = letters[v % num_letters]; v /= num_letters;
>> -		filename_template[3] = letters[v % num_letters]; v /= num_letters;
>> -		filename_template[4] = letters[v % num_letters]; v /= num_letters;
>> -		filename_template[5] = letters[v % num_letters]; v /= num_letters;
>> +		for (i = 0; i < 6; i++) {
>> +			filename_template[i] = letters[v % num_letters];
>> +			v /= num_letters;
>> +		}
> 
> I'm not sure the readability is changed much either way. But it does
> enable this additional cleanup on top:
> 
> -- >8 --
> Subject: git_mkstemps_mode(): replace magic numbers with computed value
> 
> The magic number "6" appears several times in the function, and is
> related to the size of the "XXXXXX" string we expect to find in the
> template. Let's pull that "XXXXXX" into a constant array, whose size we
> can get at compile time with ARRAY_SIZE().

Removing magic numbers is always a good change. Thanks!

> Note that we probably can't just change this value, since callers will
> be feeding us a certain number of X's, but it hopefully makes the
> function itself easier to follow.
> 
> While we're here, let's do the same with the "letters" array (which we
> _could_ modify if we wanted to include more characters).
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  wrapper.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/wrapper.c b/wrapper.c
> index c23ac6adcd..e1eaef2e16 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -441,7 +441,9 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
>  		"abcdefghijklmnopqrstuvwxyz"
>  		"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
>  		"0123456789";
> -	static const int num_letters = 62;
> +	static const int num_letters = ARRAY_SIZE(letters) - 1;
> +	static const char x_pattern[] = "XXXXXX";
> +	static const int num_x = ARRAY_SIZE(x_pattern) - 1;
>  	uint64_t value;
>  	struct timeval tv;
>  	char *filename_template;
> @@ -450,12 +452,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
>  
>  	len = strlen(pattern);
>  
> -	if (len < 6 + suffix_len) {
> +	if (len < num_x + suffix_len) {
>  		errno = EINVAL;
>  		return -1;
>  	}
>  
> -	if (strncmp(&pattern[len - 6 - suffix_len], "XXXXXX", 6)) {
> +	if (strncmp(&pattern[len - num_x - suffix_len], x_pattern, num_x)) {
>  		errno = EINVAL;
>  		return -1;
>  	}
> @@ -466,12 +468,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
>  	 */
>  	gettimeofday(&tv, NULL);
>  	value = ((uint64_t)tv.tv_usec << 16) ^ tv.tv_sec ^ getpid();
> -	filename_template = &pattern[len - 6 - suffix_len];
> +	filename_template = &pattern[len - num_x - suffix_len];
>  	for (count = 0; count < TMP_MAX; ++count) {
>  		uint64_t v = value;
>  		int i;
>  		/* Fill in the random bits. */
> -		for (i = 0; i < 6; i++) {
> +		for (i = 0; i < num_x; i++) {
>  			filename_template[i] = letters[v % num_letters];
>  			v /= num_letters;
>  		}
>