* [PATCH] upload-pack: use argv_array for pack_objects
@ 2016-02-25 12:13 Michael Procter
2016-02-26 2:14 ` Eric Sunshine
0 siblings, 1 reply; 3+ messages in thread
From: Michael Procter @ 2016-02-25 12:13 UTC (permalink / raw)
To: git
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(-)
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] upload-pack: use argv_array for pack_objects
2016-02-25 12:13 [PATCH] upload-pack: use argv_array for pack_objects Michael Procter
@ 2016-02-26 2:14 ` Eric Sunshine
2016-02-26 10:59 ` Jeff King
0 siblings, 1 reply; 3+ messages in thread
From: Eric Sunshine @ 2016-02-26 2:14 UTC (permalink / raw)
To: Michael Procter; +Cc: Git List
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).
> - 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] upload-pack: use argv_array for pack_objects
2016-02-26 2:14 ` Eric Sunshine
@ 2016-02-26 10:59 ` Jeff King
0 siblings, 0 replies; 3+ messages in thread
From: Jeff King @ 2016-02-26 10:59 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Michael Procter, Git List
On Thu, Feb 25, 2016 at 09:14:01PM -0500, Eric Sunshine wrote:
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-02-26 10:59 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-02-25 12:13 [PATCH] upload-pack: use argv_array for pack_objects Michael Procter
2016-02-26 2:14 ` Eric Sunshine
2016-02-26 10:59 ` Jeff King
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).