Re: bash_completion outside repo

[Jeff King <peff@peff.net>]

On Fri, Sep 11, 2009 at 11:09:34AM -0400, Jeff King wrote:

> Assuming you make such a patch, that will clear up the original issue. I
> wonder if we should fix "git config --list". The current semantics seem
> a little crazy to me, but it is a scriptable interface. I'm inclined to
> call this a bug, though.

And here is a patch to fix it.

-- >8 --
Subject: [PATCH] config: treat non-existent config files as empty

The git_config() function signals error by returning -1 in
two instances:

  1. An actual error occurs in opening a config file (parse
     errors cause an immediate die).

  2. Of the three possible config files, none was found.

However, this second case is often not an error at all; it
simply means that the user has no configuration (they are
outside a repo, and they have no ~/.gitconfig file). This
can lead to confusing errors, such as when the bash
completion calls "git config --list" outside of a repo. If
the user has a ~/.gitconfig, the command completes
succesfully; if they do not, it complains to stderr.

This patch allows callers of git_config to distinguish
between the two cases. Error is signaled by -1, and
otherwise the return value is the number of files parsed.
This means that the traditional "git_config(...) < 0" check
for error should work, but callers who want to know whether
we parsed any files or not can still do so.

We need to tweak one use of git_config in builtin-remote
that previously assumed the return value was either '0' or
'-1'.

Signed-off-by: Jeff King <peff@peff.net>
---

This is actually a bit overengineered. Of the hundreds of calls to
git_config, there are exactly _two_ which check the return value. And
neither of them cares whether we parsed files or not; they really only
care if there was an error. So we could simply return 0 as long as there
is no error.

This also makes me wonder, though. Git can do wildly different things
(including hard-to-reverse things) based on config (e.g., just consider
gc.pruneExpire). Yet we call git_config() without ever checking for
errors. In the actual parsing routines, we die() if there is an error.
But if we fail to open the file due to some transient error, we will
silently ignore the situation.

Granted, such transient errors are unlikely. The biggest reasons for
failing to open a file are that it doesn't exist, or that we have no
permission to read it, both of which are treated explicitly in
git_config as "silently ok". But I wonder if we should simply be dying
on such an error, and git_config() should just have a void return.

 builtin-remote.c       |    3 ++-
 config.c               |    4 +---
 t/t1300-repo-config.sh |    8 ++++++++
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index 0777dd7..3bf1fe8 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -1245,7 +1245,8 @@ static int update(int argc, const char **argv)
 	for (i = 1; i < argc; i++) {
 		int groups_found = 0;
 		remote_group.name = argv[i];
-		result = git_config(get_remote_group, &groups_found);
+		if (git_config(get_remote_group, &groups_found) < 0)
+			result = -1;
 		if (!groups_found && (i != 1 || strcmp(argv[1], "default"))) {
 			struct remote *remote;
 			if (!remote_is_configured(argv[i]))
diff --git a/config.c b/config.c
index e87edea..e429674 100644
--- a/config.c
+++ b/config.c
@@ -709,9 +709,7 @@ int git_config(config_fn_t fn, void *data)
 		found += 1;
 	}
 	free(repo_config);
-	if (found == 0)
-		return -1;
-	return ret;
+	return ret == 0 ? found : ret;
 }
 
 /*
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 83b7294..db987b7 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -289,6 +289,14 @@ test_expect_success 'working --list' \
 	'git config --list > output && cmp output expect'
 
 cat > expect << EOF
+EOF
+
+test_expect_success '--list without repo produces empty output' '
+	git --git-dir=nonexistent config --list >output &&
+	test_cmp expect output
+'
+
+cat > expect << EOF
 beta.noindent sillyValue
 nextsection.nonewline wow2 for me
 EOF
-- 
1.6.5.rc0.174.g29a6d.dirty