[PATCH] remote: allow adding remote w same name as alias

[PATCH] remote: allow adding remote w same name as alias

[Anastas Dancha]

>From f80bdf3272e7bdf790ee67fb94196a8aa139331f Mon Sep 17 00:00:00 2001
From: Anastas Dancha <anapsix@random.io>
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 git@host.com: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 <anapsix@random.io>
---
 builtin/remote.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 7f28f92..7471d0a 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -180,9 +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]) ||
-			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);
 
 	strbuf_addf(&buf2, "refs/heads/test:refs/remotes/%s/test", name);
 	if (!valid_fetch_refspec(buf2.buf))
-- 
2.2.0

Re: [PATCH] remote: allow adding remote w same name as alias

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1796 bytes --]

Hi Anastas,

On Tue, 16 Dec 2014, Anastas Dancha wrote:

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

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

-       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

Re: [PATCH] remote: allow adding remote w same name as alias

[Michael J Gruber]

Anastas Dancha schrieb am 16.12.2014 um 03:30:
> From f80bdf3272e7bdf790ee67fb94196a8aa139331f Mon Sep 17 00:00:00 2001
> From: Anastas Dancha <anapsix@random.io>
> 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 git@host.com: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).

While that is true for "git remote", it is wrong for "git push" and the
like, which takes "remote name or remote URL" as a parameter. Therefore,
remote names and remote aliases need to be unique within the same namespace.

> Thanks to @mymuss for sanity check and debugging.
> 
> Signed-off-by: Anastas Dancha <anapsix@random.io>
> ---
>  builtin/remote.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

Re: [PATCH] remote: allow adding remote w same name as alias

[Anastas Dancha]

My bad Johannes,

Then I wrote "alias", I've meant the following:
```
[url "git@githost.com"]
insteadOf = myalias
pushInsteadOf = myalias
```
Unfortunately, your suggested fix will not allow my [poorly] described use case.
Hope this makes more sense now.

Thank you for looking into this.

-Anastas

On Tue, Dec 16, 2014 at 4:01 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Anastas,
>
> On Tue, 16 Dec 2014, Anastas Dancha wrote:
>
>> 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:
>
>> -       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:
>
> -       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

Re: [PATCH] remote: allow adding remote w same name as alias

[Johannes Schindelin]

Hi Anastas,

On Tue, 16 Dec 2014, Anastas Dancha wrote:

> Then I wrote "alias", I've meant the following:
> ```
> [url "git@githost.com"]
> 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 "anastas@company.com"]
		insteadOf = backup
		pushInsteadOf = backup

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

	git remote add backup me@my-laptop.local

but my suggested fix will still disallow this because the URL does not
match the url.anastas@company.com.insteadOf?

Ciao,
Johannes

Re: [PATCH] remote: allow adding remote w same name as alias

[Anastas Dancha]

Hello Johannes,

On Fri, Dec 19, 2014 at 4:37 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> [...]
> There is one bit left to clarify: let me guess, you have a $HOME/.gitconfig
> like this:
>
>         [url "anastas@company.com"]
>                 insteadOf = backup
>                 pushInsteadOf = backup
>
> and then you want to add the "backup" remote in a Git working directory
> like this:
>
>         git remote add backup me@my-laptop.local
>
> but my suggested fix will still disallow this because the URL does not
> match the url.anastas@company.com.insteadOf?
>
> Ciao,
> Johannes

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

    git remote add backup anastas@company.com
    git remote add backup anything@anyhost.com
    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

Re: [PATCH] remote: allow adding remote w same name as alias

[Michael J Gruber]

Anastas Dancha schrieb am 19.12.2014 um 16:44:
> Hello Johannes,
> 
> On Fri, Dec 19, 2014 at 4:37 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>> [...]
>> There is one bit left to clarify: let me guess, you have a $HOME/.gitconfig
>> like this:
>>
>>         [url "anastas@company.com"]
>>                 insteadOf = backup
>>                 pushInsteadOf = backup
>>
>> and then you want to add the "backup" remote in a Git working directory
>> like this:
>>
>>         git remote add backup me@my-laptop.local
>>
>> but my suggested fix will still disallow this because the URL does not
>> match the url.anastas@company.com.insteadOf?
>>
>> Ciao,
>> Johannes
> 
> Precisely that. In fact, it will not work even if you do any of these:
> 
>     git remote add backup anastas@company.com
>     git remote add backup anything@anyhost.com
>     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
> 

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

Re: [PATCH] remote: allow adding remote w same name as alias

[Anastas Dancha]

On Fri, Dec 19, 2014 at 11:30 AM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> Anastas Dancha schrieb am 19.12.2014 um 16:44:
>> Hello Johannes,
>>
>> On Fri, Dec 19, 2014 at 4:37 AM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>>> [...]
>>> There is one bit left to clarify: let me guess, you have a $HOME/.gitconfig
>>> like this:
>>>
>>>         [url "anastas@company.com"]
>>>                 insteadOf = backup
>>>                 pushInsteadOf = backup
>>>
>>> and then you want to add the "backup" remote in a Git working directory
>>> like this:
>>>
>>>         git remote add backup me@my-laptop.local
>>>
>>> but my suggested fix will still disallow this because the URL does not
>>> match the url.anastas@company.com.insteadOf?
>>>
>>> Ciao,
>>> Johannes
>>
>> Precisely that. In fact, it will not work even if you do any of these:
>>
>>     git remote add backup anastas@company.com
>>     git remote add backup anything@anyhost.com
>>     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
>>
>
> 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

I suppose it could be either. A string can be checked if there is a
remote with such name first, and if not, it could be attempted to be
used as a url.
In fact, my current workaround involves the following:

    git remote add tempname backup:product/repo.git
    git remote rename tempname backup
    git push backup

This seems to work just fine right now with no code changes.
Michael, could you explain in more detail what will break in **git
push backup** if code changes are made to eliminate the necessity of
the add/remote workaround.

Regards,
Anastas

Re: [PATCH] remote: allow adding remote w same name as alias

[Johannes Schindelin]

Hi Anastas,

On Fri, 19 Dec 2014, Anastas Dancha wrote:

> On Fri, Dec 19, 2014 at 4:37 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > [...]
> > There is one bit left to clarify: let me guess, you have a $HOME/.gitconfig
> > like this:
> >
> >         [url "anastas@company.com"]
> >                 insteadOf = backup
> >                 pushInsteadOf = backup
> >
> > and then you want to add the "backup" remote in a Git working directory
> > like this:
> >
> >         git remote add backup me@my-laptop.local
> >
> > but my suggested fix will still disallow this because the URL does not
> > match the url.anastas@company.com.insteadOf?
> 
> Precisely that. In fact, it will not work even if you do any of these:
> 
>     git remote add backup anastas@company.com

This will succeed after applying my suggested change. I even tested this
;-)

Ciao,
Johannes

[PATCH 0/2] Let `git remote add` play nicer with url.<url>.insteadOf

[Johannes Schindelin]

Anastas Dancha reported that it is not possible to add a remote when
there is already a url.<url>.insteadOf = <nick> setting in
$HOME/.gitconfig.

While it makes sense to prevent surprises when a user adds a remote and
it fetches from somewhere completely different, it makes less sense to
prevent adding a remote when it is actually the same that was specified
in the config.

Therefore we add just another check that let's `git remote add` continue
when the "existing" remote's URL is identical to the specified one.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>


Johannes Schindelin (2):
  git remote add: allow re-adding remotes with the same URL
  Add a regression test for 'git remote add <existing> <same-url>'

 builtin/remote.c  | 3 ++-
 t/t5505-remote.sh | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

-- 
2.0.0.rc3.9669.g840d1f9

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

[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 <johannes.schindelin@gmx.de>
---
 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);
 
-- 
2.0.0.rc3.9669.g840d1f9

[PATCH 2/2] Add a regression test for 'git remote add <existing> <same-url>'

[Johannes Schindelin]

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

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index ac79dd9..17c6330 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -1113,4 +1113,9 @@ test_extra_arg set-url origin newurl oldurl
 # prune takes any number of args
 # update takes any number of args
 
+test_expect_success 'add remote matching the "insteadOf" URL' '
+	git config url.xyz@example.com.insteadOf backup &&
+	git remote add backup xyz@example.com
+'
+
 test_done
-- 
2.0.0.rc3.9669.g840d1f9

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

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> 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 <johannes.schindelin@gmx.de>
> ---
>  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.

[PATCH v2 0/2] Let `git remote add` play nicer with url.<url>.insteadOf

[Johannes Schindelin]

Anastas Dancha reported that it is not possible to add a remote when
there is already a url.<url>.insteadOf = <nick> setting in
$HOME/.gitconfig.

While it makes sense to prevent surprises when a user adds a remote and
it fetches from somewhere completely different, it makes less sense to
prevent adding a remote when it is actually the same that was specified
in the config.

Therefore we add just another check that let's `git remote add` continue
when the "existing" remote's URL is identical to the specified one.

Interdiff below the diffstat (the commit message was also touched up to
stop pretending that we allow adding a remote twice when the URL did not
change).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Johannes Schindelin (2):
  git remote: allow adding remotes agreeing with url.<...>.insteadOf
  Add a regression test for 'git remote add <existing> <same-url>'

 builtin/remote.c  | 4 +++-
 t/t5505-remote.sh | 5 +++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 9168c83..b4ff468 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);
-- 
2.0.0.rc3.9669.g840d1f9

[PATCH v2 1/2] git remote: allow adding remotes agreeing with url.<...>.insteadOf

[Johannes Schindelin]

When adding a remote, we make sure that the remote does not exist
already. However, this test was not quite correct: when the
url.<...>.insteadOf config variable was set to the remote name to be
added, the code would assume that the remote exists already.

Let's allow adding remotes when there is a url.<...>.insteadOf setting
when both the name and the URL agree with the remote to be added.

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 <johannes.schindelin@gmx.de>
---
 builtin/remote.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 46ecfd9..b4ff468 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -180,7 +180,9 @@ 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);
 
-- 
2.0.0.rc3.9669.g840d1f9

[PATCH v2 2/2] Add a regression test for 'git remote add <existing> <same-url>'

[Johannes Schindelin]

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

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index ac79dd9..17c6330 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -1113,4 +1113,9 @@ test_extra_arg set-url origin newurl oldurl
 # prune takes any number of args
 # update takes any number of args
 
+test_expect_success 'add remote matching the "insteadOf" URL' '
+	git config url.xyz@example.com.insteadOf backup &&
+	git remote add backup xyz@example.com
+'
+
 test_done
-- 
2.0.0.rc3.9669.g840d1f9

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

[Johannes Schindelin]

Hi Junio,

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

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > 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 <johannes.schindelin@gmx.de>
> > ---
> >  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

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

[Johannes Schindelin]

Hi Junio,

[re-Cc:ing the list]

On Tue, 23 Dec 2014, Junio C Hamano wrote:

> On Tue, Dec 23, 2014 at 5:25 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Mon, 22 Dec 2014, Junio C Hamano wrote:
> >>
> >> 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.
> 
> 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.

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

Oh, don't get me wrong.  I do not have any interest in allowing
re-adding remotes, either, so we are on the same page.

I was just saying that it was unclear what ensures that the change
in the codepath only affects the remote setting that conflicts with
"insteadOf" without randomly (read: sometimes it allows, sometimes
it forbids) allowing re-adding remotes.