{"thread":{"id":"33306","subject":"[PATCH] push: Alias pushurl from push rewrites","startedAt":"2013-03-27T17:22:16Z","lastAt":"2013-03-29T05:29:51Z","messageCount":24,"participants":["Rob Hoelz","Jonathan Nieder","Junio C Hamano","Josh Triplett"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"212414","messageId":"20130327122216.5de0c336@hoelz.ro","threadId":"33306","inReplyTo":null,"subject":"[PATCH] push: Alias pushurl from push rewrites","fromName":"Rob Hoelz","fromEmail":"rob@hoelz.ro","sentAt":"2013-03-27T17:22:16Z","receivedAt":"2013-03-27T17:22:16Z","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 remote.c              |  6 +++-\n t/t5516-fetch-push.sh | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 82 insertions(+), 1 deletion(-)\n\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..119f043 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -244,6 +244,83 @@ test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf\n \t)\n '\n \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+\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 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 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 test_expect_success 'push with matching heads' '\n \n \tmk_test heads/master &&\n-- \n1.8.2\n"},{"id":"212424","messageId":"20130327182345.GD28148@google.com","threadId":"33306","inReplyTo":"20130327122216.5de0c336@hoelz.ro","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-03-27T18:23:45Z","receivedAt":"2013-03-27T18:23:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Rob Hoelz wrote:\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\nInteresting.\n\nSuppose I configure\n\n\t[url \"git://anongit.myserver.example.com/\"]\n\t\tinsteadOf = myserver.example.com:\n\t[url \"myserver:\"]\n\t\tpushInsteadOf = myserver.example.com:\n\nThe above code would make the insteadOf rule apply instead of\npushInsteadOf, even when pushing.  Perhaps something like the\nfollowing would work?\n\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/* No url.*.pushinsteadof configuration matched. */\n\t\t\t\tremotes[i]->pushurl[j] = alias_url(url, &rewrites);\n\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -244,6 +244,83 @@ test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf\n>  \t)\n>  '\n>  \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> +\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 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\nLooks good.  The usual style in git tests is to include no space\nafter >redirection operators:\n\n\t\tgit for-each-ref refs/remotes/origin >actual &&\n\nHope that helps,\nJonathan\n"},{"id":"212459","messageId":"20130327211554.GH28148@google.com","threadId":"33306","inReplyTo":"20130327182345.GD28148@google.com","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-03-27T21:15:54Z","receivedAt":"2013-03-27T21:15:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> Rob Hoelz wrote:\n\n>> --- a/remote.c\n>> +++ b/remote.c\n>> @@ -465,7 +465,11 @@ static void alias_all_urls(void)\n[...]\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>\n> Interesting.\n>\n> Suppose I configure\n> \n> \t[url \"git://anongit.myserver.example.com/\"]\n> \t\tinsteadOf = myserver.example.com:\n> \t[url \"myserver:\"]\n> \t\tpushInsteadOf = myserver.example.com:\n>\n> The above code would make the insteadOf rule apply instead of\n> pushInsteadOf, even when pushing.\n\nSorry, typo.  The configuration in the example above should have been\n\n\t[url \"git://anongit.myserver.example.com/\"]\n\t\tinsteadOf = myserver.example.com:\n\t[url \"myserver.example.com:\"]\n\t\tpushInsteadOf = myserver.example.com:\n\nIn other words, suppose I set url.*.insteadof to point to a faster\naddress for fetching alongside url.*.pushinsteadof requesting that the\noriginal address should still be used for pushing.\n\nThanks again for tackling this.\nJonathan\n"},{"id":"212462","messageId":"7vsj3gjy3t.fsf@alter.siamese.dyndns.org","threadId":"33306","inReplyTo":"20130327211554.GH28148@google.com","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-27T22:07:18Z","receivedAt":"2013-03-27T22:07:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Sorry, typo.  The configuration in the example above should have been\n>\n> \t[url \"git://anongit.myserver.example.com/\"]\n> \t\tinsteadOf = myserver.example.com:\n> \t[url \"myserver.example.com:\"]\n> \t\tpushInsteadOf = myserver.example.com:\n>\n> In other words, suppose I set url.*.insteadof to point to a faster\n> address for fetching alongside url.*.pushinsteadof requesting that the\n> original address should still be used for pushing.\n\nI didn't know we were even shooting for supporting the identity\nmapping:\n\n\turl.X.pushinsteadof=X\n\nbut that would certainly be nice.\n\nBy the way, it seems that the original commit 1c2eafb89bca (Add\nurl.<base>.pushInsteadOf: URL rewriting for push only, 2009-09-07)\nwanted to explicitly avoid use of pushInsteadOf aliasing for a\npushURL and it is also documented in config.txt from day one.\n\nI think the intent is \"You have a normal URL, and a way to override\nit explicitly with pushURL, or a way to rewrite it via the aliasing\nthe normal URL with pushInsteadOf. Either one or the other, but not\nboth, as having many levels of indirection would be confusing.\"\n\nWhich I can understand and sympathise.\n\nIn anay case, the change proposed in this thread seems to change\nthat, so the documentation would need to be updated.  Also the tests\nthe original commit adds explicitly checks that pushInsteadOf is\nignored, which may have to be updated (unless that test is already\nbroken).\n"},{"id":"212468","messageId":"20130327172909.532a30b6@hoelz.ro","threadId":"33306","inReplyTo":"20130327182345.GD28148@google.com","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Rob Hoelz","fromEmail":"rob@hoelz.ro","sentAt":"2013-03-27T22:29:09Z","receivedAt":"2013-03-27T22:29:09Z","isPatch":true,"sender":{"key":"rob@hoelz.ro","avatar":"https://gravatar.com/avatar/c62607c6cef695f7a6cbd0f1f3ebd359e8c471af271581a6d7e29e3fab6fb788?d=mp&s=160"},"body":"On Wed, 27 Mar 2013 11:23:45 -0700\nJonathan Nieder <jrnieder@gmail.com> wrote:\n\n> Rob Hoelz wrote:\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] =\n> > alias_url(remotes[i]->pushurl[j], &rewrites);\n> > +\t\t\tchar *copy =\n> > xstrdup(remotes[i]->pushurl[j]);\n> > +\t\t\tremotes[i]->pushurl[j] =\n> > 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] =\n> > alias_url(remotes[i]->pushurl[j], &rewrites);\n> > +\t\t\tfree(copy);\n> \n> Interesting.\n> \n> Suppose I configure\n> \n> \t[url \"git://anongit.myserver.example.com/\"]\n> \t\tinsteadOf = myserver.example.com:\n> \t[url \"myserver:\"]\n> \t\tpushInsteadOf = myserver.example.com:\n> \n> The above code would make the insteadOf rule apply instead of\n> pushInsteadOf, even when pushing.  Perhaps something like the\n> following would work?\n\nAre you sure?  I create a copy, and compare the copy to the new URL.\nIf they're the same (pushInsteadOf not found), I then use the insteadOf\nmapping to perform the alteration.  Did I introduce a bug?\n"},{"id":"212471","messageId":"20130327224735.GJ28148@google.com","threadId":"33306","inReplyTo":"20130327172909.532a30b6@hoelz.ro","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-03-27T22:47:35Z","receivedAt":"2013-03-27T22:47:35Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRob Hoelz wrote:\n> On Wed, 27 Mar 2013 11:23:45 -0700\n> Jonathan Nieder <jrnieder@gmail.com> wrote:\n\n>> Suppose I configure\n>>\n>> \t[url \"git://anongit.myserver.example.com/\"]\n>> \t\tinsteadOf = myserver.example.com:\n>> \t[url \"myserver:\"]\n>> \t\tpushInsteadOf = myserver.example.com:\n>>\n>> The above code would make the insteadOf rule apply instead of\n>> pushInsteadOf, even when pushing.  Perhaps something like the\n>> following would work?\n>\n> Are you sure?\n\nThe message you are replying to is nonsense, due to a typo while\nediting.  Did you see my followup?\n\nSorry for the confusion,\nJonathan\n"},{"id":"212472","messageId":"20130327174845.5e3081d1@hoelz.ro","threadId":"33306","inReplyTo":"7vsj3gjy3t.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Rob Hoelz","fromEmail":"rob@hoelz.ro","sentAt":"2013-03-27T22:48:45Z","receivedAt":"2013-03-27T22:48:45Z","isPatch":true,"sender":{"key":"rob@hoelz.ro","avatar":"https://gravatar.com/avatar/c62607c6cef695f7a6cbd0f1f3ebd359e8c471af271581a6d7e29e3fab6fb788?d=mp&s=160"},"body":"On Wed, 27 Mar 2013 15:07:18 -0700\nJunio C Hamano <gitster@pobox.com> wrote:\n\n> Jonathan Nieder <jrnieder@gmail.com> writes:\n> \n> > Sorry, typo.  The configuration in the example above should have\n> > been\n> >\n> > \t[url \"git://anongit.myserver.example.com/\"]\n> > \t\tinsteadOf = myserver.example.com:\n> > \t[url \"myserver.example.com:\"]\n> > \t\tpushInsteadOf = myserver.example.com:\n> >\n> > In other words, suppose I set url.*.insteadof to point to a faster\n> > address for fetching alongside url.*.pushinsteadof requesting that\n> > the original address should still be used for pushing.\n> \n> I didn't know we were even shooting for supporting the identity\n> mapping:\n> \n> \turl.X.pushinsteadof=X\n> \n> but that would certainly be nice.\n> \n> By the way, it seems that the original commit 1c2eafb89bca (Add\n> url.<base>.pushInsteadOf: URL rewriting for push only, 2009-09-07)\n> wanted to explicitly avoid use of pushInsteadOf aliasing for a\n> pushURL and it is also documented in config.txt from day one.\n> \n> I think the intent is \"You have a normal URL, and a way to override\n> it explicitly with pushURL, or a way to rewrite it via the aliasing\n> the normal URL with pushInsteadOf. Either one or the other, but not\n> both, as having many levels of indirection would be confusing.\"\n> \n> Which I can understand and sympathise.\n> \n> In anay case, the change proposed in this thread seems to change\n> that, so the documentation would need to be updated.  Also the tests\n> the original commit adds explicitly checks that pushInsteadOf is\n> ignored, which may have to be updated (unless that test is already\n> broken).\n> \n\nMy use case is that I use Github for my personal development.  I have a\nprefix for my personal repos (hoelzro: -> git://github.com/hoelzro for\nfetch, git@github.com:hoelzro/ for push) and one for all other Git repos\n(github: -> git://github.com/)  I have a few projects where I work in a\nfork, but I want to fetch updates from the original project.  So my url\nfor the origin remote is github:org/project, but my pushurl is\nhoelzro:project.  This behavior in Git currently doesn't allow me to\nwork that way.  I used to work with two remotes; origin for my repo and\nbase for the official one, but I've found that I prefer this other way.\n\nThe test that checked that pushInsteadOf + pushurl shouldn't work as I\nexpect was actually broken; I have removed it, updated the\ndocumentation, and sent a new patch to the list.\n\n-Rob\n"},{"id":"212474","messageId":"20130327175358.53903fae@hoelz.ro","threadId":"33306","inReplyTo":"20130327224735.GJ28148@google.com","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Rob Hoelz","fromEmail":"rob@hoelz.ro","sentAt":"2013-03-27T22:53:58Z","receivedAt":"2013-03-27T22:53:58Z","isPatch":true,"sender":{"key":"rob@hoelz.ro","avatar":"https://gravatar.com/avatar/c62607c6cef695f7a6cbd0f1f3ebd359e8c471af271581a6d7e29e3fab6fb788?d=mp&s=160"},"body":"On Wed, 27 Mar 2013 15:47:35 -0700\nJonathan Nieder <jrnieder@gmail.com> wrote:\n\n> Hi,\n> \n> Rob Hoelz wrote:\n> > On Wed, 27 Mar 2013 11:23:45 -0700\n> > Jonathan Nieder <jrnieder@gmail.com> wrote:\n> \n> >> Suppose I configure\n> >>\n> >> \t[url \"git://anongit.myserver.example.com/\"]\n> >> \t\tinsteadOf = myserver.example.com:\n> >> \t[url \"myserver:\"]\n> >> \t\tpushInsteadOf = myserver.example.com:\n> >>\n> >> The above code would make the insteadOf rule apply instead of\n> >> pushInsteadOf, even when pushing.  Perhaps something like the\n> >> following would work?\n> >\n> > Are you sure?\n> \n> The message you are replying to is nonsense, due to a typo while\n> editing.  Did you see my followup?\n> \n> Sorry for the confusion,\n> Jonathan\n> \n\nMy mistake; I had not seen it!  I thought you may have found a bug in\nmy implementation, so I wanted to double check. =)\n\n-Rob\n"},{"id":"212475","messageId":"20130327225656.GK28148@google.com","threadId":"33306","inReplyTo":"20130327175358.53903fae@hoelz.ro","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-03-27T22:56:56Z","receivedAt":"2013-03-27T22:56:56Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Rob Hoelz wrote:\n\n> My mistake; I had not seen it!  I thought you may have found a bug in\n> my implementation, so I wanted to double check. =)\n\nWell, I had found an unfortunate consequence of the implementation\nthat uses an unnecessary copy. :)\n\nWill follow up to the updated patch.\n"},{"id":"212478","messageId":"20130327180631.6a1a7a47@hoelz.ro","threadId":"33306","inReplyTo":"20130327225656.GK28148@google.com","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Rob Hoelz","fromEmail":"rob@hoelz.ro","sentAt":"2013-03-27T23:06:31Z","receivedAt":"2013-03-27T23:06:31Z","isPatch":true,"sender":{"key":"rob@hoelz.ro","avatar":"https://gravatar.com/avatar/c62607c6cef695f7a6cbd0f1f3ebd359e8c471af271581a6d7e29e3fab6fb788?d=mp&s=160"},"body":"On Wed, 27 Mar 2013 15:56:56 -0700\nJonathan Nieder <jrnieder@gmail.com> wrote:\n\n> Rob Hoelz wrote:\n> \n> > My mistake; I had not seen it!  I thought you may have found a bug\n> > in my implementation, so I wanted to double check. =)\n> \n> Well, I had found an unfortunate consequence of the implementation\n> that uses an unnecessary copy. :)\n> \n> Will follow up to the updated patch.\n> \n\nI actually wanted to talk about the copy thing.  I realize that this\ncould have been avoided by simply saving a pointer to the old string\nand performing a comparison, but I figured if the implementation for\nalias_url were changed in the future to use realloc or something, it\ncould potentially return the original char * with its contents\naltered.  So, by copying the string, we can avoid strange bugs in the\nfuture.\n\n-Rob\n"},{"id":"212481","messageId":"20130327230943.GA5204@jtriplet-mobl1","threadId":"33306","inReplyTo":"20130327174845.5e3081d1@hoelz.ro","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Josh Triplett","fromEmail":"josh@joshtriplett.org","sentAt":"2013-03-27T23:09:43Z","receivedAt":"2013-03-27T23:09:43Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"On Wed, Mar 27, 2013 at 05:48:45PM -0500, Rob Hoelz wrote:\n> On Wed, 27 Mar 2013 15:07:18 -0700\n> Junio C Hamano <gitster@pobox.com> wrote:\n> \n> > Jonathan Nieder <jrnieder@gmail.com> writes:\n> > \n> > > Sorry, typo.  The configuration in the example above should have\n> > > been\n> > >\n> > > \t[url \"git://anongit.myserver.example.com/\"]\n> > > \t\tinsteadOf = myserver.example.com:\n> > > \t[url \"myserver.example.com:\"]\n> > > \t\tpushInsteadOf = myserver.example.com:\n> > >\n> > > In other words, suppose I set url.*.insteadof to point to a faster\n> > > address for fetching alongside url.*.pushinsteadof requesting that\n> > > the original address should still be used for pushing.\n> > \n> > I didn't know we were even shooting for supporting the identity\n> > mapping:\n> > \n> > \turl.X.pushinsteadof=X\n> > \n> > but that would certainly be nice.\n> > \n> > By the way, it seems that the original commit 1c2eafb89bca (Add\n> > url.<base>.pushInsteadOf: URL rewriting for push only, 2009-09-07)\n> > wanted to explicitly avoid use of pushInsteadOf aliasing for a\n> > pushURL and it is also documented in config.txt from day one.\n> > \n> > I think the intent is \"You have a normal URL, and a way to override\n> > it explicitly with pushURL, or a way to rewrite it via the aliasing\n> > the normal URL with pushInsteadOf. Either one or the other, but not\n> > both, as having many levels of indirection would be confusing.\"\n> > \n> > Which I can understand and sympathise.\n> > \n> > In anay case, the change proposed in this thread seems to change\n> > that, so the documentation would need to be updated.  Also the tests\n> > the original commit adds explicitly checks that pushInsteadOf is\n> > ignored, which may have to be updated (unless that test is already\n> > broken).\n> > \n> \n> My use case is that I use Github for my personal development.  I have a\n> prefix for my personal repos (hoelzro: -> git://github.com/hoelzro for\n> fetch, git@github.com:hoelzro/ for push) and one for all other Git repos\n> (github: -> git://github.com/)  I have a few projects where I work in a\n> fork, but I want to fetch updates from the original project.  So my url\n> for the origin remote is github:org/project, but my pushurl is\n> hoelzro:project.  This behavior in Git currently doesn't allow me to\n> work that way.  I used to work with two remotes; origin for my repo and\n> base for the official one, but I've found that I prefer this other way.\n> \n> The test that checked that pushInsteadOf + pushurl shouldn't work as I\n> expect was actually broken; I have removed it, updated the\n> documentation, and sent a new patch to the list.\n\nThere's an argument for either behavior as valid.  My original patch\nspecifically documented and tested for the opposite behavior, namely\nthat pushurl overrides any pushInsteadOf, because I intended\npushInsteadOf as a fallback if you don't have an explicit pushurl set.\nFor instance, you could use pushInsteadOf to rewrite a family of\nanonymous git URLs to corresponding pushable repositories, but then use\nan explicit pushurl to override that for a specific repository.  This\nchange would break the ability to use pushurl for its original intended\npurpose, namely having a local repository where fetch comes from one\nremote repo and push goes to another.\n\nOne use case of mine: I have a .gitconfig in my git-managed home\ndirectory which sets pushInsteadOf so that I can clone via git:// and\nimmediately have working push.  I work with a number of systems that\ndon't have inbound access to each other but do have outbound access to\nthe network; on some of these \"satellite\" boxes, I can't push changes\ndirectly to the server pushInsteadOf points to, so I can explicitly set\npushurl in .git/config for that repository, which overrides the\npushInsteadOf.  This change would break that configuration.\n\n- Josh Triplett\n"},{"id":"212482","messageId":"20130327231720.GB5204@jtriplet-mobl1","threadId":"33306","inReplyTo":"20130327230943.GA5204@jtriplet-mobl1","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Josh Triplett","fromEmail":"josh@joshtriplett.org","sentAt":"2013-03-27T23:17:21Z","receivedAt":"2013-03-27T23:17:21Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"On Wed, Mar 27, 2013 at 04:09:43PM -0700, Josh Triplett wrote:\n> On Wed, Mar 27, 2013 at 05:48:45PM -0500, Rob Hoelz wrote:\n> > On Wed, 27 Mar 2013 15:07:18 -0700\n> > Junio C Hamano <gitster@pobox.com> wrote:\n> > \n> > > Jonathan Nieder <jrnieder@gmail.com> writes:\n> > > \n> > > > Sorry, typo.  The configuration in the example above should have\n> > > > been\n> > > >\n> > > > \t[url \"git://anongit.myserver.example.com/\"]\n> > > > \t\tinsteadOf = myserver.example.com:\n> > > > \t[url \"myserver.example.com:\"]\n> > > > \t\tpushInsteadOf = myserver.example.com:\n> > > >\n> > > > In other words, suppose I set url.*.insteadof to point to a faster\n> > > > address for fetching alongside url.*.pushinsteadof requesting that\n> > > > the original address should still be used for pushing.\n> > > \n> > > I didn't know we were even shooting for supporting the identity\n> > > mapping:\n> > > \n> > > \turl.X.pushinsteadof=X\n> > > \n> > > but that would certainly be nice.\n> > > \n> > > By the way, it seems that the original commit 1c2eafb89bca (Add\n> > > url.<base>.pushInsteadOf: URL rewriting for push only, 2009-09-07)\n> > > wanted to explicitly avoid use of pushInsteadOf aliasing for a\n> > > pushURL and it is also documented in config.txt from day one.\n> > > \n> > > I think the intent is \"You have a normal URL, and a way to override\n> > > it explicitly with pushURL, or a way to rewrite it via the aliasing\n> > > the normal URL with pushInsteadOf. Either one or the other, but not\n> > > both, as having many levels of indirection would be confusing.\"\n> > > \n> > > Which I can understand and sympathise.\n> > > \n> > > In anay case, the change proposed in this thread seems to change\n> > > that, so the documentation would need to be updated.  Also the tests\n> > > the original commit adds explicitly checks that pushInsteadOf is\n> > > ignored, which may have to be updated (unless that test is already\n> > > broken).\n> > > \n> > \n> > My use case is that I use Github for my personal development.  I have a\n> > prefix for my personal repos (hoelzro: -> git://github.com/hoelzro for\n> > fetch, git@github.com:hoelzro/ for push) and one for all other Git repos\n> > (github: -> git://github.com/)  I have a few projects where I work in a\n> > fork, but I want to fetch updates from the original project.  So my url\n> > for the origin remote is github:org/project, but my pushurl is\n> > hoelzro:project.  This behavior in Git currently doesn't allow me to\n> > work that way.  I used to work with two remotes; origin for my repo and\n> > base for the official one, but I've found that I prefer this other way.\n> > \n> > The test that checked that pushInsteadOf + pushurl shouldn't work as I\n> > expect was actually broken; I have removed it, updated the\n> > documentation, and sent a new patch to the list.\n> \n> There's an argument for either behavior as valid.  My original patch\n> specifically documented and tested for the opposite behavior, namely\n> that pushurl overrides any pushInsteadOf, because I intended\n> pushInsteadOf as a fallback if you don't have an explicit pushurl set.\n> For instance, you could use pushInsteadOf to rewrite a family of\n> anonymous git URLs to corresponding pushable repositories, but then use\n> an explicit pushurl to override that for a specific repository.  This\n> change would break the ability to use pushurl for its original intended\n> purpose, namely having a local repository where fetch comes from one\n> remote repo and push goes to another.\n> \n> One use case of mine: I have a .gitconfig in my git-managed home\n> directory which sets pushInsteadOf so that I can clone via git:// and\n> immediately have working push.  I work with a number of systems that\n> don't have inbound access to each other but do have outbound access to\n> the network; on some of these \"satellite\" boxes, I can't push changes\n> directly to the server pushInsteadOf points to, so I can explicitly set\n> pushurl in .git/config for that repository, which overrides the\n> pushInsteadOf.  This change would break that configuration.\n\nClarifying this use case a bit: note that it's been a while since I had\nmany such boxes, so I don't actually have any systems currently using\nthat pushurl configuration.  Still a regression in defined behavior,\nthough.\n\nWhy not just use insteadOf for your personal github prefix hoelzro:, and\nboth insteadOf and pushInsteadOf for github: in general?  Then, a\nrepository cloned via github: would work for pull and push (if you have\npush access), and you can change pushurl to your personal github alias\nif needed.\n\nThough, as Junio said, the modern push-updates-remote-heads behavior of\ngit means that both of our configurations arguably seem wrong, and we\nshould both just use separate remotes for separate repos.\n\n- Josh Triplett\n"},{"id":"212483","messageId":"20130327231819.GL28148@google.com","threadId":"33306","inReplyTo":"20130327230943.GA5204@jtriplet-mobl1","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-03-27T23:18:19Z","receivedAt":"2013-03-27T23:18:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Josh Triplett wrote:\n\n>                       I have a .gitconfig in my git-managed home\n> directory which sets pushInsteadOf so that I can clone via git:// and\n> immediately have working push.  I work with a number of systems that\n> don't have inbound access to each other but do have outbound access to\n> the network; on some of these \"satellite\" boxes, I can't push changes\n> directly to the server pushInsteadOf points to, so I can explicitly set\n> pushurl in .git/config for that repository, which overrides the\n> pushInsteadOf.  This change would break that configuration.\n\nWould it?  As long as your pushurl does not start with git://, I think\nyour configuration would still work fine.\n\nAfter this patch, neither pushInsteadOf nor pushUrl overrides the\nother one.  The rule is:\n\n\t1. First, get the URL from the remote's configuration, based\n\t   on whether you are fetching or pushing.\n\n\t   (At this step, in your setup git chooses the URL specified\n\t   with pushurl in your .git/config.)\n\t\n\t2. Next, apply the most appropriate url.*.insteadOf or\n\t   url.*.pushInsteadOf rule, based on whether you are fetching\n\t   or pushing.\n\n\t   (At this step, no rewrite rules apply, so the URL is used\n\t   as is.)\n"},{"id":"212531","messageId":"7vfvzfwn55.fsf@alter.siamese.dyndns.org","threadId":"33306","inReplyTo":"20130327230943.GA5204@jtriplet-mobl1","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-28T15:37:58Z","receivedAt":"2013-03-28T15:37:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Triplett <josh@joshtriplett.org> writes:\n\n> On Wed, Mar 27, 2013 at 05:48:45PM -0500, Rob Hoelz wrote:\n> ...\n>> The test that checked that pushInsteadOf + pushurl shouldn't work as I\n>> expect was actually broken; I have removed it, updated the\n>> documentation, and sent a new patch to the list.\n>\n> There's an argument for either behavior as valid.  My original patch\n> specifically documented and tested for the opposite behavior, namely\n> that pushurl overrides any pushInsteadOf, because I intended\n> pushInsteadOf as a fallback if you don't have an explicit pushurl set.\n\nFor only this bit.\n\nI think the test in question is this one from t5516:\n\ntest_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '\n\tmk_empty &&\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\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\n\t\ttest 1 = $(git for-each-ref refs/remotes/origin | wc -l)\n\t)\n'\n\nIt defines a remote \"r\", with URL \"trash/wrong\" (used for fetch) and\npushURL \"$(pwd)/testrepo\" (used for push).  There is a pushInsteadOf\nrule to rewrite anything that goes to \"trash/*\" to be pushed to\n\"trash2/*\" instead but that shouldn't be used to rewrite an explicit\npushURL.\n\nAnd then the test pushes to \"r\" and checks if testrepo gets updated;\nin other words, it wants to make sure remote.r.pushURL defines the\nfinal destination, without pushInsteadOf getting in the way.\n\nBut $(pwd)/testrepo does not match trash/* in the first place, so\nthere is no chance for a pushInsteadOf to interfere; it looks to me\nthat it is not testing what it wants to test.\n\nPerhaps we should do something like this?  We tell it to push to\n\"testrepo/\" with pushURL, and set up a pushInsteadOf to rewrite\n\"testrepo/\" to \"trash2/\", but because for this push it comes from an\nexplicit pushURL, it still goes to \"testrepo/\".\n\nAs we do not have \"trash2/\" repository, the test not just tests the\npush goes to \"testrepo/\", but it also tests that it does not attempt\nto push to \"trash2/\", checking both sides of the coin.\n\n t/t5516-fetch-push.sh | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex d3dc5df..b5ea32c 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -230,10 +230,9 @@ test_expect_success 'push with pushInsteadOf' '\n \n test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '\n \tmk_empty &&\n-\tTRASH=\"$(pwd)/\" &&\n-\tgit config \"url.trash2/.pushInsteadOf\" trash/ &&\n+\tgit config \"url.trash2/.pushInsteadOf\" testrepo/ &&\n \tgit config remote.r.url trash/wrong &&\n-\tgit config remote.r.pushurl \"$TRASH/testrepo\" &&\n+\tgit config remote.r.pushurl \"testrepo/\" &&\n \tgit push r refs/heads/master:refs/remotes/origin/master &&\n \t(\n \t\tcd testrepo &&\n"},{"id":"212534","messageId":"7vboa3wmgz.fsf@alter.siamese.dyndns.org","threadId":"33306","inReplyTo":"20130327231819.GL28148@google.com","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-28T15:52:28Z","receivedAt":"2013-03-28T15:52:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Josh Triplett wrote:\n>\n>>                       I have a .gitconfig in my git-managed home\n>> directory which sets pushInsteadOf so that I can clone via git:// and\n>> immediately have working push.  I work with a number of systems that\n>> don't have inbound access to each other but do have outbound access to\n>> the network; on some of these \"satellite\" boxes, I can't push changes\n>> directly to the server pushInsteadOf points to, so I can explicitly set\n>> pushurl in .git/config for that repository, which overrides the\n>> pushInsteadOf.  This change would break that configuration.\n>\n> Would it?  As long as your pushurl does not start with git://, I think\n> your configuration would still work fine.\n\nThat is a good point, especially because it is very unlikely that\ngit:// was used for pushURL, given that it would not have been\nrewritten with pushInsteadOf to an authenticated transport.\n\n> After this patch, neither pushInsteadOf nor pushUrl overrides the\n> other one.  The rule is:\n>\n> \t1. First, get the URL from the remote's configuration, based\n> \t   on whether you are fetching or pushing.\n>\n> \t   (At this step, in your setup git chooses the URL specified\n> \t   with pushurl in your .git/config.)\n> \t\n> \t2. Next, apply the most appropriate url.*.insteadOf or\n> \t   url.*.pushInsteadOf rule, based on whether you are fetching\n> \t   or pushing.\n>\n> \t   (At this step, no rewrite rules apply, so the URL is used\n> \t   as is.)\n"},{"id":"212537","messageId":"20130328160104.GC16034@leaf","threadId":"33306","inReplyTo":"20130327231819.GL28148@google.com","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Josh Triplett","fromEmail":"josh@joshtriplett.org","sentAt":"2013-03-28T16:01:04Z","receivedAt":"2013-03-28T16:01:04Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"On Wed, Mar 27, 2013 at 04:18:19PM -0700, Jonathan Nieder wrote:\n> Josh Triplett wrote:\n> \n> >                       I have a .gitconfig in my git-managed home\n> > directory which sets pushInsteadOf so that I can clone via git:// and\n> > immediately have working push.  I work with a number of systems that\n> > don't have inbound access to each other but do have outbound access to\n> > the network; on some of these \"satellite\" boxes, I can't push changes\n> > directly to the server pushInsteadOf points to, so I can explicitly set\n> > pushurl in .git/config for that repository, which overrides the\n> > pushInsteadOf.  This change would break that configuration.\n> \n> Would it?  As long as your pushurl does not start with git://, I think\n> your configuration would still work fine.\n\nI had to think about it for a while, but I think you're right; I\ninferred a behavior that the patch didn't actually add or have anything\nto do with, namely having the result of applying pushInsteadOf to the\nnon-push URL override the pushUrl.\n\nOK, I take it back.  I *can* imagine configurations that this change\nwould break, since it does change intentional and documented behavior,\nbut I don't have any such configuration.  The only such configuration I\ncan imagine involves directly counting on the non-rewriting of pushUrl,\nby using pushInsteadOf to rewrite urls and then sometimes using pushUrl\nto override that and point back at the un-rewritten URL.  And while\nsupported, that does seem *odd*.\n\nObjection withdrawn; if nobody can come up with a sensible configuration\nthat relies on the documented behavior, I don't particularly care if it\nchanges.\n\n> After this patch, neither pushInsteadOf nor pushUrl overrides the\n> other one.  The rule is:\n> \n> \t1. First, get the URL from the remote's configuration, based\n> \t   on whether you are fetching or pushing.\n> \n> \t   (At this step, in your setup git chooses the URL specified\n> \t   with pushurl in your .git/config.)\n> \t\n> \t2. Next, apply the most appropriate url.*.insteadOf or\n> \t   url.*.pushInsteadOf rule, based on whether you are fetching\n> \t   or pushing.\n> \n> \t   (At this step, no rewrite rules apply, so the URL is used\n> \t   as is.)\n"},{"id":"212539","messageId":"20130328160937.GD16034@leaf","threadId":"33306","inReplyTo":"7vfvzfwn55.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Josh Triplett","fromEmail":"josh@joshtriplett.org","sentAt":"2013-03-28T16:09:37Z","receivedAt":"2013-03-28T16:09:37Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"On Thu, Mar 28, 2013 at 08:37:58AM -0700, Junio C Hamano wrote:\n> Josh Triplett <josh@joshtriplett.org> writes:\n> \n> > On Wed, Mar 27, 2013 at 05:48:45PM -0500, Rob Hoelz wrote:\n> > ...\n> >> The test that checked that pushInsteadOf + pushurl shouldn't work as I\n> >> expect was actually broken; I have removed it, updated the\n> >> documentation, and sent a new patch to the list.\n> >\n> > There's an argument for either behavior as valid.  My original patch\n> > specifically documented and tested for the opposite behavior, namely\n> > that pushurl overrides any pushInsteadOf, because I intended\n> > pushInsteadOf as a fallback if you don't have an explicit pushurl set.\n> \n> For only this bit.\n> \n> I think the test in question is this one from t5516:\n> \n> test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '\n> \tmk_empty &&\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> \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> \n> \t\ttest 1 = $(git for-each-ref refs/remotes/origin | wc -l)\n> \t)\n> '\n> \n> It defines a remote \"r\", with URL \"trash/wrong\" (used for fetch) and\n> pushURL \"$(pwd)/testrepo\" (used for push).  There is a pushInsteadOf\n> rule to rewrite anything that goes to \"trash/*\" to be pushed to\n> \"trash2/*\" instead but that shouldn't be used to rewrite an explicit\n> pushURL.\n> \n> And then the test pushes to \"r\" and checks if testrepo gets updated;\n> in other words, it wants to make sure remote.r.pushURL defines the\n> final destination, without pushInsteadOf getting in the way.\n> \n> But $(pwd)/testrepo does not match trash/* in the first place, so\n> there is no chance for a pushInsteadOf to interfere; it looks to me\n> that it is not testing what it wants to test.\n\nThat test does actually test something important: it tests that the\nresult of applying pushInsteadOf to url does *not* override pushurl.\nNot the same thing as testing that pushInsteadOf does or does not apply\nto pushurl.\n\nThe relevant sentence of the git-config manpage (in the documentation\nfor pushInsteadOf) says:\n> If a remote has an explicit pushurl, git will ignore this setting for\n> that remote.\nThat really meant what I just said above: git will prefer an explicit\npushurl over the pushInsteadOf rewrite of url.  It says nothing about\napplying pushInsteadOf to rewrite pushurl.\n\n> Perhaps we should do something like this?  We tell it to push to\n> \"testrepo/\" with pushURL, and set up a pushInsteadOf to rewrite\n> \"testrepo/\" to \"trash2/\", but because for this push it comes from an\n> explicit pushURL, it still goes to \"testrepo/\".\n> \n> As we do not have \"trash2/\" repository, the test not just tests the\n> push goes to \"testrepo/\", but it also tests that it does not attempt\n> to push to \"trash2/\", checking both sides of the coin.\n\nSensible test, assuming you want to enforce that behavior.  I don't\nstrongly care either way about that one, since it only applies if your\npushInsteadOf rewrites could apply to your pushurl, and I only ever use\npushInsteadOf to rewrite unpushable repos to pushable ones.  However...\n\n>  t/t5516-fetch-push.sh | 5 ++---\n>  1 file changed, 2 insertions(+), 3 deletions(-)\n> \n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index d3dc5df..b5ea32c 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -230,10 +230,9 @@ test_expect_success 'push with pushInsteadOf' '\n>  \n>  test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '\n>  \tmk_empty &&\n> -\tTRASH=\"$(pwd)/\" &&\n> -\tgit config \"url.trash2/.pushInsteadOf\" trash/ &&\n> +\tgit config \"url.trash2/.pushInsteadOf\" testrepo/ &&\n>  \tgit config remote.r.url trash/wrong &&\n> -\tgit config remote.r.pushurl \"$TRASH/testrepo\" &&\n> +\tgit config remote.r.pushurl \"testrepo/\" &&\n>  \tgit push r refs/heads/master:refs/remotes/origin/master &&\n>  \t(\n>  \t\tcd testrepo &&\n\n...the test you describe should appear in *addition* to this test, not\nreplacing it, because as described above this test does test one\ncritical bit of behavior, namely prefering pushurl over the\npushInsteadOf rewrite of url.\n\n- Josh Triplett\n"},{"id":"212540","messageId":"7vy5d7v71o.fsf@alter.siamese.dyndns.org","threadId":"33306","inReplyTo":"20130328160104.GC16034@leaf","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-28T16:10:59Z","receivedAt":"2013-03-28T16:10:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Triplett <josh@joshtriplett.org> writes:\n\n> OK, I take it back.  I *can* imagine configurations that this change\n> would break, since it does change intentional and documented behavior,\n> but I don't have any such configuration.  The only such configuration I\n> can imagine involves directly counting on the non-rewriting of pushUrl,\n> by using pushInsteadOf to rewrite urls and then sometimes using pushUrl\n> to override that and point back at the un-rewritten URL.  And while\n> supported, that does seem *odd*.\n>\n> Objection withdrawn; if nobody can come up with a sensible configuration\n> that relies on the documented behavior, I don't particularly care if it\n> changes.\n\nI actually do.\n\nGiven the popularity of the system, \"people involved in this thread\ncannot imagine a case that existing people may get hurt\" is very\ndifferent from \"this is not a regression\".  After merging this\nchange when people start complaining, you and Rob can hide and\nignore them, but we collectively as the Git project have to have a\nway to help them when it happens.\n"},{"id":"212546","messageId":"20130328164038.GE16034@leaf","threadId":"33306","inReplyTo":"7vy5d7v71o.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Josh Triplett","fromEmail":"josh@joshtriplett.org","sentAt":"2013-03-28T16:40:38Z","receivedAt":"2013-03-28T16:40:38Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"On Thu, Mar 28, 2013 at 09:10:59AM -0700, Junio C Hamano wrote:\n> Josh Triplett <josh@joshtriplett.org> writes:\n> \n> > OK, I take it back.  I *can* imagine configurations that this change\n> > would break, since it does change intentional and documented behavior,\n> > but I don't have any such configuration.  The only such configuration I\n> > can imagine involves directly counting on the non-rewriting of pushUrl,\n> > by using pushInsteadOf to rewrite urls and then sometimes using pushUrl\n> > to override that and point back at the un-rewritten URL.  And while\n> > supported, that does seem *odd*.\n> >\n> > Objection withdrawn; if nobody can come up with a sensible configuration\n> > that relies on the documented behavior, I don't particularly care if it\n> > changes.\n> \n> I actually do.\n> \n> Given the popularity of the system, \"people involved in this thread\n> cannot imagine a case that existing people may get hurt\" is very\n> different from \"this is not a regression\".  After merging this\n> change when people start complaining, you and Rob can hide and\n> ignore them, but we collectively as the Git project have to have a\n> way to help them when it happens.\n\nI entirely agree that it represents a regression from documented\nbehavior; I just mean that it no longer matches a specific use case I\nhad in mind with the original change.  I agree that we should hesitate\nto change that documented behavior.\n\n- Josh Triplett\n"},{"id":"212562","messageId":"7vr4iztl3z.fsf@alter.siamese.dyndns.org","threadId":"33306","inReplyTo":"20130328160937.GD16034@leaf","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-28T18:50:08Z","receivedAt":"2013-03-28T18:50:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Triplett <josh@joshtriplett.org> writes:\n\n(on url.$base.pushInsteadOf)\n>> If a remote has an explicit pushurl, git will ignore this setting for\n>> that remote.\n> That really meant what I just said above: git will prefer an explicit\n> pushurl over the pushInsteadOf rewrite of url.\n\nVery correct.\n\n> It says nothing about\n> applying pushInsteadOf to rewrite pushurl.\n\nIncorrect, I think.  If you have pushURL, pushInsteadOf is *ignored*.\nOf course, if you have both URL and pushURL, the ignored pushInsteadOf\nwill not apply to _anything_.  It will not apply to URL, and it will\ncertainly not apply to pushURL.\n\nYou are correct to point out that with the test we would want to\nmake sure that for a remote with pushURL and URL, a push goes\n\n - to pushURL;\n - not to URL;\n - not to insteadOf(URL);\n - not to pushInsteadOf(URL);\n - not to insteadOf(pushURL); and\n - not to pushInsteadOf(pushURL).\n\nI do not think it is worth checking all of them, but I agree we\nshould make sure it does not go to pushInsteadOf(URL) which you\noriginally meant to check, and we should also make sure it does not\ngo to pushInsteadOf(pushURL).\n\n>>  test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '\n>>  \tmk_empty &&\n>> -\tTRASH=\"$(pwd)/\" &&\n>> -\tgit config \"url.trash2/.pushInsteadOf\" trash/ &&\n>> +\tgit config \"url.trash2/.pushInsteadOf\" testrepo/ &&\n\nAdding\n\n\tgit config \"url.trash3/.pusnInsteadOf\" trash/wrong &&\n\nhere should be sufficient for that, no?  If we mistakenly used URL\n(i.e. trash/wrong) the push would fail.  If we mistakenly used\npushInsteadOf(URL), that is rewritten to trash3/ and again the push\nwould fail.  pushInsteadOf(pushURL) would go to trash2/ and that\nwould also fail.\n\nWe aren't checking insteadOf(URL) and insteadOf(pushURL) but\neverything else is checked, I think, so we can do without replacing\nanything.  We can just extend it, no?\n\n>>  \tgit config remote.r.url trash/wrong &&\n>> -\tgit config remote.r.pushurl \"$TRASH/testrepo\" &&\n>> +\tgit config remote.r.pushurl \"testrepo/\" &&\n>>  \tgit push r refs/heads/master:refs/remotes/origin/master &&\n>>  \t(\n>>  \t\tcd testrepo &&\n>\n> ...the test you describe should appear in *addition* to this test, not\n> replacing it, because as described above this test does test one\n> critical bit of behavior, namely prefering pushurl over the\n> pushInsteadOf rewrite of url.\n>\n> - Josh Triplett\n"},{"id":"212568","messageId":"20130328190344.GA5361@jtriplet-mobl1","threadId":"33306","inReplyTo":"7vr4iztl3z.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Josh Triplett","fromEmail":"josh@joshtriplett.org","sentAt":"2013-03-28T19:03:44Z","receivedAt":"2013-03-28T19:03:44Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"On Thu, Mar 28, 2013 at 11:50:08AM -0700, Junio C Hamano wrote:\n> Josh Triplett <josh@joshtriplett.org> writes:\n> (on url.$base.pushInsteadOf)\n> >> If a remote has an explicit pushurl, git will ignore this setting for\n> >> that remote.\n> > That really meant what I just said above: git will prefer an explicit\n> > pushurl over the pushInsteadOf rewrite of url.\n> \n> Very correct.\n> \n> > It says nothing about\n> > applying pushInsteadOf to rewrite pushurl.\n> \n> Incorrect, I think.  If you have pushURL, pushInsteadOf is *ignored*.\n> Of course, if you have both URL and pushURL, the ignored pushInsteadOf\n> will not apply to _anything_.  It will not apply to URL, and it will\n> certainly not apply to pushURL.\n\nDebatable whether the documentation sentence above really says that; it\ncertainly doesn't say it very clearly if so.  But that does match the\nactual behavior, making the proposed change a regression from the actual\nbehavior, whether the documentation clearly guarantees that behavior or\nnot.\n\n> You are correct to point out that with the test we would want to\n> make sure that for a remote with pushURL and URL, a push goes\n> \n>  - to pushURL;\n>  - not to URL;\n>  - not to insteadOf(URL);\n>  - not to pushInsteadOf(URL);\n>  - not to insteadOf(pushURL); and\n>  - not to pushInsteadOf(pushURL).\n> \n> I do not think it is worth checking all of them, but I agree we\n> should make sure it does not go to pushInsteadOf(URL) which you\n> originally meant to check, and we should also make sure it does not\n> go to pushInsteadOf(pushURL).\n\nAgreed.\n\nRelated to this, as a path forward, I do think it makes sense to have a\nsetting usable as an insteadOf that only applies to pushurl, even though\npushInsteadOf won't end up serving that purpose.  That way,\npushInsteadOf covers the \"map read-only repo url to pushable repo url\"\ncase, and insteadOfPushOnly covers the \"construct a magic prefix that\nmaps to different urls when used in url or pushurl\" case.\n\n> >>  test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '\n> >>  \tmk_empty &&\n> >> -\tTRASH=\"$(pwd)/\" &&\n> >> -\tgit config \"url.trash2/.pushInsteadOf\" trash/ &&\n> >> +\tgit config \"url.trash2/.pushInsteadOf\" testrepo/ &&\n> \n> Adding\n> \n> \tgit config \"url.trash3/.pusnInsteadOf\" trash/wrong &&\n> \n> here should be sufficient for that, no?  If we mistakenly used URL\n> (i.e. trash/wrong) the push would fail.  If we mistakenly used\n> pushInsteadOf(URL), that is rewritten to trash3/ and again the push\n> would fail.  pushInsteadOf(pushURL) would go to trash2/ and that\n> would also fail.\n> \n> We aren't checking insteadOf(URL) and insteadOf(pushURL) but\n> everything else is checked, I think, so we can do without replacing\n> anything.  We can just extend it, no?\n\nThat sounds sensible.\n\n- Josh Triplett\n"},{"id":"212570","messageId":"20130328192507.GS28148@google.com","threadId":"33306","inReplyTo":"20130328190344.GA5361@jtriplet-mobl1","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-03-28T19:25:07Z","receivedAt":"2013-03-28T19:25:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Josh Triplett wrote:\n\n> Related to this, as a path forward, I do think it makes sense to have a\n> setting usable as an insteadOf that only applies to pushurl, even though\n> pushInsteadOf won't end up serving that purpose.  That way,\n> pushInsteadOf covers the \"map read-only repo url to pushable repo url\"\n> case, and insteadOfPushOnly covers the \"construct a magic prefix that\n> maps to different urls when used in url or pushurl\" case.\n\nI hope not.  That would be making configuration even more complicated.\n\nI hope that we can fix the documentation, tests, and change\ndescription in the commit message enough to make Rob's patch a\nno-brainer.  If that's not possible, I think the current state is\nlivable, just confusing.\n\nI was happy to see Rob's patch because it brings git's behavior closer\nto following the principle of least surprise.  I am not actually that\nexcited by the use case, except the \"avoiding surprise\" part of it.\n\nHope that helps,\nJonathan\n"},{"id":"212601","messageId":"20130328235319.299cb494@hoelz.ro","threadId":"33306","inReplyTo":"20130328192507.GS28148@google.com","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Rob Hoelz","fromEmail":"rob@hoelz.ro","sentAt":"2013-03-29T04:53:19Z","receivedAt":"2013-03-29T04:53:19Z","isPatch":true,"sender":{"key":"rob@hoelz.ro","avatar":"https://gravatar.com/avatar/c62607c6cef695f7a6cbd0f1f3ebd359e8c471af271581a6d7e29e3fab6fb788?d=mp&s=160"},"body":"On Thu, 28 Mar 2013 12:25:07 -0700\nJonathan Nieder <jrnieder@gmail.com> wrote:\n\n> Josh Triplett wrote:\n> \n> > Related to this, as a path forward, I do think it makes sense to\n> > have a setting usable as an insteadOf that only applies to pushurl,\n> > even though pushInsteadOf won't end up serving that purpose.  That\n> > way, pushInsteadOf covers the \"map read-only repo url to pushable\n> > repo url\" case, and insteadOfPushOnly covers the \"construct a magic\n> > prefix that maps to different urls when used in url or pushurl\"\n> > case.\n> \n> I hope not.  That would be making configuration even more complicated.\n> \n> I hope that we can fix the documentation, tests, and change\n> description in the commit message enough to make Rob's patch a\n> no-brainer.  If that's not possible, I think the current state is\n> livable, just confusing.\n> \n> I was happy to see Rob's patch because it brings git's behavior closer\n> to following the principle of least surprise.  I am not actually that\n> excited by the use case, except the \"avoiding surprise\" part of it.\n> \n> Hope that helps,\n> Jonathan\n> \n\nThanks for all of the input, everyone.  I personally agree with\nJonathon's notion of \"principle of least surprise\", as I was quite\nsurprised when my configuration with pushInsteadOf + pushurl did not\nwork.  However, I also understand Junio's arguments about going back on\ndocumented behavior, as well as whether or not it's a good idea to have\nthis \"triangular\" remote set up.\n\nHonestly, if my workflow here is stupid and not \"Git-like\" and someone\nhas a better suggestion, I would happy to let my patch go.  Using two\nremotes is an option, but to me, using this triangular setup is just\neasier.\n\nThe only way I see this breaking an existing configuration is if a user\nhas something like url.user@server.com.pushInsteadOf = myserver:, and\npushurl = myserver:repo/.  If this behavior weren't documented, I would\nsay that such a configuration works because it relies on a bug, and\nshould use ssh://myserver:repo/ instead. I personally feel that the fact\nthat insteadOf + url works and pushInsteadOf + pushurl doesn't is\ninconsistent and confusing. However, I am one user of many, and this is\nmy first exposure to Git from a project contributor point of view.\nTherefore, I submit to the wisdom of more seasoned Git developers such\nas yourselves. =)\n\n-Rob\n"},{"id":"212603","messageId":"7v8v56rcxc.fsf@alter.siamese.dyndns.org","threadId":"33306","inReplyTo":"20130328235319.299cb494@hoelz.ro","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-29T05:29:51Z","receivedAt":"2013-03-29T05:29:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rob Hoelz <rob@hoelz.ro> writes:\n\n> Honestly, if my workflow here is stupid and not \"Git-like\" and someone\n> has a better suggestion, I would happy to let my patch go.  Using two\n> remotes is an option, but to me, using this triangular setup is just\n> easier.\n\nI think you are conflating two unrelated things.\n\nPulling from one repository to integrate others' work with yours\n(either by merging others' work to yours, or rebasing your work on\nothers'), and pushing the result of your work to another repository\nto publish, i.e. triangular workflow, is no less \"Git-like\" than\npulling from and pushing to the same repository.  Both are valid\nworkflows, and Git supports them.\n\nWhat is not correct in your set-up is that a single remote with URL\nand pushURL (or rewritten URL derived from them via pushInsteadOf\nand insteadOf) that point at two different repositories is *not* the\nway to express that triangular configuration.  You name two remotes,\npull from one and push to the other.\n\nIf you look at Ram's \"triangular workflows\" series, cf.\n\n    http://thread.gmane.org/gmane.comp.version-control.git/219387\n\nyou can see that a progress is being made to make the \"two remotes\"\nconfiguration easier to use.\n\nThe discussion on the earliest iteration of the patch series, cf.\n\n    http://thread.gmane.org/gmane.comp.version-control.git/215702\n\nshows that even I initially made the same \"pointing two different\nrepositories with URL and pushURL should be sufficient\" mistake,\nwhich was corrected by others.  The primary issue is \"remote\ntracking branches are designed to keep track of the state of the\nbranches at the named remote\"---for this reason alone, you must not\nname a logically different repository with URL and pushURL for a\nsingle remote.\n\nSo that is one thing.  tl;dr: Triangular workflow is valid.  A\nsingle remote with URL and pushURL to point at the two remote\nrepositories is not a valid way to express that workflow.\n\nThe other thing is if it is worth risking to break the backward\ncompatibility and hurting existing users in order to remove the\nstrange \"To an explicit pushURL, insteadOf rewriting will not apply\"\nexception.\n\nThe reason I didn't bring up the possible breakage of \"documented\nbehaviour\" in the earlier review of this series is exactly because\nthat special case was unintuitive, so you do not have to argue it is\nstrange, unintuitive, and would not be a way we would have designed\nthe system if we knew better. I already agree to that point, and I\nthink others do, too.  There is a gap between \"We would design it\ndifferently if we were building it now with what we know\" and \"We\nshould change it and make it ideal\" and the gap is called existing\nusers.\n\nThese two are unrelated and independent.\n\nI suspect that Ram's \"triangular\" series, when it matures, will help\nyour \"pull from there, push to another\" workflow in a different way.\nYou will just define two remotes for these two places, and you may\nno longer need \"pushInsteadOf is not ignored when you have pushURL\"\nto solve your immediate issue.\n\nBut removing the \"pushInsteadOf is ignored when explicit pushURL\nexists\" may still be a worthwhile thing to do, even if you do not\nneed it.\n"}]}