git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jeff King <peff@peff.net>
Date
Apr 18, 2012, 21:10 UTC
Message-ID
<20120418211018.GC21301@sigill.intra.peff.net>
In-Reply-To
<20120418210740.GA21214@sigill.intra.peff.net>

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
Previous: Jeff King
Message 8 of 8 in “gc: fix off-by-one in append_option”
  1. gc: fix off-by-one in append_optionJeff King, Apr 17, 2012
  2. Jeff KingApr 18, 2012
  3. Junio C HamanoApr 18, 2012
  4. Jeff KingApr 18, 2012
  5. Jeff KingApr 18, 2012
  6. 1/3 argv-array: refactor empty_argv initializationJeff King, Apr 18, 2012
  7. 2/3 argv-array: add a new "pushl" methodJeff King, Apr 18, 2012
  8. 3/3 gc: use argv-array for sub-commandsJeff King, Apr 18, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.