Re: [PATCH] fetch --all: pass --tags/--no-tags through to each remote

From: Jeff King <peff@peff.net>
Date: Sep 1, 2012, 11:22 UTC
Message-ID: <20120901112251.GA11445@sigill.intra.peff.net>
In-Reply-To: <1346473533-24175-1-git-send-email-ComputerDruid@gmail.com>

On Sat, Sep 01, 2012 at 12:25:33AM -0400, Dan Johnson wrote:

Show 15 quoted lines

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index bb9a074..c6bcbdc 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -857,6 +857,10 @@ static void add_options_to_argv(int *argc, const char **argv)
>  		argv[(*argc)++] = "--recurse-submodules";
>  	else if (recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND)
>  		argv[(*argc)++] = "--recurse-submodules=on-demand";
> +	if (tags == TAGS_SET)
> +		argv[(*argc)++] = "--tags";
> +	else if (tags == TAGS_UNSET)
> +		argv[(*argc)++] = "--no-tags";
>  	if (verbosity >= 2)
>  		argv[(*argc)++] = "-v";
>  	if (verbosity >= 1)

Hmm. We allocate argv in fetch_multiple like this:

  const char *argv[12] = { "fetch", "--append" };

and then add a bunch of options to it, along with the name of the remote. By my count, the current code can hit exactly 12 (including the terminating NULL) if all options are set. Your patch would make it possible to overflow. Of course, I may be miscounting since it is extremely error-prone to figure out the right number by tracing each possible conditional.

Maybe we should switch it to a dynamic argv_array? Like this:

  [1/2]: argv-array: add pop function
  [2/2]: fetch: use argv_array instead of hand-building arrays

-Peff