Re: [PATCHv3] Read from XDG configuration file, not write
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:53:57
Huynh Khoi Nguyen NGUYEN [off-list ref] writes:
From: NGUYEN Huynh Khoi Nguyen <redacted> Git will be able to read in $XDG_CONFIG_HOME/git/config, a new
Getting nicer.
quoted hunk
@@ -172,7 +172,9 @@ static int get_value(const char *key_, const char *regex_) const char *home = getenv("HOME"); local = repo_config = git_pathdup("config"); if (home) - global = xstrdup(mkpath("%s/.gitconfig", home)); + gitconfig_global = xstrdup(mkpath("%s/.gitconfig", home)); + if (xdg_git_path("config")) + xdg_global = xdg_git_path("config"); if (git_config_system()) system_wide = git_etc_gitconfig(); }
Hrm, xdg_git_path() returns allocated memory, and each call site
leaks its return value, no?
I didn't mean a micro-helper function like xdg_git_path() when I
suggested refactoring. I meant a helper that figures out all the
necessary bits in one go. For example, can't the above call site
look more like this?
static int get_value(const char *key_, const char *regex_)
{
int ret = -1;
char *global = NULL, *xdg = NULL, *repo_config = NULL;
const char *system_wide = NULL, *local;
struct config_include_data inc = CONFIG_INCLUDE_INIT;
config_fn_t fn;
void *data;
local = given_config_file;
if (!local) {
local = repo_config = git_pathdup("config");
if (git_config_system())
system_wide = git_etc_gitconfig();
home_config_paths(&global, &xdg);
}
...
And then the config.c::home_config_paths() may look like:
void home_config_paths(char **global, char **xdg)
{
char *xdg_home = getenv("XDG_CONFIG_HOME");
char *home = getenv("HOME");
char *to_free = NULL;
if (!home) {
*global = NULL;
} else {
if (!xdg_home) {
to_free = strdup(mkpath("%s/.config", home));
xdg_home = to_free;
}
*global = xstrdup(mkpath("%s/.gitconfig", home));
}
if (!xdg_home)
*xdg = NULL;
else
*xdg = xstrdup(mkpath("%s/git/config", xdg_home));
free(to_free);
}
quoted hunk
diff --git a/t/t1306-read-xdg-config-file.sh b/t/t1306-read-xdg-config-file.sh new file mode 100755 index 0000000..4cab38b --- /dev/null +++ b/t/t1306-read-xdg-config-file.sh@@ -0,0 +1,87 @@ +#!/bin/sh +# +# Copyright (c) 2012 Valentin Duperray, Lucien Kong, Franck Jonas, +# Thomas Nguy, Khoi Nguyen +# Grenoble INP Ensimag +# + +test_description='possibility to read $XDG_CONFIG_HOME/git/config file' + +. ./test-lib.sh + +test_expect_success 'read automatically: xdg file exists and ~/.gitconfig doesn'\''t' ' + mkdir -p .config/git && + echo "[advice]" >.config/git/config && + echo " statushints = false" >>.config/git/config && + cat >expect <<-\EOF && + # On branch master + # + # Initial commit
Use of "status" output as test vector is not such a good idea for multiple reasons. - You are aware that the "status hints" feature is currently in flux (somebody nearby is working on a patch); - Even without another topic that currently works on it, "git status" output is for human consumption and subject to improvement and also l10n. - "status" output lists tracked and untracked paths, which makes it harder to later introduce new file to the test directory that are needed for later tests, or rename ones that were used in earlier versions of the test, without having to update all of these test vectors.
+ # + # Untracked files: + # .config/ + # expect + # output
This is a good example that shows why using "status" output as a test vector is not a good idea. Notice you listed "expect" and "output" here. As my next suggestion is to rename "output" to "actual" (we typically "test_cmp expect actual"), use of "git status" output as a test vector would mean you would have to update them whenever such a change is needed.
+ nothing added to commit but untracked files present + EOF + git status >output && + test_cmp expect output +'