remote: allow adding remote w same name as alias

From f80bdf3272e7bdf790ee67fb94196a8aa139331f Mon Sep 17 00:00:00 2001

From f80bdf3272e7bdf790ee67fb94196a8aa139331f Mon Sep 17 00:00:00 2001
From: Anastas Dancha <[email protected]>
Date: Mon, 15 Dec 2014 16:30:50 -0500
Subject: [PATCH] remote: allow adding remote w same name as alias

When ~/.gitconfig contains an alias (i.e. myremote) and you are adding a new remote using the same name for remote, Git will refuse to add the remote with the same name as one of the aliases, even though the remote with such name is not setup for current repo.

$ git remote add myremote [email protected]:team/repo.git
fatal: remote myremote already exists.

The fatal error comes from strcmp(name, remote->url[0]) condition, which compares a name of the new remote with existing urls of all the remotes, including the ones from ~/.gitconfig (or global variant).
I'm not sure why that is necessary, unless Git is meant to prevent users from naming their remotes as their remote aliases..
Imho, if someone want's to git remote add myremote myremote, they should, since git-remote always takes 2 arguments, first being the new remote's name and second being new remote's url (or alias, if set).

Thanks to @mymuss for sanity check and debugging.

Signed-off-by Anastas Dancha <[email protected]>

---
 builtin/remote.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

When ~/.gitconfig contains an alias (i.e. myremote) and you are adding a new remote using the same name for remote, Git will refuse to add the remote with the same name as one of the aliases, even though the remote with such name is not setup for current repo.

Hi Anastas,

Quoted 5 lines · Anastas Dancha

When ~/.gitconfig contains an alias (i.e. myremote) and you are adding a new remote using the same name for remote, Git will refuse to add the remote with the same name as one of the aliases, even though the remote with such name is not setup for current repo.

Just to make sure we're on the same page... you are talking about

[remote "myremote"]

not

        [alias]
                myremote = ...

yes? If so, please avoid using the term "alias"...

Further, I assume that your .gitconfig lists the "myremote" without a URL?

Also:

builtin/remote.c static int add(int argc, const char **argv) · line 184

Quoted 5 lines

builtin/remote.c +2 -3

-       if (remote && (remote->url_nr > 1 || strcmp(name, remote->url[0]) ||
-                       remote->fetch_refspec_nr))
-               die(_("remote %s already exists."), name);
+       if (remote && (remote->url_nr > 1 || remote->fetch_refspec_nr))
+               die(_("remote %s %s already exists."), name, url);

The real problem here is that strcmp() is performed even if url_nr == 0, *and* that it compares the name – instead of the url – to the remote's URL. That is incorrect, so the correct fix would be:

(quoted diff) +2 -1

-       if (remote && (remote->url_nr > 1 || strcmp(name, remote->url[0]) ||
+       if (remote && (remote->url_nr > 1 ||
+                       (remote->url_nr == 1 && strcmp(url, remote->url[0])) ||
                        remote->fetch_refspec_nr))
                die(_("remote %s already exists."), name);

In other words, we would still verify that there is no existing remote, even if that remote was declared in ~/.gitconfig. However, if a remote exists without any URL, or if it has a single URL that matches the provided one, and there are no fetch refspecs, *then* there is nothing to complain about and we continue.

Ciao,
Johannes

Then I wrote "alias", I've meant the following:

```
[url "[email protected]"]
insteadOf = myalias
pushInsteadOf = myalias
```

Unfortunately, your suggested fix will not allow my [poorly] described use case.

[...]
There is one bit left to clarify: let me guess, you have a $HOME/.gitconfig like this:

[url "[email protected]"]
insteadOf = backup
pushInsteadOf = backup

and then you want to add the "backup" remote in a Git working directory like this:

git remote add backup [email protected]

but my suggested fix will still disallow this because the URL does not match the [email protected]?

Ciao,
Johannes

Hello Johannes,

Quoted 11 lines · Johannes Schindelin

[...]
There is one bit left to clarify: let me guess, you have a $HOME/.gitconfig like this:

[url "[email protected]"]
insteadOf = backup
pushInsteadOf = backup

and then you want to add the "backup" remote in a Git working directory like this:

Quoted 7 lines

git remote add backup [email protected]

but my suggested fix will still disallow this because the URL does not match the [email protected]?

Ciao,
Johannes

Precisely that. In fact, it will not work even if you do any of these:

git remote add backup [email protected]
git remote add backup [email protected]
git remote add backup backup

The original / current code and your suggested fix - both exhibit similar behaviour with the use cases I've described above.

Thanks,
Anastas

Anastas Dancha schrieb am 19.12.2014 um 16:44:

Quoted 29 lines

Hello Johannes,

Quoted 11 lines · Johannes Schindelin

[...]
There is one bit left to clarify: let me guess, you have a $HOME/.gitconfig like this:

[url "[email protected]"]
insteadOf = backup
pushInsteadOf = backup

and then you want to add the "backup" remote in a Git working directory like this:

Quoted 7 lines

git remote add backup [email protected]

but my suggested fix will still disallow this because the URL does not match the [email protected]?

Ciao,
Johannes

Precisely that. In fact, it will not work even if you do any of these:

git remote add backup [email protected]
git remote add backup [email protected]
git remote add backup backup

Quoted 6 lines

The original / current code and your suggested fix - both exhibit similar behaviour with the use cases I've described above.

Thanks,
Anastas

OK, I'll repeat it again: We cannot allow that.

"git push" can take a url or a remote as a parameter. Which one is it if you have a remote and a url (alias) with the same name?

Michael

Quoted 11 lines · Johannes Schindelin

[...]
There is one bit left to clarify: let me guess, you have a $HOME/.gitconfig like this:

[url "[email protected]"]
insteadOf = backup
pushInsteadOf = backup

and then you want to add the "backup" remote in a Git working directory like this:

Quoted 4 lines

git remote add backup [email protected]

but my suggested fix will still disallow this because the URL does not match the [email protected]?

Precisely that. In fact, it will not work even if you do any of these:

git remote add backup [email protected]

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 <[email protected]>

---
 builtin/remote.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

builtin/remote.c +0 -0

index 46ecfd9..9168c83 100644

Quoted 23 lines · Johannes Schindelin

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 <[email protected]>

---
 builtin/remote.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

builtin/remote.c +0 -0

index 46ecfd9..9168c83 100644

builtin/remote.c static int add(int argc, const char **argv) · line 186

Quoted 8 lines

builtin/remote.c +2 -1

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

Quoted 17 lines · Johannes Schindelin

Quoted 14 lines · Junio C Hamano

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.

Quoted 1 line

I hope v2 addresses your concerns.

Unfortunately I am still confused.

The way the overlong line is folded in the new version of the patch makes it easier to read, but I didn't find a reason why checking the number of fetch_refspec does not go against that goal there.

Quoted 7 lines

Quoted 1 line

I hope v2 addresses your concerns.

Unfortunately I am still confused.

The way the overlong line is folded in the new version of the patch makes it easier to read, but I didn't find a reason why checking the number of fetch_refspec does not go against that goal there.

Since you pointed out rightfully that the main goal is *not* to allow multiple git remote add to succeed when they try to add the same remote with the same URL, I changed the commit message to point out that the goal was to handle adding remotes via git remote add <nick> <url> when url.<url>.insteadOf = <nick> already exists, with the same <nick> and <url>.

Since I have no interest in opening the can of worms to allow re-adding remotes, I did not touch that part at all, I hope you do not mind.