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

From: Junio C Hamano <gitster@pobox.com>
Date: May 31, 2012, 20:13 UTC
Message-ID: <7vr4u05ct0.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1338475242-21770-1-git-send-email-Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>

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.

Show 11 quoted lines

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

Show 25 quoted lines

> 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.

Show 5 quoted lines

> +	#
> +	# 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.

Show 5 quoted lines

> +	nothing added to commit but untracked files present
> +	EOF
> +	git status >output &&
> +	test_cmp expect output
> +'