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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 28, 2013, 18:50 UTC
Message-ID: <7vr4iztl3z.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20130328160937.GD16034@leaf>

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

Show 5 quoted lines

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

Show 13 quoted lines

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