[PATCH] gc: fix off-by-one in append_option

[PATCH] gc: fix off-by-one in append_option

[Jeff King]

The append_option function appends a string to a
NULL-terminated array, taking care not to overflow the
array. However, it's comparison was over-cautious, and
checked that there was enough room for two options, not one
(plus the NULL-terminator, of course).  This could cause
very long command-lines (e.g., "git gc --aggressive" when
"--unpack-unreachable" is in use) to hit the limit.

Signed-off-by: Jeff King <peff@peff.net>
---
I noticed this when trying a "git gc --aggressive" with the current
"next". The culprit is my 7e52f56 (gc: do not explode objects which will
be immediately pruned) which adds two extra options to the repack
invocation.

This patch is enough to fix it, as the current limit is big enough when
the off-by-one error is removed.  The currentl limit is 10, which
appears to have been pulled out of thin air by 0d7566a (Add --aggressive
option to 'git gc', 2007-05-09) as "big enough".

Obviously this would be much nicer with argv_array. Unfortunately, I
don't think there is a way to have a nice initializer with argv_array.
The current code looks like this:

  static const char *argv_repack[MAX_ADD] = {"repack", "-d", "-l", NULL };

and in an ideal world you would have something like:

  static struct argv_array argv_repack = ARGV_ARRAY_INIT("repack", "-d", "-l");

Of course that would involve variadic macros, which we can't count on.
But even beyond that it's tough, because you are mixing static
initialization with a dynamic structure. You can hack around that by
copying the defaults during first push, but that's extra complexity, and
the syntax is still ugly.

What do you think of just:

  static struct argv_array argv_repack;
  [...]
  argv_init_from_string(&argv_repack, "repack -d -l");

I prefer static initialization when we can do it, but it just seems like
too much trouble in this case.

 builtin/gc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 1bc2fe3..d80a961 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -74,7 +74,7 @@ static void append_option(const char **cmd, const char *opt, int max_length)
 	for (i = 0; cmd[i]; i++)
 		;
 
-	if (i + 2 >= max_length)
+	if (i + 1 >= max_length)
 		die(_("Too many options specified"));
 	cmd[i++] = opt;
 	cmd[i] = NULL;
-- 
1.7.9.6.8.g992e5

Re: [PATCH] gc: fix off-by-one in append_option

[Jeff King]

On Tue, Apr 17, 2012 at 04:32:55PM -0700, Jeff King wrote:

> I noticed this when trying a "git gc --aggressive" with the current
> "next". The culprit is my 7e52f56 (gc: do not explode objects which will
> be immediately pruned) which adds two extra options to the repack
> invocation.
> 
> This patch is enough to fix it, as the current limit is big enough when
> the off-by-one error is removed.  The currentl limit is 10, which
> appears to have been pulled out of thin air by 0d7566a (Add --aggressive
> option to 'git gc', 2007-05-09) as "big enough".

Ugh, I take that back. If you do "git gc --aggressive -q", it will
overflow the arbitrary 10-element limit. So we need something else on
top, even if it's just bumping MAX_ADD.

> Obviously this would be much nicer with argv_array. Unfortunately, I
> don't think there is a way to have a nice initializer with argv_array.
> The current code looks like this:
> 
>   static const char *argv_repack[MAX_ADD] = {"repack", "-d", "-l", NULL };
> 
> and in an ideal world you would have something like:
> 
>   static struct argv_array argv_repack = ARGV_ARRAY_INIT("repack", "-d", "-l");
> 
> Of course that would involve variadic macros, which we can't count on.
> But even beyond that it's tough, because you are mixing static
> initialization with a dynamic structure. You can hack around that by
> copying the defaults during first push, but that's extra complexity, and
> the syntax is still ugly.

I've included a patch below that makes this look like:

  static const char repack_cmd[] = {"repack", "-d", "-l", NULL };
  static struct argv_array repack = ARGV_ARRAY_INIT_DEFAULT(repack_cmd);

which is way less nice than I would prefer, but I think may be the best
we can do with static initialization.

> What do you think of just:
> 
>   static struct argv_array argv_repack;
>   [...]
>   argv_init_from_string(&argv_repack, "repack -d -l");

I looked into this, but it seems our sq_dequote_* functions insist that
it be spelled "'repack' '-d' '-l'", so I would have to teach them to be
more liberal first.

Thoughts?

---
Here's the patch to do static initializers. I went ahead and converted
all of the commands in use. In addition to fixing repack, this gets rid
of some ugly constant indices in the prune command. The others don't
benefit immediately, but it makes tweaking them later much less
error-prone.

 argv-array.c |   10 +++++---
 argv-array.h |    1 +
 builtin/gc.c |   77 ++++++++++++++++++++++++----------------------------------
 3 files changed, 40 insertions(+), 48 deletions(-)

diff --git a/argv-array.c b/argv-array.c
index a4e0420..cb96714 100644
--- a/argv-array.c
+++ b/argv-array.c
@@ -14,10 +14,14 @@ void argv_array_init(struct argv_array *array)
 
 static void argv_array_push_nodup(struct argv_array *array, const char *value)
 {
-	if (array->argv == empty_argv)
-		array->argv = NULL;
+	if (!array->alloc) {
+		const char **old = array->argv;
+		array->argv = xmalloc((array->argc + 2) * sizeof(*array->argv));
+		memcpy(array->argv, old, array->argc * sizeof(*array->argv));
+	}
+	else
+		ALLOC_GROW(array->argv, array->argc + 2, array->alloc);
 
-	ALLOC_GROW(array->argv, array->argc + 2, array->alloc);
 	array->argv[array->argc++] = value;
 	array->argv[array->argc] = NULL;
 }
diff --git a/argv-array.h b/argv-array.h
index 74dd2b1..60d5731 100644
--- a/argv-array.h
+++ b/argv-array.h
@@ -9,6 +9,7 @@ struct argv_array {
 	int alloc;
 };
 
+#define ARGV_ARRAY_INIT_DEFAULT(def) { (def), (ARRAY_SIZE(def)-1), 0 }
 #define ARGV_ARRAY_INIT { empty_argv, 0, 0 }
 
 void argv_array_init(struct argv_array *);
diff --git a/builtin/gc.c b/builtin/gc.c
index 1bc2fe3..69dc705 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -14,6 +14,7 @@
 #include "cache.h"
 #include "parse-options.h"
 #include "run-command.h"
+#include "argv-array.h"
 
 #define FAILED_RUN "failed to run %s"
 
@@ -28,12 +29,16 @@ static int gc_auto_threshold = 6700;
 static int gc_auto_pack_limit = 50;
 static const char *prune_expire = "2.weeks.ago";
 
-#define MAX_ADD 10
-static const char *argv_pack_refs[] = {"pack-refs", "--all", "--prune", NULL};
-static const char *argv_reflog[] = {"reflog", "expire", "--all", NULL};
-static const char *argv_repack[MAX_ADD] = {"repack", "-d", "-l", NULL};
-static const char *argv_prune[] = {"prune", "--expire", NULL, NULL, NULL};
-static const char *argv_rerere[] = {"rerere", "gc", NULL};
+static const char *pack_refs_default[] = {"pack-refs", "--all", "--prune", NULL};
+static struct argv_array pack_refs_cmd = ARGV_ARRAY_INIT_DEFAULT(pack_refs_default);
+static const char *reflog_default[] = {"reflog", "expire", "--all", NULL};
+static struct argv_array reflog = ARGV_ARRAY_INIT_DEFAULT(reflog_default);
+static const char *repack_default[] = {"repack", "-d", "-l", NULL};
+static struct argv_array repack = ARGV_ARRAY_INIT_DEFAULT(repack_default);
+static const char *prune_default[] = {"prune", "--expire", NULL };
+static struct argv_array prune = ARGV_ARRAY_INIT_DEFAULT(prune_default);
+static const char *rerere_default[] = {"rerere", "gc", NULL};
+static struct argv_array rerere = ARGV_ARRAY_INIT_DEFAULT(rerere_default);
 
 static int gc_config(const char *var, const char *value, void *cb)
 {
@@ -67,19 +72,6 @@ static int gc_config(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
-static void append_option(const char **cmd, const char *opt, int max_length)
-{
-	int i;
-
-	for (i = 0; cmd[i]; i++)
-		;
-
-	if (i + 2 >= max_length)
-		die(_("Too many options specified"));
-	cmd[i++] = opt;
-	cmd[i] = NULL;
-}
-
 static int too_many_loose_objects(void)
 {
 	/*
@@ -147,13 +139,11 @@ static int too_many_packs(void)
 static void add_repack_all_option(void)
 {
 	if (prune_expire && !strcmp(prune_expire, "now"))
-		append_option(argv_repack, "-a", MAX_ADD);
+		argv_array_push(&repack, "-a");
 	else {
-		append_option(argv_repack, "-A", MAX_ADD);
-		if (prune_expire) {
-			append_option(argv_repack, "--unpack-unreachable", MAX_ADD);
-			append_option(argv_repack, prune_expire, MAX_ADD);
-		}
+		argv_array_push(&repack, "-A");
+		if (prune_expire)
+			argv_array_pushf(&repack, "--unpack-unreachable=%s", prune_expire);
 	}
 }
 
@@ -187,7 +177,6 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	int aggressive = 0;
 	int auto_gc = 0;
 	int quiet = 0;
-	char buf[80];
 
 	struct option builtin_gc_options[] = {
 		OPT__QUIET(&quiet, "suppress progress reporting"),
@@ -213,15 +202,13 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_gc_usage, builtin_gc_options);
 
 	if (aggressive) {
-		append_option(argv_repack, "-f", MAX_ADD);
-		append_option(argv_repack, "--depth=250", MAX_ADD);
-		if (aggressive_window > 0) {
-			sprintf(buf, "--window=%d", aggressive_window);
-			append_option(argv_repack, buf, MAX_ADD);
-		}
+		argv_array_push(&repack, "-f");
+		argv_array_push(&repack, "--depth=250");
+		if (aggressive_window > 0)
+			argv_array_pushf(&repack, "--window=%d", aggressive_window);
 	}
 	if (quiet)
-		append_option(argv_repack, "-q", MAX_ADD);
+		argv_array_push(&repack, "-q");
 
 	if (auto_gc) {
 		/*
@@ -239,25 +226,25 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	} else
 		add_repack_all_option();
 
-	if (pack_refs && run_command_v_opt(argv_pack_refs, RUN_GIT_CMD))
-		return error(FAILED_RUN, argv_pack_refs[0]);
+	if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))
+		return error(FAILED_RUN, pack_refs_cmd.argv[0]);
 
-	if (run_command_v_opt(argv_reflog, RUN_GIT_CMD))
-		return error(FAILED_RUN, argv_reflog[0]);
+	if (run_command_v_opt(reflog.argv, RUN_GIT_CMD))
+		return error(FAILED_RUN, reflog.argv[0]);
 
-	if (run_command_v_opt(argv_repack, RUN_GIT_CMD))
-		return error(FAILED_RUN, argv_repack[0]);
+	if (run_command_v_opt(repack.argv, RUN_GIT_CMD))
+		return error(FAILED_RUN, repack.argv[0]);
 
 	if (prune_expire) {
-		argv_prune[2] = prune_expire;
+		argv_array_push(&prune, prune_expire);
 		if (quiet)
-			argv_prune[3] = "--no-progress";
-		if (run_command_v_opt(argv_prune, RUN_GIT_CMD))
-			return error(FAILED_RUN, argv_prune[0]);
+			argv_array_push(&prune, "--no-progress");
+		if (run_command_v_opt(prune.argv, RUN_GIT_CMD))
+			return error(FAILED_RUN, prune.argv[0]);
 	}
 
-	if (run_command_v_opt(argv_rerere, RUN_GIT_CMD))
-		return error(FAILED_RUN, argv_rerere[0]);
+	if (run_command_v_opt(rerere.argv, RUN_GIT_CMD))
+		return error(FAILED_RUN, rerere.argv[0]);
 
 	if (auto_gc && too_many_loose_objects())
 		warning(_("There are too many unreachable loose objects; "

Re: [PATCH] gc: fix off-by-one in append_option

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I've included a patch below that makes this look like:
>
>   static const char repack_cmd[] = {"repack", "-d", "-l", NULL };
>   static struct argv_array repack = ARGV_ARRAY_INIT_DEFAULT(repack_cmd);
>
> which is way less nice than I would prefer, but I think may be the best
> we can do with static initialization.
>
>> What do you think of just:
>> 
>>   static struct argv_array argv_repack;
>>   [...]
>>   argv_init_from_string(&argv_repack, "repack -d -l");
>
> I looked into this, but it seems our sq_dequote_* functions insist that
> it be spelled "'repack' '-d' '-l'", so I would have to teach them to be
> more liberal first.
>
> Thoughts?

I do not know it is worth it to try to be too fancy.

I was about to suggest, immediately after seeing the first one I quoted
above, to omit NULL and instead use ARRAY_SIZE(), but I do not think that
is even worth it, as some (possibly future) caller may have only "char **"
as a usual NULL terminated array at hand.

I am perfectly OK with even without initializers, like this:

	struct argv_array repack = ARGV_ARRAY_INIT;
	argv_array_push_strings(&repack, "repack", "-d", "-l", NULL);

Re: [PATCH] gc: fix off-by-one in append_option

[Jeff King]

On Wed, Apr 18, 2012 at 12:34:14PM -0700, Junio C Hamano wrote:

> > I've included a patch below that makes this look like:
> >
> >   static const char repack_cmd[] = {"repack", "-d", "-l", NULL };
> >   static struct argv_array repack = ARGV_ARRAY_INIT_DEFAULT(repack_cmd);
>
> I do not know it is worth it to try to be too fancy.
> 
> I was about to suggest, immediately after seeing the first one I quoted
> above, to omit NULL and instead use ARRAY_SIZE(), but I do not think that
> is even worth it, as some (possibly future) caller may have only "char **"
> as a usual NULL terminated array at hand.

Actually, that is broken already, because the initializer uses
ARRAY_SIZE to set argc properly. Omitting NULL wouldn't work anyway,
though, because then the state before any push violates the invariant
(that the value is NUL-terminated).

I think it really is impossible to make it nice, because we can't count
on running _any_ code before somebody peeks at array.argv (we don't even
have an accessor, but just let people look at that directly).

> I am perfectly OK with even without initializers, like this:
> 
> 	struct argv_array repack = ARGV_ARRAY_INIT;
> 	argv_array_push_strings(&repack, "repack", "-d", "-l", NULL);

I think that is sane, and certainly the simplest. I'll send a patch in a
moment.

-Peff

Re: [PATCH] gc: fix off-by-one in append_option

[Jeff King]

On Wed, Apr 18, 2012 at 01:21:16PM -0700, Jeff King wrote:

> > I am perfectly OK with even without initializers, like this:
> > 
> > 	struct argv_array repack = ARGV_ARRAY_INIT;
> > 	argv_array_push_strings(&repack, "repack", "-d", "-l", NULL);
> 
> I think that is sane, and certainly the simplest. I'll send a patch in a
> moment.

Here it is. This goes directly on top of 7e52f56 (the tip of
jk/repack-no-explode-objects-from-old-pack). The off-by-one fix I sent
earlier can be dropped, as that code is removed by the final patch
below.

  [1/3]: argv-array: refactor empty_argv initialization
  [2/3]: argv-array: add a new "pushl" method
  [3/3]: gc: use argv-array for sub-commands

-Peff

[PATCH 1/3] argv-array: refactor empty_argv initialization

[Jeff King]

An empty argv-array is initialized to point to a static
empty NULL-terminated array.  The original implementation
separates the actual storage of the NULL-terminator from the
pointer to the list.  This makes the exposed type a "const
char **", which nicely matches the type stored by the
argv-array.

However, this indirection means that one cannot use
empty_argv to initialize a static variable, since it is
not a constant.

Instead, we can expose empty_argv directly, as an array of
pointers. The only place we use it is in the ARGV_ARRAY_INIT
initializer, and it decays to a pointer appropriately there.

Signed-off-by: Jeff King <peff@peff.net>
---
 argv-array.c |    3 +--
 argv-array.h |    2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/argv-array.c b/argv-array.c
index a4e0420..110a61b 100644
--- a/argv-array.c
+++ b/argv-array.c
@@ -2,8 +2,7 @@
 #include "argv-array.h"
 #include "strbuf.h"
 
-static const char *empty_argv_storage = NULL;
-const char **empty_argv = &empty_argv_storage;
+const char *empty_argv[] = { NULL };
 
 void argv_array_init(struct argv_array *array)
 {
diff --git a/argv-array.h b/argv-array.h
index 74dd2b1..c45c698 100644
--- a/argv-array.h
+++ b/argv-array.h
@@ -1,7 +1,7 @@
 #ifndef ARGV_ARRAY_H
 #define ARGV_ARRAY_H
 
-extern const char **empty_argv;
+extern const char *empty_argv[];
 
 struct argv_array {
 	const char **argv;
-- 
1.7.9.6.8.g992e5

[PATCH 2/3] argv-array: add a new "pushl" method

[Jeff King]

It can be convenient to push many strings in a single line
(e.g., if you are initializing an array with defaults). This
patch provides a convenience wrapper to allow this.

Signed-off-by: Jeff King <peff@peff.net>
---
I called this "pushl" after "execl", with the intent that we might
eventually have "pushv" if somebody wants it. Hopefully that makes
sense to others, but if not, I can name it something more verbose.

 Documentation/technical/api-argv-array.txt |    5 +++++
 argv-array.c                               |   11 +++++++++++
 argv-array.h                               |    1 +
 3 files changed, 17 insertions(+)

diff --git a/Documentation/technical/api-argv-array.txt b/Documentation/technical/api-argv-array.txt
index 49b3d52..1b7d8f1 100644
--- a/Documentation/technical/api-argv-array.txt
+++ b/Documentation/technical/api-argv-array.txt
@@ -37,6 +37,11 @@ Functions
 `argv_array_push`::
 	Push a copy of a string onto the end of the array.
 
+`argv_array_pushl`::
+	Push a list of strings onto the end of the array. The arguments
+	should be a list of `const char *` strings, terminated by a NULL
+	argument.
+
 `argv_array_pushf`::
 	Format a string and push it onto the end of the array. This is a
 	convenience wrapper combining `strbuf_addf` and `argv_array_push`.
diff --git a/argv-array.c b/argv-array.c
index 110a61b..0b5f889 100644
--- a/argv-array.c
+++ b/argv-array.c
@@ -38,6 +38,17 @@ void argv_array_pushf(struct argv_array *array, const char *fmt, ...)
 	argv_array_push_nodup(array, strbuf_detach(&v, NULL));
 }
 
+void argv_array_pushl(struct argv_array *array, ...)
+{
+	va_list ap;
+	const char *arg;
+
+	va_start(ap, array);
+	while((arg = va_arg(ap, const char *)))
+		argv_array_push(array, arg);
+	va_end(ap);
+}
+
 void argv_array_clear(struct argv_array *array)
 {
 	if (array->argv != empty_argv) {
diff --git a/argv-array.h b/argv-array.h
index c45c698..b93a69c 100644
--- a/argv-array.h
+++ b/argv-array.h
@@ -15,6 +15,7 @@ void argv_array_init(struct argv_array *);
 void argv_array_push(struct argv_array *, const char *);
 __attribute__((format (printf,2,3)))
 void argv_array_pushf(struct argv_array *, const char *fmt, ...);
+void argv_array_pushl(struct argv_array *, ...);
 void argv_array_clear(struct argv_array *);
 
 #endif /* ARGV_ARRAY_H */
-- 
1.7.9.6.8.g992e5

[PATCH 3/3] gc: use argv-array for sub-commands

[Jeff King]

git-gc executes many sub-commands. The argument list for
some of these is constant, but for others we add more
arguments at runtime. The latter is implemented by allocating
a constant extra number of NULLs, and either using a custom
append function, or just referencing unused slots by number.

As of commit 7e52f56, which added two new arguments, it is
possible to exceed the constant number of slots for "repack"
by running "git gc --aggressive", causing "git gc" to die.

This patch converts all of the static argv lists to use
argv-array. In addition to fixing the overflow caused by
7e52f56, it has a few advantages:

  1. We can drop the custom append function (which,
     incidentally, had an off-by-one error exacerbating the
     static limit).

  2. We can drop the ugly magic numbers used when adding
     arguments to "prune".

  3. Adding further arguments will be easier; you can just
     add new "push" calls without worrying about increasing
     any static limits.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/gc.c |   78 +++++++++++++++++++++++++---------------------------------
 1 file changed, 33 insertions(+), 45 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 1bc2fe3..9b4232c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -14,6 +14,7 @@
 #include "cache.h"
 #include "parse-options.h"
 #include "run-command.h"
+#include "argv-array.h"
 
 #define FAILED_RUN "failed to run %s"
 
@@ -28,12 +29,11 @@ static int gc_auto_threshold = 6700;
 static int gc_auto_pack_limit = 50;
 static const char *prune_expire = "2.weeks.ago";
 
-#define MAX_ADD 10
-static const char *argv_pack_refs[] = {"pack-refs", "--all", "--prune", NULL};
-static const char *argv_reflog[] = {"reflog", "expire", "--all", NULL};
-static const char *argv_repack[MAX_ADD] = {"repack", "-d", "-l", NULL};
-static const char *argv_prune[] = {"prune", "--expire", NULL, NULL, NULL};
-static const char *argv_rerere[] = {"rerere", "gc", NULL};
+static struct argv_array pack_refs_cmd = ARGV_ARRAY_INIT;
+static struct argv_array reflog = ARGV_ARRAY_INIT;
+static struct argv_array repack = ARGV_ARRAY_INIT;
+static struct argv_array prune = ARGV_ARRAY_INIT;
+static struct argv_array rerere = ARGV_ARRAY_INIT;
 
 static int gc_config(const char *var, const char *value, void *cb)
 {
@@ -67,19 +67,6 @@ static int gc_config(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
-static void append_option(const char **cmd, const char *opt, int max_length)
-{
-	int i;
-
-	for (i = 0; cmd[i]; i++)
-		;
-
-	if (i + 2 >= max_length)
-		die(_("Too many options specified"));
-	cmd[i++] = opt;
-	cmd[i] = NULL;
-}
-
 static int too_many_loose_objects(void)
 {
 	/*
@@ -147,13 +134,11 @@ static int too_many_packs(void)
 static void add_repack_all_option(void)
 {
 	if (prune_expire && !strcmp(prune_expire, "now"))
-		append_option(argv_repack, "-a", MAX_ADD);
+		argv_array_push(&repack, "-a");
 	else {
-		append_option(argv_repack, "-A", MAX_ADD);
-		if (prune_expire) {
-			append_option(argv_repack, "--unpack-unreachable", MAX_ADD);
-			append_option(argv_repack, prune_expire, MAX_ADD);
-		}
+		argv_array_push(&repack, "-A");
+		if (prune_expire)
+			argv_array_pushf(&repack, "--unpack-unreachable=%s", prune_expire);
 	}
 }
 
@@ -187,7 +172,6 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	int aggressive = 0;
 	int auto_gc = 0;
 	int quiet = 0;
-	char buf[80];
 
 	struct option builtin_gc_options[] = {
 		OPT__QUIET(&quiet, "suppress progress reporting"),
@@ -202,6 +186,12 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(builtin_gc_usage, builtin_gc_options);
 
+	argv_array_pushl(&pack_refs_cmd, "pack-refs", "--all", "--prune", NULL);
+	argv_array_pushl(&reflog, "reflog", "expire", "--all", NULL);
+	argv_array_pushl(&repack, "repack", "-d", "-l", NULL);
+	argv_array_pushl(&prune, "prune", "--expire", NULL );
+	argv_array_pushl(&rerere, "rerere", "gc", NULL);
+
 	git_config(gc_config, NULL);
 
 	if (pack_refs < 0)
@@ -213,15 +203,13 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_gc_usage, builtin_gc_options);
 
 	if (aggressive) {
-		append_option(argv_repack, "-f", MAX_ADD);
-		append_option(argv_repack, "--depth=250", MAX_ADD);
-		if (aggressive_window > 0) {
-			sprintf(buf, "--window=%d", aggressive_window);
-			append_option(argv_repack, buf, MAX_ADD);
-		}
+		argv_array_push(&repack, "-f");
+		argv_array_push(&repack, "--depth=250");
+		if (aggressive_window > 0)
+			argv_array_pushf(&repack, "--window=%d", aggressive_window);
 	}
 	if (quiet)
-		append_option(argv_repack, "-q", MAX_ADD);
+		argv_array_push(&repack, "-q");
 
 	if (auto_gc) {
 		/*
@@ -239,25 +227,25 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	} else
 		add_repack_all_option();
 
-	if (pack_refs && run_command_v_opt(argv_pack_refs, RUN_GIT_CMD))
-		return error(FAILED_RUN, argv_pack_refs[0]);
+	if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))
+		return error(FAILED_RUN, pack_refs_cmd.argv[0]);
 
-	if (run_command_v_opt(argv_reflog, RUN_GIT_CMD))
-		return error(FAILED_RUN, argv_reflog[0]);
+	if (run_command_v_opt(reflog.argv, RUN_GIT_CMD))
+		return error(FAILED_RUN, reflog.argv[0]);
 
-	if (run_command_v_opt(argv_repack, RUN_GIT_CMD))
-		return error(FAILED_RUN, argv_repack[0]);
+	if (run_command_v_opt(repack.argv, RUN_GIT_CMD))
+		return error(FAILED_RUN, repack.argv[0]);
 
 	if (prune_expire) {
-		argv_prune[2] = prune_expire;
+		argv_array_push(&prune, prune_expire);
 		if (quiet)
-			argv_prune[3] = "--no-progress";
-		if (run_command_v_opt(argv_prune, RUN_GIT_CMD))
-			return error(FAILED_RUN, argv_prune[0]);
+			argv_array_push(&prune, "--no-progress");
+		if (run_command_v_opt(prune.argv, RUN_GIT_CMD))
+			return error(FAILED_RUN, prune.argv[0]);
 	}
 
-	if (run_command_v_opt(argv_rerere, RUN_GIT_CMD))
-		return error(FAILED_RUN, argv_rerere[0]);
+	if (run_command_v_opt(rerere.argv, RUN_GIT_CMD))
+		return error(FAILED_RUN, rerere.argv[0]);
 
 	if (auto_gc && too_many_loose_objects())
 		warning(_("There are too many unreachable loose objects; "
-- 
1.7.9.6.8.g992e5