[PATCH 1/2] remote.c: silently tolerate single-level keys like remote.default

[PATCH 1/2] remote.c: silently tolerate single-level keys like remote.default

[Johannes Sixt]

If the config file contains a section like this:

    [remote]
            default = foo

(it should be '[remotes]') then commands like

    git status
    git checkout
    git branch -v

fail even though they are not obviously related to remotes. (These commands
write "ahead, behind" information and, therefore, access the configuration
of remotes.)

Typos in configuration keys usually do not hurt because they never match
in look-ups. But this case is different: it does match, but it does not
have the expected format. With this patch this situation is treated more
like a typo.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 remote.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/remote.c b/remote.c
index 91f7485..d66e2f3 100644
--- a/remote.c
+++ b/remote.c
@@ -366,7 +366,7 @@ static int handle_config(const char *key, const char *value, void *cb)
 	}
 	subkey = strrchr(name, '.');
 	if (!subkey)
-		return error("Config with no key for remote %s", name);
+		return 0;
 	remote = make_remote(name, subkey - name);
 	remote->origin = REMOTE_CONFIG;
 	if (!strcmp(subkey, ".mirror"))
-- 
1.6.3.rc1.88.g1bf9

[PATCH 2/2] builtin-help: silently tolerate unknown keys

[Johannes Sixt]

If for some reason the config file contains a key without a subkey like

    [man]
        foo = bar

then even a plain

    git help

produces an error message. With this patch such an entry is ignored.

Additionally, the warning about unknown sub-keys is removed. It could
become annoying if new sub-keys are introduced in the future, and then
the configuration is read by an old version of git that does not know
about it.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 builtin-help.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/builtin-help.c b/builtin-help.c
index 9b57a74..e7fbe9a 100644
--- a/builtin-help.c
+++ b/builtin-help.c
@@ -236,7 +236,7 @@ static int add_man_viewer_info(const char *var, const char *value)
 	const char *subkey = strrchr(name, '.');
 
 	if (!subkey)
-		return error("Config with no key for man viewer: %s", name);
+		return 0;
 
 	if (!strcmp(subkey, ".path")) {
 		if (!value)
@@ -249,7 +249,6 @@ static int add_man_viewer_info(const char *var, const char *value)
 		return add_man_viewer_cmd(name, subkey - name, value);
 	}
 
-	warning("'%s': unsupported man viewer sub key.", subkey);
 	return 0;
 }
 
-- 
1.6.3.rc1.88.g1bf9

Re: [PATCH 1/2] remote.c: silently tolerate single-level keys like remote.default

[Jeff King]

On Thu, Apr 23, 2009 at 03:49:05PM +0200, Johannes Sixt wrote:

> Typos in configuration keys usually do not hurt because they never match
> in look-ups. But this case is different: it does match, but it does not
> have the expected format. With this patch this situation is treated more
> like a typo.

I definitely think causing the command to fail is bad, but should we
perhaps still warn the user? I know that we can't catch _every_ typo in
the config, but if there is something obviously wrong that we've
detected, it is nice to let the user know.

> -		return error("Config with no key for remote %s", name);
> +		return 0;

IOW,

  + warn("Config with no key for remote %s", name);
  + return 0;

-Peff

Re: [PATCH 1/2] remote.c: silently tolerate single-level keys like remote.default

[Johannes Sixt]

On Donnerstag, 23. April 2009, Jeff King wrote:
> On Thu, Apr 23, 2009 at 03:49:05PM +0200, Johannes Sixt wrote:
> > Typos in configuration keys usually do not hurt because they never match
> > in look-ups. But this case is different: it does match, but it does not
> > have the expected format. With this patch this situation is treated more
> > like a typo.
>
> I definitely think causing the command to fail is bad, but should we
> perhaps still warn the user? I know that we can't catch _every_ typo in
> the config, but if there is something obviously wrong that we've
> detected, it is nice to let the user know.
>
> > -		return error("Config with no key for remote %s", name);
> > +		return 0;
>
> IOW,
>
>   + warn("Config with no key for remote %s", name);
>   + return 0;

I don't like this. This would warn in a number of situations where it's not 
obvious that remotes are involved, for example in 'git status'.

Also observe (and I forgot to mention that in the commit message) that this 
same exit

   if (!subkey)
        return 0;

is also taken for url.foo and branch.foo configurations a few lines above this 
change.

-- Hannes

Re: [PATCH 1/2] remote.c: silently tolerate single-level keys like remote.default

[Jeff King]

On Thu, Apr 23, 2009 at 08:37:26PM +0200, Johannes Sixt wrote:

> >   + warn("Config with no key for remote %s", name);
> >   + return 0;
> 
> I don't like this. This would warn in a number of situations where it's not 
> obvious that remotes are involved, for example in 'git status'.

Ah, I didn't think of that (I didn't really look too closely into the
affected code, and you obviously have). So let me withdraw my comment.

-Peff

Re: [PATCH 1/2] remote.c: silently tolerate single-level keys like remote.default

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> If the config file contains a section like this:
>
>     [remote]
>             default = foo
>
> (it should be '[remotes]') then commands like
>
>     git status
>     git checkout
>     git branch -v
>
> fail even though they are not obviously related to remotes. (These commands
> write "ahead, behind" information and, therefore, access the configuration
> of remotes.)
>
> Typos in configuration keys usually do not hurt because they never match
> in look-ups. But this case is different: it does match, but it does not
> have the expected format. With this patch this situation is treated more
> like a typo.

I agree with the patch text, but we should realize that this is not
"silently tolerating typos".  The existing per-remote variables such as
url, proxy, fetch, etc. are defined to be, eh, per-remote, and by
definition require three-level syntax.  There is nothing that prevents us
from actually fixing the stupid mistake that is "remotes.default = foo"
and make it "remote.default = foo".

To avoid confusion, we might want to deprecate remotes.default and make it
remotegroup.default or something like that, though.