{"thread":{"id":"47480","subject":"[PATCH] send-pack: use internal argv_array of struct child_process","startedAt":"2017-12-22T08:14:19Z","lastAt":"2017-12-22T20:11:15Z","messageCount":3,"participants":["René Scharfe","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"335161","messageId":"d532a23f-cf73-615c-976d-f4fe83309feb@web.de","threadId":"47480","inReplyTo":null,"subject":"[PATCH] send-pack: use internal argv_array of struct child_process","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-12-22T08:14:10Z","receivedAt":"2017-12-22T08:14:19Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Avoid a magic number of NULL placeholder values and a magic index by\nconstructing the command line for pack-objects using the embedded\nargv_array of the child_process. The resulting code is shorter and\neasier to extend.\n\nSigned-off-by: Rene Scharfe \n---\n send-pack.c | 28 +++++++++-------------------\n 1 file changed, 9 insertions(+), 19 deletions(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex a8cc6b266e..2112d3b27a 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -58,35 +58,25 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *extra, struc\n \t * the revision parameters to it via its stdin and\n \t * let its stdout go back to the other end.\n \t */\n-\tconst char *argv[] = {\n-\t\t\"pack-objects\",\n-\t\t\"--all-progress-implied\",\n-\t\t\"--revs\",\n-\t\t\"--stdout\",\n-\t\tNULL,\n-\t\tNULL,\n-\t\tNULL,\n-\t\tNULL,\n-\t\tNULL,\n-\t\tNULL,\n-\t};\n \tstruct child_process po = CHILD_PROCESS_INIT;\n \tFILE *po_in;\n \tint i;\n \tint rc;\n \n-\ti = 4;\n+\targv_array_push(&po.args, \"pack-objects\");\n+\targv_array_push(&po.args, \"--all-progress-implied\");\n+\targv_array_push(&po.args, \"--revs\");\n+\targv_array_push(&po.args, \"--stdout\");\n \tif (args->use_thin_pack)\n-\t\targv[i++] = \"--thin\";\n+\t\targv_array_push(&po.args, \"--thin\");\n \tif (args->use_ofs_delta)\n-\t\targv[i++] = \"--delta-base-offset\";\n+\t\targv_array_push(&po.args, \"--delta-base-offset\");\n \tif (args->quiet || !args->progress)\n-\t\targv[i++] = \"-q\";\n+\t\targv_array_push(&po.args, \"-q\");\n \tif (args->progress)\n-\t\targv[i++] = \"--progress\";\n+\t\targv_array_push(&po.args, \"--progress\");\n \tif (is_repository_shallow())\n-\t\targv[i++] = \"--shallow\";\n-\tpo.argv = argv;\n+\t\targv_array_push(&po.args, \"--shallow\");\n \tpo.in = -1;\n \tpo.out = args->stateless_rpc ? -1 : fd;\n \tpo.git_cmd = 1;\n-- \n2.15.1\n"},{"id":"335176","messageId":"CAGZ79kb43_nSm-tjv3zOw7OFzByM3WocYboB6QRr=Z7bRue_hA@mail.gmail.com","threadId":"47480","inReplyTo":"d532a23f-cf73-615c-976d-f4fe83309feb@web.de","subject":"Re: [PATCH] send-pack: use internal argv_array of struct child_process","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-22T18:09:37Z","receivedAt":"2017-12-22T18:09:43Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 22, 2017 at 12:14 AM, René Scharfe wrote:\n> Avoid a magic number of NULL placeholder values and a magic index by\n> constructing the command line for pack-objects using the embedded\n> argv_array of the child_process. The resulting code is shorter and\n> easier to extend.\n>\n> Signed-off-by: Rene Scharfe \n\nReviewed-by: Stefan Beller \n\n> ---\n> send-pack.c | 28 +++++++++-------------------\n> 1 file changed, 9 insertions(+), 19 deletions(-)\n>\n> diff --git a/send-pack.c b/send-pack.c\n> index a8cc6b266e..2112d3b27a 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -58,35 +58,25 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *extra, struc\n> * the revision parameters to it via its stdin and\n> * let its stdout go back to the other end.\n> */\n> - const char *argv[] = {\n> - \"pack-objects\",\n> - \"--all-progress-implied\",\n> - \"--revs\",\n> - \"--stdout\",\n> - NULL,\n> - NULL,\n> - NULL,\n> - NULL,\n> - NULL,\n> - NULL,\n> - };\n> struct child_process po = CHILD_PROCESS_INIT;\n> FILE *po_in;\n> int i;\n> int rc;\n>\n> - i = 4;\n> + argv_array_push(&po.args, \"pack-objects\");\n> + argv_array_push(&po.args, \"--all-progress-implied\");\n> + argv_array_push(&po.args, \"--revs\");\n> + argv_array_push(&po.args, \"--stdout\");\n\n(useless nit of the day, no need to resend:)\nThese four statements could be done as pushl(a,b,c, NULL);\nbut it would not differ in readability, so I guess either is fine.\n\nThanks,\nStefan\n\n> if (args->use_thin_pack)\n> - argv[i++] = \"--thin\";\n> + argv_array_push(&po.args, \"--thin\");\n> if (args->use_ofs_delta)\n> - argv[i++] = \"--delta-base-offset\";\n> + argv_array_push(&po.args, \"--delta-base-offset\");\n> if (args->quiet || !args->progress)\n> - argv[i++] = \"-q\";\n> + argv_array_push(&po.args, \"-q\");\n> if (args->progress)\n> - argv[i++] = \"--progress\";\n> + argv_array_push(&po.args, \"--progress\");\n> if (is_repository_shallow())\n> - argv[i++] = \"--shallow\";\n> - po.argv = argv;\n> + argv_array_push(&po.args, \"--shallow\");\n> po.in = -1;\n> po.out = args->stateless_rpc ? -1 : fd;\n> po.git_cmd = 1;\n> --\n> 2.15.1\n"},{"id":"335188","messageId":"xmqqpo76r09w.fsf@gitster.mtv.corp.google.com","threadId":"47480","inReplyTo":"CAGZ79kb43_nSm-tjv3zOw7OFzByM3WocYboB6QRr=Z7bRue_hA@mail.gmail.com","subject":"Re: [PATCH] send-pack: use internal argv_array of struct child_process","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-22T20:11:07Z","receivedAt":"2017-12-22T20:11:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> + argv_array_push(&po.args, \"pack-objects\");\n>> + argv_array_push(&po.args, \"--all-progress-implied\");\n>> + argv_array_push(&po.args, \"--revs\");\n>> + argv_array_push(&po.args, \"--stdout\");\n>\n> (useless nit of the day, no need to resend:)\n> These four statements could be done as pushl(a,b,c, NULL);\n> but it would not differ in readability, so I guess either is fine.\n\nYup. Shorter is not necessarily better. If we anticipate that we\nwill need to update the set of options later (and I suspect we may\nbe doing so soon, given what JeffH and JTan are working on\nrecently), one option (or option-argument pair) per line is a more\npreferrable form from the maintenance point of view.\n"}]}