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

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

Josh Triplett [off-list ref] writes:
(on url.$base.pushInsteadOf)

quoted

quoted

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.

quoted

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.

Debatable whether the documentation sentence above really says that; it
certainly doesn't say it very clearly if so.  But that does match the
actual behavior, making the proposed change a regression from the actual
behavior, whether the documentation clearly guarantees that behavior or
not.

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

Agreed.

Related to this, as a path forward, I do think it makes sense to have a
setting usable as an insteadOf that only applies to pushurl, even though
pushInsteadOf won't end up serving that purpose.  That way,
pushInsteadOf covers the "map read-only repo url to pushable repo url"
case, and insteadOfPushOnly covers the "construct a magic prefix that
maps to different urls when used in url or pushurl" case.

quoted

quoted

 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?

That sounds sensible.

- Josh Triplett