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

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(-)