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

On 02/15, 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.

It took me a while too to convince myself there is nothing strange
going on.  But I could neither find anything in the history, nor could
I think of any case that we could break.

Thanks for your review!

-Peff