Re: [PATCH 2/4] clone: call git_config before parse_options
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 11, 2020, 20:26 UTC
- Message-ID
- <xmqq5z8knjou.fsf@gitster.c.googlers.com>
- In-Reply-To
- <51ef776f8523d29dfe03d15f0d1958f5c456c057.1599848727.git.gitgitgadget@gmail.com>
"Sean Barag via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 11 quoted lines
> 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.
Show 11 quoted lines
> +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?
Show 25 quoted lines
> + 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.
Show 6 quoted lines
> - 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.