From: Ramsay Jones <ramsay@ramsayjones•plus.com>
To: Jeff King <peff@peff•net>, Elia Pinto <gitter.spiros@gmail•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH 01/10] builtin/commit.c: convert trivial snprintf calls to xsnprintf
Date: Fri, 3 Jun 2016 16:25:28 +0100 [thread overview]
Message-ID: <5751A168.7020408@ramsayjones.plus.com> (raw)
In-Reply-To: <20160603084917.GA28401@sigill.intra.peff.net>
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
prev parent reply other threads:[~2016-06-03 15:25 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-03 7:47 [PATCH 01/10] builtin/commit.c: convert trivial snprintf calls to xsnprintf Elia Pinto
2016-06-03 7:47 ` [PATCH 02/10] builtin/index-pack.c: " Elia Pinto
2016-06-03 8:53 ` Jeff King
2016-06-03 15:32 ` Ramsay Jones
2016-06-03 17:10 ` Jeff King
2016-06-03 7:47 ` [PATCH 03/10] builtin/tag.c: " Elia Pinto
2016-06-03 8:52 ` Jeff King
2016-06-03 15:37 ` Ramsay Jones
2016-06-03 7:47 ` [PATCH 04/10] combine-diff.c: " Elia Pinto
2016-06-03 8:54 ` Jeff King
2016-06-03 7:47 ` [PATCH 05/10] compat/inet_ntop.c: " Elia Pinto
2016-06-03 7:47 ` [PATCH 06/10] diff.c: " Elia Pinto
2016-06-03 9:03 ` Jeff King
2016-06-03 7:47 ` [PATCH 07/10] fast-import.c: " Elia Pinto
2016-06-03 7:47 ` [PATCH 08/10] refs.c: " Elia Pinto
2016-06-03 7:47 ` [PATCH 09/10] transport-helper.c: " Elia Pinto
2016-06-03 7:47 ` [PATCH 10/10] wrapper.c: " Elia Pinto
2016-06-03 9:13 ` Jeff King
2016-06-03 8:49 ` [PATCH 01/10] builtin/commit.c: " Jeff King
2016-06-03 9:04 ` Jeff King
2016-06-03 15:25 ` Ramsay Jones [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5751A168.7020408@ramsayjones.plus.com \
--to=ramsay@ramsayjones$(echo .)plus.com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitter.spiros@gmail$(echo .)com \
--cc=peff@peff$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox