# [PATCH] push: Alias pushurl from push rewrites 3 messages from 2013-03-27 to 2013-03-28. Participants: Rob Hoelz, Junio C Hamano, Jonathan Nieder. Thread: https://gitlist.dev/t/33312 ## Rob Hoelz, 2013-03-27 22:42 Subject: [PATCH] push: Alias pushurl from push rewrites Message-ID: <20130327174259.373bafe1@hoelz.ro> URL: https://gitlist.dev/e/20130327174259.373bafe1%40hoelz.ro ``` git push currently doesn't consider pushInsteadOf when using pushurl; this test tests 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 "git@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 git@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. This commit fixes that. Signed-off-by: Rob Hoelz --- Documentation/config.txt | 3 +- remote.c | 6 +++- t/t5516-fetch-push.sh | 77 +++++++++++++++++++++++++++++++++++++++++++----- 3 files changed, 75 insertions(+), 11 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index b3023b8..5610962 100644 --- 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. user.email:: Your email address to be recorded in any newly created commits. diff --git a/remote.c b/remote.c index e53a6eb..1ea240a 100644 --- 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); } 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 c31e5c1..fbe0f29 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -228,19 +228,80 @@ test_expect_success 'push with pushInsteadOf' ' ) ' -test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' ' +test_expect_success 'push with pushInsteadOf and explicit pushurl (pushurl + pushInsteadOf does rewrite in this case)' ' mk_empty && + rm -rf ro rw && TRASH="$(pwd)/" && - git config "url.trash2/.pushInsteadOf" trash/ && - git config remote.r.url trash/wrong && - git config remote.r.pushurl "$TRASH/testrepo" && + 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 testrepo && - r=$(git show-ref -s --verify refs/remotes/origin/master) && - test "z$r" = "z$the_commit" && + cd rw/testrepo && + echo "$the_commit commit refs/remotes/origin/master" > expected && + git for-each-ref refs/remotes/origin > actual && + test_cmp expected actual + ) +' - test 1 = $(git for-each-ref refs/remotes/origin | wc -l) +test_expect_success 'push without pushInsteadOf and explicit pushurl (pushurl + insteadOf is used for rewrite)' ' + 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/.insteadOf" 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 commit refs/remotes/origin/master" > expected && + git for-each-ref refs/remotes/origin > actual && + test_cmp expected actual + ) +' + +test_expect_success 'push with pushInsteadOf but without explicit pushurl (url + pushInsteadOf is used for rewrite)' ' + mk_empty && + rm -rf ro rw && + TRASH="$(pwd)/" && + mkdir ro && + mkdir rw && + git init --bare rw/testrepo && + test_config "url.file://$TRASH/ro/.insteadOf" rw: && + test_config "url.file://$TRASH/rw/.pushInsteadOf" rw: && + test_config remote.r.url rw:testrepo && + git push r refs/heads/master:refs/remotes/origin/master && + ( + cd rw/testrepo && + echo "$the_commit commit refs/remotes/origin/master" > expected && + git for-each-ref refs/remotes/origin > actual && + test_cmp expected actual + ) +' + +test_expect_success 'push without pushInsteadOf and without explicit pushurl (url + insteadOf is used for rewrite)' ' + mk_empty && + rm -rf ro rw && + TRASH="$(pwd)/" && + mkdir ro && + mkdir rw && + git init --bare rw/testrepo && + test_config "url.file://$TRASH/rw/.insteadOf" rw: && + test_config remote.r.url rw:testrepo && + git push r refs/heads/master:refs/remotes/origin/master && + ( + cd rw/testrepo && + echo "$the_commit commit refs/remotes/origin/master" > expected && + git for-each-ref refs/remotes/origin > actual && + test_cmp expected actual ) ' -- 1.8.2 ``` ## Junio C Hamano, 2013-03-27 23:08 Subject: Re: [PATCH] push: Alias pushurl from push rewrites Message-ID: <7vfvzgjv9d.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vfvzgjv9d.fsf%40alter.siamese.dyndns.org In-Reply-To: <20130327174259.373bafe1@hoelz.ro> ``` >> 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 "git@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 git@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 "git@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 "git@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 "git@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. ``` ## Jonathan Nieder, 2013-03-28 00:07 Subject: Re: [PATCH] push: Alias pushurl from push rewrites Message-ID: <20130328000704.GN28148@google.com> URL: https://gitlist.dev/e/20130328000704.GN28148%40google.com In-Reply-To: <20130327174259.373bafe1@hoelz.ro> ``` 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 strings match +the URL, Git falls back to insteadOf strings. user.email:: Your email address to be recorded in any newly created commits. diff --git i/remote.c w/remote.c index cfcb77a..7255926 100644 --- i/remote.c +++ w/remote.c @@ -466,11 +466,14 @@ static void alias_all_urls(void) if (!remotes[i]) continue; for (j = 0; j < remotes[i]->pushurl_nr; j++) { - 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); + 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 string matched. + * Apply url.*.insteadof. + */ + remotes[i]->pushurl[j] = alias_url(url, &rewrites); } add_pushurl_aliases = remotes[i]->pushurl_nr == 0; for (j = 0; j < remotes[i]->url_nr; j++) { ```