{"thread":{"id":"33312","subject":"[PATCH] push: Alias pushurl from push rewrites","startedAt":"2013-03-27T22:42:59Z","lastAt":"2013-03-28T00:07:04Z","messageCount":3,"participants":["Rob Hoelz","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"212470","messageId":"20130327174259.373bafe1@hoelz.ro","threadId":"33312","inReplyTo":null,"subject":"[PATCH] push: Alias pushurl from push rewrites","fromName":"Rob Hoelz","fromEmail":"rob@hoelz.ro","sentAt":"2013-03-27T22:42:59Z","receivedAt":"2013-03-27T22:42:59Z","isPatch":true,"sender":{"key":"rob@hoelz.ro","avatar":"https://gravatar.com/avatar/c62607c6cef695f7a6cbd0f1f3ebd359e8c471af271581a6d7e29e3fab6fb788?d=mp&s=160"},"body":"git push currently doesn't consider pushInsteadOf when\nusing pushurl; this test tests that.\n\nIf you use pushurl with an alias that has a pushInsteadOf configuration\nvalue, Git does not take advantage of it.  For example:\n\n[url \"git://github.com/\"]\n    insteadOf = github:\n[url \"git://github.com/myuser/\"]\n    insteadOf = mygithub:\n[url \"git@github.com:myuser/\"]\n    pushInsteadOf = mygithub:\n[remote \"origin\"]\n    url     = github:organization/project\n    pushurl = mygithub:project\n\nWith the above configuration, the following occurs:\n\n$ git push origin master\nfatal: remote error:\n  You can't push to git://github.com/myuser/project.git\n  Use git@github.com:myuser/project.git\n\nSo you can see that pushurl is being followed (it's not attempting to\npush to git://github.com/organization/project), but insteadOf values are\nbeing used as opposed to pushInsteadOf values for expanding the pushurl\nalias.\n\nThis commit fixes that.\n\nSigned-off-by: Rob Hoelz <rob@hoelz.ro>\n---\n Documentation/config.txt |  3 +-\n remote.c                 |  6 +++-\n t/t5516-fetch-push.sh    | 77 +++++++++++++++++++++++++++++++++++++++++++-----\n 3 files changed, 75 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex b3023b8..5610962 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2142,8 +2142,7 @@ url.<base>.pushInsteadOf::\n \tautomatically use an appropriate URL to push, even for a\n \tnever-before-seen repository on the site.  When more than one\n \tpushInsteadOf strings match a given URL, the longest match is\n-\tused.  If a remote has an explicit pushurl, Git will ignore this\n-\tsetting for that remote.\n+\tused.\n \n user.email::\n \tYour email address to be recorded in any newly created commits.\ndiff --git a/remote.c b/remote.c\nindex e53a6eb..1ea240a 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -465,7 +465,11 @@ static void alias_all_urls(void)\n \t\tif (!remotes[i])\n \t\t\tcontinue;\n \t\tfor (j = 0; j < remotes[i]->pushurl_nr; j++) {\n-\t\t\tremotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites);\n+\t\t\tchar *copy = xstrdup(remotes[i]->pushurl[j]);\n+\t\t\tremotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites_push);\n+\t\t\tif (!strcmp(copy, remotes[i]->pushurl[j]))\n+\t\t\t\tremotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites);\n+\t\t\tfree(copy);\n \t\t}\n \t\tadd_pushurl_aliases = remotes[i]->pushurl_nr == 0;\n \t\tfor (j = 0; j < remotes[i]->url_nr; j++) {\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex c31e5c1..fbe0f29 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -228,19 +228,80 @@ test_expect_success 'push with pushInsteadOf' '\n \t)\n '\n \n-test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '\n+test_expect_success 'push with pushInsteadOf and explicit pushurl (pushurl + pushInsteadOf does rewrite in this case)' '\n \tmk_empty &&\n+\trm -rf ro rw &&\n \tTRASH=\"$(pwd)/\" &&\n-\tgit config \"url.trash2/.pushInsteadOf\" trash/ &&\n-\tgit config remote.r.url trash/wrong &&\n-\tgit config remote.r.pushurl \"$TRASH/testrepo\" &&\n+\tmkdir ro &&\n+\tmkdir rw &&\n+\tgit init --bare rw/testrepo &&\n+\ttest_config \"url.file://$TRASH/ro/.insteadOf\" ro: &&\n+\ttest_config \"url.file://$TRASH/rw/.pushInsteadOf\" rw: &&\n+\ttest_config remote.r.url ro:wrong &&\n+\ttest_config remote.r.pushurl rw:testrepo &&\n \tgit push r refs/heads/master:refs/remotes/origin/master &&\n \t(\n-\t\tcd testrepo &&\n-\t\tr=$(git show-ref -s --verify refs/remotes/origin/master) &&\n-\t\ttest \"z$r\" = \"z$the_commit\" &&\n+\t\tcd rw/testrepo &&\n+\t\techo \"$the_commit commit\trefs/remotes/origin/master\" > expected &&\n+\t\tgit for-each-ref refs/remotes/origin > actual &&\n+\t\ttest_cmp expected actual\n+\t)\n+'\n \n-\t\ttest 1 = $(git for-each-ref refs/remotes/origin | wc -l)\n+test_expect_success 'push without pushInsteadOf and explicit pushurl (pushurl + insteadOf is used for rewrite)' '\n+\tmk_empty &&\n+\trm -rf ro rw &&\n+\tTRASH=\"$(pwd)/\" &&\n+\tmkdir ro &&\n+\tmkdir rw &&\n+\tgit init --bare rw/testrepo &&\n+\ttest_config \"url.file://$TRASH/ro/.insteadOf\" ro: &&\n+\ttest_config \"url.file://$TRASH/rw/.insteadOf\" rw: &&\n+\ttest_config remote.r.url ro:wrong &&\n+\ttest_config remote.r.pushurl rw:testrepo &&\n+\tgit push r refs/heads/master:refs/remotes/origin/master &&\n+\t(\n+\t\tcd rw/testrepo &&\n+\t\techo \"$the_commit commit\trefs/remotes/origin/master\" > expected &&\n+\t\tgit for-each-ref refs/remotes/origin > actual &&\n+\t\ttest_cmp expected actual\n+\t)\n+'\n+\n+test_expect_success 'push with pushInsteadOf but without explicit pushurl (url + pushInsteadOf is used for rewrite)' '\n+\tmk_empty &&\n+\trm -rf ro rw &&\n+\tTRASH=\"$(pwd)/\" &&\n+\tmkdir ro &&\n+\tmkdir rw &&\n+\tgit init --bare rw/testrepo &&\n+\ttest_config \"url.file://$TRASH/ro/.insteadOf\" rw: &&\n+\ttest_config \"url.file://$TRASH/rw/.pushInsteadOf\" rw: &&\n+\ttest_config remote.r.url rw:testrepo &&\n+\tgit push r refs/heads/master:refs/remotes/origin/master &&\n+\t(\n+\t\tcd rw/testrepo &&\n+\t\techo \"$the_commit commit\trefs/remotes/origin/master\" > expected &&\n+\t\tgit for-each-ref refs/remotes/origin > actual &&\n+\t\ttest_cmp expected actual\n+\t)\n+'\n+\n+test_expect_success 'push without pushInsteadOf and without explicit pushurl (url + insteadOf is used for rewrite)' '\n+\tmk_empty &&\n+\trm -rf ro rw &&\n+\tTRASH=\"$(pwd)/\" &&\n+\tmkdir ro &&\n+\tmkdir rw &&\n+\tgit init --bare rw/testrepo &&\n+\ttest_config \"url.file://$TRASH/rw/.insteadOf\" rw: &&\n+\ttest_config remote.r.url rw:testrepo &&\n+\tgit push r refs/heads/master:refs/remotes/origin/master &&\n+\t(\n+\t\tcd rw/testrepo &&\n+\t\techo \"$the_commit commit\trefs/remotes/origin/master\" > expected &&\n+\t\tgit for-each-ref refs/remotes/origin > actual &&\n+\t\ttest_cmp expected actual\n \t)\n '\n \n-- \n1.8.2\n"},{"id":"212480","messageId":"7vfvzgjv9d.fsf@alter.siamese.dyndns.org","threadId":"33312","inReplyTo":"20130327174259.373bafe1@hoelz.ro","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-27T23:08:46Z","receivedAt":"2013-03-27T23:08:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":">> Subject: Re: [PATCH] push: Alias pushurl from push rewrites\n\nPlease increment [PATCH v$n] for a new round, so that we can tell\nwhich one is the latest.\n\nRob Hoelz <rob@hoelz.ro> writes:\n\n> git push currently doesn't consider pushInsteadOf when\n> using pushurl; this test tests that.\n\nThis patch is no longer \"this test\", and I thought you are changing\nthe behaviour so that the command takes it into account.\n\n> If you use pushurl with an alias that has a pushInsteadOf configuration\n> value, Git does not take advantage of it.  For example:\n>\n> [url \"git://github.com/\"]\n>     insteadOf = github:\n> [url \"git://github.com/myuser/\"]\n>     insteadOf = mygithub:\n> [url \"git@github.com:myuser/\"]\n>     pushInsteadOf = mygithub:\n> [remote \"origin\"]\n>     url     = github:organization/project\n>     pushurl = mygithub:project\n\nPerhaps indent the above a bit to make it more readable?\n\nBut more importantly, isn't this a variant of what we discussed in a\nseparate thread about \"triangular workflow\", where you pull from one\nplace (org/project) and push to another (project)?  I thought the\nconclusion from the discussion was that using url/pushurl to call\ntwo logically diffent repositories with the same name is wrong. For\none thing, the \"pretend as if we immediately fetched\" update of\nremote tracking branches will go out of sync, so the above is a\nbroken configuration, with or without pushInsteadOf.\n\n> With the above configuration, the following occurs:\n>\n> $ git push origin master\n> fatal: remote error:\n>   You can't push to git://github.com/myuser/project.git\n>   Use git@github.com:myuser/project.git\n\nYup, that is a documented behaviour.\n\n> So you can see that pushurl is being followed (it's not attempting to\n> push to git://github.com/organization/project), but insteadOf values are\n> being used as opposed to pushInsteadOf values for expanding the pushurl\n> alias.\n>\n> This commit fixes that.\n\nSaying \"fixes\" before justifying why such a patch that changes a\ndocumented behaviour is a good idea is a bit weak, to say the least.\n\nCare to justify with a non-triangular example, where origin is\nassociated to logically the same repository?\n\nThat is, currently you can do either:\n\n     ; Fetch anonymously\n     [url \"git://github.com/me/\"]\n        insteadOf = github:\n\n     ; Pushing needs to go over ssh\n     [url \"git@github.com:me/\"]\n        pushInsteadOf = github:\n\n     ; The repository\n     [remote \"origin\"]\n        url = github:project\n\nor:\n\n     ; Fetch anonymously\n     [url \"git://github.com/me/\"]\n        insteadOf = githubf:\n\n     ; Pushing needs to go over ssh\n     [url \"git@github.com:me/\"]\n        insteadOf = githubp:\n\n     ; The repository\n     [remote \"origin\"]\n        url = githubf:project\n        pushUrl = githubp:project\n\nYou would need to make a convincing argument to justify why allowing:\n\n     ; Fetch anonymously\n     [url \"git://github.com/me/\"]\n        insteadOf = github:\n\n     ; Pushing needs to go over ssh\n     [url \"git@github.com:me/\"]\n        pushInsteadOf = github:\n\n     ; The repository\n     [remote \"origin\"]\n        url = github:project\n        pushUrl = github:project\n\nis a good idea.  \n\nI also suspect there could be people who rely on the documented\nbehaviour; they can manually rewrite their pushURL to what insteadOf\nsetting has been rewriting to work it around, but to them, this\nchange may be a needless regression.  I do not offhand how severe a\nregression it is, though.\n"},{"id":"212485","messageId":"20130328000704.GN28148@google.com","threadId":"33312","inReplyTo":"20130327174259.373bafe1@hoelz.ro","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-03-28T00:07:04Z","receivedAt":"2013-03-28T00:07:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRob Hoelz wrote:\n\n> git push currently doesn't consider pushInsteadOf when\n> using pushurl; this test tests that.\n\nI'd leave out this paragraph, since it is redundant next to the rest\nof the commit message (except that you have added tests, which ideally\nevery bugfix patch would do :)).\n\n[...]\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -2142,8 +2142,7 @@ url.<base>.pushInsteadOf::\n>  \tautomatically use an appropriate URL to push, even for a\n>  \tnever-before-seen repository on the site.  When more than one\n>  \tpushInsteadOf strings match a given URL, the longest match is\n> -\tused.  If a remote has an explicit pushurl, Git will ignore this\n> -\tsetting for that remote.\n> +\tused.\n\nOld-timers used to the previous behavior might not guess immediately\nhow this interacts with pushurl.  (If I understand the initial\ndiscussion at [1] correctly, an earlier, unreleased version of the\nfeature would push to the rewritten fetch url *in addition to* the\nunmodified push url.  So there's more than one possible behavior here.)\n\nHow about:\n\n\turl.<base>.pushInsteadOf\n\n\t\tAny URL that starts with this value will not be pushed to;\n\t\tinstead, it will be rewritten ... even for a\n\t\tnever-before-seen repository on the site.\n\n\t\tThis rewriting takes place even for explicit push URLs\n\t\tset using the `remote.<name>.pushurl` configuration\n\t\tvariable.\n\n\t\tWhen more than one pushInsteadOf string matches a given\n\t\tURL, the longest match is used.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/127889\n\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -465,7 +465,11 @@ static void alias_all_urls(void)\n>  \t\tif (!remotes[i])\n>  \t\t\tcontinue;\n>  \t\tfor (j = 0; j < remotes[i]->pushurl_nr; j++) {\n> -\t\t\tremotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites);\n> +\t\t\tchar *copy = xstrdup(remotes[i]->pushurl[j]);\n> +\t\t\tremotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites_push);\n> +\t\t\tif (!strcmp(copy, remotes[i]->pushurl[j]))\n> +\t\t\t\tremotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites);\n> +\t\t\tfree(copy);\n\nThis is relying on !strcmp to detect that no pushInsteadOf rule\nmatched the URL.  By contrast, the existing pushInsteadOf support\ndoes the following:\n\n\tconst char *pushurl = alias_url(url, &rewrites_push);\n\tif (pushurl != url)\n\t\tadd_pushurl(remote, pushurl);\n\nBecause it compares pointers, not strings, it is able to correctly\ntreat the identity substitution as a real match.  It also avoids some\nallocation churn.  This new alias_url() call site should follow the\nsame convention.\n\nLooking at that existing code also made me worry \"Are we applying\nthe pushinsteadof subsitution twice?\"\n\nSo let's see what happens:\n\n\tCaller calls into remote_get() to learn about remote \"origin\".\n\t... which in turn calls read_config()\n\t... which sets the config machinery in motion with callback handle_config()\n\t... which stores rewrite rules in 'rewrites' and 'rewrites_push' and\n\t    unmodified URLs in remotes[i]->url[], remotes[i]->pushurl[]\n\n\tNow read_config() calls alias_all_urls() to tweak the url and\n\tpushurl fields in place.  For each remote:\n\t 1. If a pushurl matches an *.insteadof rewrite rule, rewrite it.\n\t 2. Check if any pushurls exist.\n\t 3. If a url matches a *.pushinsteadof rule and no raw pushurls\n\t    existed, use the rewritten url as a push url.\n\n\t    If a url matches a *.insteadof rewrite rule, rewrite it.\n\nWith your tweak, step (1) above just also checks for *.pushinsteadof\nin addition to *.insteadof, which should be safe (modulo the string\ncomparison vs pointer comparison detail mentioned above)\n\nThere's also the legacy .git/remotes and .git/branches code paths,\nwhich are basically the same except there's no place for a pushurl.\n\nHow about the below, for squashing in?\n\ndiff --git i/Documentation/config.txt w/Documentation/config.txt\nindex 665c0de..25565ca 100644\n--- i/Documentation/config.txt\n+++ w/Documentation/config.txt\n@@ -2150,9 +2150,14 @@ url.<base>.pushInsteadOf::\n \taccess methods, some of which do not allow push, this feature\n \tallows people to specify a pull-only URL and have Git\n \tautomatically use an appropriate URL to push, even for a\n-\tnever-before-seen repository on the site.  When more than one\n-\tpushInsteadOf strings match a given URL, the longest match is\n-\tused.\n+\tnever-before-seen repository on the site.\n++\n+This rewriting takes place even for explicit push URLs set\n+using the `remote.<name>.pushurl` configuration variable.\n++\n+When more than one pushInsteadOf string matches a given URL,\n+the longest match is used.  If no pushInsteadOf strings match\n+the URL, Git falls back to insteadOf strings.\n \n user.email::\n \tYour email address to be recorded in any newly created commits.\ndiff --git i/remote.c w/remote.c\nindex cfcb77a..7255926 100644\n--- i/remote.c\n+++ w/remote.c\n@@ -466,11 +466,14 @@ static void alias_all_urls(void)\n \t\tif (!remotes[i])\n \t\t\tcontinue;\n \t\tfor (j = 0; j < remotes[i]->pushurl_nr; j++) {\n-\t\t\tchar *copy = xstrdup(remotes[i]->pushurl[j]);\n-\t\t\tremotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites_push);\n-\t\t\tif (!strcmp(copy, remotes[i]->pushurl[j]))\n-\t\t\t\tremotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites);\n-\t\t\tfree(copy);\n+\t\t\tconst char *url = remotes[i]->pushurl[j];\n+\t\t\tremotes[i]->pushurl[j] = alias_url(url, &rewrites_push);\n+\t\t\tif (remotes[i]->pushurl[j] == url)\n+\t\t\t\t/*\n+\t\t\t\t * No url.*.pushinsteadof string matched.\n+\t\t\t\t * Apply url.*.insteadof.\n+\t\t\t\t */\n+\t\t\t\tremotes[i]->pushurl[j] = alias_url(url, &rewrites);\n \t\t}\n \t\tadd_pushurl_aliases = remotes[i]->pushurl_nr == 0;\n \t\tfor (j = 0; j < remotes[i]->url_nr; j++) {\n"}]}