Re: [PATCHv2] Possibility to read both from ~/.gitconfig and from $XDG_CONFIG_HOME/git/config
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:53:56
Huynh Khoi Nguyen NGUYEN [off-list ref] writes:
From: NGUYEN Huynh Khoi Nguyen <redacted> Git will read both in $XDG_CONFIG_HOME/git/config and in ~/.gitconfig in this order: .git/config > ~/.gitconfig > $XDG_CONFIG_HOME/git/config > /etc/gitconfig If $XDG_CONFIG_HOME is either not set or empty, $HOME/.config/git/config will be used.
Is it just me who finds the above three lines extremely unreadable? Also can you give this patch a bit more sensible title? "Possibility to" does not tell us much---anything is possible if you change code after all. I see the patch does not touch the writing codepath, which is probably a good thing, but the log message should explicitly state that.
quoted hunk
@@ -194,7 +194,7 @@ See also <<FILES>>. FILES ----- -If not set explicitly with '--file', there are three files where +If not set explicitly with '--file', there are four files where 'git config' will search for configuration options: $GIT_DIR/config::@@ -204,6 +204,9 @@ $GIT_DIR/config:: User-specific configuration file. Also called "global" configuration file. +$XDG_CONFIG_HOME/git/config:: + Second user-specific configuration file. ~/.gitconfig has priority. +
I am not sure in what way $HOME/.gitconfig has "priority". Your proposed log message says that You read from $HOME/.gitconfig and then from $XDG_CONFIG_HOME/git/config, which means that any single-valued variable set in $HOME/.gitconfig will be overwritten by whatever is in $XDG_CONFIG_HOME/git/config, no? That sounds like you are giving priority to the latter to me. And for multi-valued variables, settings from both files are read, so there isn't much inherent priority between the two, except for variables for which the definition order matters, of course. If you read only from $HOME/.gitconfig if exists, and read from $XDG_CONFIG_HOME/git/config only when $HOME/.gitconfig does not, then you are giving $HOME/.gitconfig a priority, but that is not what the patch is doing as far as I can tell.
quoted hunk
diff --git a/builtin/config.c b/builtin/config.c index 33c8820..38dba4f 100644 --- a/builtin/config.c +++ b/builtin/config.c@@ -161,7 +161,7 @@ static int show_config(const char *key_, const char *value_, void *cb) static int get_value(const char *key_, const char *regex_) { int ret = -1; - char *global = NULL, *repo_config = NULL; + char *gitconfig_global = NULL, *xdg_global = NULL, *repo_config = NULL; const char *system_wide = NULL, *local; struct config_include_data inc = CONFIG_INCLUDE_INIT; config_fn_t fn;@@ -171,8 +171,15 @@ static int get_value(const char *key_, const char *regex_) if (!local) { const char *home = getenv("HOME"); local = repo_config = git_pathdup("config"); - if (home) - global = xstrdup(mkpath("%s/.gitconfig", home)); + if (home) { + const char *xdg_config_home = getenv("XDG_CONFIG_HOME"); + if (xdg_config_home)
This is logically wrong; even when you fail to read $HOME, you may
be able to read $XDG_CONFIG_HOME, no? It shouldn't be nested inside
"if (home)" at all, methinks.
It would be more like
global = xdg_global = NULL;
if (HOME exists?)
global = $HOME/.gitconfig
if (XDG_CONFIG_HOME exists?)
xdg_global = $XDG_CONFIG_HOME/git/config
else if (HOME exists?)
xdg_global = $HOME/.config/git/config
no?
quoted hunk
@@ -381,7 +393,25 @@ int cmd_config(int argc, const char **argv, const char *prefix) if (use_global_config) { char *home = getenv("HOME"); if (home) { - char *user_config = xstrdup(mkpath("%s/.gitconfig", home)); + char *user_config; + const char *gitconfig_path = mkpath("%s/.gitconfig", home); + const char *xdg_config_path = NULL; + const char *xdg_config_home = NULL; + + xdg_config_home = getenv("XDG_CONFIG_HOME"); + if (xdg_config_home) + xdg_config_path = mkpath("%s/git/config", xdg_config_home); + else + xdg_config_path = mkpath("%s/.config/git/config", home); + + if (access(gitconfig_path, R_OK) && !access(xdg_config_path, R_OK) && + (actions == ACTION_LIST || + actions == ACTION_GET_COLOR || + actions == ACTION_GET_COLORBOOL)) + user_config = xstrdup(xdg_config_path); + else + user_config = xstrdup(gitconfig_path); + given_config_file = user_config; } else { die("$HOME not set");
Exactly the same comment applies here. You seem to always write to $HOME/.gitconfig, so missing $HOME may be an error if the action is to store, but if you are reading and if $XDG_CONFIG_HOME is set, you do not have to have $HOME set, no? Even when there is $HOME, if there is no $HOME/.gitconfig file, you wouldn't want to give an error, so missing $HOME environment should be treated pretty much the same way as missing $HOME/.gitconfig file for the purpose of reading, no?
quoted hunk
diff --git a/config.c b/config.c index 71ef171..53557dc 100644 --- a/config.c +++ b/config.c@@ -939,10 +939,23 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config) home = getenv("HOME"); if (home) { - char buf[PATH_MAX]; - char *user_config = mksnpath(buf, sizeof(buf), "%s/.gitconfig", home); - if (!access(user_config, R_OK)) { - ret += git_config_from_file(fn, user_config, data); + const char *gitconfig_path = xstrdup(mkpath("%s/.gitconfig", home)); + const char *xdg_config_path = NULL; + const char *xdg_config_home = NULL; + + xdg_config_home = getenv("XDG_CONFIG_HOME"); + if (xdg_config_home) + xdg_config_path = xstrdup(mkpath("%s/git/config", xdg_config_home)); + else + xdg_config_path = xstrdup(mkpath("%s/.config/git/config", home));
Exactly the same comment applies here, too.
The original that read from $HOME/.gitconfig was simple enough so
having three copies of getenv("HOME") was perfectly fine, but as you
are introduce this much complexity to to decide which two files to
read from, the code added this patch needs to be refactored and
three copies of the same logic need to be consolidated, I would have
to say.