From fd93d2e60ea66fc3796904ff53ead3ef4755b137 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Apr 2012 14:08:49 -0700 Subject: [PATCH 1/3] argv-array: refactor empty_argv initialization 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 Signed-off-by: Junio C Hamano --- 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 a4e04201e6..110a61b93a 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 74dd2b1bc0..c45c698d53 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; From d15bbe1379a12e2d9a5f18f7dc96b38afafc6c5d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Apr 2012 14:10:05 -0700 Subject: [PATCH 2/3] argv-array: add a new "pushl" method 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 Signed-off-by: Junio C Hamano --- 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 49b3d52952..1b7d8f140c 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 110a61b93a..0b5f8898a1 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 c45c698d53..b93a69c36c 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 */ From 234587fc87b156dc20461fc61353beeb904b43bc Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Apr 2012 14:10:19 -0700 Subject: [PATCH 3/3] gc: use argv-array for sub-commands 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 Signed-off-by: Junio C Hamano --- builtin/gc.c | 78 ++++++++++++++++++++++------------------------------ 1 file changed, 33 insertions(+), 45 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index 1bc2fe336e..9b4232c8f3 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; "