{"thread":{"id":"36663","subject":"[RFC/PATCH 0/6] build argv_array into run-command","startedAt":"2014-05-15T08:29:44Z","lastAt":"2014-05-15T16:48:42Z","messageCount":9,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"241650","messageId":"20140515082943.GA26473@sigill.intra.peff.net","threadId":"36663","inReplyTo":null,"subject":"[RFC/PATCH 0/6] build argv_array into run-command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-15T08:29:44Z","receivedAt":"2014-05-15T08:29:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The memory ownership of the \"argv\" array of a \"struct child_process\" can\nbe tricky. The child_process does not own the memory, but it must remain\nvalid until finish_command runs. That's easy for cases where we call\nstart_command and finish_command in the same function: you can use a\nlocal array variable, or use an argv_array and cleanup afterwards.\n\nBut it's easy to screw up in cases where you want to start a command in\none function and finish it in another, either by pointing to invalid\nstorage during finish_command, or by leaking dynamically allocated\nmemory.\n\nThis series sticks an argv_array inside the \"struct child_process\",\nwhich we clean up automatically.  Because some callers might not want to\nuse it, it's optional. If you provide \"argv\", we use that, and\notherwise fall back to the internal array.\n\nThe first commit below does that. The second fixes an uninitialized\nmemory access. 3, 4, and 5 plug memory leaks. 6 is just a cleanup for\nconsistency with the changes in 4 and 5.\n\nAnd in 2, 3, and 5 we are introducing argv_array into new spots, which\nsimplifies the code and gets rid of magic numbers.\n\n  [1/6]: run-command: store an optional argv_array\n  [2/6]: run_column_filter: use argv_array\n  [3/6]: git_connect: use argv_array\n  [4/6]: get_helper: use run-command's internal argv_array\n  [5/6]: get_exporter: use argv_array\n  [6/6]: get_importer: use run-command's internal argv_array\n\n-Peff\n"},{"id":"241651","messageId":"20140515083326.GA26866@sigill.intra.peff.net","threadId":"36663","inReplyTo":"20140515082943.GA26473@sigill.intra.peff.net","subject":"[PATCH 1/6] run-command: store an optional argv_array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-15T08:33:26Z","receivedAt":"2014-05-15T08:33:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"All child_process structs need to point to an argv. For\nflexibility, we do not mandate the use of a dynamic\nargv_array. However, because the child_process does not own\nthe memory, this can make memory management with a\nseparate argv_array difficult.\n\nFor example, if a function calls start_command but not\nfinish_command, the argv memory must persist. The code needs\nto arrange to clean up the argv_array separately after\nfinish_command runs. As a result, some of our code in this\nsituation just leaks the memory.\n\nTo help such cases, this patch adds a built-in argv_array to\nthe child_process, which gets cleaned up automatically (both\nin finish_command and when start_command fails).  Callers\nmay use it if they choose, but can continue to use the raw\nargv if they wish.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is the most RFC part of the series, because I really didn't know\nwhat to call it. We have two arrays in the struct now: the \"argv\" array\nwhich can point to anything, and the new internal argv_array struct,\nwhich here I called \"args\". That name seems confusingly similar; I\nwanted something short since it will be used in a lot of calls, but\ncouldn't think of anything better. Suggestions welcome.\n\nI did toy with the idea of just forcing all callers to use the\nargv_array, and calling it \"argv\". But that seemed unnecessarily\ndisruptive.\n\n Documentation/technical/api-run-command.txt | 7 +++++++\n run-command.c                               | 9 ++++++++-\n run-command.h                               | 3 +++\n 3 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt\nindex 5d7d7f2..69510ae 100644\n--- a/Documentation/technical/api-run-command.txt\n+++ b/Documentation/technical/api-run-command.txt\n@@ -109,6 +109,13 @@ terminated), of which .argv[0] is the program name to run (usually\n without a path). If the command to run is a git command, set argv[0] to\n the command name without the 'git-' prefix and set .git_cmd = 1.\n \n+Note that the ownership of the memory pointed to by .argv stays with the\n+caller, but it should survive until `finish_command` completes. If the\n+.argv member is NULL, `start_command` will point it at the .args\n+`argv_array` (so you may use one or the other, but you must use exactly\n+one). The memory in .args will be cleaned up automatically during\n+`finish_command` (or during `start_command` when it is unsuccessful).\n+\n The members .in, .out, .err are used to redirect stdin, stdout,\n stderr as follows:\n \ndiff --git a/run-command.c b/run-command.c\nindex 75abc47..be07d4a 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -279,6 +279,9 @@ int start_command(struct child_process *cmd)\n \tint failed_errno;\n \tchar *str;\n \n+\tif (!cmd->argv)\n+\t\tcmd->argv = cmd->args.argv;\n+\n \t/*\n \t * In case of errors we must keep the promise to close FDs\n \t * that have been passed in via ->in and ->out.\n@@ -328,6 +331,7 @@ int start_command(struct child_process *cmd)\n fail_pipe:\n \t\t\terror(\"cannot create %s pipe for %s: %s\",\n \t\t\t\tstr, cmd->argv[0], strerror(failed_errno));\n+\t\t\targv_array_clear(&cmd->args);\n \t\t\terrno = failed_errno;\n \t\t\treturn -1;\n \t\t}\n@@ -519,6 +523,7 @@ fail_pipe:\n \t\t\tclose_pair(fderr);\n \t\telse if (cmd->err)\n \t\t\tclose(cmd->err);\n+\t\targv_array_clear(&cmd->args);\n \t\terrno = failed_errno;\n \t\treturn -1;\n \t}\n@@ -543,7 +548,9 @@ fail_pipe:\n \n int finish_command(struct child_process *cmd)\n {\n-\treturn wait_or_whine(cmd->pid, cmd->argv[0]);\n+\tint ret = wait_or_whine(cmd->pid, cmd->argv[0]);\n+\targv_array_clear(&cmd->args);\n+\treturn ret;\n }\n \n int run_command(struct child_process *cmd)\ndiff --git a/run-command.h b/run-command.h\nindex 3653bfa..ea73de3 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -5,8 +5,11 @@\n #include <pthread.h>\n #endif\n \n+#include \"argv-array.h\"\n+\n struct child_process {\n \tconst char **argv;\n+\tstruct argv_array args;\n \tpid_t pid;\n \t/*\n \t * Using .in, .out, .err:\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"241652","messageId":"20140515083340.GB26866@sigill.intra.peff.net","threadId":"36663","inReplyTo":"20140515082943.GA26473@sigill.intra.peff.net","subject":"[PATCH 2/6] run_column_filter: use argv_array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-15T08:33:40Z","receivedAt":"2014-05-15T08:33:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We currently set up the argv array by hand in a fixed-size\nstack-local array. Using an argv array is more readable, as\nit handles buffer allocation us (not to mention makes it\nobvious we do not overflow the array).\n\nHowever, there's a more subtle benefit, too. We leave the\nfunction having run start_command (with the child_process\nin a static global), and then later run finish_command from\nanother function. That means when we run finish_command,\nneither column_process.argv nor the memory it points to is\nvalid any longer.\n\nMost of the time finish_command does not bother looking at\nargv, but it may if it encounters an error (e.g., waitpid\nfailure or signal death). This is unusual, which is why\nnobody has noticed. But by using run-command's built-in\nargv_array, the memory ownership is handled for us\nautomatically.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n column.c | 43 +++++++++++++------------------------------\n 1 file changed, 13 insertions(+), 30 deletions(-)\n\ndiff --git a/column.c b/column.c\nindex 8d1ce88..1a468de 100644\n--- a/column.c\n+++ b/column.c\n@@ -370,46 +370,29 @@ static struct child_process column_process;\n \n int run_column_filter(int colopts, const struct column_options *opts)\n {\n-\tconst char *av[10];\n-\tint ret, ac = 0;\n-\tstruct strbuf sb_colopt  = STRBUF_INIT;\n-\tstruct strbuf sb_width   = STRBUF_INIT;\n-\tstruct strbuf sb_padding = STRBUF_INIT;\n+\tstruct argv_array *argv;\n \n \tif (fd_out != -1)\n \t\treturn -1;\n \n-\tav[ac++] = \"column\";\n-\tstrbuf_addf(&sb_colopt, \"--raw-mode=%d\", colopts);\n-\tav[ac++] = sb_colopt.buf;\n-\tif (opts && opts->width) {\n-\t\tstrbuf_addf(&sb_width, \"--width=%d\", opts->width);\n-\t\tav[ac++] = sb_width.buf;\n-\t}\n-\tif (opts && opts->indent) {\n-\t\tav[ac++] = \"--indent\";\n-\t\tav[ac++] = opts->indent;\n-\t}\n-\tif (opts && opts->padding) {\n-\t\tstrbuf_addf(&sb_padding, \"--padding=%d\", opts->padding);\n-\t\tav[ac++] = sb_padding.buf;\n-\t}\n-\tav[ac] = NULL;\n+\tmemset(&column_process, 0, sizeof(column_process));\n+\targv = &column_process.args;\n+\n+\targv_array_push(argv, \"column\");\n+\targv_array_pushf(argv, \"--raw-mode=%d\", colopts);\n+\tif (opts && opts->width)\n+\t\targv_array_pushf(argv, \"--width=%d\", opts->width);\n+\tif (opts && opts->indent)\n+\t\targv_array_pushf(argv, \"--indent=%s\", opts->indent);\n+\tif (opts && opts->padding)\n+\t\targv_array_pushf(argv, \"--padding=%d\", opts->padding);\n \n \tfflush(stdout);\n-\tmemset(&column_process, 0, sizeof(column_process));\n \tcolumn_process.in = -1;\n \tcolumn_process.out = dup(1);\n \tcolumn_process.git_cmd = 1;\n-\tcolumn_process.argv = av;\n-\n-\tret = start_command(&column_process);\n-\n-\tstrbuf_release(&sb_colopt);\n-\tstrbuf_release(&sb_width);\n-\tstrbuf_release(&sb_padding);\n \n-\tif (ret)\n+\tif (start_command(&column_process))\n \t\treturn -2;\n \n \tfd_out = dup(1);\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"241653","messageId":"20140515083409.GC26866@sigill.intra.peff.net","threadId":"36663","inReplyTo":"20140515082943.GA26473@sigill.intra.peff.net","subject":"[PATCH 3/6] git_connect: use argv_array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-15T08:34:09Z","receivedAt":"2014-05-15T08:34:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This avoids magic numbers when we allocate fixed-size argv\narrays, and makes it more obvious that we are not\noverflowing.\n\nIt is also the first step to fixing a memory leak. When\ngit_connect returns a child_process struct, the argv array\nin the struct is dynamically allocated, but the individual\nstrings are not (they are either owned elsewhere, or are\nfreed). Later, in finish_connect, we free the array but\nleave the strings alone.\n\nThis works for the child_process created by git_connect, but\nif we use transport_take_over, we may also end up with a\nchild_process created by transport-helper's get_helper.\nIn that case, the strings are freshly allocated, and we\nwould want to free them.  However, we have no idea in\nfinish_connect which type we have.\n\nBy consistently using run-command's internal argv-array, we\ndo not have to worry about this issue at all; finish_command\ntakes care of it for us, and we can drop our manual free\nentirely.\n\nNote that this actually makes the get_helper leak slightly\nworse; now we are leaking both the strings and the array.\nBut when we adjust it in a future patch, that leak will go\naway entirely.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n connect.c | 28 ++++++++++------------------\n 1 file changed, 10 insertions(+), 18 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex a983d06..94a6650 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -534,22 +534,18 @@ static int git_use_proxy(const char *host)\n static struct child_process *git_proxy_connect(int fd[2], char *host)\n {\n \tconst char *port = STR(DEFAULT_GIT_PORT);\n-\tconst char **argv;\n \tstruct child_process *proxy;\n \n \tget_host_and_port(&host, &port);\n \n-\targv = xmalloc(sizeof(*argv) * 4);\n-\targv[0] = git_proxy_command;\n-\targv[1] = host;\n-\targv[2] = port;\n-\targv[3] = NULL;\n \tproxy = xcalloc(1, sizeof(*proxy));\n-\tproxy->argv = argv;\n+\targv_array_push(&proxy->args, git_proxy_command);\n+\targv_array_push(&proxy->args, host);\n+\targv_array_push(&proxy->args, port);\n \tproxy->in = -1;\n \tproxy->out = -1;\n \tif (start_command(proxy))\n-\t\tdie(\"cannot start proxy %s\", argv[0]);\n+\t\tdie(\"cannot start proxy %s\", git_proxy_command);\n \tfd[0] = proxy->out; /* read from proxy stdout */\n \tfd[1] = proxy->in;  /* write to proxy stdin */\n \treturn proxy;\n@@ -663,7 +659,6 @@ struct child_process *git_connect(int fd[2], const char *url,\n \tchar *hostandport, *path;\n \tstruct child_process *conn = &no_fork;\n \tenum protocol protocol;\n-\tconst char **arg;\n \tstruct strbuf cmd = STRBUF_INIT;\n \n \t/* Without this we cannot rely on waitpid() to tell\n@@ -707,7 +702,6 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\tsq_quote_buf(&cmd, path);\n \n \t\tconn->in = conn->out = -1;\n-\t\tconn->argv = arg = xcalloc(7, sizeof(*arg));\n \t\tif (protocol == PROTO_SSH) {\n \t\t\tconst char *ssh = getenv(\"GIT_SSH\");\n \t\t\tint putty = ssh && strcasestr(ssh, \"plink\");\n@@ -718,22 +712,21 @@ struct child_process *git_connect(int fd[2], const char *url,\n \n \t\t\tif (!ssh) ssh = \"ssh\";\n \n-\t\t\t*arg++ = ssh;\n+\t\t\targv_array_push(&conn->args, ssh);\n \t\t\tif (putty && !strcasestr(ssh, \"tortoiseplink\"))\n-\t\t\t\t*arg++ = \"-batch\";\n+\t\t\t\targv_array_push(&conn->args, \"-batch\");\n \t\t\tif (port) {\n \t\t\t\t/* P is for PuTTY, p is for OpenSSH */\n-\t\t\t\t*arg++ = putty ? \"-P\" : \"-p\";\n-\t\t\t\t*arg++ = port;\n+\t\t\t\targv_array_push(&conn->args, putty ? \"-P\" : \"-p\");\n+\t\t\t\targv_array_push(&conn->args, port);\n \t\t\t}\n-\t\t\t*arg++ = ssh_host;\n+\t\t\targv_array_push(&conn->args, ssh_host);\n \t\t} else {\n \t\t\t/* remove repo-local variables from the environment */\n \t\t\tconn->env = local_repo_env;\n \t\t\tconn->use_shell = 1;\n \t\t}\n-\t\t*arg++ = cmd.buf;\n-\t\t*arg = NULL;\n+\t\targv_array_push(&conn->args, cmd.buf);\n \n \t\tif (start_command(conn))\n \t\t\tdie(\"unable to fork\");\n@@ -759,7 +752,6 @@ int finish_connect(struct child_process *conn)\n \t\treturn 0;\n \n \tcode = finish_command(conn);\n-\tfree(conn->argv);\n \tfree(conn);\n \treturn code;\n }\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"241654","messageId":"20140515083418.GD26866@sigill.intra.peff.net","threadId":"36663","inReplyTo":"20140515082943.GA26473@sigill.intra.peff.net","subject":"[PATCH 4/6] get_helper: use run-command's internal argv_array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-15T08:34:18Z","receivedAt":"2014-05-15T08:34:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The get_helper functions dynamically allocates an\nargv_array, feeds it to start_command, and then returns. We\nthen have to later clean up the memory manually after\ncalling finish_command. We can make this simpler by just\nusing run-command's internal argv_array, which handles\ncleanup for us.\n\nThis also prevents a memory leak in the case that\ntransport_take_over is used, in which case we free the child\nin finish_connect, which does not manually free the array.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n transport-helper.c | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex b468e4f..fefd34f 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -101,7 +101,6 @@ static void do_take_over(struct transport *transport)\n static struct child_process *get_helper(struct transport *transport)\n {\n \tstruct helper_data *data = transport->data;\n-\tstruct argv_array argv = ARGV_ARRAY_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct child_process *helper;\n \tconst char **refspecs = NULL;\n@@ -123,10 +122,9 @@ static struct child_process *get_helper(struct transport *transport)\n \thelper->in = -1;\n \thelper->out = -1;\n \thelper->err = 0;\n-\targv_array_pushf(&argv, \"git-remote-%s\", data->name);\n-\targv_array_push(&argv, transport->remote->name);\n-\targv_array_push(&argv, remove_ext_force(transport->url));\n-\thelper->argv = argv_array_detach(&argv, NULL);\n+\targv_array_pushf(&helper->args, \"git-remote-%s\", data->name);\n+\targv_array_push(&helper->args, transport->remote->name);\n+\targv_array_push(&helper->args, remove_ext_force(transport->url));\n \thelper->git_cmd = 0;\n \thelper->silent_exec_failure = 1;\n \n@@ -245,7 +243,6 @@ static int disconnect_helper(struct transport *transport)\n \t\tclose(data->helper->out);\n \t\tfclose(data->out);\n \t\tres = finish_command(data->helper);\n-\t\targv_array_free_detached(data->helper->argv);\n \t\tfree(data->helper);\n \t\tdata->helper = NULL;\n \t}\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"241655","messageId":"20140515083444.GE26866@sigill.intra.peff.net","threadId":"36663","inReplyTo":"20140515082943.GA26473@sigill.intra.peff.net","subject":"[PATCH 5/6] get_exporter: use argv_array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-15T08:34:44Z","receivedAt":"2014-05-15T08:34:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This simplifies the code and avoids a fixed array size that\nwe might accidentally overflow. It also prevents a leak\nafter finish_command is run, by using the argv_array that\nrun-command manages for us.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n transport-helper.c | 26 ++++++++++----------------\n 1 file changed, 10 insertions(+), 16 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex fefd34f..9f8f3b1 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -418,30 +418,24 @@ static int get_exporter(struct transport *transport,\n {\n \tstruct helper_data *data = transport->data;\n \tstruct child_process *helper = get_helper(transport);\n-\tint argc = 0, i;\n-\tstruct strbuf tmp = STRBUF_INIT;\n+\tint i;\n \n \tmemset(fastexport, 0, sizeof(*fastexport));\n \n \t/* we need to duplicate helper->in because we want to use it after\n \t * fastexport is done with it. */\n \tfastexport->out = dup(helper->in);\n-\tfastexport->argv = xcalloc(6 + revlist_args->nr, sizeof(*fastexport->argv));\n-\tfastexport->argv[argc++] = \"fast-export\";\n-\tfastexport->argv[argc++] = \"--use-done-feature\";\n-\tfastexport->argv[argc++] = data->signed_tags ?\n-\t\t\"--signed-tags=verbatim\" : \"--signed-tags=warn-strip\";\n-\tif (data->export_marks) {\n-\t\tstrbuf_addf(&tmp, \"--export-marks=%s.tmp\", data->export_marks);\n-\t\tfastexport->argv[argc++] = strbuf_detach(&tmp, NULL);\n-\t}\n-\tif (data->import_marks) {\n-\t\tstrbuf_addf(&tmp, \"--import-marks=%s\", data->import_marks);\n-\t\tfastexport->argv[argc++] = strbuf_detach(&tmp, NULL);\n-\t}\n+\targv_array_push(&fastexport->args, \"fast-export\");\n+\targv_array_push(&fastexport->args, \"--use-done-feature\");\n+\targv_array_push(&fastexport->args, data->signed_tags ?\n+\t\t\"--signed-tags=verbatim\" : \"--signed-tags=warn-strip\");\n+\tif (data->export_marks)\n+\t\targv_array_pushf(&fastexport->args, \"--export-marks=%s.tmp\", data->export_marks);\n+\tif (data->import_marks)\n+\t\targv_array_pushf(&fastexport->args, \"--import-marks=%s\", data->import_marks);\n \n \tfor (i = 0; i < revlist_args->nr; i++)\n-\t\tfastexport->argv[argc++] = revlist_args->items[i].string;\n+\t\targv_array_push(&fastexport->args, revlist_args->items[i].string);\n \n \tfastexport->git_cmd = 1;\n \treturn start_command(fastexport);\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"241656","messageId":"20140515083506.GF26866@sigill.intra.peff.net","threadId":"36663","inReplyTo":"20140515082943.GA26473@sigill.intra.peff.net","subject":"[PATCH 6/6] get_importer: use run-command's internal argv_array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-15T08:35:06Z","receivedAt":"2014-05-15T08:35:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This saves a few lines and lets us avoid having to clean up\nthe memory manually when the command finishes.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n transport-helper.c | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 9f8f3b1..d9063d7 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -394,18 +394,16 @@ static int get_importer(struct transport *transport, struct child_process *fasti\n {\n \tstruct child_process *helper = get_helper(transport);\n \tstruct helper_data *data = transport->data;\n-\tstruct argv_array argv = ARGV_ARRAY_INIT;\n \tint cat_blob_fd, code;\n \tmemset(fastimport, 0, sizeof(*fastimport));\n \tfastimport->in = helper->out;\n-\targv_array_push(&argv, \"fast-import\");\n-\targv_array_push(&argv, debug ? \"--stats\" : \"--quiet\");\n+\targv_array_push(&fastimport->args, \"fast-import\");\n+\targv_array_push(&fastimport->args, debug ? \"--stats\" : \"--quiet\");\n \n \tif (data->bidi_import) {\n \t\tcat_blob_fd = xdup(helper->in);\n-\t\targv_array_pushf(&argv, \"--cat-blob-fd=%d\", cat_blob_fd);\n+\t\targv_array_pushf(&fastimport->args, \"--cat-blob-fd=%d\", cat_blob_fd);\n \t}\n-\tfastimport->argv = argv.argv;\n \tfastimport->git_cmd = 1;\n \n \tcode = start_command(fastimport);\n@@ -476,7 +474,6 @@ static int fetch_with_import(struct transport *transport,\n \n \tif (finish_command(&fastimport))\n \t\tdie(\"Error while running fast-import\");\n-\targv_array_free_detached(fastimport.argv);\n \n \t/*\n \t * The fast-import stream of a remote helper that advertises\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"241657","messageId":"20140515084102.GA28461@sigill.intra.peff.net","threadId":"36663","inReplyTo":"20140515082943.GA26473@sigill.intra.peff.net","subject":"[PATCH 7/6] argv-array: drop \"detach\" code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-15T08:41:03Z","receivedAt":"2014-05-15T08:41:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The argv_array_detach function (and associated free() function) was\nreally only useful for transferring ownership of the memory to a \"struct\nchild_process\". Now that we have an internal argv_array in that struct,\nthere are no callers left.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is a bonus enabled by the earlier patches. However, there is one\ncommit in pu that uses it when dealing with environment variables.\nHowever, it is actually leaking memory, and should probably just use the\narray directly (and it's one of my commits that's due to be re-rolled\nanyway).\n\n Documentation/technical/api-argv-array.txt |  8 --------\n argv-array.c                               | 20 --------------------\n argv-array.h                               |  2 --\n 3 files changed, 30 deletions(-)\n\ndiff --git a/Documentation/technical/api-argv-array.txt b/Documentation/technical/api-argv-array.txt\nindex a6b7d83..1a79781 100644\n--- a/Documentation/technical/api-argv-array.txt\n+++ b/Documentation/technical/api-argv-array.txt\n@@ -53,11 +53,3 @@ Functions\n `argv_array_clear`::\n \tFree all memory associated with the array and return it to the\n \tinitial, empty state.\n-\n-`argv_array_detach`::\n-\tDetach the argv array from the `struct argv_array`, transferring\n-\townership of the allocated array and strings.\n-\n-`argv_array_free_detached`::\n-\tFree the memory allocated by a `struct argv_array` that was later\n-\tdetached and is now no longer needed.\ndiff --git a/argv-array.c b/argv-array.c\nindex 9e960d5..256741d 100644\n--- a/argv-array.c\n+++ b/argv-array.c\n@@ -68,23 +68,3 @@ void argv_array_clear(struct argv_array *array)\n \t}\n \targv_array_init(array);\n }\n-\n-const char **argv_array_detach(struct argv_array *array, int *argc)\n-{\n-\tconst char **argv =\n-\t\tarray->argv == empty_argv || array->argc == 0 ? NULL : array->argv;\n-\tif (argc)\n-\t\t*argc = array->argc;\n-\targv_array_init(array);\n-\treturn argv;\n-}\n-\n-void argv_array_free_detached(const char **argv)\n-{\n-\tif (argv) {\n-\t\tint i;\n-\t\tfor (i = 0; argv[i]; i++)\n-\t\t\tfree((char **)argv[i]);\n-\t\tfree(argv);\n-\t}\n-}\ndiff --git a/argv-array.h b/argv-array.h\nindex 85ba438..c65e6e8 100644\n--- a/argv-array.h\n+++ b/argv-array.h\n@@ -19,7 +19,5 @@ LAST_ARG_MUST_BE_NULL\n void argv_array_pushl(struct argv_array *, ...);\n void argv_array_pop(struct argv_array *);\n void argv_array_clear(struct argv_array *);\n-const char **argv_array_detach(struct argv_array *array, int *argc);\n-void argv_array_free_detached(const char **argv);\n \n #endif /* ARGV_ARRAY_H */\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"241695","messageId":"xmqqiop7gf85.fsf@gitster.dls.corp.google.com","threadId":"36663","inReplyTo":"20140515082943.GA26473@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 0/6] build argv_array into run-command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-15T16:48:42Z","receivedAt":"2014-05-15T16:48:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The memory ownership of the \"argv\" array of a \"struct child_process\" can\n> be tricky. The child_process does not own the memory, but it must remain\n> valid until finish_command runs. That's easy for cases where we call\n> start_command and finish_command in the same function: you can use a\n> local array variable, or use an argv_array and cleanup afterwards.\n>\n> But it's easy to screw up in cases where you want to start a command in\n> one function and finish it in another, either by pointing to invalid\n> storage during finish_command, or by leaking dynamically allocated\n> memory.\n>\n> This series sticks an argv_array inside the \"struct child_process\",\n> which we clean up automatically.  Because some callers might not want to\n> use it, it's optional. If you provide \"argv\", we use that, and\n> otherwise fall back to the internal array.\n>\n> The first commit below does that. The second fixes an uninitialized\n> memory access. 3, 4, and 5 plug memory leaks. 6 is just a cleanup for\n> consistency with the changes in 4 and 5.\n>\n> And in 2, 3, and 5 we are introducing argv_array into new spots, which\n> simplifies the code and gets rid of magic numbers.\n\nNicely done.\n"}]}