Re: [PATCH 1/2] git remote add: allow re-adding remotes with the same URL

Johannes Schindelin [off-list ref] writes:

When we need to fold an overlong line, it is easier to read if the
line is folded at an operator with higher precedence, i.e. this line

        if (A && (B || (C && D) || E))

folded like this

        if (A && (B || (C &&
                   D) ||
            E))

is harder to read than when folded like this

        if (A && (B ||
                  (C && D) ||
                   E))

So, it is an error if we have "remote" and if

 (1) URL for the remote is defined already twice or more; or
 (2) we are adding a nickname (i.e. not a URL) and it is different
     from what we already have; or
 (3) we already have fetch_refspec

The way I read the log message's rationale was that this is to allow
replacing an existing remote's URL; wouldn't checking the existence
of fetch_refspec go against that goal?

Puzzled.  Either the code is wrong or I am mislead by the
explanation in the log.

Hi Junio,

On Mon, 22 Dec 2014, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

When adding a remote, we make sure that the remote does not exist
already.

For convenience, we allow re-adding remotes with the same URLs.
This also handles the case that there is an "[url ...] insteadOf"
setting in the config.

It might seem like a mistake to compare against remote->url[0] without
verifying that remote->url_nr >=1, but at this point a missing URL has
been filled by the name already, therefore url_nr cannot be zero.

Noticed by Anastas Dancha.

Signed-off-by: Johannes Schindelin <redacted>
---
 builtin/remote.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 46ecfd9..9168c83 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c

@@ -180,7 +180,8 @@ 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]) ||
+	if (remote && (remote->url_nr > 1 || (strcmp(name, remote->url[0]) &&
+				strcmp(url, remote->url[0])) ||
 			remote->fetch_refspec_nr))
 		die(_("remote %s already exists."), name);

When we need to fold an overlong line, it is easier to read if the
line is folded at an operator with higher precedence, i.e. this line

        if (A && (B || (C && D) || E))

folded like this

        if (A && (B || (C &&
                   D) ||
            E))

is harder to read than when folded like this

        if (A && (B ||
                  (C && D) ||
                   E))

So, it is an error if we have "remote" and if

 (1) URL for the remote is defined already twice or more; or
 (2) we are adding a nickname (i.e. not a URL) and it is different
     from what we already have; or
 (3) we already have fetch_refspec

The way I read the log message's rationale was that this is to allow
replacing an existing remote's URL; wouldn't checking the existence
of fetch_refspec go against that goal?

Puzzled.  Either the code is wrong or I am mislead by the
explanation in the log.

I hope v2 addresses your concerns.

Ciao,
Dscho