On Thu, Mar 28, 2013 at 11:50:08AM -0700, Junio C Hamano wrote: > Josh Triplett <josh@xxxxxxxxxxxxxxxx> 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. > 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. > >> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html