From: Jeff King <peff@peff.net>
Date: Wed, 26 Nov 2025 15:02:15 GMT
Subject: Re: [BUG] `git clone '-c KEY=VALUE'` no longer works
Message-ID: <20251126150215.GB4143292@coredump.intra.peff.net>
In-Reply-To: <xmqq8qftrcqb.fsf@gitster.g>

On Tue, Nov 25, 2025 at 02:03:56PM -0800, Junio C Hamano wrote:

> The first step of the "right right thing" may look something like
> this.  As this thread analyzed so far, this awkward lenience exists
> only in "clone -c <key>=<value>" in that the keyname is trimmed, so
> isolating the damage within the clone's code path would be the right
> approach, if we want to keep this awkward lenience alive a little
> bit longer.

That's not entirely true. It is in any code that calls
git_config_parse_parameter(), which includes the old-style parser for
GIT_CONFIG_PARAMETERS.

So:

  $ GIT_CONFIG_PARAMETERS="' foo.bar =baz'" git.v2.51.0 config foo.bar
  baz

  $ GIT_CONFIG_PARAMETERS="' foo.bar =baz'" git.v2.52.0 config foo.bar
  error: invalid key:  foo.bar
  fatal: unable to parse command-line config

That doesn't trigger via "git -c", because we use the "new" form these
days (so it started rejecting the extra whitespace in 2021). And you'd
only see it if you hand-crafted the variable, or an old version of Git
set parameters that were then parsed by a newer one.

So whether that is a case we care about is up for debate. But if we are
going to accommodate backwards compatibility, we have to decide where to
draw the line.

> diff --git c/builtin/clone.c w/builtin/clone.c
> index c990f398ef..4ea8c92a6b 100644
> --- c/builtin/clone.c
> +++ w/builtin/clone.c
> @@ -779,7 +779,26 @@ static void write_config(struct string_list *config)
>  	int i;
>  
>  	for (i = 0; i < config->nr; i++) {
> -		if (git_config_parse_parameter(config->items[i].string,
> +		/*
> +		 * NEEDSWORK: a backward compatibility wart that made
> +		 * us tolerate (note the leading whitespace before
> +		 * the variable name)
> +		 *
> +		 * $ git clone '-c foo.bar=baz'
> +		 *
> +		 * and treated as if the leading whitespace before the
> +		 * variable name did not exist.  Apparently a third
> +		 * party tool "Bamboo" relies on this past stupidity
> +		 * of ours.
> +		 *
> +		 * Eventually we should deprecate and remove this.
> +		 */
> +		const char *trimleft = config->items[i].string;
> +
> +		while (*trimleft && isspace(*trimleft))
> +			trimleft++;

The old code actually trimmed both sides. So:

  $ GIT_CONFIG_PARAMETERS="'foo.bar =baz'" git.v2.51.0 config foo.bar
  baz

  $ GIT_CONFIG_PARAMETERS="'foo.bar =baz'" git.v2.52.0 config foo.bar
  error: invalid key: foo.bar
  fatal: unable to parse command-line config

And I think the latter would still fail with your patch. Again, that
might not matter to us, if all we care about is making:

  git clone '-c foo.bar=baz' ...

work as before. But I'm still skeptical that is worthwhile (especially
given that nobody noticed the same change to "git -c" a few years ago).

-Peff