From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Date: Thu, 31 May 2012 22:06:48 GMT
Subject: Re: [PATCHv2] Possibility to read both from ~/.gitconfig and from $XDG_CONFIG_HOME/git/config
Message-ID: <4FC7EB78.5090903@ramsay1.demon.co.uk>
In-Reply-To: <7vr4u1xrkp.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:
> Huynh Khoi Nguyen NGUYEN  <Huynh-Khoi-Nguyen.Nguyen@ensimag.imag.fr>
[...]

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

I have not tried this patch (or the v3 version, which I haven't read yet), but
is it likely that this has re-introduced the bug addressed by commit 05bab3ea
("config.c: Fix a static buffer overwrite bug by avoiding mkpath()", 19-11-2011)?.

I don't know the answer, but I suspect that it may have done just that. (indeed, it
may well have made the bug more likely to appear).

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

I agree. Also, using mksnpath() in the refactored code (rather than
mkpath()) would be a good idea. :-P

ATB,
Ramsay Jones