| Message ID | 51ef776f8523d29dfe03d15f0d1958f5c456c057.1599848727.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | clone: allow configurable default for -o/--origin | expand |
On 9/11/2020 2:25 PM, Sean Barag via GitGitGadget wrote: > 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. > > [1] https://lore.kernel.org/git/pull.710.git.1598456751674.gitgitgadget@gmail.com/ > [2] https://github.com/gitgitgadget/git/pull/727#issuecomment-689740195 > > Signed-off-by: Sean Barag <sean@barag.org> > Thanks-to: Junio C Hamano <gitster@pobox.com> > Thanks-to: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > builtin/clone.c | 26 +++++++++++++++++++++++++- > 1 file changed, 25 insertions(+), 1 deletion(-) > > diff --git a/builtin/clone.c b/builtin/clone.c > index b087ee40c2..bf095815f0 100644 > --- a/builtin/clone.c > +++ b/builtin/clone.c > @@ -851,8 +851,21 @@ static int checkout(int submodule_progress) > return err; > } > > +static int git_clone_config(const char *k, const char *v, void *cb) > +{ > + return git_default_config(k, v, cb); > +} > + Introducing this no-op seems a bit premature, but as long as it makes your future patches cleaner, then it is appropriate. > 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 > + */ > + int apply_failed = git_default_config(key, value, data); However, you change this to git_clone_config() in patch 4. Perhaps use git_clone_config() here instead? Thanks, -Stolee
"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.
> 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. > > +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. > > + 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
diff --git a/builtin/clone.c b/builtin/clone.c index b087ee40c2..bf095815f0 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -851,8 +851,21 @@ static int checkout(int submodule_progress) return err; } +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 + */ + int apply_failed = git_default_config(key, value, data); + 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); - 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); if (option_bare) { if (option_mirror)