{"thread":{"id":"44853","subject":"[PATCHv2 2/2] builtin/commit.c: drop use snprintf via dynamic allocation","startedAt":"2017-01-11T07:10:45Z","lastAt":"2017-01-11T21:48:38Z","messageCount":3,"participants":["Elia Pinto","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"309200","messageId":"20170111071032.27797-2-gitter.spiros@gmail.com","threadId":"44853","inReplyTo":"20170111071032.27797-1-gitter.spiros@gmail.com","subject":"[PATCHv2 2/2] builtin/commit.c: drop use snprintf via dynamic allocation","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2017-01-11T07:10:32Z","receivedAt":"2017-01-11T07:10:45Z","isPatch":false,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"In general snprintf is bad because it may silently truncate results\nif we're wrong. In this patch, instead of using xnprintf, which asserts\nthat we don't truncate, we are switching to dynamic allocation, so we can\navoid dealing with magic numbers in the code.\n\nHelped-by: Jeff King <peff@peff.net> \nSigned-off-by: Elia Pinto <gitter.spiros@gmail.com>\n---\nThis is the second version of the patch.\n\nI have split the original commit in two, as discussed here\nhttp://public-inbox.org/git/20161213132717.42965-1-gitter.spiros@gmail.com/.\n\n builtin/commit.c | 10 ++++------\n 1 file changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 09bcc0f13..37228330c 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1526,12 +1526,10 @@ static int git_commit_config(const char *k, const char *v, void *cb)\n static int run_rewrite_hook(const unsigned char *oldsha1,\n \t\t\t    const unsigned char *newsha1)\n {\n-\t/* oldsha1 SP newsha1 LF NUL */\n-\tstatic char buf[2*40 + 3];\n+\tchar *buf;\n \tstruct child_process proc = CHILD_PROCESS_INIT;\n \tconst char *argv[3];\n \tint code;\n-\tsize_t n;\n \n \targv[0] = find_hook(\"post-rewrite\");\n \tif (!argv[0])\n@@ -1547,11 +1545,11 @@ static int run_rewrite_hook(const unsigned char *oldsha1,\n \tcode = start_command(&proc);\n \tif (code)\n \t\treturn code;\n-\tn = snprintf(buf, sizeof(buf), \"%s %s\\n\",\n-\t\t     sha1_to_hex(oldsha1), sha1_to_hex(newsha1));\n+\tbuf = xstrfmt(\"%s %s\\n\", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));\n \tsigchain_push(SIGPIPE, SIG_IGN);\n-\twrite_in_full(proc.in, buf, n);\n+\twrite_in_full(proc.in, buf, strlen(buf));\n \tclose(proc.in);\n+\tfree(buf);\n \tsigchain_pop(SIGPIPE);\n \treturn finish_command(&proc);\n }\n-- \n2.11.0.154.g5f5f154\n\n"},{"id":"309201","messageId":"20170111071032.27797-1-gitter.spiros@gmail.com","threadId":"44853","inReplyTo":null,"subject":"[PATCHv2 1/2] builtin/commit.c: drop use snprintf via dynamic allocation","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2017-01-11T07:10:31Z","receivedAt":"2017-01-11T07:10:48Z","isPatch":false,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"In general snprintf is bad because it may silently truncate results if we're\nwrong. In this patch where we use PATH_MAX, we'd want to handle larger\npaths anyway, so we switch to dynamic allocation.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Elia Pinto <gitter.spiros@gmail.com>\n---\nThis is the second version of the patch.\n\nI have split the original commit in two, as discussed here\nhttp://public-inbox.org/git/20161213132717.42965-1-gitter.spiros@gmail.com/.\n\n builtin/commit.c | 22 +++++++++++-----------\n 1 file changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 0ed634b26..09bcc0f13 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -960,15 +960,16 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\treturn 0;\n \n \tif (use_editor) {\n-\t\tchar index[PATH_MAX];\n-\t\tconst char *env[2] = { NULL };\n-\t\tenv[0] =  index;\n-\t\tsnprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\", index_file);\n-\t\tif (launch_editor(git_path_commit_editmsg(), NULL, env)) {\n+\t\tstruct argv_array env = ARGV_ARRAY_INIT;\n+\n+\t\targv_array_pushf(&env, \"GIT_INDEX_FILE=%s\", index_file);\n+\t\tif (launch_editor(git_path_commit_editmsg(), NULL, env.argv)) {\n \t\t\tfprintf(stderr,\n \t\t\t_(\"Please supply the message using either -m or -F option.\\n\"));\n+\t\t\targv_array_clear(&env);\n \t\t\texit(1);\n \t\t}\n+\t\targv_array_clear(&env);\n \t}\n \n \tif (!no_verify &&\n@@ -1557,23 +1558,22 @@ static int run_rewrite_hook(const unsigned char *oldsha1,\n \n int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...)\n {\n-\tconst char *hook_env[3] =  { NULL };\n-\tchar index[PATH_MAX];\n+\tstruct argv_array hook_env = ARGV_ARRAY_INIT;\n \tva_list args;\n \tint ret;\n \n-\tsnprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\", index_file);\n-\thook_env[0] = index;\n+\targv_array_pushf(&hook_env, \"GIT_INDEX_FILE=%s\", index_file);\n \n \t/*\n \t * Let the hook know that no editor will be launched.\n \t */\n \tif (!editor_is_used)\n-\t\thook_env[1] = \"GIT_EDITOR=:\";\n+\t\targv_array_push(&hook_env, \"GIT_EDITOR=:\");\n \n \tva_start(args, name);\n-\tret = run_hook_ve(hook_env, name, args);\n+\tret = run_hook_ve(hook_env.argv,name, args);\n \tva_end(args);\n+\targv_array_clear(&hook_env);\n \n \treturn ret;\n }\n-- \n2.11.0.154.g5f5f154\n\n"},{"id":"309235","messageId":"xmqqtw95mfo3.fsf@gitster.mtv.corp.google.com","threadId":"44853","inReplyTo":"20170111071032.27797-1-gitter.spiros@gmail.com","subject":"Re: [PATCHv2 1/2] builtin/commit.c: drop use snprintf via dynamic allocation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-11T21:48:28Z","receivedAt":"2017-01-11T21:48:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elia Pinto <gitter.spiros@gmail.com> writes:\n\n> In general snprintf is bad because it may silently truncate results if we're\n> wrong. In this patch where we use PATH_MAX, we'd want to handle larger\n> paths anyway, so we switch to dynamic allocation.\n>\n> Helped-by: Jeff King <peff@peff.net>\n> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>\n> ---\n> This is the second version of the patch.\n>\n> I have split the original commit in two, as discussed here\n> http://public-inbox.org/git/20161213132717.42965-1-gitter.spiros@gmail.com/.\n\nAnd both halves have the same title?\n\nI think the the primary value of this one is that it removes the\nPATH_MAX limitation from the environment setting that points to a\nfilename.  Reducing the number of snprintf() call is a side effect,\neven though that may have been what motivated you originally.  The\noriginal code used snprintf() correctly after all.\n\nThe primary value of 2/2 is reduction of the cognitive burden from\nthe programmers---switching to xstrfmt(), they no longer have to\ncount bytes needed for static allocation.  Reducing the number of\nsnprintf() call is a side effect, even though that may have been\nwhat motivated you originally.  The original code used snprintf()\ncorrectly after all.\n\nThe code looks correct in both patches (except that in 1/2 _clear()\nimmediately before exit() wouldn't have to be there, but it does not\nhurt to have one either).  They need to be better explained.\n\nThanks.\n\n>  builtin/commit.c | 22 +++++++++++-----------\n>  1 file changed, 11 insertions(+), 11 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 0ed634b26..09bcc0f13 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -960,15 +960,16 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>  \t\treturn 0;\n>  \n>  \tif (use_editor) {\n> -\t\tchar index[PATH_MAX];\n> -\t\tconst char *env[2] = { NULL };\n> -\t\tenv[0] =  index;\n> -\t\tsnprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\", index_file);\n> -\t\tif (launch_editor(git_path_commit_editmsg(), NULL, env)) {\n> +\t\tstruct argv_array env = ARGV_ARRAY_INIT;\n> +\n> +\t\targv_array_pushf(&env, \"GIT_INDEX_FILE=%s\", index_file);\n> +\t\tif (launch_editor(git_path_commit_editmsg(), NULL, env.argv)) {\n>  \t\t\tfprintf(stderr,\n>  \t\t\t_(\"Please supply the message using either -m or -F option.\\n\"));\n> +\t\t\targv_array_clear(&env);\n>  \t\t\texit(1);\n>  \t\t}\n> +\t\targv_array_clear(&env);\n>  \t}\n>  \n>  \tif (!no_verify &&\n> @@ -1557,23 +1558,22 @@ static int run_rewrite_hook(const unsigned char *oldsha1,\n>  \n>  int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...)\n>  {\n> -\tconst char *hook_env[3] =  { NULL };\n> -\tchar index[PATH_MAX];\n> +\tstruct argv_array hook_env = ARGV_ARRAY_INIT;\n>  \tva_list args;\n>  \tint ret;\n>  \n> -\tsnprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\", index_file);\n> -\thook_env[0] = index;\n> +\targv_array_pushf(&hook_env, \"GIT_INDEX_FILE=%s\", index_file);\n>  \n>  \t/*\n>  \t * Let the hook know that no editor will be launched.\n>  \t */\n>  \tif (!editor_is_used)\n> -\t\thook_env[1] = \"GIT_EDITOR=:\";\n> +\t\targv_array_push(&hook_env, \"GIT_EDITOR=:\");\n>  \n>  \tva_start(args, name);\n> -\tret = run_hook_ve(hook_env, name, args);\n> +\tret = run_hook_ve(hook_env.argv,name, args);\n>  \tva_end(args);\n> +\targv_array_clear(&hook_env);\n>  \n>  \treturn ret;\n>  }\n"}]}