[PATCH] remote: filter out invalid remote configurations

[PATCH] remote: filter out invalid remote configurations

[Carlos Martín Nieto]

In remote's configuration callback, anything that looks like
'remote.<name>.*' creates a remote '<name>'. This remote may not end
up having any configuration for a remote, but it's still in the list,
so 'git remote' shows it, which means something like

    [remote "bogus"]
        hocus = pocus

will show a remote 'bogus' in the listing, even though it won't work
as a remote name for either git-fetch or git-push.

Filter out the remotes that we created which have no urls in order to
work around such configuration entries.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>

---

Due to git's callback-based config, it seemed a lot simpler to let it
do it wrong and then filter out what won't be usable, rather than
delaying the creation of a remote until we're sure we do want it.

The tests that made use of a remote 'existing' with just .fetch seem
to be written that way because they can get away with it, rather than
any assertion that it should be allowed in day-to-day git usage, but
correct me if I'm wrong.

 remote.c          | 17 +++++++++++++++++
 t/t5505-remote.sh |  2 ++
 t/t7201-co.sh     |  2 +-
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index 68eb99b..00a1d7a 100644
--- a/remote.c
+++ b/remote.c
@@ -141,6 +141,9 @@ static struct remote *make_remote(const char *name, int len)
 	int i;
 
 	for (i = 0; i < remotes_nr; i++) {
+		if (!remotes[i])
+			continue;
+
 		if (len ? (!strncmp(name, remotes[i]->name, len) &&
 			   !remotes[i]->name[len]) :
 		    !strcmp(name, remotes[i]->name))
@@ -469,6 +472,19 @@ static int handle_config(const char *key, const char *value, void *cb)
 	return 0;
 }
 
+static void filter_valid_remotes(void)
+{
+	int i;
+	for (i = 0; i < remotes_nr; i++) {
+		if (!remotes[i])
+			continue;
+
+		/* It's not a remote unless it has at least one url */
+		if (remotes[i]->url_nr == 0 && remotes[i]->pushurl_nr == 0)
+			remotes[i] = NULL;
+	}
+}
+
 static void alias_all_urls(void)
 {
 	int i, j;
@@ -504,6 +520,7 @@ static void read_config(void)
 			make_branch(head_ref + strlen("refs/heads/"), 0);
 	}
 	git_config(handle_config, NULL);
+	filter_valid_remotes();
 	alias_all_urls();
 }
 
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index dd10ff0..848e7b7 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -130,9 +130,11 @@ to delete them, use:
 EOF
 	} &&
 	git tag footag &&
+	git remote add oops .
 	git config --add remote.oops.fetch "+refs/*:refs/*" &&
 	git remote remove oops 2>actual1 &&
 	git branch foobranch &&
+	git remote add oops .
 	git config --add remote.oops.fetch "+refs/*:refs/*" &&
 	git remote rm oops 2>actual2 &&
 	git branch -d foobranch &&
diff --git a/t/t7201-co.sh b/t/t7201-co.sh
index 0c9ec0a..4647f1c 100755
--- a/t/t7201-co.sh
+++ b/t/t7201-co.sh
@@ -431,7 +431,7 @@ test_expect_success 'detach a symbolic link HEAD' '
 
 test_expect_success \
     'checkout with --track fakes a sensible -b <name>' '
-    git config remote.origin.fetch "+refs/heads/*:refs/remotes/origin/*" &&
+    git remote add origin . &&
     git update-ref refs/remotes/origin/koala/bear renamer &&
 
     git checkout --track origin/koala/bear &&
-- 
1.8.4.561.g1c3d45d

Re: [PATCH] remote: filter out invalid remote configurations

[Junio C Hamano]

Carlos Martín Nieto <cmn@elego.de> writes:

> In remote's configuration callback, anything that looks like
> 'remote.<name>.*' creates a remote '<name>'. This remote may not end
> up having any configuration for a remote, but it's still in the list,
> so 'git remote' shows it, which means something like
>
>     [remote "bogus"]
>         hocus = pocus
>
> will show a remote 'bogus' in the listing, even though it won't work
> as a remote name for either git-fetch or git-push.

Isn't this something the user may want to be aware of, though?
Hiding these would rob a chance for such an entry to be noticed from
the user---is it a good change?

> Filter out the remotes that we created which have no urls in order to
> work around such configuration entries.

Re: [PATCH] remote: filter out invalid remote configurations

[Carlos Martín Nieto]

On Tue, 2013-08-27 at 07:50 -0700, Junio C Hamano wrote:
> Carlos Martín Nieto <cmn@elego.de> writes:
> 
> > In remote's configuration callback, anything that looks like
> > 'remote.<name>.*' creates a remote '<name>'. This remote may not end
> > up having any configuration for a remote, but it's still in the list,
> > so 'git remote' shows it, which means something like
> >
> >     [remote "bogus"]
> >         hocus = pocus
> >
> > will show a remote 'bogus' in the listing, even though it won't work
> > as a remote name for either git-fetch or git-push.
> 
> Isn't this something the user may want to be aware of, though?
> Hiding these would rob a chance for such an entry to be noticed from
> the user---is it a good change?

If we want to help the user know that there's something a bit odd in
their configuration, shouldn't we tell them instead of hoping they
stumble upon it? Otherwise IMO it's more confusing if git-remote does
show the remote when git-fetch is interpreting the argument as a path.

   cmn

Re: [PATCH] remote: filter out invalid remote configurations

[Junio C Hamano]

Carlos Martín Nieto <cmn@elego.de> writes:

> On Tue, 2013-08-27 at 07:50 -0700, Junio C Hamano wrote:
>> Carlos Martín Nieto <cmn@elego.de> writes:
>> 
>> > In remote's configuration callback, anything that looks like
>> > 'remote.<name>.*' creates a remote '<name>'. This remote may not end
>> > up having any configuration for a remote, but it's still in the list,
>> > so 'git remote' shows it, which means something like
>> >
>> >     [remote "bogus"]
>> >         hocus = pocus
>> >
>> > will show a remote 'bogus' in the listing, even though it won't work
>> > as a remote name for either git-fetch or git-push.
>> 
>> Isn't this something the user may want to be aware of, though?
>> Hiding these would rob a chance for such an entry to be noticed from
>> the user---is it a good change?
>
> If we want to help the user know that there's something a bit odd in
> their configuration, shouldn't we tell them instead of hoping they
> stumble upon it?

Yeah, I agree that "git remote" that tells the above "bogus" is
fishy is better than just hides it.