threads / patch / 34776

patchremote: filter out invalid remote configurations

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

## tl;dr

4 messages between Aug 27, 2013 and Aug 30, 2013. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Carlos Martín Nieto· Aug 27, 2013, 13:06 UTC · lore

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(-)

Show changes to 3 files +20 −1

remote.c, t/t5505-remote.sh, t/t7201-co.sh

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

Junio C Hamano· Aug 27, 2013, 14:50 UTC · re: Carlos Martín Nieto · lore

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

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

Show 10 quoted lines

> 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.

Carlos Martín Nieto· Aug 30, 2013, 12:41 UTC · re: Junio C Hamano · lore

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

On Tue, 2013-08-27 at 07:50 -0700, Junio C Hamano wrote:

Show 16 quoted lines

> 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

Junio C Hamano· Aug 30, 2013, 16:58 UTC · re: Carlos Martín Nieto · lore

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

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

Show 21 quoted lines

> 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.