{"thread":{"id":"33210","subject":"[PATCH] push: Alias pushurl from push rewrites","startedAt":"2013-03-17T22:50:40Z","lastAt":"2013-03-18T20:59:53Z","messageCount":4,"participants":["Rob Hoelz","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"211538","messageId":"20130317235040.4cdc9ef2@hoelz.ro","threadId":"33210","inReplyTo":null,"subject":"[PATCH] push: Alias pushurl from push rewrites","fromName":"Rob Hoelz","fromEmail":"rob@hoelz.ro","sentAt":"2013-03-17T22:50:40Z","receivedAt":"2013-03-17T22:50:40Z","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 tests and fixes 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\nSigned-off-by: Rob Hoelz <rob@hoelz.ro>\n---\n remote.c              |  2 +-\n t/t5516-fetch-push.sh | 20 ++++++++++++++++++++\n 2 files changed, 21 insertions(+), 1 deletion(-)\n\ndiff --git a/remote.c b/remote.c\nindex ca1f8f2..de7a915 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -465,7 +465,7 @@ 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\tremotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites_push);\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 b5417cc..272e225 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -244,6 +244,26 @@ test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf\n \t)\n '\n \n+test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf does rewrite in this case)' '\n+\tmk_empty &&\n+\tTRASH=\"$(pwd)/\" &&\n+\tmkdir ro &&\n+\tmkdir rw &&\n+\tgit init --bare rw/testrepo &&\n+\tgit config \"url.file://$TRASH/ro/.insteadOf\" ro: &&\n+\tgit config \"url.file://$TRASH/rw/.pushInsteadOf\" rw: &&\n+\tgit config remote.r.url ro:wrong &&\n+\tgit 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\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 test_expect_success 'push with matching heads' '\n \n \tmk_test heads/master &&\n-- \n1.8.2\n"},{"id":"211539","messageId":"7vd2uxppk0.fsf@alter.siamese.dyndns.org","threadId":"33210","inReplyTo":"20130317235040.4cdc9ef2@hoelz.ro","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-17T23:35:59Z","receivedAt":"2013-03-17T23:35:59Z","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> git push currently doesn't consider pushInsteadOf when\n> using pushurl; this tests and fixes that.\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\nIncomplete sentence?  For example [this is an example configuration]\nand then what happens?  Something like \"with the sample\nconfiguration, 'git push origin' should follow pushurl and then turn\nit into X, but instead it ends up accessing Y\".\n\nIf there is no pushInsteadOf, does it still follow insteadOf?  Is it\ntested already?\n\nWouldn't you want to cover all the combinations to negative cases\n(i.e. making sure the codepath to support a new case does not affect\nbehaviour of the code outside the new case)?  A remote with and\nwithout pushurl (two combinations) and a pseudo URL scheme with and\nwithout pushInsteadOf (again, two combinations) will give you four\ncases.\n\n\nThanks.\n\n>\n> Signed-off-by: Rob Hoelz <rob@hoelz.ro>\n> ---\n>  remote.c              |  2 +-\n>  t/t5516-fetch-push.sh | 20 ++++++++++++++++++++\n>  2 files changed, 21 insertions(+), 1 deletion(-)\n>\n> diff --git a/remote.c b/remote.c\n> index ca1f8f2..de7a915 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -465,7 +465,7 @@ 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\tremotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites_push);\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> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index b5417cc..272e225 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -244,6 +244,26 @@ test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf\n>  \t)\n>  '\n>  \n> +test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf does rewrite in this case)' '\n> +\tmk_empty &&\n> +\tTRASH=\"$(pwd)/\" &&\n> +\tmkdir ro &&\n> +\tmkdir rw &&\n> +\tgit init --bare rw/testrepo &&\n> +\tgit config \"url.file://$TRASH/ro/.insteadOf\" ro: &&\n> +\tgit config \"url.file://$TRASH/rw/.pushInsteadOf\" rw: &&\n> +\tgit config remote.r.url ro:wrong &&\n> +\tgit 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\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>  test_expect_success 'push with matching heads' '\n>  \n>  \tmk_test heads/master &&\n"},{"id":"211554","messageId":"5146E608.9070506@hoelz.ro","threadId":"33210","inReplyTo":"7vd2uxppk0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Rob Hoelz","fromEmail":"rob@hoelz.ro","sentAt":"2013-03-18T10:01:44Z","receivedAt":"2013-03-18T10:01:44Z","isPatch":true,"sender":{"key":"rob@hoelz.ro","avatar":"https://gravatar.com/avatar/c62607c6cef695f7a6cbd0f1f3ebd359e8c471af271581a6d7e29e3fab6fb788?d=mp&s=160"},"body":"On 3/18/13 12:35 AM, Junio C Hamano wrote:\n> Rob Hoelz <rob@hoelz.ro> writes:\n>\n>> git push currently doesn't consider pushInsteadOf when\n>> using pushurl; this tests and fixes that.\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> Incomplete sentence?  For example [this is an example configuration]\n> and then what happens?  Something like \"with the sample\n> configuration, 'git push origin' should follow pushurl and then turn\n> it into X, but instead it ends up accessing Y\".\n>\n> If there is no pushInsteadOf, does it still follow insteadOf?  Is it\n> tested already?\n>\n> Wouldn't you want to cover all the combinations to negative cases\n> (i.e. making sure the codepath to support a new case does not affect\n> behaviour of the code outside the new case)?  A remote with and\n> without pushurl (two combinations) and a pseudo URL scheme with and\n> without pushInsteadOf (again, two combinations) will give you four\n> cases.\n>\n>\n> Thanks.\nSorry; that's a good point.  With the above configuration, the following\nfails:\n\n$ git push origin master\n\nWith the following message:\n\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\nI haven't tried without pushInsteadOf; I will add a test for it later\ntoday.  I assume that if pushInsteadOf isn't found, insteadOf should be\nused?  I will also consider the other test cases you described.\n\nThanks again for the feedback!\n>\n>> Signed-off-by: Rob Hoelz <rob@hoelz.ro>\n>> ---\n>>  remote.c              |  2 +-\n>>  t/t5516-fetch-push.sh | 20 ++++++++++++++++++++\n>>  2 files changed, 21 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/remote.c b/remote.c\n>> index ca1f8f2..de7a915 100644\n>> --- a/remote.c\n>> +++ b/remote.c\n>> @@ -465,7 +465,7 @@ 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\tremotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites_push);\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>> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n>> index b5417cc..272e225 100755\n>> --- a/t/t5516-fetch-push.sh\n>> +++ b/t/t5516-fetch-push.sh\n>> @@ -244,6 +244,26 @@ test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf\n>>  \t)\n>>  '\n>>  \n>> +test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf does rewrite in this case)' '\n>> +\tmk_empty &&\n>> +\tTRASH=\"$(pwd)/\" &&\n>> +\tmkdir ro &&\n>> +\tmkdir rw &&\n>> +\tgit init --bare rw/testrepo &&\n>> +\tgit config \"url.file://$TRASH/ro/.insteadOf\" ro: &&\n>> +\tgit config \"url.file://$TRASH/rw/.pushInsteadOf\" rw: &&\n>> +\tgit config remote.r.url ro:wrong &&\n>> +\tgit 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\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>>  test_expect_success 'push with matching heads' '\n>>  \n>>  \tmk_test heads/master &&\n"},{"id":"211604","messageId":"20130318215953.1a7345f0@hoelz.ro","threadId":"33210","inReplyTo":"7vd2uxppk0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] push: Alias pushurl from push rewrites","fromName":"Rob Hoelz","fromEmail":"rob@hoelz.ro","sentAt":"2013-03-18T20:59:53Z","receivedAt":"2013-03-18T20:59:53Z","isPatch":true,"sender":{"key":"rob@hoelz.ro","avatar":"https://gravatar.com/avatar/c62607c6cef695f7a6cbd0f1f3ebd359e8c471af271581a6d7e29e3fab6fb788?d=mp&s=160"},"body":"On Sun, 17 Mar 2013 16:35:59 -0700\nJunio C Hamano <gitster@pobox.com> wrote:\n\n> Rob Hoelz <rob@hoelz.ro> writes:\n> \n> > git push currently doesn't consider pushInsteadOf when\n> > using pushurl; this tests and fixes that.\n> >\n> > If you use pushurl with an alias that has a pushInsteadOf\n> > configuration value, Git does not take advantage of it.  For\n> > 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> \n> Incomplete sentence?  For example [this is an example configuration]\n> and then what happens?  Something like \"with the sample\n> configuration, 'git push origin' should follow pushurl and then turn\n> it into X, but instead it ends up accessing Y\".\n> \n> If there is no pushInsteadOf, does it still follow insteadOf?  Is it\n> tested already?\n> \n> Wouldn't you want to cover all the combinations to negative cases\n> (i.e. making sure the codepath to support a new case does not affect\n> behaviour of the code outside the new case)?  A remote with and\n> without pushurl (two combinations) and a pseudo URL scheme with and\n> without pushInsteadOf (again, two combinations) will give you four\n> cases.\n> \n> \n> Thanks.\n\nI've taken your advice, and an amended patch follows.\n\n> \n> >\n> > Signed-off-by: Rob Hoelz <rob@hoelz.ro>\n> > ---\n> >  remote.c              |  2 +-\n> >  t/t5516-fetch-push.sh | 20 ++++++++++++++++++++\n> >  2 files changed, 21 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/remote.c b/remote.c\n> > index ca1f8f2..de7a915 100644\n> > --- a/remote.c\n> > +++ b/remote.c\n> > @@ -465,7 +465,7 @@ 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\tremotes[i]->pushurl[j] =\n> > alias_url(remotes[i]->pushurl[j], &rewrites_push); }\n> >  \t\tadd_pushurl_aliases = remotes[i]->pushurl_nr == 0;\n> >  \t\tfor (j = 0; j < remotes[i]->url_nr; j++) {\n> > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> > index b5417cc..272e225 100755\n> > --- a/t/t5516-fetch-push.sh\n> > +++ b/t/t5516-fetch-push.sh\n> > @@ -244,6 +244,26 @@ test_expect_success 'push with pushInsteadOf\n> > and explicit pushurl (pushInsteadOf )\n> >  '\n> >  \n> > +test_expect_success 'push with pushInsteadOf and explicit pushurl\n> > (pushInsteadOf does rewrite in this case)' '\n> > +\tmk_empty &&\n> > +\tTRASH=\"$(pwd)/\" &&\n> > +\tmkdir ro &&\n> > +\tmkdir rw &&\n> > +\tgit init --bare rw/testrepo &&\n> > +\tgit config \"url.file://$TRASH/ro/.insteadOf\" ro: &&\n> > +\tgit config \"url.file://$TRASH/rw/.pushInsteadOf\" rw: &&\n> > +\tgit config remote.r.url ro:wrong &&\n> > +\tgit 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\tr=$(git show-ref -s --verify\n> > refs/remotes/origin/master) &&\n> > +\t\ttest \"z$r\" = \"z$the_commit\" &&\n> > +\n> > +\t\ttest 1 = $(git for-each-ref refs/remotes/origin |\n> > wc -l)\n> > +\t)\n> > +'\n> > +\n> >  test_expect_success 'push with matching heads' '\n> >  \n> >  \tmk_test heads/master &&\n> \n"}]}