[PATCH] push: Alias pushurl from push rewrites

[PATCH] push: Alias pushurl from push rewrites

[Rob Hoelz]

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

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

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

[Junio C Hamano]

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.

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

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

[Rob Hoelz]

On 3/18/13 12:35 AM, Junio C Hamano wrote:
> 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!
>
>> 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 &&

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

[Rob Hoelz]

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

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

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

[PATCH] push: Alias pushurl from push rewrites

[Rob Hoelz]

git push currently doesn't consider pushInsteadOf when
using pushurl; this test tests 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

With the above configuration, the following occurs:

$ git push origin master
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.

This commit fixes that.

Signed-off-by: Rob Hoelz <rob@hoelz.ro>
---
 remote.c              |  2 +-
 t/t5516-fetch-push.sh | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 82 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..709f506 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -244,6 +244,87 @@ test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf
 	)
 '
 
+test_expect_success 'push with pushInsteadOf and explicit pushurl (pushurl + pushInsteadOf does rewrite in this case)' '
+	mk_empty &&
+	rm -rf ro rw &&
+	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 without pushInsteadOf and explicit pushurl (pushurl + insteadOf is used for rewrite)' '
+	mk_empty &&
+	rm -rf ro rw &&
+	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/.insteadOf" 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 pushInsteadOf but without explicit pushurl (url + pushInsteadOf is used for rewrite)' '
+	mk_empty &&
+	rm -rf ro rw &&
+	TRASH="$(pwd)/" &&
+	mkdir ro &&
+	mkdir rw &&
+	git init --bare rw/testrepo &&
+	git config "url.file://$TRASH/ro/.insteadOf" rw: &&
+	git config "url.file://$TRASH/rw/.pushInsteadOf" rw: &&
+	git config remote.r.url 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 without pushInsteadOf and without explicit pushurl (url + insteadOf is used for rewrite)' '
+	mk_empty &&
+	rm -rf ro rw &&
+	TRASH="$(pwd)/" &&
+	mkdir ro &&
+	mkdir rw &&
+	git init --bare rw/testrepo &&
+	git config "url.file://$TRASH/ro/.insteadOf" rw: &&
+	git config remote.r.url 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

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

[Jonathan Nieder]

Hi,

Rob Hoelz wrote:

> [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
>
> With the above configuration, the following occurs:
>
> $ git push origin master
> 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.

At first glance it is not always obvious how overlapping settings like
these should interact.  Thanks for an instructive example that makes
the right behavior obvious.

Test nits:

[...]
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -244,6 +244,87 @@ test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf
>  	)
>  '
>  
> +test_expect_success 'push with pushInsteadOf and explicit pushurl (pushurl + pushInsteadOf does rewrite in this case)' '
> +	mk_empty &&
> +	rm -rf ro rw &&
> +	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: &&

The surrounding tests don't do this, but I wonder if it would make
sense to use test_config instead of 'git config' here.

That way, the test's settings wouldn't affect other tests, and in
particular if someone later decides to refactor the file by reordering
tests, she could be confident that that would not break anything.

In most of the surrounding tests it does not matter because 'git
config' is run in a subdirectory that is not reused later.  Patches
fixing the exceptions below.

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

To produce more useful "./t5516-fetch-push.sh -v -i" output when the
comparison fails:

	echo "$the_commit commit refs/remotes/origin/master" >expect &&
	(
		cd rw/testrepo &&
		git for-each-ref refs/remotes/origin
	) >actual &&
	test_cmp expect actual

Hope that helps,

Jonathan Nieder (3):
  push test: use test_config where appropriate
  push test: simplify check of push result
  push test: rely on &&-chaining instead of 'if bad; then echo Oops; fi'

 t/t5516-fetch-push.sh | 156 +++++++++++++++++++++-----------------------------
 1 file changed, 65 insertions(+), 91 deletions(-)

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Test nits:
> ...
> Hope that helps,
>
> Jonathan Nieder (3):
>   push test: use test_config where appropriate
>   push test: simplify check of push result
>   push test: rely on &&-chaining instead of 'if bad; then echo Oops; fi'

Are these supposed to be follow-up patches?  Preparatory steps that
Rob can reroll on top?  Something else?

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

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> Test nits:
>> ...
>> Hope that helps,
>>
>> Jonathan Nieder (3):
>>   push test: use test_config where appropriate
>>   push test: simplify check of push result
>>   push test: rely on &&-chaining instead of 'if bad; then echo Oops; fi'
>
> Are these supposed to be follow-up patches?  Preparatory steps that
> Rob can reroll on top?  Something else?

Preparatory steps.

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>>> Test nits:
>>> ...
>>> Hope that helps,
>>>
>>> Jonathan Nieder (3):
>>>   push test: use test_config where appropriate
>>>   push test: simplify check of push result
>>>   push test: rely on &&-chaining instead of 'if bad; then echo Oops; fi'
>>
>> Are these supposed to be follow-up patches?  Preparatory steps that
>> Rob can reroll on top?  Something else?
>
> Preparatory steps.

OK, thanks.  I'll queue these first then.

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

[Rob Hoelz]

On 3/19/13 7:08 PM, Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Junio C Hamano wrote:
>>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>>> Test nits:
>>>> ...
>>>> Hope that helps,
>>>>
>>>> Jonathan Nieder (3):
>>>>   push test: use test_config where appropriate
>>>>   push test: simplify check of push result
>>>>   push test: rely on &&-chaining instead of 'if bad; then echo Oops; fi'
>>> Are these supposed to be follow-up patches?  Preparatory steps that
>>> Rob can reroll on top?  Something else?
>> Preparatory steps.
> OK, thanks.  I'll queue these first then.
>
Should I apply these to my patch to move things along?  What's the next
step for me?

Thanks,
Rob

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

[Junio C Hamano]

Rob Hoelz <rob@hoelz.ro> writes:

> On 3/19/13 7:08 PM, Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>
>>> Junio C Hamano wrote:
>>>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>>>> Test nits:
>>>>> ...
>>>>> Hope that helps,
>>>>>
>>>>> Jonathan Nieder (3):
>>>>>   push test: use test_config where appropriate
>>>>>   push test: simplify check of push result
>>>>>   push test: rely on &&-chaining instead of 'if bad; then echo Oops; fi'
>>>> Are these supposed to be follow-up patches?  Preparatory steps that
>>>> Rob can reroll on top?  Something else?
>>> Preparatory steps.
>> OK, thanks.  I'll queue these first then.
>>
> Should I apply these to my patch to move things along?  What's the next
> step for me?

You would fetch from nearby git.git mirror, find the tip of
Janathan's series and redo your patch on top.  Perhaps the session
would go like this:

    $ git fetch git://git.kernel.org/pub/scm/git/git.git/ pu
    $ git log --oneline --first-parent ..FETCH_HEAD | grep jn/push-tests
    83a072a Merge branch 'jn/push-tests' into pu
    $ git checkout -b rh/push-pushurl-pushinsteadof 83a072a
    ... redo the work, perhaps with combinations of:
    $ git cherry-pick -n $your_original_branch
    $ edit t/t5516-fetch-push.sh
    ... and then
    $ make test
    $ git commit

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

[Rob Hoelz]

On Wed, 20 Mar 2013 07:35:58 -0700
Junio C Hamano <gitster@pobox.com> wrote:

> Rob Hoelz <rob@hoelz.ro> writes:
> 
> > On 3/19/13 7:08 PM, Junio C Hamano wrote:
> >> Jonathan Nieder <jrnieder@gmail.com> writes:
> >>
> >>> Junio C Hamano wrote:
> >>>> Jonathan Nieder <jrnieder@gmail.com> writes:
> >>>>> Test nits:
> >>>>> ...
> >>>>> Hope that helps,
> >>>>>
> >>>>> Jonathan Nieder (3):
> >>>>>   push test: use test_config where appropriate
> >>>>>   push test: simplify check of push result
> >>>>>   push test: rely on &&-chaining instead of 'if bad; then echo
> >>>>> Oops; fi'
> >>>> Are these supposed to be follow-up patches?  Preparatory steps
> >>>> that Rob can reroll on top?  Something else?
> >>> Preparatory steps.
> >> OK, thanks.  I'll queue these first then.
> >>
> > Should I apply these to my patch to move things along?  What's the
> > next step for me?
> 
> You would fetch from nearby git.git mirror, find the tip of
> Janathan's series and redo your patch on top.  Perhaps the session
> would go like this:
> 
>     $ git fetch git://git.kernel.org/pub/scm/git/git.git/ pu
>     $ git log --oneline --first-parent ..FETCH_HEAD | grep
> jn/push-tests 83a072a Merge branch 'jn/push-tests' into pu
>     $ git checkout -b rh/push-pushurl-pushinsteadof 83a072a
>     ... redo the work, perhaps with combinations of:
>     $ git cherry-pick -n $your_original_branch
>     $ edit t/t5516-fetch-push.sh
>     ... and then
>     $ make test
>     $ git commit
> 

Ok, changes applied.  New patch coming.

[PATCH] push: Alias pushurl from push rewrites

[Rob Hoelz]

git push currently doesn't consider pushInsteadOf when
using pushurl; this test tests 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

With the above configuration, the following occurs:

$ git push origin master
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.

This commit fixes that.

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

diff --git a/remote.c b/remote.c
index e53a6eb..1ea240a 100644
--- a/remote.c
+++ b/remote.c
@@ -465,7 +465,11 @@ 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);
+			char *copy = xstrdup(remotes[i]->pushurl[j]);
+			remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites_push);
+			if (!strcmp(copy, remotes[i]->pushurl[j]))
+				remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites);
+			free(copy);
 		}
 		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 c31e5c1..119f043 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -244,6 +244,83 @@ test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf
 	)
 '
 
+test_expect_success 'push with pushInsteadOf and explicit pushurl (pushurl + pushInsteadOf does rewrite in this case)' '
+	mk_empty &&
+	rm -rf ro rw &&
+	TRASH="$(pwd)/" &&
+	mkdir ro &&
+	mkdir rw &&
+	git init --bare rw/testrepo &&
+	test_config "url.file://$TRASH/ro/.insteadOf" ro: &&
+	test_config "url.file://$TRASH/rw/.pushInsteadOf" rw: &&
+	test_config remote.r.url ro:wrong &&
+	test_config remote.r.pushurl rw:testrepo &&
+	git push r refs/heads/master:refs/remotes/origin/master &&
+	(
+		cd rw/testrepo &&
+		echo "$the_commit commit	refs/remotes/origin/master" > expected &&
+		git for-each-ref refs/remotes/origin > actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'push without pushInsteadOf and explicit pushurl (pushurl + insteadOf is used for rewrite)' '
+	mk_empty &&
+	rm -rf ro rw &&
+	TRASH="$(pwd)/" &&
+	mkdir ro &&
+	mkdir rw &&
+	git init --bare rw/testrepo &&
+	test_config "url.file://$TRASH/ro/.insteadOf" ro: &&
+	test_config "url.file://$TRASH/rw/.insteadOf" rw: &&
+	test_config remote.r.url ro:wrong &&
+	test_config remote.r.pushurl rw:testrepo &&
+	git push r refs/heads/master:refs/remotes/origin/master &&
+	(
+		cd rw/testrepo &&
+		echo "$the_commit commit	refs/remotes/origin/master" > expected &&
+		git for-each-ref refs/remotes/origin > actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'push with pushInsteadOf but without explicit pushurl (url + pushInsteadOf is used for rewrite)' '
+	mk_empty &&
+	rm -rf ro rw &&
+	TRASH="$(pwd)/" &&
+	mkdir ro &&
+	mkdir rw &&
+	git init --bare rw/testrepo &&
+	test_config "url.file://$TRASH/ro/.insteadOf" rw: &&
+	test_config "url.file://$TRASH/rw/.pushInsteadOf" rw: &&
+	test_config remote.r.url rw:testrepo &&
+	git push r refs/heads/master:refs/remotes/origin/master &&
+	(
+		cd rw/testrepo &&
+		echo "$the_commit commit	refs/remotes/origin/master" > expected &&
+		git for-each-ref refs/remotes/origin > actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'push without pushInsteadOf and without explicit pushurl (url + insteadOf is used for rewrite)' '
+	mk_empty &&
+	rm -rf ro rw &&
+	TRASH="$(pwd)/" &&
+	mkdir ro &&
+	mkdir rw &&
+	git init --bare rw/testrepo &&
+	test_config "url.file://$TRASH/rw/.insteadOf" rw: &&
+	test_config remote.r.url rw:testrepo &&
+	git push r refs/heads/master:refs/remotes/origin/master &&
+	(
+		cd rw/testrepo &&
+		echo "$the_commit commit	refs/remotes/origin/master" > expected &&
+		git for-each-ref refs/remotes/origin > actual &&
+		test_cmp expected actual
+	)
+'
+
 test_expect_success 'push with matching heads' '
 
 	mk_test heads/master &&
-- 
1.8.2

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

[Jonathan Nieder]

Rob Hoelz wrote:

> --- a/remote.c
> +++ b/remote.c
> @@ -465,7 +465,11 @@ 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);
> +			char *copy = xstrdup(remotes[i]->pushurl[j]);
> +			remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites_push);
> +			if (!strcmp(copy, remotes[i]->pushurl[j]))
> +				remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites);
> +			free(copy);

Interesting.

Suppose I configure

	[url "git://anongit.myserver.example.com/"]
		insteadOf = myserver.example.com:
	[url "myserver:"]
		pushInsteadOf = myserver.example.com:

The above code would make the insteadOf rule apply instead of
pushInsteadOf, even when pushing.  Perhaps something like the
following would work?

			const char *url = remotes[i]->pushurl[j];
			remotes[i]->pushurl[j] = alias_url(url, &rewrites_push);
			if (remotes[i]->pushurl[j] == url)
				/* No url.*.pushinsteadof configuration matched. */
				remotes[i]->pushurl[j] = alias_url(url, &rewrites);

> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -244,6 +244,83 @@ test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf
>  	)
>  '
>  
> +test_expect_success 'push with pushInsteadOf and explicit pushurl (pushurl + pushInsteadOf does rewrite in this case)' '
> +	mk_empty &&
> +	rm -rf ro rw &&
> +	TRASH="$(pwd)/" &&
> +	mkdir ro &&
> +	mkdir rw &&
> +	git init --bare rw/testrepo &&
> +	test_config "url.file://$TRASH/ro/.insteadOf" ro: &&
> +	test_config "url.file://$TRASH/rw/.pushInsteadOf" rw: &&
> +	test_config remote.r.url ro:wrong &&
> +	test_config remote.r.pushurl rw:testrepo &&
> +	git push r refs/heads/master:refs/remotes/origin/master &&
> +	(
> +		cd rw/testrepo &&
> +		echo "$the_commit commit	refs/remotes/origin/master" > expected &&
> +		git for-each-ref refs/remotes/origin > actual &&
> +		test_cmp expected actual
> +	)

Looks good.  The usual style in git tests is to include no space
after >redirection operators:

		git for-each-ref refs/remotes/origin >actual &&

Hope that helps,
Jonathan

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

[Jonathan Nieder]

Jonathan Nieder wrote:
> Rob Hoelz wrote:

>> --- a/remote.c
>> +++ b/remote.c
>> @@ -465,7 +465,11 @@ static void alias_all_urls(void)
[...]
>> -			remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites);
>> +			char *copy = xstrdup(remotes[i]->pushurl[j]);
>> +			remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites_push);
>> +			if (!strcmp(copy, remotes[i]->pushurl[j]))
>> +				remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites);
>> +			free(copy);
>
> Interesting.
>
> Suppose I configure
> 
> 	[url "git://anongit.myserver.example.com/"]
> 		insteadOf = myserver.example.com:
> 	[url "myserver:"]
> 		pushInsteadOf = myserver.example.com:
>
> The above code would make the insteadOf rule apply instead of
> pushInsteadOf, even when pushing.

Sorry, typo.  The configuration in the example above should have been

	[url "git://anongit.myserver.example.com/"]
		insteadOf = myserver.example.com:
	[url "myserver.example.com:"]
		pushInsteadOf = myserver.example.com:

In other words, suppose I set url.*.insteadof to point to a faster
address for fetching alongside url.*.pushinsteadof requesting that the
original address should still be used for pushing.

Thanks again for tackling this.
Jonathan

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Sorry, typo.  The configuration in the example above should have been
>
> 	[url "git://anongit.myserver.example.com/"]
> 		insteadOf = myserver.example.com:
> 	[url "myserver.example.com:"]
> 		pushInsteadOf = myserver.example.com:
>
> In other words, suppose I set url.*.insteadof to point to a faster
> address for fetching alongside url.*.pushinsteadof requesting that the
> original address should still be used for pushing.

I didn't know we were even shooting for supporting the identity
mapping:

	url.X.pushinsteadof=X

but that would certainly be nice.

By the way, it seems that the original commit 1c2eafb89bca (Add
url.<base>.pushInsteadOf: URL rewriting for push only, 2009-09-07)
wanted to explicitly avoid use of pushInsteadOf aliasing for a
pushURL and it is also documented in config.txt from day one.

I think the intent is "You have a normal URL, and a way to override
it explicitly with pushURL, or a way to rewrite it via the aliasing
the normal URL with pushInsteadOf. Either one or the other, but not
both, as having many levels of indirection would be confusing."

Which I can understand and sympathise.

In anay case, the change proposed in this thread seems to change
that, so the documentation would need to be updated.  Also the tests
the original commit adds explicitly checks that pushInsteadOf is
ignored, which may have to be updated (unless that test is already
broken).

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

[Rob Hoelz]

On Wed, 27 Mar 2013 15:07:18 -0700
Junio C Hamano <gitster@pobox.com> wrote:

> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> > Sorry, typo.  The configuration in the example above should have
> > been
> >
> > 	[url "git://anongit.myserver.example.com/"]
> > 		insteadOf = myserver.example.com:
> > 	[url "myserver.example.com:"]
> > 		pushInsteadOf = myserver.example.com:
> >
> > In other words, suppose I set url.*.insteadof to point to a faster
> > address for fetching alongside url.*.pushinsteadof requesting that
> > the original address should still be used for pushing.
> 
> I didn't know we were even shooting for supporting the identity
> mapping:
> 
> 	url.X.pushinsteadof=X
> 
> but that would certainly be nice.
> 
> By the way, it seems that the original commit 1c2eafb89bca (Add
> url.<base>.pushInsteadOf: URL rewriting for push only, 2009-09-07)
> wanted to explicitly avoid use of pushInsteadOf aliasing for a
> pushURL and it is also documented in config.txt from day one.
> 
> I think the intent is "You have a normal URL, and a way to override
> it explicitly with pushURL, or a way to rewrite it via the aliasing
> the normal URL with pushInsteadOf. Either one or the other, but not
> both, as having many levels of indirection would be confusing."
> 
> Which I can understand and sympathise.
> 
> In anay case, the change proposed in this thread seems to change
> that, so the documentation would need to be updated.  Also the tests
> the original commit adds explicitly checks that pushInsteadOf is
> ignored, which may have to be updated (unless that test is already
> broken).
> 

My use case is that I use Github for my personal development.  I have a
prefix for my personal repos (hoelzro: -> git://github.com/hoelzro for
fetch, git@github.com:hoelzro/ for push) and one for all other Git repos
(github: -> git://github.com/)  I have a few projects where I work in a
fork, but I want to fetch updates from the original project.  So my url
for the origin remote is github:org/project, but my pushurl is
hoelzro:project.  This behavior in Git currently doesn't allow me to
work that way.  I used to work with two remotes; origin for my repo and
base for the official one, but I've found that I prefer this other way.

The test that checked that pushInsteadOf + pushurl shouldn't work as I
expect was actually broken; I have removed it, updated the
documentation, and sent a new patch to the list.

-Rob

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

[Josh Triplett]

On Wed, Mar 27, 2013 at 05:48:45PM -0500, Rob Hoelz wrote:
> On Wed, 27 Mar 2013 15:07:18 -0700
> Junio C Hamano <gitster@pobox.com> wrote:
> 
> > Jonathan Nieder <jrnieder@gmail.com> writes:
> > 
> > > Sorry, typo.  The configuration in the example above should have
> > > been
> > >
> > > 	[url "git://anongit.myserver.example.com/"]
> > > 		insteadOf = myserver.example.com:
> > > 	[url "myserver.example.com:"]
> > > 		pushInsteadOf = myserver.example.com:
> > >
> > > In other words, suppose I set url.*.insteadof to point to a faster
> > > address for fetching alongside url.*.pushinsteadof requesting that
> > > the original address should still be used for pushing.
> > 
> > I didn't know we were even shooting for supporting the identity
> > mapping:
> > 
> > 	url.X.pushinsteadof=X
> > 
> > but that would certainly be nice.
> > 
> > By the way, it seems that the original commit 1c2eafb89bca (Add
> > url.<base>.pushInsteadOf: URL rewriting for push only, 2009-09-07)
> > wanted to explicitly avoid use of pushInsteadOf aliasing for a
> > pushURL and it is also documented in config.txt from day one.
> > 
> > I think the intent is "You have a normal URL, and a way to override
> > it explicitly with pushURL, or a way to rewrite it via the aliasing
> > the normal URL with pushInsteadOf. Either one or the other, but not
> > both, as having many levels of indirection would be confusing."
> > 
> > Which I can understand and sympathise.
> > 
> > In anay case, the change proposed in this thread seems to change
> > that, so the documentation would need to be updated.  Also the tests
> > the original commit adds explicitly checks that pushInsteadOf is
> > ignored, which may have to be updated (unless that test is already
> > broken).
> > 
> 
> My use case is that I use Github for my personal development.  I have a
> prefix for my personal repos (hoelzro: -> git://github.com/hoelzro for
> fetch, git@github.com:hoelzro/ for push) and one for all other Git repos
> (github: -> git://github.com/)  I have a few projects where I work in a
> fork, but I want to fetch updates from the original project.  So my url
> for the origin remote is github:org/project, but my pushurl is
> hoelzro:project.  This behavior in Git currently doesn't allow me to
> work that way.  I used to work with two remotes; origin for my repo and
> base for the official one, but I've found that I prefer this other way.
> 
> The test that checked that pushInsteadOf + pushurl shouldn't work as I
> expect was actually broken; I have removed it, updated the
> documentation, and sent a new patch to the list.

There's an argument for either behavior as valid.  My original patch
specifically documented and tested for the opposite behavior, namely
that pushurl overrides any pushInsteadOf, because I intended
pushInsteadOf as a fallback if you don't have an explicit pushurl set.
For instance, you could use pushInsteadOf to rewrite a family of
anonymous git URLs to corresponding pushable repositories, but then use
an explicit pushurl to override that for a specific repository.  This
change would break the ability to use pushurl for its original intended
purpose, namely having a local repository where fetch comes from one
remote repo and push goes to another.

One use case of mine: I have a .gitconfig in my git-managed home
directory which sets pushInsteadOf so that I can clone via git:// and
immediately have working push.  I work with a number of systems that
don't have inbound access to each other but do have outbound access to
the network; on some of these "satellite" boxes, I can't push changes
directly to the server pushInsteadOf points to, so I can explicitly set
pushurl in .git/config for that repository, which overrides the
pushInsteadOf.  This change would break that configuration.

- Josh Triplett

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

[Josh Triplett]

On Wed, Mar 27, 2013 at 04:09:43PM -0700, Josh Triplett wrote:
> On Wed, Mar 27, 2013 at 05:48:45PM -0500, Rob Hoelz wrote:
> > On Wed, 27 Mar 2013 15:07:18 -0700
> > Junio C Hamano <gitster@pobox.com> wrote:
> > 
> > > Jonathan Nieder <jrnieder@gmail.com> writes:
> > > 
> > > > Sorry, typo.  The configuration in the example above should have
> > > > been
> > > >
> > > > 	[url "git://anongit.myserver.example.com/"]
> > > > 		insteadOf = myserver.example.com:
> > > > 	[url "myserver.example.com:"]
> > > > 		pushInsteadOf = myserver.example.com:
> > > >
> > > > In other words, suppose I set url.*.insteadof to point to a faster
> > > > address for fetching alongside url.*.pushinsteadof requesting that
> > > > the original address should still be used for pushing.
> > > 
> > > I didn't know we were even shooting for supporting the identity
> > > mapping:
> > > 
> > > 	url.X.pushinsteadof=X
> > > 
> > > but that would certainly be nice.
> > > 
> > > By the way, it seems that the original commit 1c2eafb89bca (Add
> > > url.<base>.pushInsteadOf: URL rewriting for push only, 2009-09-07)
> > > wanted to explicitly avoid use of pushInsteadOf aliasing for a
> > > pushURL and it is also documented in config.txt from day one.
> > > 
> > > I think the intent is "You have a normal URL, and a way to override
> > > it explicitly with pushURL, or a way to rewrite it via the aliasing
> > > the normal URL with pushInsteadOf. Either one or the other, but not
> > > both, as having many levels of indirection would be confusing."
> > > 
> > > Which I can understand and sympathise.
> > > 
> > > In anay case, the change proposed in this thread seems to change
> > > that, so the documentation would need to be updated.  Also the tests
> > > the original commit adds explicitly checks that pushInsteadOf is
> > > ignored, which may have to be updated (unless that test is already
> > > broken).
> > > 
> > 
> > My use case is that I use Github for my personal development.  I have a
> > prefix for my personal repos (hoelzro: -> git://github.com/hoelzro for
> > fetch, git@github.com:hoelzro/ for push) and one for all other Git repos
> > (github: -> git://github.com/)  I have a few projects where I work in a
> > fork, but I want to fetch updates from the original project.  So my url
> > for the origin remote is github:org/project, but my pushurl is
> > hoelzro:project.  This behavior in Git currently doesn't allow me to
> > work that way.  I used to work with two remotes; origin for my repo and
> > base for the official one, but I've found that I prefer this other way.
> > 
> > The test that checked that pushInsteadOf + pushurl shouldn't work as I
> > expect was actually broken; I have removed it, updated the
> > documentation, and sent a new patch to the list.
> 
> There's an argument for either behavior as valid.  My original patch
> specifically documented and tested for the opposite behavior, namely
> that pushurl overrides any pushInsteadOf, because I intended
> pushInsteadOf as a fallback if you don't have an explicit pushurl set.
> For instance, you could use pushInsteadOf to rewrite a family of
> anonymous git URLs to corresponding pushable repositories, but then use
> an explicit pushurl to override that for a specific repository.  This
> change would break the ability to use pushurl for its original intended
> purpose, namely having a local repository where fetch comes from one
> remote repo and push goes to another.
> 
> One use case of mine: I have a .gitconfig in my git-managed home
> directory which sets pushInsteadOf so that I can clone via git:// and
> immediately have working push.  I work with a number of systems that
> don't have inbound access to each other but do have outbound access to
> the network; on some of these "satellite" boxes, I can't push changes
> directly to the server pushInsteadOf points to, so I can explicitly set
> pushurl in .git/config for that repository, which overrides the
> pushInsteadOf.  This change would break that configuration.

Clarifying this use case a bit: note that it's been a while since I had
many such boxes, so I don't actually have any systems currently using
that pushurl configuration.  Still a regression in defined behavior,
though.

Why not just use insteadOf for your personal github prefix hoelzro:, and
both insteadOf and pushInsteadOf for github: in general?  Then, a
repository cloned via github: would work for pull and push (if you have
push access), and you can change pushurl to your personal github alias
if needed.

Though, as Junio said, the modern push-updates-remote-heads behavior of
git means that both of our configurations arguably seem wrong, and we
should both just use separate remotes for separate repos.

- Josh Triplett

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

[Jonathan Nieder]

Josh Triplett wrote:

>                       I have a .gitconfig in my git-managed home
> directory which sets pushInsteadOf so that I can clone via git:// and
> immediately have working push.  I work with a number of systems that
> don't have inbound access to each other but do have outbound access to
> the network; on some of these "satellite" boxes, I can't push changes
> directly to the server pushInsteadOf points to, so I can explicitly set
> pushurl in .git/config for that repository, which overrides the
> pushInsteadOf.  This change would break that configuration.

Would it?  As long as your pushurl does not start with git://, I think
your configuration would still work fine.

After this patch, neither pushInsteadOf nor pushUrl overrides the
other one.  The rule is:

	1. First, get the URL from the remote's configuration, based
	   on whether you are fetching or pushing.

	   (At this step, in your setup git chooses the URL specified
	   with pushurl in your .git/config.)
	
	2. Next, apply the most appropriate url.*.insteadOf or
	   url.*.pushInsteadOf rule, based on whether you are fetching
	   or pushing.

	   (At this step, no rewrite rules apply, so the URL is used
	   as is.)

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Josh Triplett wrote:
>
>>                       I have a .gitconfig in my git-managed home
>> directory which sets pushInsteadOf so that I can clone via git:// and
>> immediately have working push.  I work with a number of systems that
>> don't have inbound access to each other but do have outbound access to
>> the network; on some of these "satellite" boxes, I can't push changes
>> directly to the server pushInsteadOf points to, so I can explicitly set
>> pushurl in .git/config for that repository, which overrides the
>> pushInsteadOf.  This change would break that configuration.
>
> Would it?  As long as your pushurl does not start with git://, I think
> your configuration would still work fine.

That is a good point, especially because it is very unlikely that
git:// was used for pushURL, given that it would not have been
rewritten with pushInsteadOf to an authenticated transport.

> After this patch, neither pushInsteadOf nor pushUrl overrides the
> other one.  The rule is:
>
> 	1. First, get the URL from the remote's configuration, based
> 	   on whether you are fetching or pushing.
>
> 	   (At this step, in your setup git chooses the URL specified
> 	   with pushurl in your .git/config.)
> 	
> 	2. Next, apply the most appropriate url.*.insteadOf or
> 	   url.*.pushInsteadOf rule, based on whether you are fetching
> 	   or pushing.
>
> 	   (At this step, no rewrite rules apply, so the URL is used
> 	   as is.)

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

[Josh Triplett]

On Wed, Mar 27, 2013 at 04:18:19PM -0700, Jonathan Nieder wrote:
> Josh Triplett wrote:
> 
> >                       I have a .gitconfig in my git-managed home
> > directory which sets pushInsteadOf so that I can clone via git:// and
> > immediately have working push.  I work with a number of systems that
> > don't have inbound access to each other but do have outbound access to
> > the network; on some of these "satellite" boxes, I can't push changes
> > directly to the server pushInsteadOf points to, so I can explicitly set
> > pushurl in .git/config for that repository, which overrides the
> > pushInsteadOf.  This change would break that configuration.
> 
> Would it?  As long as your pushurl does not start with git://, I think
> your configuration would still work fine.

I had to think about it for a while, but I think you're right; I
inferred a behavior that the patch didn't actually add or have anything
to do with, namely having the result of applying pushInsteadOf to the
non-push URL override the pushUrl.

OK, I take it back.  I *can* imagine configurations that this change
would break, since it does change intentional and documented behavior,
but I don't have any such configuration.  The only such configuration I
can imagine involves directly counting on the non-rewriting of pushUrl,
by using pushInsteadOf to rewrite urls and then sometimes using pushUrl
to override that and point back at the un-rewritten URL.  And while
supported, that does seem *odd*.

Objection withdrawn; if nobody can come up with a sensible configuration
that relies on the documented behavior, I don't particularly care if it
changes.

> After this patch, neither pushInsteadOf nor pushUrl overrides the
> other one.  The rule is:
> 
> 	1. First, get the URL from the remote's configuration, based
> 	   on whether you are fetching or pushing.
> 
> 	   (At this step, in your setup git chooses the URL specified
> 	   with pushurl in your .git/config.)
> 	
> 	2. Next, apply the most appropriate url.*.insteadOf or
> 	   url.*.pushInsteadOf rule, based on whether you are fetching
> 	   or pushing.
> 
> 	   (At this step, no rewrite rules apply, so the URL is used
> 	   as is.)

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

[Junio C Hamano]

Josh Triplett <josh@joshtriplett.org> writes:

> OK, I take it back.  I *can* imagine configurations that this change
> would break, since it does change intentional and documented behavior,
> but I don't have any such configuration.  The only such configuration I
> can imagine involves directly counting on the non-rewriting of pushUrl,
> by using pushInsteadOf to rewrite urls and then sometimes using pushUrl
> to override that and point back at the un-rewritten URL.  And while
> supported, that does seem *odd*.
>
> Objection withdrawn; if nobody can come up with a sensible configuration
> that relies on the documented behavior, I don't particularly care if it
> changes.

I actually do.

Given the popularity of the system, "people involved in this thread
cannot imagine a case that existing people may get hurt" is very
different from "this is not a regression".  After merging this
change when people start complaining, you and Rob can hide and
ignore them, but we collectively as the Git project have to have a
way to help them when it happens.

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

[Josh Triplett]

On Thu, Mar 28, 2013 at 09:10:59AM -0700, Junio C Hamano wrote:
> Josh Triplett <josh@joshtriplett.org> writes:
> 
> > OK, I take it back.  I *can* imagine configurations that this change
> > would break, since it does change intentional and documented behavior,
> > but I don't have any such configuration.  The only such configuration I
> > can imagine involves directly counting on the non-rewriting of pushUrl,
> > by using pushInsteadOf to rewrite urls and then sometimes using pushUrl
> > to override that and point back at the un-rewritten URL.  And while
> > supported, that does seem *odd*.
> >
> > Objection withdrawn; if nobody can come up with a sensible configuration
> > that relies on the documented behavior, I don't particularly care if it
> > changes.
> 
> I actually do.
> 
> Given the popularity of the system, "people involved in this thread
> cannot imagine a case that existing people may get hurt" is very
> different from "this is not a regression".  After merging this
> change when people start complaining, you and Rob can hide and
> ignore them, but we collectively as the Git project have to have a
> way to help them when it happens.

I entirely agree that it represents a regression from documented
behavior; I just mean that it no longer matches a specific use case I
had in mind with the original change.  I agree that we should hesitate
to change that documented behavior.

- Josh Triplett

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

[Junio C Hamano]

Josh Triplett <josh@joshtriplett.org> writes:

> On Wed, Mar 27, 2013 at 05:48:45PM -0500, Rob Hoelz wrote:
> ...
>> The test that checked that pushInsteadOf + pushurl shouldn't work as I
>> expect was actually broken; I have removed it, updated the
>> documentation, and sent a new patch to the list.
>
> There's an argument for either behavior as valid.  My original patch
> specifically documented and tested for the opposite behavior, namely
> that pushurl overrides any pushInsteadOf, because I intended
> pushInsteadOf as a fallback if you don't have an explicit pushurl set.

For only this bit.

I think the test in question is this one from t5516:

test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '
	mk_empty &&
	TRASH="$(pwd)/" &&
	git config "url.trash2/.pushInsteadOf" trash/ &&
	git config remote.r.url trash/wrong &&
	git config remote.r.pushurl "$TRASH/testrepo" &&
	git push r refs/heads/master:refs/remotes/origin/master &&
	(
		cd 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)
	)
'

It defines a remote "r", with URL "trash/wrong" (used for fetch) and
pushURL "$(pwd)/testrepo" (used for push).  There is a pushInsteadOf
rule to rewrite anything that goes to "trash/*" to be pushed to
"trash2/*" instead but that shouldn't be used to rewrite an explicit
pushURL.

And then the test pushes to "r" and checks if testrepo gets updated;
in other words, it wants to make sure remote.r.pushURL defines the
final destination, without pushInsteadOf getting in the way.

But $(pwd)/testrepo does not match trash/* in the first place, so
there is no chance for a pushInsteadOf to interfere; it looks to me
that it is not testing what it wants to test.

Perhaps we should do something like this?  We tell it to push to
"testrepo/" with pushURL, and set up a pushInsteadOf to rewrite
"testrepo/" to "trash2/", but because for this push it comes from an
explicit pushURL, it still goes to "testrepo/".

As we do not have "trash2/" repository, the test not just tests the
push goes to "testrepo/", but it also tests that it does not attempt
to push to "trash2/", checking both sides of the coin.

 t/t5516-fetch-push.sh | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index d3dc5df..b5ea32c 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -230,10 +230,9 @@ test_expect_success 'push with pushInsteadOf' '
 
 test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '
 	mk_empty &&
-	TRASH="$(pwd)/" &&
-	git config "url.trash2/.pushInsteadOf" trash/ &&
+	git config "url.trash2/.pushInsteadOf" testrepo/ &&
 	git config remote.r.url trash/wrong &&
-	git config remote.r.pushurl "$TRASH/testrepo" &&
+	git config remote.r.pushurl "testrepo/" &&
 	git push r refs/heads/master:refs/remotes/origin/master &&
 	(
 		cd testrepo &&

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

[Josh Triplett]

On Thu, Mar 28, 2013 at 08:37:58AM -0700, Junio C Hamano wrote:
> Josh Triplett <josh@joshtriplett.org> writes:
> 
> > On Wed, Mar 27, 2013 at 05:48:45PM -0500, Rob Hoelz wrote:
> > ...
> >> The test that checked that pushInsteadOf + pushurl shouldn't work as I
> >> expect was actually broken; I have removed it, updated the
> >> documentation, and sent a new patch to the list.
> >
> > There's an argument for either behavior as valid.  My original patch
> > specifically documented and tested for the opposite behavior, namely
> > that pushurl overrides any pushInsteadOf, because I intended
> > pushInsteadOf as a fallback if you don't have an explicit pushurl set.
> 
> For only this bit.
> 
> I think the test in question is this one from t5516:
> 
> test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '
> 	mk_empty &&
> 	TRASH="$(pwd)/" &&
> 	git config "url.trash2/.pushInsteadOf" trash/ &&
> 	git config remote.r.url trash/wrong &&
> 	git config remote.r.pushurl "$TRASH/testrepo" &&
> 	git push r refs/heads/master:refs/remotes/origin/master &&
> 	(
> 		cd 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)
> 	)
> '
> 
> It defines a remote "r", with URL "trash/wrong" (used for fetch) and
> pushURL "$(pwd)/testrepo" (used for push).  There is a pushInsteadOf
> rule to rewrite anything that goes to "trash/*" to be pushed to
> "trash2/*" instead but that shouldn't be used to rewrite an explicit
> pushURL.
> 
> And then the test pushes to "r" and checks if testrepo gets updated;
> in other words, it wants to make sure remote.r.pushURL defines the
> final destination, without pushInsteadOf getting in the way.
> 
> But $(pwd)/testrepo does not match trash/* in the first place, so
> there is no chance for a pushInsteadOf to interfere; it looks to me
> that it is not testing what it wants to test.

That test does actually test something important: it tests that the
result of applying pushInsteadOf to url does *not* override pushurl.
Not the same thing as testing that pushInsteadOf does or does not apply
to pushurl.

The relevant sentence of the git-config manpage (in the documentation
for pushInsteadOf) says:
> If a remote has an explicit pushurl, git will ignore this setting for
> that remote.
That really meant what I just said above: git will prefer an explicit
pushurl over the pushInsteadOf rewrite of url.  It says nothing about
applying pushInsteadOf to rewrite pushurl.

> Perhaps we should do something like this?  We tell it to push to
> "testrepo/" with pushURL, and set up a pushInsteadOf to rewrite
> "testrepo/" to "trash2/", but because for this push it comes from an
> explicit pushURL, it still goes to "testrepo/".
> 
> As we do not have "trash2/" repository, the test not just tests the
> push goes to "testrepo/", but it also tests that it does not attempt
> to push to "trash2/", checking both sides of the coin.

Sensible test, assuming you want to enforce that behavior.  I don't
strongly care either way about that one, since it only applies if your
pushInsteadOf rewrites could apply to your pushurl, and I only ever use
pushInsteadOf to rewrite unpushable repos to pushable ones.  However...

>  t/t5516-fetch-push.sh | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index d3dc5df..b5ea32c 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -230,10 +230,9 @@ test_expect_success 'push with pushInsteadOf' '
>  
>  test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '
>  	mk_empty &&
> -	TRASH="$(pwd)/" &&
> -	git config "url.trash2/.pushInsteadOf" trash/ &&
> +	git config "url.trash2/.pushInsteadOf" testrepo/ &&
>  	git config remote.r.url trash/wrong &&
> -	git config remote.r.pushurl "$TRASH/testrepo" &&
> +	git config remote.r.pushurl "testrepo/" &&
>  	git push r refs/heads/master:refs/remotes/origin/master &&
>  	(
>  		cd testrepo &&

...the test you describe should appear in *addition* to this test, not
replacing it, because as described above this test does test one
critical bit of behavior, namely prefering pushurl over the
pushInsteadOf rewrite of url.

- Josh Triplett

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

[Junio C Hamano]

Josh Triplett <josh@joshtriplett.org> writes:

(on url.$base.pushInsteadOf)
>> If a remote has an explicit pushurl, git will ignore this setting for
>> that remote.
> That really meant what I just said above: git will prefer an explicit
> pushurl over the pushInsteadOf rewrite of url.

Very correct.

> It says nothing about
> applying pushInsteadOf to rewrite pushurl.

Incorrect, I think.  If you have pushURL, pushInsteadOf is *ignored*.
Of course, if you have both URL and pushURL, the ignored pushInsteadOf
will not apply to _anything_.  It will not apply to URL, and it will
certainly not apply to pushURL.

You are correct to point out that with the test we would want to
make sure that for a remote with pushURL and URL, a push goes

 - to pushURL;
 - not to URL;
 - not to insteadOf(URL);
 - not to pushInsteadOf(URL);
 - not to insteadOf(pushURL); and
 - not to pushInsteadOf(pushURL).

I do not think it is worth checking all of them, but I agree we
should make sure it does not go to pushInsteadOf(URL) which you
originally meant to check, and we should also make sure it does not
go to pushInsteadOf(pushURL).

>>  test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '
>>  	mk_empty &&
>> -	TRASH="$(pwd)/" &&
>> -	git config "url.trash2/.pushInsteadOf" trash/ &&
>> +	git config "url.trash2/.pushInsteadOf" testrepo/ &&

Adding

	git config "url.trash3/.pusnInsteadOf" trash/wrong &&

here should be sufficient for that, no?  If we mistakenly used URL
(i.e. trash/wrong) the push would fail.  If we mistakenly used
pushInsteadOf(URL), that is rewritten to trash3/ and again the push
would fail.  pushInsteadOf(pushURL) would go to trash2/ and that
would also fail.

We aren't checking insteadOf(URL) and insteadOf(pushURL) but
everything else is checked, I think, so we can do without replacing
anything.  We can just extend it, no?

>>  	git config remote.r.url trash/wrong &&
>> -	git config remote.r.pushurl "$TRASH/testrepo" &&
>> +	git config remote.r.pushurl "testrepo/" &&
>>  	git push r refs/heads/master:refs/remotes/origin/master &&
>>  	(
>>  		cd testrepo &&
>
> ...the test you describe should appear in *addition* to this test, not
> replacing it, because as described above this test does test one
> critical bit of behavior, namely prefering pushurl over the
> pushInsteadOf rewrite of url.
>
> - Josh Triplett

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

[Josh Triplett]

On Thu, Mar 28, 2013 at 11:50:08AM -0700, Junio C Hamano wrote:
> Josh Triplett <josh@joshtriplett.org> writes:
> (on url.$base.pushInsteadOf)
> >> If a remote has an explicit pushurl, git will ignore this setting for
> >> that remote.
> > That really meant what I just said above: git will prefer an explicit
> > pushurl over the pushInsteadOf rewrite of url.
> 
> Very correct.
> 
> > It says nothing about
> > applying pushInsteadOf to rewrite pushurl.
> 
> Incorrect, I think.  If you have pushURL, pushInsteadOf is *ignored*.
> Of course, if you have both URL and pushURL, the ignored pushInsteadOf
> will not apply to _anything_.  It will not apply to URL, and it will
> certainly not apply to pushURL.

Debatable whether the documentation sentence above really says that; it
certainly doesn't say it very clearly if so.  But that does match the
actual behavior, making the proposed change a regression from the actual
behavior, whether the documentation clearly guarantees that behavior or
not.

> You are correct to point out that with the test we would want to
> make sure that for a remote with pushURL and URL, a push goes
> 
>  - to pushURL;
>  - not to URL;
>  - not to insteadOf(URL);
>  - not to pushInsteadOf(URL);
>  - not to insteadOf(pushURL); and
>  - not to pushInsteadOf(pushURL).
> 
> I do not think it is worth checking all of them, but I agree we
> should make sure it does not go to pushInsteadOf(URL) which you
> originally meant to check, and we should also make sure it does not
> go to pushInsteadOf(pushURL).

Agreed.

Related to this, as a path forward, I do think it makes sense to have a
setting usable as an insteadOf that only applies to pushurl, even though
pushInsteadOf won't end up serving that purpose.  That way,
pushInsteadOf covers the "map read-only repo url to pushable repo url"
case, and insteadOfPushOnly covers the "construct a magic prefix that
maps to different urls when used in url or pushurl" case.

> >>  test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '
> >>  	mk_empty &&
> >> -	TRASH="$(pwd)/" &&
> >> -	git config "url.trash2/.pushInsteadOf" trash/ &&
> >> +	git config "url.trash2/.pushInsteadOf" testrepo/ &&
> 
> Adding
> 
> 	git config "url.trash3/.pusnInsteadOf" trash/wrong &&
> 
> here should be sufficient for that, no?  If we mistakenly used URL
> (i.e. trash/wrong) the push would fail.  If we mistakenly used
> pushInsteadOf(URL), that is rewritten to trash3/ and again the push
> would fail.  pushInsteadOf(pushURL) would go to trash2/ and that
> would also fail.
> 
> We aren't checking insteadOf(URL) and insteadOf(pushURL) but
> everything else is checked, I think, so we can do without replacing
> anything.  We can just extend it, no?

That sounds sensible.

- Josh Triplett

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

[Jonathan Nieder]

Josh Triplett wrote:

> Related to this, as a path forward, I do think it makes sense to have a
> setting usable as an insteadOf that only applies to pushurl, even though
> pushInsteadOf won't end up serving that purpose.  That way,
> pushInsteadOf covers the "map read-only repo url to pushable repo url"
> case, and insteadOfPushOnly covers the "construct a magic prefix that
> maps to different urls when used in url or pushurl" case.

I hope not.  That would be making configuration even more complicated.

I hope that we can fix the documentation, tests, and change
description in the commit message enough to make Rob's patch a
no-brainer.  If that's not possible, I think the current state is
livable, just confusing.

I was happy to see Rob's patch because it brings git's behavior closer
to following the principle of least surprise.  I am not actually that
excited by the use case, except the "avoiding surprise" part of it.

Hope that helps,
Jonathan

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

[Rob Hoelz]

On Thu, 28 Mar 2013 12:25:07 -0700
Jonathan Nieder <jrnieder@gmail.com> wrote:

> Josh Triplett wrote:
> 
> > Related to this, as a path forward, I do think it makes sense to
> > have a setting usable as an insteadOf that only applies to pushurl,
> > even though pushInsteadOf won't end up serving that purpose.  That
> > way, pushInsteadOf covers the "map read-only repo url to pushable
> > repo url" case, and insteadOfPushOnly covers the "construct a magic
> > prefix that maps to different urls when used in url or pushurl"
> > case.
> 
> I hope not.  That would be making configuration even more complicated.
> 
> I hope that we can fix the documentation, tests, and change
> description in the commit message enough to make Rob's patch a
> no-brainer.  If that's not possible, I think the current state is
> livable, just confusing.
> 
> I was happy to see Rob's patch because it brings git's behavior closer
> to following the principle of least surprise.  I am not actually that
> excited by the use case, except the "avoiding surprise" part of it.
> 
> Hope that helps,
> Jonathan
> 

Thanks for all of the input, everyone.  I personally agree with
Jonathon's notion of "principle of least surprise", as I was quite
surprised when my configuration with pushInsteadOf + pushurl did not
work.  However, I also understand Junio's arguments about going back on
documented behavior, as well as whether or not it's a good idea to have
this "triangular" remote set up.

Honestly, if my workflow here is stupid and not "Git-like" and someone
has a better suggestion, I would happy to let my patch go.  Using two
remotes is an option, but to me, using this triangular setup is just
easier.

The only way I see this breaking an existing configuration is if a user
has something like url.user@server.com.pushInsteadOf = myserver:, and
pushurl = myserver:repo/.  If this behavior weren't documented, I would
say that such a configuration works because it relies on a bug, and
should use ssh://myserver:repo/ instead. I personally feel that the fact
that insteadOf + url works and pushInsteadOf + pushurl doesn't is
inconsistent and confusing. However, I am one user of many, and this is
my first exposure to Git from a project contributor point of view.
Therefore, I submit to the wisdom of more seasoned Git developers such
as yourselves. =)

-Rob

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

[Junio C Hamano]

Rob Hoelz <rob@hoelz.ro> writes:

> Honestly, if my workflow here is stupid and not "Git-like" and someone
> has a better suggestion, I would happy to let my patch go.  Using two
> remotes is an option, but to me, using this triangular setup is just
> easier.

I think you are conflating two unrelated things.

Pulling from one repository to integrate others' work with yours
(either by merging others' work to yours, or rebasing your work on
others'), and pushing the result of your work to another repository
to publish, i.e. triangular workflow, is no less "Git-like" than
pulling from and pushing to the same repository.  Both are valid
workflows, and Git supports them.

What is not correct in your set-up is that a single remote with URL
and pushURL (or rewritten URL derived from them via pushInsteadOf
and insteadOf) that point at two different repositories is *not* the
way to express that triangular configuration.  You name two remotes,
pull from one and push to the other.

If you look at Ram's "triangular workflows" series, cf.

    http://thread.gmane.org/gmane.comp.version-control.git/219387

you can see that a progress is being made to make the "two remotes"
configuration easier to use.

The discussion on the earliest iteration of the patch series, cf.

    http://thread.gmane.org/gmane.comp.version-control.git/215702

shows that even I initially made the same "pointing two different
repositories with URL and pushURL should be sufficient" mistake,
which was corrected by others.  The primary issue is "remote
tracking branches are designed to keep track of the state of the
branches at the named remote"---for this reason alone, you must not
name a logically different repository with URL and pushURL for a
single remote.

So that is one thing.  tl;dr: Triangular workflow is valid.  A
single remote with URL and pushURL to point at the two remote
repositories is not a valid way to express that workflow.

The other thing is if it is worth risking to break the backward
compatibility and hurting existing users in order to remove the
strange "To an explicit pushURL, insteadOf rewriting will not apply"
exception.

The reason I didn't bring up the possible breakage of "documented
behaviour" in the earlier review of this series is exactly because
that special case was unintuitive, so you do not have to argue it is
strange, unintuitive, and would not be a way we would have designed
the system if we knew better. I already agree to that point, and I
think others do, too.  There is a gap between "We would design it
differently if we were building it now with what we know" and "We
should change it and make it ideal" and the gap is called existing
users.

These two are unrelated and independent.

I suspect that Ram's "triangular" series, when it matures, will help
your "pull from there, push to another" workflow in a different way.
You will just define two remotes for these two places, and you may
no longer need "pushInsteadOf is not ignored when you have pushURL"
to solve your immediate issue.

But removing the "pushInsteadOf is ignored when explicit pushURL
exists" may still be a worthwhile thing to do, even if you do not
need it.

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

[Rob Hoelz]

On Wed, 27 Mar 2013 11:23:45 -0700
Jonathan Nieder <jrnieder@gmail.com> wrote:

> Rob Hoelz wrote:
> 
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -465,7 +465,11 @@ 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);
> > +			char *copy =
> > xstrdup(remotes[i]->pushurl[j]);
> > +			remotes[i]->pushurl[j] =
> > alias_url(remotes[i]->pushurl[j], &rewrites_push);
> > +			if (!strcmp(copy, remotes[i]->pushurl[j]))
> > +				remotes[i]->pushurl[j] =
> > alias_url(remotes[i]->pushurl[j], &rewrites);
> > +			free(copy);
> 
> Interesting.
> 
> Suppose I configure
> 
> 	[url "git://anongit.myserver.example.com/"]
> 		insteadOf = myserver.example.com:
> 	[url "myserver:"]
> 		pushInsteadOf = myserver.example.com:
> 
> The above code would make the insteadOf rule apply instead of
> pushInsteadOf, even when pushing.  Perhaps something like the
> following would work?

Are you sure?  I create a copy, and compare the copy to the new URL.
If they're the same (pushInsteadOf not found), I then use the insteadOf
mapping to perform the alteration.  Did I introduce a bug?

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

[Jonathan Nieder]

Hi,

Rob Hoelz wrote:
> On Wed, 27 Mar 2013 11:23:45 -0700
> Jonathan Nieder <jrnieder@gmail.com> wrote:

>> Suppose I configure
>>
>> 	[url "git://anongit.myserver.example.com/"]
>> 		insteadOf = myserver.example.com:
>> 	[url "myserver:"]
>> 		pushInsteadOf = myserver.example.com:
>>
>> The above code would make the insteadOf rule apply instead of
>> pushInsteadOf, even when pushing.  Perhaps something like the
>> following would work?
>
> Are you sure?

The message you are replying to is nonsense, due to a typo while
editing.  Did you see my followup?

Sorry for the confusion,
Jonathan

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

[Rob Hoelz]

On Wed, 27 Mar 2013 15:47:35 -0700
Jonathan Nieder <jrnieder@gmail.com> wrote:

> Hi,
> 
> Rob Hoelz wrote:
> > On Wed, 27 Mar 2013 11:23:45 -0700
> > Jonathan Nieder <jrnieder@gmail.com> wrote:
> 
> >> Suppose I configure
> >>
> >> 	[url "git://anongit.myserver.example.com/"]
> >> 		insteadOf = myserver.example.com:
> >> 	[url "myserver:"]
> >> 		pushInsteadOf = myserver.example.com:
> >>
> >> The above code would make the insteadOf rule apply instead of
> >> pushInsteadOf, even when pushing.  Perhaps something like the
> >> following would work?
> >
> > Are you sure?
> 
> The message you are replying to is nonsense, due to a typo while
> editing.  Did you see my followup?
> 
> Sorry for the confusion,
> Jonathan
> 

My mistake; I had not seen it!  I thought you may have found a bug in
my implementation, so I wanted to double check. =)

-Rob

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

[Jonathan Nieder]

Rob Hoelz wrote:

> My mistake; I had not seen it!  I thought you may have found a bug in
> my implementation, so I wanted to double check. =)

Well, I had found an unfortunate consequence of the implementation
that uses an unnecessary copy. :)

Will follow up to the updated patch.

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

[Rob Hoelz]

On Wed, 27 Mar 2013 15:56:56 -0700
Jonathan Nieder <jrnieder@gmail.com> wrote:

> Rob Hoelz wrote:
> 
> > My mistake; I had not seen it!  I thought you may have found a bug
> > in my implementation, so I wanted to double check. =)
> 
> Well, I had found an unfortunate consequence of the implementation
> that uses an unnecessary copy. :)
> 
> Will follow up to the updated patch.
> 

I actually wanted to talk about the copy thing.  I realize that this
could have been avoided by simply saving a pointer to the old string
and performing a comparison, but I figured if the implementation for
alias_url were changed in the future to use realloc or something, it
could potentially return the original char * with its contents
altered.  So, by copying the string, we can avoid strange bugs in the
future.

-Rob

[PATCH] push: Alias pushurl from push rewrites

[Rob Hoelz]

git push currently doesn't consider pushInsteadOf when
using pushurl; this test tests 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

With the above configuration, the following occurs:

$ git push origin master
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.

This commit fixes that.

Signed-off-by: Rob Hoelz <rob@hoelz.ro>
---
 Documentation/config.txt |  3 +-
 remote.c                 |  6 +++-
 t/t5516-fetch-push.sh    | 77 +++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 75 insertions(+), 11 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index b3023b8..5610962 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2142,8 +2142,7 @@ url.<base>.pushInsteadOf::
 	automatically use an appropriate URL to push, even for a
 	never-before-seen repository on the site.  When more than one
 	pushInsteadOf strings match a given URL, the longest match is
-	used.  If a remote has an explicit pushurl, Git will ignore this
-	setting for that remote.
+	used.
 
 user.email::
 	Your email address to be recorded in any newly created commits.
diff --git a/remote.c b/remote.c
index e53a6eb..1ea240a 100644
--- a/remote.c
+++ b/remote.c
@@ -465,7 +465,11 @@ 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);
+			char *copy = xstrdup(remotes[i]->pushurl[j]);
+			remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites_push);
+			if (!strcmp(copy, remotes[i]->pushurl[j]))
+				remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites);
+			free(copy);
 		}
 		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 c31e5c1..fbe0f29 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -228,19 +228,80 @@ test_expect_success 'push with pushInsteadOf' '
 	)
 '
 
-test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '
+test_expect_success 'push with pushInsteadOf and explicit pushurl (pushurl + pushInsteadOf does rewrite in this case)' '
 	mk_empty &&
+	rm -rf ro rw &&
 	TRASH="$(pwd)/" &&
-	git config "url.trash2/.pushInsteadOf" trash/ &&
-	git config remote.r.url trash/wrong &&
-	git config remote.r.pushurl "$TRASH/testrepo" &&
+	mkdir ro &&
+	mkdir rw &&
+	git init --bare rw/testrepo &&
+	test_config "url.file://$TRASH/ro/.insteadOf" ro: &&
+	test_config "url.file://$TRASH/rw/.pushInsteadOf" rw: &&
+	test_config remote.r.url ro:wrong &&
+	test_config remote.r.pushurl rw:testrepo &&
 	git push r refs/heads/master:refs/remotes/origin/master &&
 	(
-		cd testrepo &&
-		r=$(git show-ref -s --verify refs/remotes/origin/master) &&
-		test "z$r" = "z$the_commit" &&
+		cd rw/testrepo &&
+		echo "$the_commit commit	refs/remotes/origin/master" > expected &&
+		git for-each-ref refs/remotes/origin > actual &&
+		test_cmp expected actual
+	)
+'
 
-		test 1 = $(git for-each-ref refs/remotes/origin | wc -l)
+test_expect_success 'push without pushInsteadOf and explicit pushurl (pushurl + insteadOf is used for rewrite)' '
+	mk_empty &&
+	rm -rf ro rw &&
+	TRASH="$(pwd)/" &&
+	mkdir ro &&
+	mkdir rw &&
+	git init --bare rw/testrepo &&
+	test_config "url.file://$TRASH/ro/.insteadOf" ro: &&
+	test_config "url.file://$TRASH/rw/.insteadOf" rw: &&
+	test_config remote.r.url ro:wrong &&
+	test_config remote.r.pushurl rw:testrepo &&
+	git push r refs/heads/master:refs/remotes/origin/master &&
+	(
+		cd rw/testrepo &&
+		echo "$the_commit commit	refs/remotes/origin/master" > expected &&
+		git for-each-ref refs/remotes/origin > actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'push with pushInsteadOf but without explicit pushurl (url + pushInsteadOf is used for rewrite)' '
+	mk_empty &&
+	rm -rf ro rw &&
+	TRASH="$(pwd)/" &&
+	mkdir ro &&
+	mkdir rw &&
+	git init --bare rw/testrepo &&
+	test_config "url.file://$TRASH/ro/.insteadOf" rw: &&
+	test_config "url.file://$TRASH/rw/.pushInsteadOf" rw: &&
+	test_config remote.r.url rw:testrepo &&
+	git push r refs/heads/master:refs/remotes/origin/master &&
+	(
+		cd rw/testrepo &&
+		echo "$the_commit commit	refs/remotes/origin/master" > expected &&
+		git for-each-ref refs/remotes/origin > actual &&
+		test_cmp expected actual
+	)
+'
+
+test_expect_success 'push without pushInsteadOf and without explicit pushurl (url + insteadOf is used for rewrite)' '
+	mk_empty &&
+	rm -rf ro rw &&
+	TRASH="$(pwd)/" &&
+	mkdir ro &&
+	mkdir rw &&
+	git init --bare rw/testrepo &&
+	test_config "url.file://$TRASH/rw/.insteadOf" rw: &&
+	test_config remote.r.url rw:testrepo &&
+	git push r refs/heads/master:refs/remotes/origin/master &&
+	(
+		cd rw/testrepo &&
+		echo "$the_commit commit	refs/remotes/origin/master" > expected &&
+		git for-each-ref refs/remotes/origin > actual &&
+		test_cmp expected actual
 	)
 '
 
-- 
1.8.2

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

[Junio C Hamano]

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

Please increment [PATCH v$n] for a new round, so that we can tell
which one is the latest.

Rob Hoelz <rob@hoelz.ro> writes:

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

This patch is no longer "this test", and I thought you are changing
the behaviour so that the command takes it into account.

> 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

Perhaps indent the above a bit to make it more readable?

But more importantly, isn't this a variant of what we discussed in a
separate thread about "triangular workflow", where you pull from one
place (org/project) and push to another (project)?  I thought the
conclusion from the discussion was that using url/pushurl to call
two logically diffent repositories with the same name is wrong. For
one thing, the "pretend as if we immediately fetched" update of
remote tracking branches will go out of sync, so the above is a
broken configuration, with or without pushInsteadOf.

> With the above configuration, the following occurs:
>
> $ git push origin master
> fatal: remote error:
>   You can't push to git://github.com/myuser/project.git
>   Use git@github.com:myuser/project.git

Yup, that is a documented behaviour.

> 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.
>
> This commit fixes that.

Saying "fixes" before justifying why such a patch that changes a
documented behaviour is a good idea is a bit weak, to say the least.

Care to justify with a non-triangular example, where origin is
associated to logically the same repository?

That is, currently you can do either:

     ; Fetch anonymously
     [url "git://github.com/me/"]
        insteadOf = github:

     ; Pushing needs to go over ssh
     [url "git@github.com:me/"]
        pushInsteadOf = github:

     ; The repository
     [remote "origin"]
        url = github:project

or:

     ; Fetch anonymously
     [url "git://github.com/me/"]
        insteadOf = githubf:

     ; Pushing needs to go over ssh
     [url "git@github.com:me/"]
        insteadOf = githubp:

     ; The repository
     [remote "origin"]
        url = githubf:project
        pushUrl = githubp:project

You would need to make a convincing argument to justify why allowing:

     ; Fetch anonymously
     [url "git://github.com/me/"]
        insteadOf = github:

     ; Pushing needs to go over ssh
     [url "git@github.com:me/"]
        pushInsteadOf = github:

     ; The repository
     [remote "origin"]
        url = github:project
        pushUrl = github:project

is a good idea.  

I also suspect there could be people who rely on the documented
behaviour; they can manually rewrite their pushURL to what insteadOf
setting has been rewriting to work it around, but to them, this
change may be a needless regression.  I do not offhand how severe a
regression it is, though.

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

[Jonathan Nieder]

Hi,

Rob Hoelz wrote:

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

I'd leave out this paragraph, since it is redundant next to the rest
of the commit message (except that you have added tests, which ideally
every bugfix patch would do :)).

[...]
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2142,8 +2142,7 @@ url.<base>.pushInsteadOf::
>  	automatically use an appropriate URL to push, even for a
>  	never-before-seen repository on the site.  When more than one
>  	pushInsteadOf strings match a given URL, the longest match is
> -	used.  If a remote has an explicit pushurl, Git will ignore this
> -	setting for that remote.
> +	used.

Old-timers used to the previous behavior might not guess immediately
how this interacts with pushurl.  (If I understand the initial
discussion at [1] correctly, an earlier, unreleased version of the
feature would push to the rewritten fetch url *in addition to* the
unmodified push url.  So there's more than one possible behavior here.)

How about:

	url.<base>.pushInsteadOf

		Any URL that starts with this value will not be pushed to;
		instead, it will be rewritten ... even for a
		never-before-seen repository on the site.

		This rewriting takes place even for explicit push URLs
		set using the `remote.<name>.pushurl` configuration
		variable.

		When more than one pushInsteadOf string matches a given
		URL, the longest match is used.

[1] http://thread.gmane.org/gmane.comp.version-control.git/127889

> --- a/remote.c
> +++ b/remote.c
> @@ -465,7 +465,11 @@ 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);
> +			char *copy = xstrdup(remotes[i]->pushurl[j]);
> +			remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites_push);
> +			if (!strcmp(copy, remotes[i]->pushurl[j]))
> +				remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites);
> +			free(copy);

This is relying on !strcmp to detect that no pushInsteadOf rule
matched the URL.  By contrast, the existing pushInsteadOf support
does the following:

	const char *pushurl = alias_url(url, &rewrites_push);
	if (pushurl != url)
		add_pushurl(remote, pushurl);

Because it compares pointers, not strings, it is able to correctly
treat the identity substitution as a real match.  It also avoids some
allocation churn.  This new alias_url() call site should follow the
same convention.

Looking at that existing code also made me worry "Are we applying
the pushinsteadof subsitution twice?"

So let's see what happens:

	Caller calls into remote_get() to learn about remote "origin".
	... which in turn calls read_config()
	... which sets the config machinery in motion with callback handle_config()
	... which stores rewrite rules in 'rewrites' and 'rewrites_push' and
	    unmodified URLs in remotes[i]->url[], remotes[i]->pushurl[]

	Now read_config() calls alias_all_urls() to tweak the url and
	pushurl fields in place.  For each remote:
	 1. If a pushurl matches an *.insteadof rewrite rule, rewrite it.
	 2. Check if any pushurls exist.
	 3. If a url matches a *.pushinsteadof rule and no raw pushurls
	    existed, use the rewritten url as a push url.

	    If a url matches a *.insteadof rewrite rule, rewrite it.

With your tweak, step (1) above just also checks for *.pushinsteadof
in addition to *.insteadof, which should be safe (modulo the string
comparison vs pointer comparison detail mentioned above)

There's also the legacy .git/remotes and .git/branches code paths,
which are basically the same except there's no place for a pushurl.

How about the below, for squashing in?

diff --git i/Documentation/config.txt w/Documentation/config.txt
index 665c0de..25565ca 100644
--- i/Documentation/config.txt
+++ w/Documentation/config.txt
@@ -2150,9 +2150,14 @@ url.<base>.pushInsteadOf::
 	access methods, some of which do not allow push, this feature
 	allows people to specify a pull-only URL and have Git
 	automatically use an appropriate URL to push, even for a
-	never-before-seen repository on the site.  When more than one
-	pushInsteadOf strings match a given URL, the longest match is
-	used.
+	never-before-seen repository on the site.
++
+This rewriting takes place even for explicit push URLs set
+using the `remote.<name>.pushurl` configuration variable.
++
+When more than one pushInsteadOf string matches a given URL,
+the longest match is used.  If no pushInsteadOf strings match
+the URL, Git falls back to insteadOf strings.
 
 user.email::
 	Your email address to be recorded in any newly created commits.
diff --git i/remote.c w/remote.c
index cfcb77a..7255926 100644
--- i/remote.c
+++ w/remote.c
@@ -466,11 +466,14 @@ static void alias_all_urls(void)
 		if (!remotes[i])
 			continue;
 		for (j = 0; j < remotes[i]->pushurl_nr; j++) {
-			char *copy = xstrdup(remotes[i]->pushurl[j]);
-			remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites_push);
-			if (!strcmp(copy, remotes[i]->pushurl[j]))
-				remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites);
-			free(copy);
+			const char *url = remotes[i]->pushurl[j];
+			remotes[i]->pushurl[j] = alias_url(url, &rewrites_push);
+			if (remotes[i]->pushurl[j] == url)
+				/*
+				 * No url.*.pushinsteadof string matched.
+				 * Apply url.*.insteadof.
+				 */
+				remotes[i]->pushurl[j] = alias_url(url, &rewrites);
 		}
 		add_pushurl_aliases = remotes[i]->pushurl_nr == 0;
 		for (j = 0; j < remotes[i]->url_nr; j++) {