Re: [PATCH 4/4] remote: use remote_is_configured() for add and rename

Hi Peff & Thomas,

On Mon, 15 Feb 2016, Jeff King wrote:

On Mon, Feb 15, 2016 at 06:42:30PM +0100, Thomas Gummerer wrote:

quoted

Both remote add and remote rename use a slightly different hand-rolled
check if the remote exits.  The hand-rolled check may have some subtle
cases in which it might fail to detect when a remote already exists.
One such case was fixed in fb86e32 ("git remote: allow adding remotes
agreeing with url.<...>.insteadOf").  Another case is when a remote is
configured as follows:

  [remote "foo"]
    vcs = bar

If we try to run `git remote add foo bar` with the above remote
configuration, git segfaults.  This change fixes it.

In addition, git remote rename $existing foo with the configuration for
foo as above silently succeeds, even though foo already exists,
modifying its configuration.  With this patch it fails with "remote foo
already exists".

Checking is_configured() certainly sounds like a better test, but...

quoted

diff --git a/builtin/remote.c b/builtin/remote.c
index 981c487..bd57f1b 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c

@@ -186,10 +186,7 @@ static int add(int argc, const char **argv)
 	url = argv[1];
 
 	remote = remote_get(name);
-	if (remote && (remote->url_nr > 1 ||
-			(strcmp(name, remote->url[0]) &&
-				strcmp(url, remote->url[0])) ||
-			remote->fetch_refspec_nr))
+	if (remote_is_configured(remote))
 		die(_("remote %s already exists."), name);

This original is quite confusing. I thought at first that there was
perhaps something going on with allowing repeated re-configuration of
the same remote, as long as some parameters matched. I.e., I am
wondering if there is a case here that does _not_ segfault, that we
would be breaking.

But reading over fb86e32dcc, I think I have convinced myself that it was
merely an ad-hoc check for "is_configured", and using that function is a
better replacement.

Yes, yes, yes. Y'all are absolutely correct. I shoulda added a test case
right away, to make sure not only that what I fixed does not get broken in
the future, but also to document *what* was fixed, exactly.

So, belatedly, here goes a patch that verifies what that commit was
supposed to fix, and yes, it passes with Thomas' changes (Junio, would you
please apply this on top of tg/git-remote?):

-- snipsnap --
From: Johannes Schindelin <redacted>
Date: Wed, 17 Feb 2016 14:45:59 +0100
Subject: [PATCH] t5505: 'remote add x y' should work when url.y.insteadOf = x

This is the test missing from fb86e32 (git remote: allow adding
remotes agreeing with url.<...>.insteadOf, 2014-12-23): we should
allow adding a remote with the URL when it agrees with the
url.<...>.insteadOf setting.

Signed-off-by: Johannes Schindelin <redacted>
---
 t/t5505-remote.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 94079a0..19e8e34 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh

@@ -51,6 +51,11 @@ test_expect_success setup '
 	git clone one test
 '
 
+test_expect_success 'add remote whose URL agrees with url.<...>.insteadOf' '
+	git config url.git@host.com:team/repo.git.insteadOf myremote &&
+	git remote add myremote git@host.com:team/repo.git
+'
+
 test_expect_success C_LOCALE_OUTPUT 'remote information for the origin' '
 	(
 		cd test &&

-- 
2.7.1.windows.2