Re: [PATCH v2 1/8] config: Trivial rename in preparation for parseopt.

Felipe Contreras [off-list ref] writes:

Does the author of 93a56c2 (git-config: print error message if the config
file cannot be read, 2007-10-12) have any comment on this change (cc:ed)?

On Tue, Feb 17, 2009 at 3:45 AM, Junio C Hamano [off-list ref] wrote:

Felipe Contreras [off-list ref] writes:

quoted

When using the --list option general errors where not properly reported,
only errors related with the 'file'. Now they are reported, and 'file'
is irrelevant.
...

@@ -299,10 +300,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
              else if (!strcmp(argv[1], "--list") || !strcmp(argv[1], "-l")) {
                      if (argc != 2)
                              usage(git_config_set_usage);
-                     if (git_config(show_all_config, NULL) < 0 &&
-                                     file && errno)
-                             die("unable to read config file %s: %s", file,
-                                 strerror(errno));
+                     if (git_config(show_all_config, NULL) < 0)
+                             die("error processing config file(s)");

Does the author of 93a56c2 (git-config: print error message if the config
file cannot be read, 2007-10-12) have any comment on this change (cc:ed)?

I looked at the debian bug report[1], the guy complains about 2 things:

1) git-config --file fails silently if the filename isn't absolute

This is still fixed.

2) git-config -l --file doesn't do what you might expect and list the
contents of the specified file. Instead it ignores the --file option since it
came after the -l. Nice way to shoot oneself in the foot.

This is now fixed after the parseopt patch.

Also, before this patch 'git config --global -l' would fail silently
if there isn't any ~/.gitconfig. Now at least git reports "error
processing config file(s)".

A more ideal solution would be:

if (config_exclusive_filename)
	die("unable to read config file %s: %s",
	    config_exclusive_filename, strerror(errno));
else
	die("error processing config file(s)");

So, if a file is specified with --file, --global, or --system, then
the correct error would be reported.

I digged a bit more and it turns out if there's parse error
git_config() will die immediately, and the only time it will return a
negative value is when the config file(s) are not present, at which
point there will be an errno set, and when config_exclusive_filename
is specified that means the errno will be the one of fopen trying to
open that file.

Still, "error processing config file(s)" will be reported when no file
is specified 'git config -l', there isn't any repo config file (cwd is
not in a git repo).

Even better I think would be to allow 'git config -l' to work even if
we are not in a git repo, and error only when there isn't any config
file (repo, system or global).

This is how it would look like:

 int git_config(config_fn_t fn, void *data)
 {
-       int ret = 0;
+       int ret = 0, found = 0;
        char *repo_config = NULL;
        const char *home = NULL;

        /* Setting $GIT_CONFIG makes git read _only_ the given config file. */
        if (config_exclusive_filename)
                return git_config_from_file(fn,
config_exclusive_filename, data);
-       if (git_config_system() && !access(git_etc_gitconfig(), R_OK))
+       if (git_config_system() && !access(git_etc_gitconfig(), R_OK)) {
                ret += git_config_from_file(fn, git_etc_gitconfig(),
                                            data);
+               found += 1;
+       }

        home = getenv("HOME");
        if (git_config_global() && home) {
                char *user_config = xstrdup(mkpath("%s/.gitconfig", home));
-               if (!access(user_config, R_OK))
+               if (!access(user_config, R_OK)) {
                        ret += git_config_from_file(fn, user_config, data);
+                       found += 1;
+               }
                free(user_config);
        }

        repo_config = git_pathdup("config");
-       ret += git_config_from_file(fn, repo_config, data);
+       if (!access(repo_config, R_OK)) {
+               ret += git_config_from_file(fn, repo_config, data);
+               found += 1;
+       }
        free(repo_config);
+       if (found == 0)
+               error("no config file found");
        return ret;
 }

[1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=445208

-- 
Felipe Contreras

On Mon, Feb 16, 2009 at 05:45:00PM -0800, Junio C Hamano wrote:

Felipe Contreras [off-list ref] writes:

quoted

When using the --list option general errors where not properly reported,
only errors related with the 'file'. Now they are reported, and 'file'
is irrelevant.
...

@@ -299,10 +300,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		else if (!strcmp(argv[1], "--list") || !strcmp(argv[1], "-l")) {
 			if (argc != 2)
 				usage(git_config_set_usage);
-			if (git_config(show_all_config, NULL) < 0 &&
-					file && errno)
-				die("unable to read config file %s: %s", file,
-				    strerror(errno));
+			if (git_config(show_all_config, NULL) < 0)
+				die("error processing config file(s)");

Does the author of 93a56c2 (git-config: print error message if the config
file cannot be read, 2007-10-12) have any comment on this change (cc:ed)?

Hm, we lose some information from the error message when called with
--file, but it seems to improve on other cases.  The filename doesn't
matter that much, but it would be nice to know the reason.  I wouldn't
object against this hunk though if that isn't possible.

Regards, Gerrit.

Hi,

On Tue, 17 Feb 2009, Gerrit Pape wrote:

On Mon, Feb 16, 2009 at 05:45:00PM -0800, Junio C Hamano wrote:

quoted

Felipe Contreras [off-list ref] writes:

quoted

When using the --list option general errors where not properly reported,
only errors related with the 'file'. Now they are reported, and 'file'
is irrelevant.
...

@@ -299,10 +300,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		else if (!strcmp(argv[1], "--list") || !strcmp(argv[1], "-l")) {
 			if (argc != 2)
 				usage(git_config_set_usage);
-			if (git_config(show_all_config, NULL) < 0 &&
-					file && errno)
-				die("unable to read config file %s: %s", file,
-				    strerror(errno));
+			if (git_config(show_all_config, NULL) < 0)
+				die("error processing config file(s)");

Does the author of 93a56c2 (git-config: print error message if the config
file cannot be read, 2007-10-12) have any comment on this change (cc:ed)?

Hm, we lose some information from the error message when called with
--file, but it seems to improve on other cases.  The filename doesn't
matter that much, but it would be nice to know the reason.  I wouldn't
object against this hunk though if that isn't possible.

The point is: when _not_ using --file, the output could be wrong 
(mentioning another config file than the one having an issue), or not be 
shown at all -- I haven't checked, but both options to not look good to 
me.

Ciao,
Dscho

Hi,

On Tue, 17 Feb 2009, Felipe Contreras wrote:

        free(repo_config);
+       if (found == 0)
+               error("no config file found");
        return ret;

Err, you mean "return error(..)"?

But it might be not an error at all: think of "cd / && git ls-remote $URL" 
without /etc/gitconfig nor $HOME/.gitconfig.

Ciao,
Dscho

On Tue, Feb 17, 2009 at 2:01 PM, Johannes Schindelin
[off-list ref] wrote:

Hi,

On Tue, 17 Feb 2009, Felipe Contreras wrote:

quoted

        free(repo_config);
+       if (found == 0)
+               error("no config file found");
        return ret;

Err, you mean "return error(..)"?

But it might be not an error at all: think of "cd / && git ls-remote $URL"
without /etc/gitconfig nor $HOME/.gitconfig.

Yeah, I thought about that when doing the patch. I've removed the
error(), now it will only return -1. Almost nobody is checking the
return code anyways.

-- 
Felipe Contreras