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

[Josh Triplett <josh@joshtriplett.org>]

On Thu, Mar 28, 2013 at 08:37:58AM -0700, Junio C Hamano wrote:
> 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.

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

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

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