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

From: Josh Triplett <josh@joshtriplett.org>
Date: Mar 28, 2013, 19:03 UTC
Message-ID: <20130328190344.GA5361@jtriplet-mobl1>
In-Reply-To: <7vr4iztl3z.fsf@alter.siamese.dyndns.org>

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

Show 16 quoted lines

> Josh Triplett <josh@joshtriplett.org> writes:
> (on url.$base.pushInsteadOf)
> >> 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.
> 
> > 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.

Show 14 quoted lines

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

Show 19 quoted lines

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