Re: [PATCH] push: Alias pushurl from push rewrites
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 28, 2013, 15:37 UTC
- Message-ID
- <7vfvzfwn55.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <20130327230943.GA5204@jtriplet-mobl1>
Josh Triplett <josh@joshtriplett.org> writes:
Show 10 quoted lines
> 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 &&