Re: [PATCHv3] Read from XDG configuration file, not write

[Junio C Hamano <gitster@pobox.com>]

Huynh Khoi Nguyen NGUYEN  <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
writes:

> From: NGUYEN Huynh Khoi Nguyen <nguyenhu@ensibm.imag.fr>
>
> Git will be able to read in $XDG_CONFIG_HOME/git/config, a new

Getting nicer.

> @@ -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);
	}


> 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
> +'