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

[Felipe Contreras <felipe.contreras@gmail.com>]

On Tue, Feb 17, 2009 at 3:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> 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