Re: [PATCHv2 1/2] builtin/commit.c: drop use snprintf via dynamic allocation
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 11, 2017, 21:48 UTC
- Message-ID
- <xmqqtw95mfo3.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170111071032.27797-1-gitter.spiros@gmail.com>
Elia Pinto <gitter.spiros@gmail.com> writes:
Show 11 quoted lines
> In general snprintf is bad because it may silently truncate results if we're > wrong. In this patch where we use PATH_MAX, we'd want to handle larger > paths anyway, so we switch to dynamic allocation. > > Helped-by: Jeff King <peff@peff.net> > Signed-off-by: Elia Pinto <gitter.spiros@gmail.com> > --- > This is the second version of the patch. > > I have split the original commit in two, as discussed here > http://public-inbox.org/git/20161213132717.42965-1-gitter.spiros@gmail.com/.
And both halves have the same title?
I think the the primary value of this one is that it removes the PATH_MAX limitation from the environment setting that points to a filename. Reducing the number of snprintf() call is a side effect, even though that may have been what motivated you originally. The original code used snprintf() correctly after all.
The primary value of 2/2 is reduction of the cognitive burden from the programmers---switching to xstrfmt(), they no longer have to count bytes needed for static allocation. Reducing the number of snprintf() call is a side effect, even though that may have been what motivated you originally. The original code used snprintf() correctly after all.
The code looks correct in both patches (except that in 1/2 _clear() immediately before exit() wouldn't have to be there, but it does not hurt to have one either). They need to be better explained.
Thanks.
Show 58 quoted lines
> builtin/commit.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 0ed634b26..09bcc0f13 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -960,15 +960,16 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> return 0;
>
> if (use_editor) {
> - char index[PATH_MAX];
> - const char *env[2] = { NULL };
> - env[0] = index;
> - snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
> - if (launch_editor(git_path_commit_editmsg(), NULL, env)) {
> + struct argv_array env = ARGV_ARRAY_INIT;
> +
> + argv_array_pushf(&env, "GIT_INDEX_FILE=%s", index_file);
> + if (launch_editor(git_path_commit_editmsg(), NULL, env.argv)) {
> fprintf(stderr,
> _("Please supply the message using either -m or -F option.\n"));
> + argv_array_clear(&env);
> exit(1);
> }
> + argv_array_clear(&env);
> }
>
> if (!no_verify &&
> @@ -1557,23 +1558,22 @@ 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);
> + ret = run_hook_ve(hook_env.argv,name, args);
> va_end(args);
> + argv_array_clear(&hook_env);
>
> return ret;
> }