{"thread":{"id":"41515","subject":"[PATCH] upload-pack: use argv_array for pack_objects","startedAt":"2016-02-25T12:13:26Z","lastAt":"2016-02-26T10:59:26Z","messageCount":3,"participants":["Michael Procter","Eric Sunshine","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"279312","messageId":"1456402406-13617-1-git-send-email-michael@procter.org.uk","threadId":"41515","inReplyTo":null,"subject":"[PATCH] upload-pack: use argv_array for pack_objects","fromName":"Michael Procter","fromEmail":"michael@procter.org.uk","sentAt":"2016-02-25T12:13:26Z","receivedAt":"2016-02-25T12:13:26Z","isPatch":true,"sender":{"key":"michael@procter.org.uk","avatar":null},"body":"Use the argv_array in the child_process structure, to avoid having to\nmanually maintain an array size.\n\nSigned-off-by: Michael Procter <michael@procter.org.uk>\n---\n upload-pack.c | 25 +++++++++++--------------\n 1 file changed, 11 insertions(+), 14 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex b3f6653..dc802a0 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -90,35 +90,32 @@ static void create_pack_file(void)\n \t\t\"corruption on the remote side.\";\n \tint buffered = -1;\n \tssize_t sz;\n-\tconst char *argv[13];\n-\tint i, arg = 0;\n+\tint i;\n \tFILE *pipe_fd;\n \n \tif (shallow_nr) {\n-\t\targv[arg++] = \"--shallow-file\";\n-\t\targv[arg++] = \"\";\n+\t\targv_array_push(&pack_objects.args, \"--shallow-file\");\n+\t\targv_array_push(&pack_objects.args, \"\");\n \t}\n-\targv[arg++] = \"pack-objects\";\n-\targv[arg++] = \"--revs\";\n+\targv_array_push(&pack_objects.args, \"pack-objects\");\n+\targv_array_push(&pack_objects.args, \"--revs\");\n \tif (use_thin_pack)\n-\t\targv[arg++] = \"--thin\";\n+\t\targv_array_push(&pack_objects.args, \"--thin\");\n \n-\targv[arg++] = \"--stdout\";\n+\targv_array_push(&pack_objects.args, \"--stdout\");\n \tif (shallow_nr)\n-\t\targv[arg++] = \"--shallow\";\n+\t\targv_array_push(&pack_objects.args, \"--shallow\");\n \tif (!no_progress)\n-\t\targv[arg++] = \"--progress\";\n+\t\targv_array_push(&pack_objects.args, \"--progress\");\n \tif (use_ofs_delta)\n-\t\targv[arg++] = \"--delta-base-offset\";\n+\t\targv_array_push(&pack_objects.args, \"--delta-base-offset\");\n \tif (use_include_tag)\n-\t\targv[arg++] = \"--include-tag\";\n-\targv[arg++] = NULL;\n+\t\targv_array_push(&pack_objects.args, \"--include-tag\");\n \n \tpack_objects.in = -1;\n \tpack_objects.out = -1;\n \tpack_objects.err = -1;\n \tpack_objects.git_cmd = 1;\n-\tpack_objects.argv = argv;\n \n \tif (start_command(&pack_objects))\n \t\tdie(\"git upload-pack: unable to fork git-pack-objects\");\n-- \n2.7.2.368.g02f4152\n"},{"id":"279477","messageId":"CAPig+cRSKFsO+ygnQGxj+HjO+9Y_7YdubvqscNH7X1A6gM8OtQ@mail.gmail.com","threadId":"41515","inReplyTo":"1456402406-13617-1-git-send-email-michael@procter.org.uk","subject":"Re: [PATCH] upload-pack: use argv_array for pack_objects","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-26T02:14:01Z","receivedAt":"2016-02-26T02:14:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Feb 25, 2016 at 7:13 AM, Michael Procter <michael@procter.org.uk> wrote:\n> Use the argv_array in the child_process structure, to avoid having to\n> manually maintain an array size.\n>\n> Signed-off-by: Michael Procter <michael@procter.org.uk>\n> ---\n> diff --git a/upload-pack.c b/upload-pack.c\n> @@ -90,35 +90,32 @@ static void create_pack_file(void)\n>                 \"corruption on the remote side.\";\n>         int buffered = -1;\n>         ssize_t sz;\n> -       const char *argv[13];\n> -       int i, arg = 0;\n> +       int i;\n>         FILE *pipe_fd;\n>\n>         if (shallow_nr) {\n> -               argv[arg++] = \"--shallow-file\";\n> -               argv[arg++] = \"\";\n> +               argv_array_push(&pack_objects.args, \"--shallow-file\");\n> +               argv_array_push(&pack_objects.args, \"\");\n>         }\n\nNot worth a re-roll, but:\n\n    if (shallow_nr)\n        argv_array_pushv(..., \"--shallow-file\", \"\", NULL);\n\nwould have made it clearer that the two arguments are related (and\nallowed you to drop the braces).\n\n> -       argv[arg++] = \"pack-objects\";\n> -       argv[arg++] = \"--revs\";\n> +       argv_array_push(&pack_objects.args, \"pack-objects\");\n> +       argv_array_push(&pack_objects.args, \"--revs\");\n>         if (use_thin_pack)\n> -               argv[arg++] = \"--thin\";\n> +               argv_array_push(&pack_objects.args, \"--thin\");\n>\n> -       argv[arg++] = \"--stdout\";\n> +       argv_array_push(&pack_objects.args, \"--stdout\");\n>         if (shallow_nr)\n> -               argv[arg++] = \"--shallow\";\n> +               argv_array_push(&pack_objects.args, \"--shallow\");\n>         if (!no_progress)\n> -               argv[arg++] = \"--progress\";\n> +               argv_array_push(&pack_objects.args, \"--progress\");\n>         if (use_ofs_delta)\n> -               argv[arg++] = \"--delta-base-offset\";\n> +               argv_array_push(&pack_objects.args, \"--delta-base-offset\");\n>         if (use_include_tag)\n> -               argv[arg++] = \"--include-tag\";\n> -       argv[arg++] = NULL;\n> +               argv_array_push(&pack_objects.args, \"--include-tag\");\n>\n>         pack_objects.in = -1;\n>         pack_objects.out = -1;\n>         pack_objects.err = -1;\n>         pack_objects.git_cmd = 1;\n> -       pack_objects.argv = argv;\n>\n>         if (start_command(&pack_objects))\n>                 die(\"git upload-pack: unable to fork git-pack-objects\");\n> --\n> 2.7.2.368.g02f4152\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"279513","messageId":"20160226105926.GB12472@sigill.intra.peff.net","threadId":"41515","inReplyTo":"CAPig+cRSKFsO+ygnQGxj+HjO+9Y_7YdubvqscNH7X1A6gM8OtQ@mail.gmail.com","subject":"Re: [PATCH] upload-pack: use argv_array for pack_objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-26T10:59:26Z","receivedAt":"2016-02-26T10:59:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 25, 2016 at 09:14:01PM -0500, Eric Sunshine wrote:\n\n> On Thu, Feb 25, 2016 at 7:13 AM, Michael Procter <michael@procter.org.uk> wrote:\n> > Use the argv_array in the child_process structure, to avoid having to\n> > manually maintain an array size.\n> >\n> > Signed-off-by: Michael Procter <michael@procter.org.uk>\n> > ---\n> > diff --git a/upload-pack.c b/upload-pack.c\n> > @@ -90,35 +90,32 @@ static void create_pack_file(void)\n> >                 \"corruption on the remote side.\";\n> >         int buffered = -1;\n> >         ssize_t sz;\n> > -       const char *argv[13];\n> > -       int i, arg = 0;\n> > +       int i;\n> >         FILE *pipe_fd;\n> >\n> >         if (shallow_nr) {\n> > -               argv[arg++] = \"--shallow-file\";\n> > -               argv[arg++] = \"\";\n> > +               argv_array_push(&pack_objects.args, \"--shallow-file\");\n> > +               argv_array_push(&pack_objects.args, \"\");\n> >         }\n> \n> Not worth a re-roll, but:\n> \n>     if (shallow_nr)\n>         argv_array_pushv(..., \"--shallow-file\", \"\", NULL);\n> \n> would have made it clearer that the two arguments are related (and\n> allowed you to drop the braces).\n\nThis same pattern repeats in a few places, and I always want to say:\n\n  argv_array_pushf(..., \"--shallow-file=%s\", file);\n\nbut the parsing side is lazy. It wouldn't be too hard to fix (and would\nmake the option nicer for users, too!), but perhaps it would be better\nstill to convert git.c to use the standard parse-options.\n\nThat is more work, though. Maybe it would be good for the\nlow-hanging-fruit list.\n\nOther than that, Michael's patch looks fine to me (unsurprising, since I\nalready saw it off-list and offered my commentary :) ). I agree it's not\nworth a re-roll for just this point.\n\n-Peff\n"}]}