Re: [PATCH 01/10] builtin/commit.c: convert trivial snprintf calls to xsnprintf

[Ramsay Jones <ramsay@ramsayjones.plus.com>]

On 03/06/16 09:49, Jeff King wrote:
> On Fri, Jun 03, 2016 at 07:47:15AM +0000, Elia Pinto wrote:
> 
[snip]

> For this particular change:
> 
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 443ff91..c65abaa 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1552,7 +1552,7 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
>>  	code = start_command(&proc);
>>  	if (code)
>>  		return code;
>> -	n = snprintf(buf, sizeof(buf), "%s %s\n",
>> +	n = xsnprintf(buf, sizeof(buf), "%s %s\n",
>>  		     sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
> 
> I think it's the first type, as earlier we have:
> 
> 	/* oldsha1 SP newsha1 LF NUL */
> 	static char buf[2*40 + 3];
> 
> So unless that calculation is wrong, this would never truncate. The move
> to xsnprintf is an improvement, 

I was going to suggest, if we stay with the static buffer, that the size
expression be changed to '2 * GIT_SHA1_HEXSZ + 3'. However, ...

>                                  but I wonder if we would be happier
> still with:
> 
>   buf = xstrfmt("%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
> 
> Then we do not even have to wonder about the size computation. True, it
> uses a heap buffer when we do not need to, but I find it hard to care
> about grabbing 80 bytes of heap when we are in the midst of exec-ing an
> entirely new process.

... I agree with this also.

> 
> By the way, there are some other uses of snprintf in the same file, that
> I think would fall into the type 2 I mentioned above (they use PATH_MAX,
> which I think is shorter than necessary on some systems).
> 
> Those ones would also benefit from using higher-level constructs. Like:
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 443ff91..9336724 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1563,24 +1563,23 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
>  
>  int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...)
>  {
> -	const char *hook_env[3] =  { NULL };
> -	char index[PATH_MAX];
> +	struct argv_array hook_env = ARGV_ARRAY_INIT;
>  	va_list args;
>  	int ret;
>  
> -	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
> -	hook_env[0] = index;
> +	argv_array_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);
>  
>  	/*
>  	 * Let the hook know that no editor will be launched.
>  	 */
>  	if (!editor_is_used)
> -		hook_env[1] = "GIT_EDITOR=:";
> +		argv_array_push(&hook_env, "GIT_EDITOR=:");
>  
>  	va_start(args, name);
>  	ret = run_hook_ve(hook_env, name, args);
>  	va_end(args);
>  
> +	argv_array_clear(&hook_env);
>  	return ret;
>  }

Indeed.

ATB,
Ramsay Jones