threads / patch / 19024

patch, 2 partsremote.c: silently tolerate single-level keys like remote.default

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

## tl;dr

6 messages between Apr 23, 2009 and Apr 24, 2009. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Johannes Sixt· Apr 23, 2009, 13:49 UTC · lore
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(-)
Show changes to remote.c +1 −1
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
Johannes Sixt· Apr 23, 2009, 13:49 UTC · re: Johannes Sixt · lore

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

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(-)
Show changes to builtin-help.c +1 −2
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
Jeff King· Apr 23, 2009, 16:40 UTC · re: Johannes Sixt · lore

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

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
Johannes Sixt· Apr 23, 2009, 18:37 UTC · re: Jeff King · lore

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

On Donnerstag, 23. April 2009, Jeff King wrote:
Show 18 quoted lines
> 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
Jeff King· Apr 23, 2009, 20:01 UTC · re: Johannes Sixt · lore

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

On Thu, Apr 23, 2009 at 08:37:26PM +0200, Johannes Sixt wrote:
Show 5 quoted lines
> >   + 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
Junio C Hamano· Apr 24, 2009, 16:19 UTC · re: Johannes Sixt · lore

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

Johannes Sixt <j6t@kdbg.org> writes:
Show 19 quoted lines
> 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.

← back to recent threads