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

Josh Triplett [off-list ref] writes:

On Wed, Mar 27, 2013 at 05:48:45PM -0500, Rob Hoelz wrote:
...

quoted

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

On Thu, Mar 28, 2013 at 08:37:58AM -0700, Junio C Hamano wrote:

Josh Triplett [off-list ref] writes:

quoted

On Wed, Mar 27, 2013 at 05:48:45PM -0500, Rob Hoelz wrote:
...

quoted

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

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