Re: [PATCH] config: fix several access(NULL) calls

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 16, 2012, 16:35 UTC
Message-ID: <7vipdn4qbk.fsf@alter.siamese.dyndns.org>
In-Reply-To: <vpqmx3011la.fsf@bauges.imag.fr>

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

Show 11 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> 	if (use_global_config) {
>>                 if (is $HOME/.gitconfig usable?) {
>> 			use it;
>
> Yes, but when $HOME is unset, the question doesn't really make sense.
> Maybe the file exists, but we can't know since the user broke his
> configuration by unsetting $HOME. The intent was to avoid writing to the
> XDG file unless it was very clear that the user wanted it, so in doubt,
> dying seems the best option.

I would think that it is plausible that the user wanted to write into XDG one and used "unset HOME" as a way to signal that wish.

Are there ways to force writing into XDG ones without having to remove the $HOME/ ones (perhaps the user wants to keep them for use with older versions of Git on a different machine that shares the same $HOME directory)? Temporarily unsetting HOME may be how a user might achieve it.

If you want to disallow such a use case, that is fine, but at least the logic needs to be described in comment. Perhaps based on one of your rewrites earlier in the thread, like this?

 builtin/config.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 67945b2..a788409 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -387,13 +387,26 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 
 		home_config_paths(&user_config, &xdg_config, "config");
 
-		if (user_config && access(user_config, R_OK) &&
+		if (!user_config)
+			/*
+			 * We do not know HOME/.gitconfig exists or
+			 * not, hence we do not know if we should
+			 * write to XDG location, so we error out even
+			 * if XDG_CONFIG_HOME is set and points at a
+			 * sane location.
+			 *
+			 * In other words, we forbid the user from
+			 * telling us to write to XDG location,
+			 * pretending that $HOME/.gitconfig does not
+			 * exist, by temporarily unsetting HOME.
+			 */
+			die("$HOME not set");
+
+		if (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");
+			given_config_file = user_config;
 	}
 	else if (use_system_config)
 		given_config_file = git_etc_gitconfig();