threads / patch / 47480

patchsend-pack: use internal argv_array of struct child_process

Subject: [PATCH] send-pack: use internal argv_array of struct child_process

## tl;dr

3 messages between Dec 22, 2017 and Dec 22, 2017. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

René Scharfe· Dec 22, 2017, 08:14 UTC · lore

Avoid a magic number of NULL placeholder values and a magic index by constructing the command line for pack-objects using the embedded argv_array of the child_process. The resulting code is shorter and easier to extend.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 send-pack.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)
Show changes to send-pack.c +9 −19
diff --git a/send-pack.c b/send-pack.c
index a8cc6b266e..2112d3b27a 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -58,35 +58,25 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *extra, struc
 	 * the revision parameters to it via its stdin and
 	 * let its stdout go back to the other end.
 	 */
-	const char *argv[] = {
-		"pack-objects",
-		"--all-progress-implied",
-		"--revs",
-		"--stdout",
-		NULL,
-		NULL,
-		NULL,
-		NULL,
-		NULL,
-		NULL,
-	};
 	struct child_process po = CHILD_PROCESS_INIT;
 	FILE *po_in;
 	int i;
 	int rc;
 
-	i = 4;
+	argv_array_push(&po.args, "pack-objects");
+	argv_array_push(&po.args, "--all-progress-implied");
+	argv_array_push(&po.args, "--revs");
+	argv_array_push(&po.args, "--stdout");
 	if (args->use_thin_pack)
-		argv[i++] = "--thin";
+		argv_array_push(&po.args, "--thin");
 	if (args->use_ofs_delta)
-		argv[i++] = "--delta-base-offset";
+		argv_array_push(&po.args, "--delta-base-offset");
 	if (args->quiet || !args->progress)
-		argv[i++] = "-q";
+		argv_array_push(&po.args, "-q");
 	if (args->progress)
-		argv[i++] = "--progress";
+		argv_array_push(&po.args, "--progress");
 	if (is_repository_shallow())
-		argv[i++] = "--shallow";
-	po.argv = argv;
+		argv_array_push(&po.args, "--shallow");
 	po.in = -1;
 	po.out = args->stateless_rpc ? -1 : fd;
 	po.git_cmd = 1;
-- 
2.15.1
Stefan Beller· Dec 22, 2017, 18:09 UTC · re: René Scharfe · lore

Re: [PATCH] send-pack: use internal argv_array of struct child_process

On Fri, Dec 22, 2017 at 12:14 AM, René Scharfe <l.s.r@web.de> wrote:
Show 6 quoted lines
> Avoid a magic number of NULL placeholder values and a magic index by
> constructing the command line for pack-objects using the embedded
> argv_array of the child_process.  The resulting code is shorter and
> easier to extend.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
Reviewed-by: Stefan Beller <sbeller@google.com>
Show 34 quoted lines
> ---
>  send-pack.c | 28 +++++++++-------------------
>  1 file changed, 9 insertions(+), 19 deletions(-)
>
> diff --git a/send-pack.c b/send-pack.c
> index a8cc6b266e..2112d3b27a 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -58,35 +58,25 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *extra, struc
>          * the revision parameters to it via its stdin and
>          * let its stdout go back to the other end.
>          */
> -       const char *argv[] = {
> -               "pack-objects",
> -               "--all-progress-implied",
> -               "--revs",
> -               "--stdout",
> -               NULL,
> -               NULL,
> -               NULL,
> -               NULL,
> -               NULL,
> -               NULL,
> -       };
>         struct child_process po = CHILD_PROCESS_INIT;
>         FILE *po_in;
>         int i;
>         int rc;
>
> -       i = 4;
> +       argv_array_push(&po.args, "pack-objects");
> +       argv_array_push(&po.args, "--all-progress-implied");
> +       argv_array_push(&po.args, "--revs");
> +       argv_array_push(&po.args, "--stdout");

(useless nit of the day, no need to resend:) These four statements could be done as pushl(a,b,c, NULL); but it would not differ in readability, so I guess either is fine.

Thanks, Stefan

Show 21 quoted lines
>         if (args->use_thin_pack)
> -               argv[i++] = "--thin";
> +               argv_array_push(&po.args, "--thin");
>         if (args->use_ofs_delta)
> -               argv[i++] = "--delta-base-offset";
> +               argv_array_push(&po.args, "--delta-base-offset");
>         if (args->quiet || !args->progress)
> -               argv[i++] = "-q";
> +               argv_array_push(&po.args, "-q");
>         if (args->progress)
> -               argv[i++] = "--progress";
> +               argv_array_push(&po.args, "--progress");
>         if (is_repository_shallow())
> -               argv[i++] = "--shallow";
> -       po.argv = argv;
> +               argv_array_push(&po.args, "--shallow");
>         po.in = -1;
>         po.out = args->stateless_rpc ? -1 : fd;
>         po.git_cmd = 1;
> --
> 2.15.1
Junio C Hamano· Dec 22, 2017, 20:11 UTC · re: Stefan Beller · lore

Re: [PATCH] send-pack: use internal argv_array of struct child_process

Stefan Beller <sbeller@google.com> writes:
Show 8 quoted lines
>> +       argv_array_push(&po.args, "pack-objects");
>> +       argv_array_push(&po.args, "--all-progress-implied");
>> +       argv_array_push(&po.args, "--revs");
>> +       argv_array_push(&po.args, "--stdout");
>
> (useless nit of the day, no need to resend:)
> These four statements could be done as pushl(a,b,c, NULL);
> but it would not differ in readability, so I guess either is fine.

Yup. Shorter is not necessarily better. If we anticipate that we will need to update the set of options later (and I suspect we may be doing so soon, given what JeffH and JTan are working on recently), one option (or option-argument pair) per line is a more preferrable form from the maintenance point of view.

← back to recent threads