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

[Junio C Hamano <gitster@pobox.com>]

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