threads / patch / 38182

patchremote: allow adding remote w same name as alias

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

## tl;dr

19 messages between Dec 16, 2014 and Dec 23, 2014. Diffs are folded; open one to read it.

replies: 18people: 4as markdown or json

Anastas Dancha· Dec 16, 2014, 02:30 UTC · lore

>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(-)

Show changes to builtin/remote.c +2 −3

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

Johannes Schindelin· Dec 16, 2014, 09:01 UTC · re: Anastas Dancha · lore

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

Hi Anastas,

On Tue, 16 Dec 2014, Anastas Dancha wrote:

Show 5 quoted lines

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

Show 5 quoted lines

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

Anastas Dancha· Dec 16, 2014, 14:57 UTC · re: Johannes Schindelin · lore

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

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:

Show 49 quoted lines

> 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

Johannes Schindelin· Dec 19, 2014, 09:37 UTC · re: Anastas Dancha · lore

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

Hi Anastas,

On Tue, 16 Dec 2014, Anastas Dancha wrote:

Show 8 quoted lines

> 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

Anastas Dancha· Dec 19, 2014, 15:44 UTC · re: Johannes Schindelin · lore

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

Hello Johannes,

On Fri, Dec 19, 2014 at 4:37 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 18 quoted lines

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

Michael J Gruber· Dec 19, 2014, 16:30 UTC · re: Anastas Dancha · lore

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

Anastas Dancha schrieb am 19.12.2014 um 16:44:

Show 35 quoted lines

> 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

Anastas Dancha· Dec 20, 2014, 20:57 UTC · re: Michael J Gruber · lore

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

On Fri, Dec 19, 2014 at 11:30 AM, Michael J Gruber <git@drmicha.warpmail.net> wrote:

Show 43 quoted lines

> 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

Johannes Schindelin· Dec 21, 2014, 20:40 UTC · re: Anastas Dancha · lore

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

Hi Anastas,

On Fri, 19 Dec 2014, Anastas Dancha wrote:

Show 21 quoted lines

> 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

Michael J Gruber· Dec 16, 2014, 09:33 UTC · re: Anastas Dancha · lore

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

Anastas Dancha schrieb am 16.12.2014 um 03:30:

Show 25 quoted lines

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

Show 6 quoted lines

> 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(-)

Johannes Schindelin· Dec 22, 2014, 17:06 UTC · re: Anastas Dancha · lore

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

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

Johannes Schindelin· Dec 22, 2014, 17:06 UTC · re: Johannes Schindelin · lore

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

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(-)

Show changes to builtin/remote.c +2 −1

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

Junio C Hamano· Dec 22, 2014, 17:49 UTC · re: Johannes Schindelin · lore

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

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

Show 31 quoted lines

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

Johannes Schindelin· Dec 23, 2014, 13:25 UTC · re: Junio C Hamano · lore

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

Hi Junio,

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

Show 64 quoted lines

> 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

Johannes Schindelin· Dec 22, 2014, 17:06 UTC · re: Johannes Schindelin · lore

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

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

Show changes to t/t5505-remote.sh +5 −0

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

Johannes Schindelin· Dec 23, 2014, 13:24 UTC · re: Anastas Dancha · lore

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

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.

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(-)

Show changes to builtin/remote.c +2 −1

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

Johannes Schindelin· Dec 23, 2014, 13:25 UTC · re: Johannes Schindelin · lore

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

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.

Noticed by Anastas Dancha.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/remote.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Show changes to builtin/remote.c +3 −1

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

Johannes Schindelin· Dec 23, 2014, 13:25 UTC · re: Johannes Schindelin · lore

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

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

Show changes to t/t5505-remote.sh +5 −0

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

Johannes Schindelin· Dec 23, 2014, 18:26 UTC · lore

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

Hi Junio,

[re-Cc:ing the list]

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

Show 26 quoted lines

> 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

Junio C Hamano· Dec 23, 2014, 18:51 UTC · re: Johannes Schindelin · lore

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

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

Show 17 quoted lines

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