From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 12 Jul 2012 20:12:37 GMT
Subject: Re: [PATCH] config: fix several access(NULL) calls
Message-ID: <7vwr28agcq.fsf@alter.siamese.dyndns.org>
In-Reply-To: <vpq1ukgai4e.fsf@bauges.imag.fr>

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

>>> +		if (user_config && access(user_config, R_OK) &&
>>> +		    xdg_config && !access(xdg_config, R_OK))
>>>  			given_config_file = xdg_config;
>>
>> Shouldn't we be using xdg_config, if user_config is NULL and
>> xdg_config is defined and accessible?
>
> I don't think so. If user_config is NULL, it means something went wrong,
> because $HOME is unset. In this case, I'd rather die than using some
> other configuration file silently (which would be possible if
> $XDG_CONFIG_HOME is set), and this is what the code does:
>
> 		if (user_config && access(user_config, R_OK) &&
> 		    xdg_config && !access(xdg_config, R_OK))
> 			given_config_file = xdg_config;
> 		else if (user_config)
> 			given_config_file = user_config;
> 		else
> 			die("$HOME not set");
>
> Perhaps it could actually be made even clearer with
>
> 		if (!user_config)
> 			die("$HOME not set");
> 		else if (access(user_config, R_OK) &&
> 			 xdg_config && !access(xdg_config, R_OK))
> 			given_config_file = xdg_config;
> 		else
> 			given_config_file = user_config;

At least the code needs to break lines at different point and a comment.

	if (user_config &&
		access(user_config, R_OK) &&
                (xdg_config && !access(xdg_config, R_OK)))
                /*
                 * Even if we have usable XDG stuff, we want to
		 * error out on missing HOME!!!
                 */
		use xdg config;
	else if (user_config)
		use user config;
	else
		unusable situation;

But is it really true that we want to error out on missing HOME if
we have usable XDG stuff?