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

[Rob Hoelz <rob@hoelz.ro>]

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