threads / patch / 41515

patchupload-pack: use argv_array for pack_objects

Subject: [PATCH] upload-pack: use argv_array for pack_objects

## tl;dr

3 messages between Feb 25, 2016 and Feb 26, 2016. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Michael Procter· Feb 25, 2016, 12:13 UTC · lore

Use the argv_array in the child_process structure, to avoid having to manually maintain an array size.

Signed-off-by: Michael Procter <michael@procter.org.uk>
---
 upload-pack.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

Show changes to upload-pack.c +11 −14

diff --git a/upload-pack.c b/upload-pack.c
index b3f6653..dc802a0 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -90,35 +90,32 @@ static void create_pack_file(void)
 		"corruption on the remote side.";
 	int buffered = -1;
 	ssize_t sz;
-	const char *argv[13];
-	int i, arg = 0;
+	int i;
 	FILE *pipe_fd;
 
 	if (shallow_nr) {
-		argv[arg++] = "--shallow-file";
-		argv[arg++] = "";
+		argv_array_push(&pack_objects.args, "--shallow-file");
+		argv_array_push(&pack_objects.args, "");
 	}
-	argv[arg++] = "pack-objects";
-	argv[arg++] = "--revs";
+	argv_array_push(&pack_objects.args, "pack-objects");
+	argv_array_push(&pack_objects.args, "--revs");
 	if (use_thin_pack)
-		argv[arg++] = "--thin";
+		argv_array_push(&pack_objects.args, "--thin");
 
-	argv[arg++] = "--stdout";
+	argv_array_push(&pack_objects.args, "--stdout");
 	if (shallow_nr)
-		argv[arg++] = "--shallow";
+		argv_array_push(&pack_objects.args, "--shallow");
 	if (!no_progress)
-		argv[arg++] = "--progress";
+		argv_array_push(&pack_objects.args, "--progress");
 	if (use_ofs_delta)
-		argv[arg++] = "--delta-base-offset";
+		argv_array_push(&pack_objects.args, "--delta-base-offset");
 	if (use_include_tag)
-		argv[arg++] = "--include-tag";
-	argv[arg++] = NULL;
+		argv_array_push(&pack_objects.args, "--include-tag");
 
 	pack_objects.in = -1;
 	pack_objects.out = -1;
 	pack_objects.err = -1;
 	pack_objects.git_cmd = 1;
-	pack_objects.argv = argv;
 
 	if (start_command(&pack_objects))
 		die("git upload-pack: unable to fork git-pack-objects");

-- 
2.7.2.368.g02f4152

Eric Sunshine· Feb 26, 2016, 02:14 UTC · re: Michael Procter · lore

Re: [PATCH] upload-pack: use argv_array for pack_objects

On Thu, Feb 25, 2016 at 7:13 AM, Michael Procter <michael@procter.org.uk> wrote:

Show 21 quoted lines

> Use the argv_array in the child_process structure, to avoid having to
> manually maintain an array size.
>
> Signed-off-by: Michael Procter <michael@procter.org.uk>
> ---
> diff --git a/upload-pack.c b/upload-pack.c
> @@ -90,35 +90,32 @@ static void create_pack_file(void)
>                 "corruption on the remote side.";
>         int buffered = -1;
>         ssize_t sz;
> -       const char *argv[13];
> -       int i, arg = 0;
> +       int i;
>         FILE *pipe_fd;
>
>         if (shallow_nr) {
> -               argv[arg++] = "--shallow-file";
> -               argv[arg++] = "";
> +               argv_array_push(&pack_objects.args, "--shallow-file");
> +               argv_array_push(&pack_objects.args, "");
>         }

Not worth a re-roll, but:

    if (shallow_nr)
        argv_array_pushv(..., "--shallow-file", "", NULL);

would have made it clearer that the two arguments are related (and allowed you to drop the braces).

Show 39 quoted lines

> -       argv[arg++] = "pack-objects";
> -       argv[arg++] = "--revs";
> +       argv_array_push(&pack_objects.args, "pack-objects");
> +       argv_array_push(&pack_objects.args, "--revs");
>         if (use_thin_pack)
> -               argv[arg++] = "--thin";
> +               argv_array_push(&pack_objects.args, "--thin");
>
> -       argv[arg++] = "--stdout";
> +       argv_array_push(&pack_objects.args, "--stdout");
>         if (shallow_nr)
> -               argv[arg++] = "--shallow";
> +               argv_array_push(&pack_objects.args, "--shallow");
>         if (!no_progress)
> -               argv[arg++] = "--progress";
> +               argv_array_push(&pack_objects.args, "--progress");
>         if (use_ofs_delta)
> -               argv[arg++] = "--delta-base-offset";
> +               argv_array_push(&pack_objects.args, "--delta-base-offset");
>         if (use_include_tag)
> -               argv[arg++] = "--include-tag";
> -       argv[arg++] = NULL;
> +               argv_array_push(&pack_objects.args, "--include-tag");
>
>         pack_objects.in = -1;
>         pack_objects.out = -1;
>         pack_objects.err = -1;
>         pack_objects.git_cmd = 1;
> -       pack_objects.argv = argv;
>
>         if (start_command(&pack_objects))
>                 die("git upload-pack: unable to fork git-pack-objects");
> --
> 2.7.2.368.g02f4152
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jeff King· Feb 26, 2016, 10:59 UTC · re: Eric Sunshine · lore

Re: [PATCH] upload-pack: use argv_array for pack_objects

On Thu, Feb 25, 2016 at 09:14:01PM -0500, Eric Sunshine wrote:

Show 30 quoted lines

> On Thu, Feb 25, 2016 at 7:13 AM, Michael Procter <michael@procter.org.uk> wrote:
> > Use the argv_array in the child_process structure, to avoid having to
> > manually maintain an array size.
> >
> > Signed-off-by: Michael Procter <michael@procter.org.uk>
> > ---
> > diff --git a/upload-pack.c b/upload-pack.c
> > @@ -90,35 +90,32 @@ static void create_pack_file(void)
> >                 "corruption on the remote side.";
> >         int buffered = -1;
> >         ssize_t sz;
> > -       const char *argv[13];
> > -       int i, arg = 0;
> > +       int i;
> >         FILE *pipe_fd;
> >
> >         if (shallow_nr) {
> > -               argv[arg++] = "--shallow-file";
> > -               argv[arg++] = "";
> > +               argv_array_push(&pack_objects.args, "--shallow-file");
> > +               argv_array_push(&pack_objects.args, "");
> >         }
> 
> Not worth a re-roll, but:
> 
>     if (shallow_nr)
>         argv_array_pushv(..., "--shallow-file", "", NULL);
> 
> would have made it clearer that the two arguments are related (and
> allowed you to drop the braces).

This same pattern repeats in a few places, and I always want to say:

  argv_array_pushf(..., "--shallow-file=%s", file);

but the parsing side is lazy. It wouldn't be too hard to fix (and would make the option nicer for users, too!), but perhaps it would be better still to convert git.c to use the standard parse-options.

That is more work, though. Maybe it would be good for the low-hanging-fruit list.

Other than that, Michael's patch looks fine to me (unsurprising, since I already saw it off-list and offered my commentary :) ). I agree it's not worth a re-roll for just this point.

-Peff