Re: [PATCH 2/4] clone: call git_config before parse_options

[Junio C Hamano <gitster@pobox.com>]

"Sean Barag via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Sean Barag <sean@barag.org>
>
> While Junio's request [1] was to avoids the unusual  "write config then
> immediately read it" pattern that exists in `cmd_clone`, Johannes
> mentioned that --template can write new config values that aren't
> automatically included in the environment [2]. This requires a config
> re-read after `init_db` is called.
>
> Moving the initial config up does allow settings from config to be
> overwritten by ones provided via CLI options in a more natural way
> though, so that part of Junio's suggestion remains.

The title says what the code does after this change.  The code calls
git_config() before calling parse_options(), but not much in the
proposed log message explains what the patch tries to achieve by
doing so.

The above refers to suggestions but does not describe what problem
the patch is trying to address and what approach is taken to address
it.

> Signed-off-by: Sean Barag <sean@barag.org>
> Thanks-to: Junio C Hamano <gitster@pobox.com>
> Thanks-to: Johannes Schindelin <johannes.schindelin@gmx.de>

Usually these two are spelled Helped-by: in this project, and are
given in chronological order.  People gave input to you and then
finally you send out a signed copy, so your sign-off is placed at
the end of the sequence.

> +static int git_clone_config(const char *k, const char *v, void *cb)
> +{
> +	return git_default_config(k, v, cb);
> +}
> +
>  static int write_one_config(const char *key, const char *value, void *data)
>  {
> +	/*
> +	 * give git_config_default a chance to write config values back to the environment, since
> +	 * git_config_set_multivar_gently only deals with config-file writes
> +	 */

Overlong lines...

> +	int apply_failed = git_default_config(key, value, data);

Not git_clone_config()?  Presumably you'll make git_clone_config()
recognise more variables than git_default_config() does, and the
caller of this helper wants us to recognise "clone.*" that are
ignored by git_default_config() callback, no?

> +	if (apply_failed)
> +		return apply_failed;
> +
>  	return git_config_set_multivar_gently(key,
>  					      value ? value : "true",
>  					      CONFIG_REGEX_NONE, 0);
> @@ -964,6 +977,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	struct strvec ref_prefixes = STRVEC_INIT;
>  
>  	packet_trace_identity("clone");
> +
> +	git_config(git_clone_config, NULL);
> +
>  	argc = parse_options(argc, argv, prefix, builtin_clone_options,
>  			     builtin_clone_usage, 0);
>  
> @@ -1125,9 +1141,17 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	if (real_git_dir)
>  		git_dir = real_git_dir;
>  
> +	/*
> +	 * additional config can be injected with -c, make sure it's included
> +	 * after init_db, which clears the entire config environment.
> +	 */
>  	write_config(&option_config);

The comment that explains the location is very much appropriate.

> -	git_config(git_default_config, NULL);
> +	/*
> +	 * re-read config after init_db and write_config to pick up any config
> +	 * injected by --template and --config, respectively
> +	 */
> +	git_config(git_clone_config, NULL);

Does this call read from the freshly written file?

I thought git_config() iterates over the in-core configset that was
read by the first call to git_config(), which in turn calls
git_config_check_init() and calls repo_read_config() only once to
populate the in-core configset, and I suspect we are not clearing
it in between.