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

Junio C Hamano [off-list ref] writes:

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

Anyone else have an opinion on this?

In short, the question is whether

  export XDG_CONFIG_HOME=some-existing-dir
  unset HOME
  git config foo.baz boz

should die("$HOME is unset") or use the XDG config file.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

When $HOME is unset, home_config_paths fails and returns NULL pointers
for user_config and xdg_config. Valgrind complains with Syscall param
access(pathname) points to unaddressable byte(s).

Don't call blindly access() on these variables, but test them for
NULL-ness before.

The when the XDG configuration file can be found but not $HOME/.gitconfig
requires a bit of attention. We chose to error out in "git config --set"
if $HOME is unset anyway.

Signed-off-by: Matthieu Moy <redacted>
---

Before I forget about it, here's the patch assuming people do want to
error out when $HOME is unset. It should be functionally equivalent to
the previous one, but the code should be clearer.

 builtin/config.c | 15 +++++++++++----
 config.c         |  4 ++--
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index e8e1c0a..f064d6e 100644
--- a/builtin/config.c
+++ b/builtin/config.c

@@ -387,12 +387,19 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 
 		home_config_paths(&user_config, &xdg_config, "config");
 
-		if (access(user_config, R_OK) && !access(xdg_config, R_OK))
+		if (!user_config)
+			/*
+			 * Don't even try to access the xdg_config, as
+			 * unset $HOME means something is really
+			 * broken and should be fixed. Silently
+			 * writing to xdg_config may be confusing.
+			 */
+			die("$HOME not set");
+		else 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();

diff --git a/config.c b/config.c
index d28a499..6b97503 100644
--- a/config.c
+++ b/config.c

@@ -940,12 +940,12 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 		found += 1;
 	}
 
-	if (!access(xdg_config, R_OK)) {
+	if (xdg_config && !access(xdg_config, R_OK)) {
 		ret += git_config_from_file(fn, xdg_config, data);
 		found += 1;
 	}
 
-	if (!access(user_config, R_OK)) {
+	if (user_config && !access(user_config, R_OK)) {
 		ret += git_config_from_file(fn, user_config, data);
 		found += 1;
 	}

-- 
1.7.11.1.30.g7e1baf9.dirty

On Fri, Jul 13, 2012 at 10:48:18AM +0200, Matthieu Moy wrote:

Junio C Hamano [off-list ref] writes:

quoted

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

Anyone else have an opinion on this?

In short, the question is whether

  export XDG_CONFIG_HOME=some-existing-dir
  unset HOME
  git config foo.baz boz

should die("$HOME is unset") or use the XDG config file.

What did previous versions of git do? From my reading of 21cf32279, the
previous behavior was that if $HOME was not set, git would silently
avoid reading from $HOME/.gitconfig entirely. Wouldn't dying be a huge
regression?

-Peff