Re: [PATCH] push: Alias pushurl from push rewrites
Rob Hoelz writes: > Honestly, if my workflow here is stupid and not "Git-like" and someone > has a better suggestion, I would happy to let my patch go. Using two > remotes is an option, but to me, using this triangular setup is just > easier. I think you are conflating two unrelated things. Pulling from one repository to integrate others' work with yours (either by merging others' work to yours, or rebasing your work on others'), and pushing the result of your work to another repository to publish, i.e. triangular workflow, is no less "Git-like" than pulling from and pushing to the same repository. Both are valid workflows, and Git supports them. What is not correct in your set-up is that a single remote with URL and pushURL (or rewritten URL derived from them via pushInsteadOf and insteadOf) that point at two different repositories is *not* the way to express that triangular configuration. You name two remotes, pull from one and push to the other. If you look at Ram's "triangular workflows" series, cf. http://thread.gmane.org/gmane.comp.version-control.git/219387 you can see that a progress is being made to make the "two remotes" configuration easier to use. The discussion on the earliest iteration of the patch series, cf. http://thread.gmane.org/gmane.comp.version-control.git/215702 shows that even I initially made the same "pointing two different repositories with URL and pushURL should be sufficient" mistake, which was corrected by others. The primary issue is "remote tracking branches are designed to keep track of the state of the branches at the named remote"---for this reason alone, you must not name a logically different repository with URL and pushURL for a single remote. So that is one thing. tl;dr: Triangular workflow is valid. A single remote with URL and pushURL to point at the two remote repositories is not a valid way to express that workflow. The other thing is if it is worth risking to break the backward compatibility and hurting existing users in order to remove the strange "To an explicit pushURL, insteadOf rewriting will not apply" exception. The reason I didn't bring up the possible breakage of "documented behaviour" in the earlier review of this series is exactly because that special case was unintuitive, so you do not have to argue it is strange, unintuitive, and would not be a way we would have designed the system if we knew better. I already agree to that point, and I think others do, too. There is a gap between "We would design it differently if we were building it now with what we know" and "We should change it and make it ideal" and the gap is called existing users. These two are unrelated and independent. I suspect that Ram's "triangular" series, when it matures, will help your "pull from there, push to another" workflow in a different way. You will just define two remotes for these two places, and you may no longer need "pushInsteadOf is not ignored when you have pushURL" to solve your immediate issue. But removing the "pushInsteadOf is ignored when explicit pushURL exists" may still be a worthwhile thing to do, even if you do not need it. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
On Thu, 28 Mar 2013 12:25:07 -0700 Jonathan Nieder wrote: > Josh Triplett wrote: > > > 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. > > I hope not. That would be making configuration even more complicated. > > I hope that we can fix the documentation, tests, and change > description in the commit message enough to make Rob's patch a > no-brainer. If that's not possible, I think the current state is > livable, just confusing. > > I was happy to see Rob's patch because it brings git's behavior closer > to following the principle of least surprise. I am not actually that > excited by the use case, except the "avoiding surprise" part of it. > > Hope that helps, > Jonathan > Thanks for all of the input, everyone. I personally agree with Jonathon's notion of "principle of least surprise", as I was quite surprised when my configuration with pushInsteadOf + pushurl did not work. However, I also understand Junio's arguments about going back on documented behavior, as well as whether or not it's a good idea to have this "triangular" remote set up. Honestly, if my workflow here is stupid and not "Git-like" and someone has a better suggestion, I would happy to let my patch go. Using two remotes is an option, but to me, using this triangular setup is just easier. The only way I see this breaking an existing configuration is if a user has something like url.u...@server.com.pushInsteadOf = myserver:, and pushurl = myserver:repo/. If this behavior weren't documented, I would say that such a configuration works because it relies on a bug, and should use ssh://myserver:repo/ instead. I personally feel that the fact that insteadOf + url works and pushInsteadOf + pushurl doesn't is inconsistent and confusing. However, I am one user of many, and this is my first exposure to Git from a project contributor point of view. Therefore, I submit to the wisdom of more seasoned Git developers such as yourselves. =) -Rob -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
Josh Triplett wrote: > 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. I hope not. That would be making configuration even more complicated. I hope that we can fix the documentation, tests, and change description in the commit message enough to make Rob's patch a no-brainer. If that's not possible, I think the current state is livable, just confusing. I was happy to see Rob's patch because it brings git's behavior closer to following the principle of least surprise. I am not actually that excited by the use case, except the "avoiding surprise" part of it. Hope that helps, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
On Thu, Mar 28, 2013 at 11:50:08AM -0700, Junio C Hamano wrote: > Josh Triplett 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
Josh Triplett 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. 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). >> 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? >> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
On Thu, Mar 28, 2013 at 09:10:59AM -0700, Junio C Hamano wrote: > Josh Triplett writes: > > > OK, I take it back. I *can* imagine configurations that this change > > would break, since it does change intentional and documented behavior, > > but I don't have any such configuration. The only such configuration I > > can imagine involves directly counting on the non-rewriting of pushUrl, > > by using pushInsteadOf to rewrite urls and then sometimes using pushUrl > > to override that and point back at the un-rewritten URL. And while > > supported, that does seem *odd*. > > > > Objection withdrawn; if nobody can come up with a sensible configuration > > that relies on the documented behavior, I don't particularly care if it > > changes. > > I actually do. > > Given the popularity of the system, "people involved in this thread > cannot imagine a case that existing people may get hurt" is very > different from "this is not a regression". After merging this > change when people start complaining, you and Rob can hide and > ignore them, but we collectively as the Git project have to have a > way to help them when it happens. I entirely agree that it represents a regression from documented behavior; I just mean that it no longer matches a specific use case I had in mind with the original change. I agree that we should hesitate to change that documented behavior. - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
Josh Triplett writes: > OK, I take it back. I *can* imagine configurations that this change > would break, since it does change intentional and documented behavior, > but I don't have any such configuration. The only such configuration I > can imagine involves directly counting on the non-rewriting of pushUrl, > by using pushInsteadOf to rewrite urls and then sometimes using pushUrl > to override that and point back at the un-rewritten URL. And while > supported, that does seem *odd*. > > Objection withdrawn; if nobody can come up with a sensible configuration > that relies on the documented behavior, I don't particularly care if it > changes. I actually do. Given the popularity of the system, "people involved in this thread cannot imagine a case that existing people may get hurt" is very different from "this is not a regression". After merging this change when people start complaining, you and Rob can hide and ignore them, but we collectively as the Git project have to have a way to help them when it happens. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
On Thu, Mar 28, 2013 at 08:37:58AM -0700, Junio C Hamano wrote: > Josh Triplett 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
On Wed, Mar 27, 2013 at 04:18:19PM -0700, Jonathan Nieder wrote: > Josh Triplett wrote: > > > I have a .gitconfig in my git-managed home > > directory which sets pushInsteadOf so that I can clone via git:// and > > immediately have working push. I work with a number of systems that > > don't have inbound access to each other but do have outbound access to > > the network; on some of these "satellite" boxes, I can't push changes > > directly to the server pushInsteadOf points to, so I can explicitly set > > pushurl in .git/config for that repository, which overrides the > > pushInsteadOf. This change would break that configuration. > > Would it? As long as your pushurl does not start with git://, I think > your configuration would still work fine. I had to think about it for a while, but I think you're right; I inferred a behavior that the patch didn't actually add or have anything to do with, namely having the result of applying pushInsteadOf to the non-push URL override the pushUrl. OK, I take it back. I *can* imagine configurations that this change would break, since it does change intentional and documented behavior, but I don't have any such configuration. The only such configuration I can imagine involves directly counting on the non-rewriting of pushUrl, by using pushInsteadOf to rewrite urls and then sometimes using pushUrl to override that and point back at the un-rewritten URL. And while supported, that does seem *odd*. Objection withdrawn; if nobody can come up with a sensible configuration that relies on the documented behavior, I don't particularly care if it changes. > After this patch, neither pushInsteadOf nor pushUrl overrides the > other one. The rule is: > > 1. First, get the URL from the remote's configuration, based > on whether you are fetching or pushing. > > (At this step, in your setup git chooses the URL specified > with pushurl in your .git/config.) > > 2. Next, apply the most appropriate url.*.insteadOf or > url.*.pushInsteadOf rule, based on whether you are fetching > or pushing. > > (At this step, no rewrite rules apply, so the URL is used > as is.) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
Jonathan Nieder writes: > Josh Triplett wrote: > >> I have a .gitconfig in my git-managed home >> directory which sets pushInsteadOf so that I can clone via git:// and >> immediately have working push. I work with a number of systems that >> don't have inbound access to each other but do have outbound access to >> the network; on some of these "satellite" boxes, I can't push changes >> directly to the server pushInsteadOf points to, so I can explicitly set >> pushurl in .git/config for that repository, which overrides the >> pushInsteadOf. This change would break that configuration. > > Would it? As long as your pushurl does not start with git://, I think > your configuration would still work fine. That is a good point, especially because it is very unlikely that git:// was used for pushURL, given that it would not have been rewritten with pushInsteadOf to an authenticated transport. > After this patch, neither pushInsteadOf nor pushUrl overrides the > other one. The rule is: > > 1. First, get the URL from the remote's configuration, based > on whether you are fetching or pushing. > > (At this step, in your setup git chooses the URL specified > with pushurl in your .git/config.) > > 2. Next, apply the most appropriate url.*.insteadOf or > url.*.pushInsteadOf rule, based on whether you are fetching > or pushing. > > (At this step, no rewrite rules apply, so the URL is used > as is.) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
Josh Triplett 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. 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 && -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
Hi,
Rob Hoelz wrote:
> git push currently doesn't consider pushInsteadOf when
> using pushurl; this test tests that.
I'd leave out this paragraph, since it is redundant next to the rest
of the commit message (except that you have added tests, which ideally
every bugfix patch would do :)).
[...]
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2142,8 +2142,7 @@ url..pushInsteadOf::
> automatically use an appropriate URL to push, even for a
> never-before-seen repository on the site. When more than one
> pushInsteadOf strings match a given URL, the longest match is
> - used. If a remote has an explicit pushurl, Git will ignore this
> - setting for that remote.
> + used.
Old-timers used to the previous behavior might not guess immediately
how this interacts with pushurl. (If I understand the initial
discussion at [1] correctly, an earlier, unreleased version of the
feature would push to the rewritten fetch url *in addition to* the
unmodified push url. So there's more than one possible behavior here.)
How about:
url..pushInsteadOf
Any URL that starts with this value will not be pushed to;
instead, it will be rewritten ... even for a
never-before-seen repository on the site.
This rewriting takes place even for explicit push URLs
set using the `remote..pushurl` configuration
variable.
When more than one pushInsteadOf string matches a given
URL, the longest match is used.
[1] http://thread.gmane.org/gmane.comp.version-control.git/127889
> --- a/remote.c
> +++ b/remote.c
> @@ -465,7 +465,11 @@ static void alias_all_urls(void)
> if (!remotes[i])
> continue;
> for (j = 0; j < remotes[i]->pushurl_nr; j++) {
> - remotes[i]->pushurl[j] =
> alias_url(remotes[i]->pushurl[j], &rewrites);
> + char *copy = xstrdup(remotes[i]->pushurl[j]);
> + remotes[i]->pushurl[j] =
> alias_url(remotes[i]->pushurl[j], &rewrites_push);
> + if (!strcmp(copy, remotes[i]->pushurl[j]))
> + remotes[i]->pushurl[j] =
> alias_url(remotes[i]->pushurl[j], &rewrites);
> + free(copy);
This is relying on !strcmp to detect that no pushInsteadOf rule
matched the URL. By contrast, the existing pushInsteadOf support
does the following:
const char *pushurl = alias_url(url, &rewrites_push);
if (pushurl != url)
add_pushurl(remote, pushurl);
Because it compares pointers, not strings, it is able to correctly
treat the identity substitution as a real match. It also avoids some
allocation churn. This new alias_url() call site should follow the
same convention.
Looking at that existing code also made me worry "Are we applying
the pushinsteadof subsitution twice?"
So let's see what happens:
Caller calls into remote_get() to learn about remote "origin".
... which in turn calls read_config()
... which sets the config machinery in motion with callback
handle_config()
... which stores rewrite rules in 'rewrites' and 'rewrites_push' and
unmodified URLs in remotes[i]->url[], remotes[i]->pushurl[]
Now read_config() calls alias_all_urls() to tweak the url and
pushurl fields in place. For each remote:
1. If a pushurl matches an *.insteadof rewrite rule, rewrite it.
2. Check if any pushurls exist.
3. If a url matches a *.pushinsteadof rule and no raw pushurls
existed, use the rewritten url as a push url.
If a url matches a *.insteadof rewrite rule, rewrite it.
With your tweak, step (1) above just also checks for *.pushinsteadof
in addition to *.insteadof, which should be safe (modulo the string
comparison vs pointer comparison detail mentioned above)
There's also the legacy .git/remotes and .git/branches code paths,
which are basically the same except there's no place for a pushurl.
How about the below, for squashing in?
diff --git i/Documentation/config.txt w/Documentation/config.txt
index 665c0de..25565ca 100644
--- i/Documentation/config.txt
+++ w/Documentation/config.txt
@@ -2150,9 +2150,14 @@ url..pushInsteadOf::
access methods, some of which do not allow push, this feature
allows people to specify a pull-only URL and have Git
automatically use an appropriate URL to push, even for a
- never-before-seen repository on the site. When more than one
- pushInsteadOf strings match a given URL, the longest match is
- used.
+ never-before-seen repository on the site.
++
+This rewriting takes place even for explicit push URLs set
+using the `remote..pushurl` configuration variable.
++
+When more than one pushInsteadOf string matches a given URL,
+the longest match is used. If no pushInsteadOf str
Re: [PATCH] push: Alias pushurl from push rewrites
Josh Triplett wrote: > I have a .gitconfig in my git-managed home > directory which sets pushInsteadOf so that I can clone via git:// and > immediately have working push. I work with a number of systems that > don't have inbound access to each other but do have outbound access to > the network; on some of these "satellite" boxes, I can't push changes > directly to the server pushInsteadOf points to, so I can explicitly set > pushurl in .git/config for that repository, which overrides the > pushInsteadOf. This change would break that configuration. Would it? As long as your pushurl does not start with git://, I think your configuration would still work fine. After this patch, neither pushInsteadOf nor pushUrl overrides the other one. The rule is: 1. First, get the URL from the remote's configuration, based on whether you are fetching or pushing. (At this step, in your setup git chooses the URL specified with pushurl in your .git/config.) 2. Next, apply the most appropriate url.*.insteadOf or url.*.pushInsteadOf rule, based on whether you are fetching or pushing. (At this step, no rewrite rules apply, so the URL is used as is.) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
On Wed, Mar 27, 2013 at 04:09:43PM -0700, Josh Triplett wrote: > On Wed, Mar 27, 2013 at 05:48:45PM -0500, Rob Hoelz wrote: > > On Wed, 27 Mar 2013 15:07:18 -0700 > > Junio C Hamano wrote: > > > > > Jonathan Nieder writes: > > > > > > > Sorry, typo. The configuration in the example above should have > > > > been > > > > > > > > [url "git://anongit.myserver.example.com/"] > > > > insteadOf = myserver.example.com: > > > > [url "myserver.example.com:"] > > > > pushInsteadOf = myserver.example.com: > > > > > > > > In other words, suppose I set url.*.insteadof to point to a faster > > > > address for fetching alongside url.*.pushinsteadof requesting that > > > > the original address should still be used for pushing. > > > > > > I didn't know we were even shooting for supporting the identity > > > mapping: > > > > > > url.X.pushinsteadof=X > > > > > > but that would certainly be nice. > > > > > > By the way, it seems that the original commit 1c2eafb89bca (Add > > > url..pushInsteadOf: URL rewriting for push only, 2009-09-07) > > > wanted to explicitly avoid use of pushInsteadOf aliasing for a > > > pushURL and it is also documented in config.txt from day one. > > > > > > I think the intent is "You have a normal URL, and a way to override > > > it explicitly with pushURL, or a way to rewrite it via the aliasing > > > the normal URL with pushInsteadOf. Either one or the other, but not > > > both, as having many levels of indirection would be confusing." > > > > > > Which I can understand and sympathise. > > > > > > In anay case, the change proposed in this thread seems to change > > > that, so the documentation would need to be updated. Also the tests > > > the original commit adds explicitly checks that pushInsteadOf is > > > ignored, which may have to be updated (unless that test is already > > > broken). > > > > > > > My use case is that I use Github for my personal development. I have a > > prefix for my personal repos (hoelzro: -> git://github.com/hoelzro for > > fetch, g...@github.com:hoelzro/ for push) and one for all other Git repos > > (github: -> git://github.com/) I have a few projects where I work in a > > fork, but I want to fetch updates from the original project. So my url > > for the origin remote is github:org/project, but my pushurl is > > hoelzro:project. This behavior in Git currently doesn't allow me to > > work that way. I used to work with two remotes; origin for my repo and > > base for the official one, but I've found that I prefer this other way. > > > > 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 instance, you could use pushInsteadOf to rewrite a family of > anonymous git URLs to corresponding pushable repositories, but then use > an explicit pushurl to override that for a specific repository. This > change would break the ability to use pushurl for its original intended > purpose, namely having a local repository where fetch comes from one > remote repo and push goes to another. > > One use case of mine: I have a .gitconfig in my git-managed home > directory which sets pushInsteadOf so that I can clone via git:// and > immediately have working push. I work with a number of systems that > don't have inbound access to each other but do have outbound access to > the network; on some of these "satellite" boxes, I can't push changes > directly to the server pushInsteadOf points to, so I can explicitly set > pushurl in .git/config for that repository, which overrides the > pushInsteadOf. This change would break that configuration. Clarifying this use case a bit: note that it's been a while since I had many such boxes, so I don't actually have any systems currently using that pushurl configuration. Still a regression in defined behavior, though. Why not just use insteadOf for your personal github prefix hoelzro:, and both insteadOf and pushInsteadOf for github: in general? Then, a repository cloned via github: would work for pull and push (if you have push access), and you can change pushurl to your personal github alias if needed. Though, as Junio said, the modern push-updates-remote-heads behavior of git means that both of our configurations arguably seem wrong, and we should both just use separate remotes for separate repos. - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
On Wed, Mar 27, 2013 at 05:48:45PM -0500, Rob Hoelz wrote: > On Wed, 27 Mar 2013 15:07:18 -0700 > Junio C Hamano wrote: > > > Jonathan Nieder writes: > > > > > Sorry, typo. The configuration in the example above should have > > > been > > > > > > [url "git://anongit.myserver.example.com/"] > > > insteadOf = myserver.example.com: > > > [url "myserver.example.com:"] > > > pushInsteadOf = myserver.example.com: > > > > > > In other words, suppose I set url.*.insteadof to point to a faster > > > address for fetching alongside url.*.pushinsteadof requesting that > > > the original address should still be used for pushing. > > > > I didn't know we were even shooting for supporting the identity > > mapping: > > > > url.X.pushinsteadof=X > > > > but that would certainly be nice. > > > > By the way, it seems that the original commit 1c2eafb89bca (Add > > url..pushInsteadOf: URL rewriting for push only, 2009-09-07) > > wanted to explicitly avoid use of pushInsteadOf aliasing for a > > pushURL and it is also documented in config.txt from day one. > > > > I think the intent is "You have a normal URL, and a way to override > > it explicitly with pushURL, or a way to rewrite it via the aliasing > > the normal URL with pushInsteadOf. Either one or the other, but not > > both, as having many levels of indirection would be confusing." > > > > Which I can understand and sympathise. > > > > In anay case, the change proposed in this thread seems to change > > that, so the documentation would need to be updated. Also the tests > > the original commit adds explicitly checks that pushInsteadOf is > > ignored, which may have to be updated (unless that test is already > > broken). > > > > My use case is that I use Github for my personal development. I have a > prefix for my personal repos (hoelzro: -> git://github.com/hoelzro for > fetch, g...@github.com:hoelzro/ for push) and one for all other Git repos > (github: -> git://github.com/) I have a few projects where I work in a > fork, but I want to fetch updates from the original project. So my url > for the origin remote is github:org/project, but my pushurl is > hoelzro:project. This behavior in Git currently doesn't allow me to > work that way. I used to work with two remotes; origin for my repo and > base for the official one, but I've found that I prefer this other way. > > 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 instance, you could use pushInsteadOf to rewrite a family of anonymous git URLs to corresponding pushable repositories, but then use an explicit pushurl to override that for a specific repository. This change would break the ability to use pushurl for its original intended purpose, namely having a local repository where fetch comes from one remote repo and push goes to another. One use case of mine: I have a .gitconfig in my git-managed home directory which sets pushInsteadOf so that I can clone via git:// and immediately have working push. I work with a number of systems that don't have inbound access to each other but do have outbound access to the network; on some of these "satellite" boxes, I can't push changes directly to the server pushInsteadOf points to, so I can explicitly set pushurl in .git/config for that repository, which overrides the pushInsteadOf. This change would break that configuration. - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
>> Subject: Re: [PATCH] push: Alias pushurl from push rewrites Please increment [PATCH v$n] for a new round, so that we can tell which one is the latest. Rob Hoelz writes: > git push currently doesn't consider pushInsteadOf when > using pushurl; this test tests that. This patch is no longer "this test", and I thought you are changing the behaviour so that the command takes it into account. > If you use pushurl with an alias that has a pushInsteadOf configuration > value, Git does not take advantage of it. For example: > > [url "git://github.com/"] > insteadOf = github: > [url "git://github.com/myuser/"] > insteadOf = mygithub: > [url "g...@github.com:myuser/"] > pushInsteadOf = mygithub: > [remote "origin"] > url = github:organization/project > pushurl = mygithub:project Perhaps indent the above a bit to make it more readable? But more importantly, isn't this a variant of what we discussed in a separate thread about "triangular workflow", where you pull from one place (org/project) and push to another (project)? I thought the conclusion from the discussion was that using url/pushurl to call two logically diffent repositories with the same name is wrong. For one thing, the "pretend as if we immediately fetched" update of remote tracking branches will go out of sync, so the above is a broken configuration, with or without pushInsteadOf. > With the above configuration, the following occurs: > > $ git push origin master > fatal: remote error: > You can't push to git://github.com/myuser/project.git > Use g...@github.com:myuser/project.git Yup, that is a documented behaviour. > So you can see that pushurl is being followed (it's not attempting to > push to git://github.com/organization/project), but insteadOf values are > being used as opposed to pushInsteadOf values for expanding the pushurl > alias. > > This commit fixes that. Saying "fixes" before justifying why such a patch that changes a documented behaviour is a good idea is a bit weak, to say the least. Care to justify with a non-triangular example, where origin is associated to logically the same repository? That is, currently you can do either: ; Fetch anonymously [url "git://github.com/me/"] insteadOf = github: ; Pushing needs to go over ssh [url "g...@github.com:me/"] pushInsteadOf = github: ; The repository [remote "origin"] url = github:project or: ; Fetch anonymously [url "git://github.com/me/"] insteadOf = githubf: ; Pushing needs to go over ssh [url "g...@github.com:me/"] insteadOf = githubp: ; The repository [remote "origin"] url = githubf:project pushUrl = githubp:project You would need to make a convincing argument to justify why allowing: ; Fetch anonymously [url "git://github.com/me/"] insteadOf = github: ; Pushing needs to go over ssh [url "g...@github.com:me/"] pushInsteadOf = github: ; The repository [remote "origin"] url = github:project pushUrl = github:project is a good idea. I also suspect there could be people who rely on the documented behaviour; they can manually rewrite their pushURL to what insteadOf setting has been rewriting to work it around, but to them, this change may be a needless regression. I do not offhand how severe a regression it is, though. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
On Wed, 27 Mar 2013 15:56:56 -0700 Jonathan Nieder wrote: > Rob Hoelz wrote: > > > My mistake; I had not seen it! I thought you may have found a bug > > in my implementation, so I wanted to double check. =) > > Well, I had found an unfortunate consequence of the implementation > that uses an unnecessary copy. :) > > Will follow up to the updated patch. > I actually wanted to talk about the copy thing. I realize that this could have been avoided by simply saving a pointer to the old string and performing a comparison, but I figured if the implementation for alias_url were changed in the future to use realloc or something, it could potentially return the original char * with its contents altered. So, by copying the string, we can avoid strange bugs in the future. -Rob -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
Rob Hoelz wrote: > My mistake; I had not seen it! I thought you may have found a bug in > my implementation, so I wanted to double check. =) Well, I had found an unfortunate consequence of the implementation that uses an unnecessary copy. :) Will follow up to the updated patch. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
On Wed, 27 Mar 2013 15:47:35 -0700 Jonathan Nieder wrote: > Hi, > > Rob Hoelz wrote: > > On Wed, 27 Mar 2013 11:23:45 -0700 > > Jonathan Nieder wrote: > > >> Suppose I configure > >> > >>[url "git://anongit.myserver.example.com/"] > >>insteadOf = myserver.example.com: > >>[url "myserver:"] > >>pushInsteadOf = myserver.example.com: > >> > >> The above code would make the insteadOf rule apply instead of > >> pushInsteadOf, even when pushing. Perhaps something like the > >> following would work? > > > > Are you sure? > > The message you are replying to is nonsense, due to a typo while > editing. Did you see my followup? > > Sorry for the confusion, > Jonathan > My mistake; I had not seen it! I thought you may have found a bug in my implementation, so I wanted to double check. =) -Rob -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
On Wed, 27 Mar 2013 15:07:18 -0700 Junio C Hamano wrote: > Jonathan Nieder writes: > > > Sorry, typo. The configuration in the example above should have > > been > > > > [url "git://anongit.myserver.example.com/"] > > insteadOf = myserver.example.com: > > [url "myserver.example.com:"] > > pushInsteadOf = myserver.example.com: > > > > In other words, suppose I set url.*.insteadof to point to a faster > > address for fetching alongside url.*.pushinsteadof requesting that > > the original address should still be used for pushing. > > I didn't know we were even shooting for supporting the identity > mapping: > > url.X.pushinsteadof=X > > but that would certainly be nice. > > By the way, it seems that the original commit 1c2eafb89bca (Add > url..pushInsteadOf: URL rewriting for push only, 2009-09-07) > wanted to explicitly avoid use of pushInsteadOf aliasing for a > pushURL and it is also documented in config.txt from day one. > > I think the intent is "You have a normal URL, and a way to override > it explicitly with pushURL, or a way to rewrite it via the aliasing > the normal URL with pushInsteadOf. Either one or the other, but not > both, as having many levels of indirection would be confusing." > > Which I can understand and sympathise. > > In anay case, the change proposed in this thread seems to change > that, so the documentation would need to be updated. Also the tests > the original commit adds explicitly checks that pushInsteadOf is > ignored, which may have to be updated (unless that test is already > broken). > My use case is that I use Github for my personal development. I have a prefix for my personal repos (hoelzro: -> git://github.com/hoelzro for fetch, g...@github.com:hoelzro/ for push) and one for all other Git repos (github: -> git://github.com/) I have a few projects where I work in a fork, but I want to fetch updates from the original project. So my url for the origin remote is github:org/project, but my pushurl is hoelzro:project. This behavior in Git currently doesn't allow me to work that way. I used to work with two remotes; origin for my repo and base for the official one, but I've found that I prefer this other way. 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. -Rob -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
Hi, Rob Hoelz wrote: > On Wed, 27 Mar 2013 11:23:45 -0700 > Jonathan Nieder wrote: >> Suppose I configure >> >> [url "git://anongit.myserver.example.com/"] >> insteadOf = myserver.example.com: >> [url "myserver:"] >> pushInsteadOf = myserver.example.com: >> >> The above code would make the insteadOf rule apply instead of >> pushInsteadOf, even when pushing. Perhaps something like the >> following would work? > > Are you sure? The message you are replying to is nonsense, due to a typo while editing. Did you see my followup? Sorry for the confusion, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
On Wed, 27 Mar 2013 11:23:45 -0700
Jonathan Nieder wrote:
> Rob Hoelz wrote:
>
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -465,7 +465,11 @@ static void alias_all_urls(void)
> > if (!remotes[i])
> > continue;
> > for (j = 0; j < remotes[i]->pushurl_nr; j++) {
> > - remotes[i]->pushurl[j] =
> > alias_url(remotes[i]->pushurl[j], &rewrites);
> > + char *copy =
> > xstrdup(remotes[i]->pushurl[j]);
> > + remotes[i]->pushurl[j] =
> > alias_url(remotes[i]->pushurl[j], &rewrites_push);
> > + if (!strcmp(copy, remotes[i]->pushurl[j]))
> > + remotes[i]->pushurl[j] =
> > alias_url(remotes[i]->pushurl[j], &rewrites);
> > + free(copy);
>
> Interesting.
>
> Suppose I configure
>
> [url "git://anongit.myserver.example.com/"]
> insteadOf = myserver.example.com:
> [url "myserver:"]
> pushInsteadOf = myserver.example.com:
>
> The above code would make the insteadOf rule apply instead of
> pushInsteadOf, even when pushing. Perhaps something like the
> following would work?
Are you sure? I create a copy, and compare the copy to the new URL.
If they're the same (pushInsteadOf not found), I then use the insteadOf
mapping to perform the alteration. Did I introduce a bug?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
Jonathan Nieder writes: > Sorry, typo. The configuration in the example above should have been > > [url "git://anongit.myserver.example.com/"] > insteadOf = myserver.example.com: > [url "myserver.example.com:"] > pushInsteadOf = myserver.example.com: > > In other words, suppose I set url.*.insteadof to point to a faster > address for fetching alongside url.*.pushinsteadof requesting that the > original address should still be used for pushing. I didn't know we were even shooting for supporting the identity mapping: url.X.pushinsteadof=X but that would certainly be nice. By the way, it seems that the original commit 1c2eafb89bca (Add url..pushInsteadOf: URL rewriting for push only, 2009-09-07) wanted to explicitly avoid use of pushInsteadOf aliasing for a pushURL and it is also documented in config.txt from day one. I think the intent is "You have a normal URL, and a way to override it explicitly with pushURL, or a way to rewrite it via the aliasing the normal URL with pushInsteadOf. Either one or the other, but not both, as having many levels of indirection would be confusing." Which I can understand and sympathise. In anay case, the change proposed in this thread seems to change that, so the documentation would need to be updated. Also the tests the original commit adds explicitly checks that pushInsteadOf is ignored, which may have to be updated (unless that test is already broken). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
Jonathan Nieder wrote: > Rob Hoelz wrote: >> --- a/remote.c >> +++ b/remote.c >> @@ -465,7 +465,11 @@ static void alias_all_urls(void) [...] >> -remotes[i]->pushurl[j] = >> alias_url(remotes[i]->pushurl[j], &rewrites); >> +char *copy = xstrdup(remotes[i]->pushurl[j]); >> +remotes[i]->pushurl[j] = >> alias_url(remotes[i]->pushurl[j], &rewrites_push); >> +if (!strcmp(copy, remotes[i]->pushurl[j])) >> +remotes[i]->pushurl[j] = >> alias_url(remotes[i]->pushurl[j], &rewrites); >> +free(copy); > > Interesting. > > Suppose I configure > > [url "git://anongit.myserver.example.com/"] > insteadOf = myserver.example.com: > [url "myserver:"] > pushInsteadOf = myserver.example.com: > > The above code would make the insteadOf rule apply instead of > pushInsteadOf, even when pushing. Sorry, typo. The configuration in the example above should have been [url "git://anongit.myserver.example.com/"] insteadOf = myserver.example.com: [url "myserver.example.com:"] pushInsteadOf = myserver.example.com: In other words, suppose I set url.*.insteadof to point to a faster address for fetching alongside url.*.pushinsteadof requesting that the original address should still be used for pushing. Thanks again for tackling this. Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
Rob Hoelz wrote:
> --- a/remote.c
> +++ b/remote.c
> @@ -465,7 +465,11 @@ static void alias_all_urls(void)
> if (!remotes[i])
> continue;
> for (j = 0; j < remotes[i]->pushurl_nr; j++) {
> - remotes[i]->pushurl[j] =
> alias_url(remotes[i]->pushurl[j], &rewrites);
> + char *copy = xstrdup(remotes[i]->pushurl[j]);
> + remotes[i]->pushurl[j] =
> alias_url(remotes[i]->pushurl[j], &rewrites_push);
> + if (!strcmp(copy, remotes[i]->pushurl[j]))
> + remotes[i]->pushurl[j] =
> alias_url(remotes[i]->pushurl[j], &rewrites);
> + free(copy);
Interesting.
Suppose I configure
[url "git://anongit.myserver.example.com/"]
insteadOf = myserver.example.com:
[url "myserver:"]
pushInsteadOf = myserver.example.com:
The above code would make the insteadOf rule apply instead of
pushInsteadOf, even when pushing. Perhaps something like the
following would work?
const char *url = remotes[i]->pushurl[j];
remotes[i]->pushurl[j] = alias_url(url, &rewrites_push);
if (remotes[i]->pushurl[j] == url)
/* No url.*.pushinsteadof configuration
matched. */
remotes[i]->pushurl[j] = alias_url(url,
&rewrites);
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -244,6 +244,83 @@ test_expect_success 'push with pushInsteadOf and
> explicit pushurl (pushInsteadOf
> )
> '
>
> +test_expect_success 'push with pushInsteadOf and explicit pushurl (pushurl +
> pushInsteadOf does rewrite in this case)' '
> + mk_empty &&
> + rm -rf ro rw &&
> + TRASH="$(pwd)/" &&
> + mkdir ro &&
> + mkdir rw &&
> + git init --bare rw/testrepo &&
> + test_config "url.file://$TRASH/ro/.insteadOf" ro: &&
> + test_config "url.file://$TRASH/rw/.pushInsteadOf" rw: &&
> + test_config remote.r.url ro:wrong &&
> + test_config remote.r.pushurl rw:testrepo &&
> + git push r refs/heads/master:refs/remotes/origin/master &&
> + (
> + cd rw/testrepo &&
> + echo "$the_commit commitrefs/remotes/origin/master" >
> expected &&
> + git for-each-ref refs/remotes/origin > actual &&
> + test_cmp expected actual
> + )
Looks good. The usual style in git tests is to include no space
after >redirection operators:
git for-each-ref refs/remotes/origin >actual &&
Hope that helps,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
On Wed, 20 Mar 2013 07:35:58 -0700 Junio C Hamano wrote: > Rob Hoelz writes: > > > On 3/19/13 7:08 PM, Junio C Hamano wrote: > >> Jonathan Nieder writes: > >> > >>> Junio C Hamano wrote: > Jonathan Nieder writes: > > Test nits: > > ... > > Hope that helps, > > > > Jonathan Nieder (3): > > push test: use test_config where appropriate > > push test: simplify check of push result > > push test: rely on &&-chaining instead of 'if bad; then echo > > Oops; fi' > Are these supposed to be follow-up patches? Preparatory steps > that Rob can reroll on top? Something else? > >>> Preparatory steps. > >> OK, thanks. I'll queue these first then. > >> > > Should I apply these to my patch to move things along? What's the > > next step for me? > > You would fetch from nearby git.git mirror, find the tip of > Janathan's series and redo your patch on top. Perhaps the session > would go like this: > > $ git fetch git://git.kernel.org/pub/scm/git/git.git/ pu > $ git log --oneline --first-parent ..FETCH_HEAD | grep > jn/push-tests 83a072a Merge branch 'jn/push-tests' into pu > $ git checkout -b rh/push-pushurl-pushinsteadof 83a072a > ... redo the work, perhaps with combinations of: > $ git cherry-pick -n $your_original_branch > $ edit t/t5516-fetch-push.sh > ... and then > $ make test > $ git commit > Ok, changes applied. New patch coming. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
Rob Hoelz writes: > On 3/19/13 7:08 PM, Junio C Hamano wrote: >> Jonathan Nieder writes: >> >>> Junio C Hamano wrote: Jonathan Nieder writes: > Test nits: > ... > Hope that helps, > > Jonathan Nieder (3): > push test: use test_config where appropriate > push test: simplify check of push result > push test: rely on &&-chaining instead of 'if bad; then echo Oops; fi' Are these supposed to be follow-up patches? Preparatory steps that Rob can reroll on top? Something else? >>> Preparatory steps. >> OK, thanks. I'll queue these first then. >> > Should I apply these to my patch to move things along? What's the next > step for me? You would fetch from nearby git.git mirror, find the tip of Janathan's series and redo your patch on top. Perhaps the session would go like this: $ git fetch git://git.kernel.org/pub/scm/git/git.git/ pu $ git log --oneline --first-parent ..FETCH_HEAD | grep jn/push-tests 83a072a Merge branch 'jn/push-tests' into pu $ git checkout -b rh/push-pushurl-pushinsteadof 83a072a ... redo the work, perhaps with combinations of: $ git cherry-pick -n $your_original_branch $ edit t/t5516-fetch-push.sh ... and then $ make test $ git commit -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
On 3/19/13 7:08 PM, Junio C Hamano wrote: > Jonathan Nieder writes: > >> Junio C Hamano wrote: >>> Jonathan Nieder writes: Test nits: ... Hope that helps, Jonathan Nieder (3): push test: use test_config where appropriate push test: simplify check of push result push test: rely on &&-chaining instead of 'if bad; then echo Oops; fi' >>> Are these supposed to be follow-up patches? Preparatory steps that >>> Rob can reroll on top? Something else? >> Preparatory steps. > OK, thanks. I'll queue these first then. > Should I apply these to my patch to move things along? What's the next step for me? Thanks, Rob -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
Jonathan Nieder writes: > Junio C Hamano wrote: >> Jonathan Nieder writes: > >>> Test nits: >>> ... >>> Hope that helps, >>> >>> Jonathan Nieder (3): >>> push test: use test_config where appropriate >>> push test: simplify check of push result >>> push test: rely on &&-chaining instead of 'if bad; then echo Oops; fi' >> >> Are these supposed to be follow-up patches? Preparatory steps that >> Rob can reroll on top? Something else? > > Preparatory steps. OK, thanks. I'll queue these first then. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
Junio C Hamano wrote: > Jonathan Nieder writes: >> Test nits: >> ... >> Hope that helps, >> >> Jonathan Nieder (3): >> push test: use test_config where appropriate >> push test: simplify check of push result >> push test: rely on &&-chaining instead of 'if bad; then echo Oops; fi' > > Are these supposed to be follow-up patches? Preparatory steps that > Rob can reroll on top? Something else? Preparatory steps. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
Jonathan Nieder writes: > Test nits: > ... > Hope that helps, > > Jonathan Nieder (3): > push test: use test_config where appropriate > push test: simplify check of push result > push test: rely on &&-chaining instead of 'if bad; then echo Oops; fi' Are these supposed to be follow-up patches? Preparatory steps that Rob can reroll on top? Something else? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
Hi, Rob Hoelz wrote: > [url "git://github.com/"] > insteadOf = github: > [url "git://github.com/myuser/"] > insteadOf = mygithub: > [url "g...@github.com:myuser/"] > pushInsteadOf = mygithub: > [remote "origin"] > url = github:organization/project > pushurl = mygithub:project > > With the above configuration, the following occurs: > > $ git push origin master > fatal: remote error: > You can't push to git://github.com/myuser/project.git > Use g...@github.com:myuser/project.git > > So you can see that pushurl is being followed (it's not attempting to > push to git://github.com/organization/project), but insteadOf values are > being used as opposed to pushInsteadOf values for expanding the pushurl > alias. At first glance it is not always obvious how overlapping settings like these should interact. Thanks for an instructive example that makes the right behavior obvious. Test nits: [...] > --- a/t/t5516-fetch-push.sh > +++ b/t/t5516-fetch-push.sh > @@ -244,6 +244,87 @@ test_expect_success 'push with pushInsteadOf and > explicit pushurl (pushInsteadOf > ) > ' > > +test_expect_success 'push with pushInsteadOf and explicit pushurl (pushurl + > pushInsteadOf does rewrite in this case)' ' > + mk_empty && > + rm -rf ro rw && > + TRASH="$(pwd)/" && > + mkdir ro && > + mkdir rw && > + git init --bare rw/testrepo && > + git config "url.file://$TRASH/ro/.insteadOf" ro: && > + git config "url.file://$TRASH/rw/.pushInsteadOf" rw: && The surrounding tests don't do this, but I wonder if it would make sense to use test_config instead of 'git config' here. That way, the test's settings wouldn't affect other tests, and in particular if someone later decides to refactor the file by reordering tests, she could be confident that that would not break anything. In most of the surrounding tests it does not matter because 'git config' is run in a subdirectory that is not reused later. Patches fixing the exceptions below. > + git config remote.r.url ro:wrong && > + git config remote.r.pushurl rw:testrepo && > + git push r refs/heads/master:refs/remotes/origin/master && > + ( > + cd rw/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) > + ) To produce more useful "./t5516-fetch-push.sh -v -i" output when the comparison fails: echo "$the_commit commit refs/remotes/origin/master" >expect && ( cd rw/testrepo && git for-each-ref refs/remotes/origin ) >actual && test_cmp expect actual Hope that helps, Jonathan Nieder (3): push test: use test_config where appropriate push test: simplify check of push result push test: rely on &&-chaining instead of 'if bad; then echo Oops; fi' t/t5516-fetch-push.sh | 156 +- 1 file changed, 65 insertions(+), 91 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
On Sun, 17 Mar 2013 16:35:59 -0700
Junio C Hamano wrote:
> Rob Hoelz writes:
>
> > git push currently doesn't consider pushInsteadOf when
> > using pushurl; this tests and fixes that.
> >
> > If you use pushurl with an alias that has a pushInsteadOf
> > configuration value, Git does not take advantage of it. For
> > example:
> >
> > [url "git://github.com/"]
> > insteadOf = github:
> > [url "git://github.com/myuser/"]
> > insteadOf = mygithub:
> > [url "g...@github.com:myuser/"]
> > pushInsteadOf = mygithub:
> > [remote "origin"]
> > url = github:organization/project
> > pushurl = mygithub:project
>
> Incomplete sentence? For example [this is an example configuration]
> and then what happens? Something like "with the sample
> configuration, 'git push origin' should follow pushurl and then turn
> it into X, but instead it ends up accessing Y".
>
> If there is no pushInsteadOf, does it still follow insteadOf? Is it
> tested already?
>
> Wouldn't you want to cover all the combinations to negative cases
> (i.e. making sure the codepath to support a new case does not affect
> behaviour of the code outside the new case)? A remote with and
> without pushurl (two combinations) and a pseudo URL scheme with and
> without pushInsteadOf (again, two combinations) will give you four
> cases.
>
>
> Thanks.
I've taken your advice, and an amended patch follows.
>
> >
> > Signed-off-by: Rob Hoelz
> > ---
> > remote.c | 2 +-
> > t/t5516-fetch-push.sh | 20
> > 2 files changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/remote.c b/remote.c
> > index ca1f8f2..de7a915 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -465,7 +465,7 @@ static void alias_all_urls(void)
> > if (!remotes[i])
> > continue;
> > for (j = 0; j < remotes[i]->pushurl_nr; j++) {
> > - remotes[i]->pushurl[j] =
> > alias_url(remotes[i]->pushurl[j], &rewrites);
> > + remotes[i]->pushurl[j] =
> > alias_url(remotes[i]->pushurl[j], &rewrites_push); }
> > add_pushurl_aliases = remotes[i]->pushurl_nr == 0;
> > for (j = 0; j < remotes[i]->url_nr; j++) {
> > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> > index b5417cc..272e225 100755
> > --- a/t/t5516-fetch-push.sh
> > +++ b/t/t5516-fetch-push.sh
> > @@ -244,6 +244,26 @@ test_expect_success 'push with pushInsteadOf
> > and explicit pushurl (pushInsteadOf )
> > '
> >
> > +test_expect_success 'push with pushInsteadOf and explicit pushurl
> > (pushInsteadOf does rewrite in this case)' '
> > + mk_empty &&
> > + TRASH="$(pwd)/" &&
> > + mkdir ro &&
> > + mkdir rw &&
> > + git init --bare rw/testrepo &&
> > + git config "url.file://$TRASH/ro/.insteadOf" ro: &&
> > + git config "url.file://$TRASH/rw/.pushInsteadOf" rw: &&
> > + git config remote.r.url ro:wrong &&
> > + git config remote.r.pushurl rw:testrepo &&
> > + git push r refs/heads/master:refs/remotes/origin/master &&
> > + (
> > + cd rw/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)
> > + )
> > +'
> > +
> > test_expect_success 'push with matching heads' '
> >
> > mk_test heads/master &&
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
On 3/18/13 12:35 AM, Junio C Hamano wrote:
> Rob Hoelz writes:
>
>> git push currently doesn't consider pushInsteadOf when
>> using pushurl; this tests and fixes that.
>>
>> If you use pushurl with an alias that has a pushInsteadOf configuration
>> value, Git does not take advantage of it. For example:
>>
>> [url "git://github.com/"]
>> insteadOf = github:
>> [url "git://github.com/myuser/"]
>> insteadOf = mygithub:
>> [url "g...@github.com:myuser/"]
>> pushInsteadOf = mygithub:
>> [remote "origin"]
>> url = github:organization/project
>> pushurl = mygithub:project
> Incomplete sentence? For example [this is an example configuration]
> and then what happens? Something like "with the sample
> configuration, 'git push origin' should follow pushurl and then turn
> it into X, but instead it ends up accessing Y".
>
> If there is no pushInsteadOf, does it still follow insteadOf? Is it
> tested already?
>
> Wouldn't you want to cover all the combinations to negative cases
> (i.e. making sure the codepath to support a new case does not affect
> behaviour of the code outside the new case)? A remote with and
> without pushurl (two combinations) and a pseudo URL scheme with and
> without pushInsteadOf (again, two combinations) will give you four
> cases.
>
>
> Thanks.
Sorry; that's a good point. With the above configuration, the following
fails:
$ git push origin master
With the following message:
fatal: remote error:
You can't push to git://github.com/myuser/project.git
Use g...@github.com:myuser/project.git
So you can see that pushurl is being followed (it's not attempting to
push to git://github.com/organization/project), but insteadOf values are
being used as opposed to pushInsteadOf values for expanding the pushurl
alias.
I haven't tried without pushInsteadOf; I will add a test for it later
today. I assume that if pushInsteadOf isn't found, insteadOf should be
used? I will also consider the other test cases you described.
Thanks again for the feedback!
>
>> Signed-off-by: Rob Hoelz
>> ---
>> remote.c | 2 +-
>> t/t5516-fetch-push.sh | 20
>> 2 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/remote.c b/remote.c
>> index ca1f8f2..de7a915 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -465,7 +465,7 @@ static void alias_all_urls(void)
>> if (!remotes[i])
>> continue;
>> for (j = 0; j < remotes[i]->pushurl_nr; j++) {
>> -remotes[i]->pushurl[j] =
>> alias_url(remotes[i]->pushurl[j], &rewrites);
>> +remotes[i]->pushurl[j] =
>> alias_url(remotes[i]->pushurl[j], &rewrites_push);
>> }
>> add_pushurl_aliases = remotes[i]->pushurl_nr == 0;
>> for (j = 0; j < remotes[i]->url_nr; j++) {
>> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
>> index b5417cc..272e225 100755
>> --- a/t/t5516-fetch-push.sh
>> +++ b/t/t5516-fetch-push.sh
>> @@ -244,6 +244,26 @@ test_expect_success 'push with pushInsteadOf and
>> explicit pushurl (pushInsteadOf
>> )
>> '
>>
>> +test_expect_success 'push with pushInsteadOf and explicit pushurl
>> (pushInsteadOf does rewrite in this case)' '
>> +mk_empty &&
>> +TRASH="$(pwd)/" &&
>> +mkdir ro &&
>> +mkdir rw &&
>> +git init --bare rw/testrepo &&
>> +git config "url.file://$TRASH/ro/.insteadOf" ro: &&
>> +git config "url.file://$TRASH/rw/.pushInsteadOf" rw: &&
>> +git config remote.r.url ro:wrong &&
>> +git config remote.r.pushurl rw:testrepo &&
>> +git push r refs/heads/master:refs/remotes/origin/master &&
>> +(
>> +cd rw/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)
>> +)
>> +'
>> +
>> test_expect_success 'push with matching heads' '
>>
>> mk_test heads/master &&
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] push: Alias pushurl from push rewrites
Rob Hoelz writes:
> git push currently doesn't consider pushInsteadOf when
> using pushurl; this tests and fixes that.
>
> If you use pushurl with an alias that has a pushInsteadOf configuration
> value, Git does not take advantage of it. For example:
>
> [url "git://github.com/"]
> insteadOf = github:
> [url "git://github.com/myuser/"]
> insteadOf = mygithub:
> [url "g...@github.com:myuser/"]
> pushInsteadOf = mygithub:
> [remote "origin"]
> url = github:organization/project
> pushurl = mygithub:project
Incomplete sentence? For example [this is an example configuration]
and then what happens? Something like "with the sample
configuration, 'git push origin' should follow pushurl and then turn
it into X, but instead it ends up accessing Y".
If there is no pushInsteadOf, does it still follow insteadOf? Is it
tested already?
Wouldn't you want to cover all the combinations to negative cases
(i.e. making sure the codepath to support a new case does not affect
behaviour of the code outside the new case)? A remote with and
without pushurl (two combinations) and a pseudo URL scheme with and
without pushInsteadOf (again, two combinations) will give you four
cases.
Thanks.
>
> Signed-off-by: Rob Hoelz
> ---
> remote.c | 2 +-
> t/t5516-fetch-push.sh | 20
> 2 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/remote.c b/remote.c
> index ca1f8f2..de7a915 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -465,7 +465,7 @@ static void alias_all_urls(void)
> if (!remotes[i])
> continue;
> for (j = 0; j < remotes[i]->pushurl_nr; j++) {
> - remotes[i]->pushurl[j] =
> alias_url(remotes[i]->pushurl[j], &rewrites);
> + remotes[i]->pushurl[j] =
> alias_url(remotes[i]->pushurl[j], &rewrites_push);
> }
> add_pushurl_aliases = remotes[i]->pushurl_nr == 0;
> for (j = 0; j < remotes[i]->url_nr; j++) {
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index b5417cc..272e225 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -244,6 +244,26 @@ test_expect_success 'push with pushInsteadOf and
> explicit pushurl (pushInsteadOf
> )
> '
>
> +test_expect_success 'push with pushInsteadOf and explicit pushurl
> (pushInsteadOf does rewrite in this case)' '
> + mk_empty &&
> + TRASH="$(pwd)/" &&
> + mkdir ro &&
> + mkdir rw &&
> + git init --bare rw/testrepo &&
> + git config "url.file://$TRASH/ro/.insteadOf" ro: &&
> + git config "url.file://$TRASH/rw/.pushInsteadOf" rw: &&
> + git config remote.r.url ro:wrong &&
> + git config remote.r.pushurl rw:testrepo &&
> + git push r refs/heads/master:refs/remotes/origin/master &&
> + (
> + cd rw/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)
> + )
> +'
> +
> test_expect_success 'push with matching heads' '
>
> mk_test heads/master &&
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
