On 06/12, Jeff King wrote:
> On Mon, Jun 12, 2017 at 06:38:17PM -0700, Jonathan Nieder wrote:
>
> > Brandon Williams wrote:
> > > On 06/12, Jonathan Nieder wrote:
> >
> > >> Alternatively, could this patch rename git_config_with_options? That
> > >> way any other patch in flight that calls git_config_with_options would
> > >> conflict with this patch, giving us an opportunity to make sure it
> > >> also sets git_dir. As another nice side benefit it would make it easy
> > >> for someone reading the patch to verify it didn't miss any callers.
> > [...]
> > > And I don't know if I agree with renaming a function just to rename it.
> >
> > I forgot to say: another way to accomplish the same thing can be to
> > reorder the function's arguments. The relevant thing is to make code
> > that calls the function without being aware of the new requirements
> > fail to compile.
>
> If the parameter is now required, then it might make sense for it to
> become an actual function parameter instead of being stuffed into the
> config_options struct. That would give you your breaking change, plus
> make it more obvious to the reader that it is not optional.
>
> The downside is that has to get shuttled around manually through the
> callstack. Most of the damage is in builtin/config.c, where we call
> git_config_with_options() a lot.
>
> include_by_gitdir is also a bit annoying, as we pass around the
> config_options struct through our void-pointer callbacks. But we can
> solve that by sticking the git_dir into the include_data struct (whose
> exact purpose is to carry the information we need to handle includes).
>
> The patch below (on top of Brandon's series does that).
I really don't understand why this has to be so difficult and why a
'breaking change' is even needed. Duy just added the 'git_dir' field to
the config_options struct in April of this year (2185fde56 config:
handle conditional include when $GIT_DIR is not set up) and now we want
to strip it out again? That's not even two months. Seems very counter
productive and makes the api more unwieldy.
>
> > >>> + if (have_git_dir())
> > >>> + opts.git_dir = get_git_common_dir();
> > >>
> > >> curious: Why get_git_common_dir() instead of get_git_dir()?
> > >
> > > Needs to be commondir since the config is stored in the common git
> > > directory and not a per worktree git directory.
> >
> > *puzzled* Why wasn't this needed before, then? The rest of the patch
> > should result in no functional change, but this part seems different.
>
> Now I'm puzzled, too. The original that got filled in lazily by the
> config functions was always get_git_dir(). I can buy the argument that
> this was a bug (I'm not familiar enough with worktree to say one way or
> the other), but if it's a fix it should definitely go into another
> patch.
Well actually... in do_git_config_sequence 'git_path("config")' is
called which will convert gitdir to commondir under the hood. you can't
use vanilla gitdir because the config isn't stored in a worktree's
gitdir but rather in the commondir as the config is shared by all
worktrees.
So maybe we actually need to add a field to the 'config_options' struct
of 'commondir' such that the commondir can be used to load the actual
config file and 'gitdir' can be used to handle the 'IncludeIf' stuff.
>
> ---
> builtin/config.c | 17 ++++++++++----
> config.c | 43 +++++++++++++++++++----------------
> config.h | 4 ++--
> 3 files changed, 37 insertions(+), 27 deletions(-)
>
> diff --git a/builtin/config.c b/builtin/config.c
> index 90f49a6ee..f5dd6f7ff 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -29,6 +29,7 @@ static int actions, types;
> static int end_null;
> static int respect_includes_opt = -1;
> static struct config_options config_options;
> +const char *config_git_dir;
> static int show_origin;
>
> #define ACTION_GET (1<<0)
> @@ -244,7 +245,9 @@ static int get_value(const char *key_, const char *regex_)
> }
>
> git_config_with_options(collect_config, &values,
> - &given_config_source, &config_options);
> + &given_config_source,
> + config_git_dir,
> + &config_options);
>
> ret = !values.nr;
>
> @@ -322,7 +325,8 @@ static void get_color(const char *var, const char *def_color)
> get_color_found = 0;
> parsed_color[0] = '\0';
> git_config_with_options(git_get_color_config, NULL,
> - &given_config_source, &config_options);
> + &given_config_source,
> + config_git_dir, &config_options);
>
> if (!get_color_found && def_color) {
> if (color_parse(def_color, parsed_color) < 0)
> @@ -354,7 +358,8 @@ static int get_colorbool(const char *var, int print)
> get_diff_color_found = -1;
> get_color_ui_found = -1;
> git_config_with_options(git_get_colorbool_config, NULL,
> - &given_config_source, &config_options);
> + &given_config_source,
> + config_git_dir, &config_options);
>
> if (get_colorbool_found < 0) {
> if (!strcmp(get_colorbool_slot, "color.diff"))
> @@ -443,7 +448,8 @@ static int get_urlmatch(const char *var, const char *url)
> }
>
> git_config_with_options(urlmatch_config_entry, &config,
> - &given_config_source, &config_options);
> + &given_config_source, config_git_dir,
> + &config_options);
>
> ret = !values.nr;
>
> @@ -540,7 +546,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> else
> config_options.respect_includes = respect_includes_opt;
> if (have_git_dir())
> - config_options.git_dir = get_git_common_dir();
> + config_git_dir = get_git_common_dir();
>
> if (end_null) {
> term = '\0';
> @@ -587,6 +593,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> check_argc(argc, 0, 0);
> if (git_config_with_options(show_all_config, NULL,
> &given_config_source,
> + config_git_dir,
> &config_options) < 0) {
> if (given_config_source.file)
> die_errno("unable to read config file '%s'",
> diff --git a/config.c b/config.c
> index 4e2842689..e1566f7b4 100644
> --- a/config.c
> +++ b/config.c
> @@ -208,21 +208,18 @@ static int prepare_include_condition_pattern(struct strbuf *pat)
> return prefix;
> }
>
> -static int include_by_gitdir(const struct config_options *opts,
> +static int include_by_gitdir(const struct config_include_data *inc,
> const char *cond, size_t cond_len, int icase)
> {
> struct strbuf text = STRBUF_INIT;
> struct strbuf pattern = STRBUF_INIT;
> int ret = 0, prefix;
> - const char *git_dir;
> int already_tried_absolute = 0;
>
> - if (opts->git_dir)
> - git_dir = opts->git_dir;
> - else
> + if (!inc->git_dir)
> goto done;
>
> - strbuf_realpath(&text, git_dir, 1);
> + strbuf_realpath(&text, inc->git_dir, 1);
> strbuf_add(&pattern, cond, cond_len);
> prefix = prepare_include_condition_pattern(&pattern);
>
> @@ -256,7 +253,7 @@ static int include_by_gitdir(const struct config_options *opts,
> * which'll do the right thing
> */
> strbuf_reset(&text);
> - strbuf_add_absolute_path(&text, git_dir);
> + strbuf_add_absolute_path(&text, inc->git_dir);
> already_tried_absolute = 1;
> goto again;
> }
> @@ -266,14 +263,14 @@ static int include_by_gitdir(const struct config_options *opts,
> return ret;
> }
>
> -static int include_condition_is_true(const struct config_options *opts,
> +static int include_condition_is_true(const struct config_include_data *inc,
> const char *cond, size_t cond_len)
> {
>
> if (skip_prefix_mem(cond, cond_len, "gitdir:", &cond, &cond_len))
> - return include_by_gitdir(opts, cond, cond_len, 0);
> + return include_by_gitdir(inc, cond, cond_len, 0);
> else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len))
> - return include_by_gitdir(opts, cond, cond_len, 1);
> + return include_by_gitdir(inc, cond, cond_len, 1);
>
> /* unknown conditionals are always false */
> return 0;
> @@ -298,7 +295,7 @@ int git_config_include(const char *var, const char *value, void *data)
> ret = handle_path_include(value, inc);
>
> if (!parse_config_key(var, "includeif", &cond, &cond_len, &key) &&
> - (cond && include_condition_is_true(inc->opts, cond, cond_len)) &&
> + (cond && include_condition_is_true(inc, cond, cond_len)) &&
> !strcmp(key, "path"))
> ret = handle_path_include(value, inc);
>
> @@ -1537,6 +1534,7 @@ int git_config_system(void)
> }
>
> static int do_git_config_sequence(const struct config_options *opts,
> + const char *git_dir,
> config_fn_t fn, void *data)
> {
> int ret = 0;
> @@ -1544,8 +1542,8 @@ static int do_git_config_sequence(const struct config_options *opts,
> char *user_config = expand_user_path("~/.gitconfig", 0);
> char *repo_config;
>
> - if (opts->git_dir)
> - repo_config = mkpathdup("%s/config", opts->git_dir);
> + if (git_dir)
> + repo_config = mkpathdup("%s/config", git_dir);
> else
> repo_config = NULL;
>
> @@ -1578,6 +1576,7 @@ static int do_git_config_sequence(const struct config_options *opts,
>
> int git_config_with_options(config_fn_t fn, void *data,
> struct git_config_source *config_source,
> + const char *git_dir,
> const struct config_options *opts)
> {
> struct config_include_data inc = CONFIG_INCLUDE_INIT;
> @@ -1585,7 +1584,7 @@ int git_config_with_options(config_fn_t fn, void *data,
> if (opts->respect_includes) {
> inc.fn = fn;
> inc.data = data;
> - inc.opts = opts;
> + inc.git_dir = git_dir;
> fn = git_config_include;
> data = &inc;
> }
> @@ -1601,17 +1600,18 @@ int git_config_with_options(config_fn_t fn, void *data,
> else if (config_source && config_source->blob)
> return git_config_from_blob_ref(fn, config_source->blob, data);
>
> - return do_git_config_sequence(opts, fn, data);
> + return do_git_config_sequence(opts, git_dir, fn, data);
> }
>
> static void git_config_raw(config_fn_t fn, void *data)
> {
> struct config_options opts = {0};
> + const char *git_dir;
>
> opts.respect_includes = 1;
> if (have_git_dir())
> - opts.git_dir = get_git_common_dir();
> - if (git_config_with_options(fn, data, NULL, &opts) < 0)
> + git_dir = get_git_common_dir();
> + if (git_config_with_options(fn, data, NULL, git_dir, &opts) < 0)
> /*
> * git_config_with_options() normally returns only
> * zero, as most errors are fatal, and
> @@ -1653,11 +1653,12 @@ void read_early_config(config_fn_t cb, void *data)
> {
> struct config_options opts = {0};
> struct strbuf buf = STRBUF_INIT;
> + const char *git_dir;
>
> opts.respect_includes = 1;
>
> if (have_git_dir())
> - opts.git_dir = get_git_dir();
> + git_dir = get_git_dir();
> /*
> * When setup_git_directory() was not yet asked to discover the
> * GIT_DIR, we ask discover_git_directory() to figure out whether there
> @@ -1667,9 +1668,11 @@ void read_early_config(config_fn_t cb, void *data)
> * call).
> */
> else if (discover_git_directory(&buf))
> - opts.git_dir = buf.buf;
> + git_dir = buf.buf;
> + else
> + git_dir = NULL;
>
> - git_config_with_options(cb, data, NULL, &opts);
> + git_config_with_options(cb, data, NULL, git_dir, &opts);
>
> strbuf_release(&buf);
> }
> diff --git a/config.h b/config.h
> index c70599bd5..47a8e8845 100644
> --- a/config.h
> +++ b/config.h
> @@ -30,7 +30,6 @@ enum config_origin_type {
>
> struct config_options {
> unsigned int respect_includes : 1;
> - const char *git_dir;
> };
>
> typedef int (*config_fn_t)(const char *, const char *, void *);
> @@ -46,6 +45,7 @@ extern void read_early_config(config_fn_t cb, void *data);
> extern void git_config(config_fn_t fn, void *);
> extern int git_config_with_options(config_fn_t fn, void *,
> struct git_config_source *config_source,
> + const char *git_dir,
> const struct config_options *opts);
> extern int git_parse_ulong(const char *, unsigned long *);
> extern int git_parse_maybe_bool(const char *);
> @@ -97,7 +97,7 @@ struct config_include_data {
> int depth;
> config_fn_t fn;
> void *data;
> - const struct config_options *opts;
> + const char *git_dir;
> };
> #define CONFIG_INCLUDE_INIT { 0 }
> extern int git_config_include(const char *name, const char *value, void *data);
--
Brandon Williams