Elia Pinto <gitter.spiros@xxxxxxxxx> writes: > 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@xxxxxxxx> > Signed-off-by: Elia Pinto <gitter.spiros@xxxxxxxxx> > --- > 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@xxxxxxxxx/. 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. > 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; > }