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).
> >>> + 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.
---
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);