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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html