{"thread":{"id":"36172","subject":"[PATCH] add: Use struct argv_array in run_add_interactive()","startedAt":"2014-03-15T11:14:40Z","lastAt":"2014-03-17T09:28:05Z","messageCount":4,"participants":["Fabian Ruch","Thomas Rast","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"236788","messageId":"53243620.8080401@gmail.com","threadId":"36172","inReplyTo":null,"subject":"[PATCH] add: Use struct argv_array in run_add_interactive()","fromName":"Fabian Ruch","fromEmail":"bafain@gmail.com","sentAt":"2014-03-15T11:14:40Z","receivedAt":"2014-03-15T11:14:40Z","isPatch":true,"sender":{"key":"bafain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1150972?v=4"},"body":"run_add_interactive() in builtin/add.c manually computes array bounds\nand allocates a static args array to build the add--interactive command\nline, which is error-prone. Use the argv-array helper functions instead.\n\nSigned-off-by: Fabian Ruch \n---\n builtin/add.c | 21 ++++++++++-----------\n 1 file changed, 10 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 4b045ba..459208a 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -15,6 +15,7 @@\n #include \"diffcore.h\"\n #include \"revision.h\"\n #include \"bulk-checkin.h\"\n+#include \"argv-array.h\"\n \n static const char * const builtin_add_usage[] = {\n \tN_(\"git add [options] [--] ...\"),\n@@ -141,23 +142,21 @@ static void refresh(int verbose, const struct pathspec *pathspec)\n int run_add_interactive(const char *revision, const char *patch_mode,\n \t\t\tconst struct pathspec *pathspec)\n {\n-\tint status, ac, i;\n-\tconst char **args;\n+\tint status, i;\n+\tstruct argv_array argv = ARGV_ARRAY_INIT;\n \n-\targs = xcalloc(sizeof(const char *), (pathspec->nr + 6));\n-\tac = 0;\n-\targs[ac++] = \"add--interactive\";\n+\targv_array_push(&argv, \"add--interactive\");\n \tif (patch_mode)\n-\t\targs[ac++] = patch_mode;\n+\t\targv_array_push(&argv, patch_mode);\n \tif (revision)\n-\t\targs[ac++] = revision;\n-\targs[ac++] = \"--\";\n+\t\targv_array_push(&argv, revision);\n+\targv_array_push(&argv, \"--\");\n \tfor (i = 0; i < pathspec->nr; i++)\n \t\t/* pass original pathspec, to be re-parsed */\n-\t\targs[ac++] = pathspec->items[i].original;\n+\t\targv_array_push(&argv, pathspec->items[i].original);\n \n-\tstatus = run_command_v_opt(args, RUN_GIT_CMD);\n-\tfree(args);\n+\tstatus = run_command_v_opt(argv.argv, RUN_GIT_CMD);\n+\targv_array_clear(&argv);\n \treturn status;\n }\n \n-- \n1.9.0\n"},{"id":"236791","messageId":"53244A95.8050308@gmail.com","threadId":"36172","inReplyTo":"53243620.8080401@gmail.com","subject":"Re: [PATCH] add: Use struct argv_array in run_add_interactive()","fromName":"Fabian Ruch","fromEmail":"bafain@gmail.com","sentAt":"2014-03-15T12:41:57Z","receivedAt":"2014-03-15T12:41:57Z","isPatch":true,"sender":{"key":"bafain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1150972?v=4"},"body":"On 03/15/2014 12:14 PM, Fabian Ruch wrote:\n> run_add_interactive() in builtin/add.c manually computes array bounds\n> and allocates a static args array to build the add--interactive command\n> line, which is error-prone. Use the argv-array helper functions instead.\n> \n> Signed-off-by: Fabian Ruch \n> ---\n> builtin/add.c | 21 ++++++++++-----------\n> 1 file changed, 10 insertions(+), 11 deletions(-)\n\nI should mention that I am applying to this year's edition of Google's\nSummer of Code.\n"},{"id":"236825","messageId":"87a9cqxtcy.fsf@thomasrast.ch","threadId":"36172","inReplyTo":"53243620.8080401@gmail.com","subject":"Re: [PATCH] add: Use struct argv_array in run_add_interactive()","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2014-03-16T11:42:21Z","receivedAt":"2014-03-16T11:42:21Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Fabian Ruch writes:\n\n> run_add_interactive() in builtin/add.c manually computes array bounds\n> and allocates a static args array to build the add--interactive command\n> line, which is error-prone. Use the argv-array helper functions instead.\n>\n> Signed-off-by: Fabian Ruch \n\nThanks, this is a nicely done cleanup.\n\n> ---\n> builtin/add.c | 21 ++++++++++-----------\n> 1 file changed, 10 insertions(+), 11 deletions(-)\n>\n> diff --git a/builtin/add.c b/builtin/add.c\n> index 4b045ba..459208a 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -15,6 +15,7 @@\n> #include \"diffcore.h\"\n> #include \"revision.h\"\n> #include \"bulk-checkin.h\"\n> +#include \"argv-array.h\"\n> \n> static const char * const builtin_add_usage[] = {\n> \tN_(\"git add [options] [--] ...\"),\n> @@ -141,23 +142,21 @@ static void refresh(int verbose, const struct pathspec *pathspec)\n> int run_add_interactive(const char *revision, const char *patch_mode,\n> \t\t\tconst struct pathspec *pathspec)\n> {\n> +\tint status, i;\n> +\tstruct argv_array argv = ARGV_ARRAY_INIT;\n> \n> -\targs = xcalloc(sizeof(const char *), (pathspec->nr + 6));\n> -\tac = 0;\n> -\targs[ac++] = \"add--interactive\";\n> +\targv_array_push(&argv, \"add--interactive\");\n> \tif (patch_mode)\n> -\t\targs[ac++] = patch_mode;\n> +\t\targv_array_push(&argv, patch_mode);\n> \tif (revision)\n> -\t\targs[ac++] = revision;\n> -\targs[ac++] = \"--\";\n> +\t\targv_array_push(&argv, revision);\n> +\targv_array_push(&argv, \"--\");\n> \tfor (i = 0; i < pathspec->nr; i++)\n> \t\t/* pass original pathspec, to be re-parsed */\n> -\t\targs[ac++] = pathspec->items[i].original;\n> +\t\targv_array_push(&argv, pathspec->items[i].original);\n> \n> -\tstatus = run_command_v_opt(args, RUN_GIT_CMD);\n> -\tfree(args);\n> +\tstatus = run_command_v_opt(argv.argv, RUN_GIT_CMD);\n> +\targv_array_clear(&argv);\n> \treturn status;\n> }\n\n-- \nThomas Rast\ntr@thomasrast.ch\n"},{"id":"236866","messageId":"CAPig+cQVLd9kxa1gV1GpWN=NBt8DvVo9uKNDTdjwzjF7wCt67g@mail.gmail.com","threadId":"36172","inReplyTo":"87a9cqxtcy.fsf@thomasrast.ch","subject":"Re: [PATCH] add: Use struct argv_array in run_add_interactive()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-17T09:28:05Z","receivedAt":"2014-03-17T09:28:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 16, 2014 at 7:42 AM, Thomas Rast wrote:\n> Fabian Ruch writes:\n>\n>> run_add_interactive() in builtin/add.c manually computes array bounds\n>> and allocates a static args array to build the add--interactive command\n>> line, which is error-prone. Use the argv-array helper functions instead.\n>>\n>> Signed-off-by: Fabian Ruch \n>\n> Thanks, this is a nicely done cleanup.\n\nI second that. Nicely done. You didn't give reviewers any opportunity\nto provide constructive feedback. :-)\n\n>> ---\n>> builtin/add.c | 21 ++++++++++-----------\n>> 1 file changed, 10 insertions(+), 11 deletions(-)\n>>\n>> diff --git a/builtin/add.c b/builtin/add.c\n>> index 4b045ba..459208a 100644\n>> --- a/builtin/add.c\n>> +++ b/builtin/add.c\n>> @@ -15,6 +15,7 @@\n>> #include \"diffcore.h\"\n>> #include \"revision.h\"\n>> #include \"bulk-checkin.h\"\n>> +#include \"argv-array.h\"\n>>\n>> static const char * const builtin_add_usage[] = {\n>> N_(\"git add [options] [--] ...\"),\n>> @@ -141,23 +142,21 @@ static void refresh(int verbose, const struct pathspec *pathspec)\n>> int run_add_interactive(const char *revision, const char *patch_mode,\n>> const struct pathspec *pathspec)\n>> {\n>> + int status, i;\n>> + struct argv_array argv = ARGV_ARRAY_INIT;\n>>\n>> - args = xcalloc(sizeof(const char *), (pathspec->nr + 6));\n>> - ac = 0;\n>> - args[ac++] = \"add--interactive\";\n>> + argv_array_push(&argv, \"add--interactive\");\n>> if (patch_mode)\n>> - args[ac++] = patch_mode;\n>> + argv_array_push(&argv, patch_mode);\n>> if (revision)\n>> - args[ac++] = revision;\n>> - args[ac++] = \"--\";\n>> + argv_array_push(&argv, revision);\n>> + argv_array_push(&argv, \"--\");\n>> for (i = 0; i < pathspec->nr; i++)\n>> /* pass original pathspec, to be re-parsed */\n>> - args[ac++] = pathspec->items[i].original;\n>> + argv_array_push(&argv, pathspec->items[i].original);\n>>\n>> - status = run_command_v_opt(args, RUN_GIT_CMD);\n>> - free(args);\n>> + status = run_command_v_opt(argv.argv, RUN_GIT_CMD);\n>> + argv_array_clear(&argv);\n>> return status;\n>> }\n"}]}