Re: [PATCH 2/4] clone: call git_config before parse_options
- From
Sean Barag <sean@barag.org>
- Date
- Sep 16, 2020, 16:12 UTC
- Message-ID
- <20200916161227.1873247-1-sean@barag.org>
- In-Reply-To
- <xmqq5z8knjou.fsf@gitster.c.googlers.com>
Show 20 quoted lines
> From: Junio C Hamano <gitster@pobox.com> > > 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.
Thanks for the pointer - I completely agree. Rewriting for v2.
Show 13 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...How embarrassing! Re-wrapped for v2.
Show 6 quoted 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?
Great catch! This gets changed to `git_clone_config` in patch 4/4 anyway, so there's no need for the confusing intermediate state in 2/4. Will be fixed in v2.
-- Thanks for being so patient with a newbie :) Sean