threads / patch / 33210

patchpush: Alias pushurl from push rewrites

Subject: [PATCH] push: Alias pushurl from push rewrites

## tl;dr

4 messages between Mar 17, 2013 and Mar 18, 2013. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Rob Hoelz· Mar 17, 2013, 22:50 UTC · lore

git push currently doesn't consider pushInsteadOf when using pushurl; this tests and fixes that.

If you use pushurl with an alias that has a pushInsteadOf configuration value, Git does not take advantage of it. For example:

[url "git://github.com/"]
    insteadOf = github:
[url "git://github.com/myuser/"]
    insteadOf = mygithub:
[url "git@github.com:myuser/"]
    pushInsteadOf = mygithub:
[remote "origin"]
    url     = github:organization/project
    pushurl = mygithub:project

Signed-off-by: Rob Hoelz <rob@hoelz.ro>
---
 remote.c              |  2 +-
 t/t5516-fetch-push.sh | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

Show changes to 2 files +21 −1

remote.c, t/t5516-fetch-push.sh

diff --git a/remote.c b/remote.c
index ca1f8f2..de7a915 100644
--- a/remote.c
+++ b/remote.c
@@ -465,7 +465,7 @@ static void alias_all_urls(void)
 		if (!remotes[i])
 			continue;
 		for (j = 0; j < remotes[i]->pushurl_nr; j++) {
-			remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites);
+			remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites_push);
 		}
 		add_pushurl_aliases = remotes[i]->pushurl_nr == 0;
 		for (j = 0; j < remotes[i]->url_nr; j++) {
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index b5417cc..272e225 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -244,6 +244,26 @@ test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf
 	)
 '
 
+test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf does rewrite in this case)' '
+	mk_empty &&
+	TRASH="$(pwd)/" &&
+	mkdir ro &&
+	mkdir rw &&
+	git init --bare rw/testrepo &&
+	git config "url.file://$TRASH/ro/.insteadOf" ro: &&
+	git config "url.file://$TRASH/rw/.pushInsteadOf" rw: &&
+	git config remote.r.url ro:wrong &&
+	git config remote.r.pushurl rw:testrepo &&
+	git push r refs/heads/master:refs/remotes/origin/master &&
+	(
+		cd rw/testrepo &&
+		r=$(git show-ref -s --verify refs/remotes/origin/master) &&
+		test "z$r" = "z$the_commit" &&
+
+		test 1 = $(git for-each-ref refs/remotes/origin | wc -l)
+	)
+'
+
 test_expect_success 'push with matching heads' '
 
 	mk_test heads/master &&

-- 
1.8.2

Junio C Hamano· Mar 17, 2013, 23:35 UTC · re: Rob Hoelz · lore

Re: [PATCH] push: Alias pushurl from push rewrites

Rob Hoelz <rob@hoelz.ro> writes:

Show 15 quoted lines

> git push currently doesn't consider pushInsteadOf when
> using pushurl; this tests and fixes that.
>
> If you use pushurl with an alias that has a pushInsteadOf configuration
> value, Git does not take advantage of it.  For example:
>
> [url "git://github.com/"]
>     insteadOf = github:
> [url "git://github.com/myuser/"]
>     insteadOf = mygithub:
> [url "git@github.com:myuser/"]
>     pushInsteadOf = mygithub:
> [remote "origin"]
>     url     = github:organization/project
>     pushurl = mygithub:project

Incomplete sentence? For example [this is an example configuration] and then what happens? Something like "with the sample configuration, 'git push origin' should follow pushurl and then turn it into X, but instead it ends up accessing Y".

If there is no pushInsteadOf, does it still follow insteadOf? Is it tested already?

Wouldn't you want to cover all the combinations to negative cases (i.e. making sure the codepath to support a new case does not affect behaviour of the code outside the new case)? A remote with and without pushurl (two combinations) and a pseudo URL scheme with and without pushInsteadOf (again, two combinations) will give you four cases.

Thanks.

Show 51 quoted lines

>
> Signed-off-by: Rob Hoelz <rob@hoelz.ro>
> ---
>  remote.c              |  2 +-
>  t/t5516-fetch-push.sh | 20 ++++++++++++++++++++
>  2 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/remote.c b/remote.c
> index ca1f8f2..de7a915 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -465,7 +465,7 @@ static void alias_all_urls(void)
>  		if (!remotes[i])
>  			continue;
>  		for (j = 0; j < remotes[i]->pushurl_nr; j++) {
> -			remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites);
> +			remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites_push);
>  		}
>  		add_pushurl_aliases = remotes[i]->pushurl_nr == 0;
>  		for (j = 0; j < remotes[i]->url_nr; j++) {
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index b5417cc..272e225 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -244,6 +244,26 @@ test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf
>  	)
>  '
>  
> +test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf does rewrite in this case)' '
> +	mk_empty &&
> +	TRASH="$(pwd)/" &&
> +	mkdir ro &&
> +	mkdir rw &&
> +	git init --bare rw/testrepo &&
> +	git config "url.file://$TRASH/ro/.insteadOf" ro: &&
> +	git config "url.file://$TRASH/rw/.pushInsteadOf" rw: &&
> +	git config remote.r.url ro:wrong &&
> +	git config remote.r.pushurl rw:testrepo &&
> +	git push r refs/heads/master:refs/remotes/origin/master &&
> +	(
> +		cd rw/testrepo &&
> +		r=$(git show-ref -s --verify refs/remotes/origin/master) &&
> +		test "z$r" = "z$the_commit" &&
> +
> +		test 1 = $(git for-each-ref refs/remotes/origin | wc -l)
> +	)
> +'
> +
>  test_expect_success 'push with matching heads' '
>  
>  	mk_test heads/master &&

Rob Hoelz· Mar 18, 2013, 10:01 UTC · re: Junio C Hamano · lore

Re: [PATCH] push: Alias pushurl from push rewrites

On 3/18/13 12:35 AM, Junio C Hamano wrote:

Show 34 quoted lines

> Rob Hoelz <rob@hoelz.ro> writes:
>
>> git push currently doesn't consider pushInsteadOf when
>> using pushurl; this tests and fixes that.
>>
>> If you use pushurl with an alias that has a pushInsteadOf configuration
>> value, Git does not take advantage of it.  For example:
>>
>> [url "git://github.com/"]
>>     insteadOf = github:
>> [url "git://github.com/myuser/"]
>>     insteadOf = mygithub:
>> [url "git@github.com:myuser/"]
>>     pushInsteadOf = mygithub:
>> [remote "origin"]
>>     url     = github:organization/project
>>     pushurl = mygithub:project
> Incomplete sentence?  For example [this is an example configuration]
> and then what happens?  Something like "with the sample
> configuration, 'git push origin' should follow pushurl and then turn
> it into X, but instead it ends up accessing Y".
>
> If there is no pushInsteadOf, does it still follow insteadOf?  Is it
> tested already?
>
> Wouldn't you want to cover all the combinations to negative cases
> (i.e. making sure the codepath to support a new case does not affect
> behaviour of the code outside the new case)?  A remote with and
> without pushurl (two combinations) and a pseudo URL scheme with and
> without pushInsteadOf (again, two combinations) will give you four
> cases.
>
>
> Thanks.

Sorry; that's a good point. With the above configuration, the following fails:

$ git push origin master

With the following message:

fatal: remote error:
  You can't push to git://github.com/myuser/project.git
  Use git@github.com:myuser/project.git

So you can see that pushurl is being followed (it's not attempting to push to git://github.com/organization/project), but insteadOf values are being used as opposed to pushInsteadOf values for expanding the pushurl alias.

I haven't tried without pushInsteadOf; I will add a test for it later today. I assume that if pushInsteadOf isn't found, insteadOf should be used? I will also consider the other test cases you described.

Thanks again for the feedback!

Show 51 quoted lines

>
>> Signed-off-by: Rob Hoelz <rob@hoelz.ro>
>> ---
>>  remote.c              |  2 +-
>>  t/t5516-fetch-push.sh | 20 ++++++++++++++++++++
>>  2 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/remote.c b/remote.c
>> index ca1f8f2..de7a915 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -465,7 +465,7 @@ static void alias_all_urls(void)
>>  		if (!remotes[i])
>>  			continue;
>>  		for (j = 0; j < remotes[i]->pushurl_nr; j++) {
>> -			remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites);
>> +			remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites_push);
>>  		}
>>  		add_pushurl_aliases = remotes[i]->pushurl_nr == 0;
>>  		for (j = 0; j < remotes[i]->url_nr; j++) {
>> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
>> index b5417cc..272e225 100755
>> --- a/t/t5516-fetch-push.sh
>> +++ b/t/t5516-fetch-push.sh
>> @@ -244,6 +244,26 @@ test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf
>>  	)
>>  '
>>  
>> +test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf does rewrite in this case)' '
>> +	mk_empty &&
>> +	TRASH="$(pwd)/" &&
>> +	mkdir ro &&
>> +	mkdir rw &&
>> +	git init --bare rw/testrepo &&
>> +	git config "url.file://$TRASH/ro/.insteadOf" ro: &&
>> +	git config "url.file://$TRASH/rw/.pushInsteadOf" rw: &&
>> +	git config remote.r.url ro:wrong &&
>> +	git config remote.r.pushurl rw:testrepo &&
>> +	git push r refs/heads/master:refs/remotes/origin/master &&
>> +	(
>> +		cd rw/testrepo &&
>> +		r=$(git show-ref -s --verify refs/remotes/origin/master) &&
>> +		test "z$r" = "z$the_commit" &&
>> +
>> +		test 1 = $(git for-each-ref refs/remotes/origin | wc -l)
>> +	)
>> +'
>> +
>>  test_expect_success 'push with matching heads' '
>>  
>>  	mk_test heads/master &&

Rob Hoelz· Mar 18, 2013, 20:59 UTC · re: Junio C Hamano · lore

Re: [PATCH] push: Alias pushurl from push rewrites

On Sun, 17 Mar 2013 16:35:59 -0700 Junio C Hamano <gitster@pobox.com> wrote:

Show 36 quoted lines

> Rob Hoelz <rob@hoelz.ro> writes:
> 
> > git push currently doesn't consider pushInsteadOf when
> > using pushurl; this tests and fixes that.
> >
> > If you use pushurl with an alias that has a pushInsteadOf
> > configuration value, Git does not take advantage of it.  For
> > example:
> >
> > [url "git://github.com/"]
> >     insteadOf = github:
> > [url "git://github.com/myuser/"]
> >     insteadOf = mygithub:
> > [url "git@github.com:myuser/"]
> >     pushInsteadOf = mygithub:
> > [remote "origin"]
> >     url     = github:organization/project
> >     pushurl = mygithub:project
> 
> Incomplete sentence?  For example [this is an example configuration]
> and then what happens?  Something like "with the sample
> configuration, 'git push origin' should follow pushurl and then turn
> it into X, but instead it ends up accessing Y".
> 
> If there is no pushInsteadOf, does it still follow insteadOf?  Is it
> tested already?
> 
> Wouldn't you want to cover all the combinations to negative cases
> (i.e. making sure the codepath to support a new case does not affect
> behaviour of the code outside the new case)?  A remote with and
> without pushurl (two combinations) and a pseudo URL scheme with and
> without pushInsteadOf (again, two combinations) will give you four
> cases.
> 
> 
> Thanks.

I've taken your advice, and an amended patch follows.

Show 57 quoted lines

> 
> >
> > Signed-off-by: Rob Hoelz <rob@hoelz.ro>
> > ---
> >  remote.c              |  2 +-
> >  t/t5516-fetch-push.sh | 20 ++++++++++++++++++++
> >  2 files changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/remote.c b/remote.c
> > index ca1f8f2..de7a915 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -465,7 +465,7 @@ static void alias_all_urls(void)
> >  		if (!remotes[i])
> >  			continue;
> >  		for (j = 0; j < remotes[i]->pushurl_nr; j++) {
> > -			remotes[i]->pushurl[j] =
> > alias_url(remotes[i]->pushurl[j], &rewrites);
> > +			remotes[i]->pushurl[j] =
> > alias_url(remotes[i]->pushurl[j], &rewrites_push); }
> >  		add_pushurl_aliases = remotes[i]->pushurl_nr == 0;
> >  		for (j = 0; j < remotes[i]->url_nr; j++) {
> > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> > index b5417cc..272e225 100755
> > --- a/t/t5516-fetch-push.sh
> > +++ b/t/t5516-fetch-push.sh
> > @@ -244,6 +244,26 @@ test_expect_success 'push with pushInsteadOf
> > and explicit pushurl (pushInsteadOf )
> >  '
> >  
> > +test_expect_success 'push with pushInsteadOf and explicit pushurl
> > (pushInsteadOf does rewrite in this case)' '
> > +	mk_empty &&
> > +	TRASH="$(pwd)/" &&
> > +	mkdir ro &&
> > +	mkdir rw &&
> > +	git init --bare rw/testrepo &&
> > +	git config "url.file://$TRASH/ro/.insteadOf" ro: &&
> > +	git config "url.file://$TRASH/rw/.pushInsteadOf" rw: &&
> > +	git config remote.r.url ro:wrong &&
> > +	git config remote.r.pushurl rw:testrepo &&
> > +	git push r refs/heads/master:refs/remotes/origin/master &&
> > +	(
> > +		cd rw/testrepo &&
> > +		r=$(git show-ref -s --verify
> > refs/remotes/origin/master) &&
> > +		test "z$r" = "z$the_commit" &&
> > +
> > +		test 1 = $(git for-each-ref refs/remotes/origin |
> > wc -l)
> > +	)
> > +'
> > +
> >  test_expect_success 'push with matching heads' '
> >  
> >  	mk_test heads/master &&
>