{"thread":{"id":"38672","subject":"[PATCH] t5516-fetch-push: Correct misspelled pushInsteadOf","startedAt":"2015-03-01T04:18:14Z","lastAt":"2015-03-04T20:14:26Z","messageCount":8,"participants":["Anders Kaseorg","Junio C Hamano","Michael J Gruber"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"256794","messageId":"alpine.DEB.2.10.1502282316150.7008@buzzword-bingo.mit.edu","threadId":"38672","inReplyTo":null,"subject":"[PATCH] t5516-fetch-push: Correct misspelled pushInsteadOf","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2015-03-01T04:18:14Z","receivedAt":"2015-03-01T04:18:14Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"Signed-off-by: Anders Kaseorg \n---\n t/t5516-fetch-push.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 85c7fec..594d7a6 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -238,7 +238,7 @@ test_expect_success 'push with pushInsteadOf' '\n test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '\n \tmk_empty testrepo &&\n \ttest_config \"url.trash2/.pushInsteadOf\" testrepo/ &&\n-\ttest_config \"url.trash3/.pusnInsteadOf\" trash/wrong &&\n+\ttest_config \"url.trash3/.pushInsteadOf\" trash/wrong &&\n \ttest_config remote.r.url trash/wrong &&\n \ttest_config remote.r.pushurl \"testrepo/\" &&\n \tgit push r refs/heads/master:refs/remotes/origin/master &&\n-- \n2.3.1\n"},{"id":"256856","messageId":"xmqq385nb2yg.fsf@gitster.dls.corp.google.com","threadId":"38672","inReplyTo":"alpine.DEB.2.10.1502282316150.7008@buzzword-bingo.mit.edu","subject":"Re: [PATCH] t5516-fetch-push: Correct misspelled pushInsteadOf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-02T19:43:03Z","receivedAt":"2015-03-02T19:43:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Kaseorg writes:\n\n> Signed-off-by: Anders Kaseorg \n> ---\n> t/t5516-fetch-push.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index 85c7fec..594d7a6 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -238,7 +238,7 @@ test_expect_success 'push with pushInsteadOf' '\n> test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '\n> \tmk_empty testrepo &&\n> \ttest_config \"url.trash2/.pushInsteadOf\" testrepo/ &&\n> -\ttest_config \"url.trash3/.pusnInsteadOf\" trash/wrong &&\n> +\ttest_config \"url.trash3/.pushInsteadOf\" trash/wrong &&\n> \ttest_config remote.r.url trash/wrong &&\n> \ttest_config remote.r.pushurl \"testrepo/\" &&\n> \tgit push r refs/heads/master:refs/remotes/origin/master &&\n\nInteresting.\n\nNow an obvious and natural question after seeing this change is how\nthe original test passed with misspelled configuration. Is a test\nthat pushes into \"trash/wrong\" checking the right outcome? If the\nreason why the existing tests passed without this patch is because\nthey do not test the right thing, then shouldn't they be corrected\ntogether with the above fix?\n"},{"id":"256907","messageId":"54F5D869.4010104@drmicha.warpmail.net","threadId":"38672","inReplyTo":"xmqq385nb2yg.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] t5516-fetch-push: Correct misspelled pushInsteadOf","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2015-03-03T15:51:05Z","receivedAt":"2015-03-03T15:51:05Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 02.03.2015 20:43:\n> Anders Kaseorg writes:\n> \n>> Signed-off-by: Anders Kaseorg \n>> ---\n>> t/t5516-fetch-push.sh | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n>> index 85c7fec..594d7a6 100755\n>> --- a/t/t5516-fetch-push.sh\n>> +++ b/t/t5516-fetch-push.sh\n>> @@ -238,7 +238,7 @@ test_expect_success 'push with pushInsteadOf' '\n>> test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '\n>> \tmk_empty testrepo &&\n>> \ttest_config \"url.trash2/.pushInsteadOf\" testrepo/ &&\n>> -\ttest_config \"url.trash3/.pusnInsteadOf\" trash/wrong &&\n>> +\ttest_config \"url.trash3/.pushInsteadOf\" trash/wrong &&\n>> \ttest_config remote.r.url trash/wrong &&\n>> \ttest_config remote.r.pushurl \"testrepo/\" &&\n>> \tgit push r refs/heads/master:refs/remotes/origin/master &&\n> \n> Interesting.\n> \n> Now an obvious and natural question after seeing this change is how\n> the original test passed with misspelled configuration. Is a test\n> that pushes into \"trash/wrong\" checking the right outcome? If the\n> reason why the existing tests passed without this patch is because\n> they do not test the right thing, then shouldn't they be corrected\n> together with the above fix?\n> \n\nHa, I was look there, too, just today and was wondering the same.\n\nI guess the test wanted to make sure (among other things) that\nurl.trash3/.pushInsteadOf does not affect the push to remote r (which\nhas an explicit pushurl)...\n\nMichael\n"},{"id":"256919","messageId":"xmqq8ufdc0c7.fsf@gitster.dls.corp.google.com","threadId":"38672","inReplyTo":"54F5D869.4010104@drmicha.warpmail.net","subject":"Re: [PATCH] t5516-fetch-push: Correct misspelled pushInsteadOf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-03T20:06:32Z","receivedAt":"2015-03-03T20:06:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> Junio C Hamano venit, vidit, dixit 02.03.2015 20:43:\n>> Anders Kaseorg writes:\n>> \n>>> Signed-off-by: Anders Kaseorg \n>>> ---\n>>> t/t5516-fetch-push.sh | 2 +-\n>>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>>\n>>> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n>>> index 85c7fec..594d7a6 100755\n>>> --- a/t/t5516-fetch-push.sh\n>>> +++ b/t/t5516-fetch-push.sh\n>>> @@ -238,7 +238,7 @@ test_expect_success 'push with pushInsteadOf' '\n>>> test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '\n>>> \tmk_empty testrepo &&\n>>> \ttest_config \"url.trash2/.pushInsteadOf\" testrepo/ &&\n>>> -\ttest_config \"url.trash3/.pusnInsteadOf\" trash/wrong &&\n>>> +\ttest_config \"url.trash3/.pushInsteadOf\" trash/wrong &&\n>>> \ttest_config remote.r.url trash/wrong &&\n>>> \ttest_config remote.r.pushurl \"testrepo/\" &&\n>>> \tgit push r refs/heads/master:refs/remotes/origin/master &&\n>> \n>> Interesting.\n>> \n>> Now an obvious and natural question after seeing this change is how\n>> the original test passed with misspelled configuration. Is a test\n>> that pushes into \"trash/wrong\" checking the right outcome? If the\n>> reason why the existing tests passed without this patch is because\n>> they do not test the right thing, then shouldn't they be corrected\n>> together with the above fix?\n>> \n>\n> Ha, I was look there, too, just today and was wondering the same.\n>\n> I guess the test wanted to make sure (among other things) that\n> url.trash3/.pushInsteadOf does not affect the push to remote r (which\n> has an explicit pushurl)...\n\nOK, so it makes sure the push goes to testrepo/ but it does not make\nsure trash2 or trash3 are not touched. Makes sort of sense.\n\nThanks.\n\nI am not sure how the intention of the commit 1c2eafb8 (Add\nurl..pushInsteadOf: URL rewriting for push only, 2009-09-07),\nwhich introduced the behaviour verified by this test, interacts with\nthe desire to redefine what URL and pushURL mean in our recent past,\nwhat e.g. e6196ae1 (remote: add --fetch and --both options to set-url,\n2014-11-25) wanted to do, though. Thoughts?\n"},{"id":"256980","messageId":"54F6D20D.903@drmicha.warpmail.net","threadId":"38672","inReplyTo":"xmqq8ufdc0c7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] t5516-fetch-push: Correct misspelled pushInsteadOf","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2015-03-04T09:36:13Z","receivedAt":"2015-03-04T09:36:13Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 03.03.2015 21:06:\n> Michael J Gruber writes:\n> \n>> Junio C Hamano venit, vidit, dixit 02.03.2015 20:43:\n>>> Anders Kaseorg writes:\n>>>\n>>>> Signed-off-by: Anders Kaseorg \n>>>> ---\n>>>> t/t5516-fetch-push.sh | 2 +-\n>>>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>>>\n>>>> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n>>>> index 85c7fec..594d7a6 100755\n>>>> --- a/t/t5516-fetch-push.sh\n>>>> +++ b/t/t5516-fetch-push.sh\n>>>> @@ -238,7 +238,7 @@ test_expect_success 'push with pushInsteadOf' '\n>>>> test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' '\n>>>> \tmk_empty testrepo &&\n>>>> \ttest_config \"url.trash2/.pushInsteadOf\" testrepo/ &&\n>>>> -\ttest_config \"url.trash3/.pusnInsteadOf\" trash/wrong &&\n>>>> +\ttest_config \"url.trash3/.pushInsteadOf\" trash/wrong &&\n>>>> \ttest_config remote.r.url trash/wrong &&\n>>>> \ttest_config remote.r.pushurl \"testrepo/\" &&\n>>>> \tgit push r refs/heads/master:refs/remotes/origin/master &&\n>>>\n>>> Interesting.\n>>>\n>>> Now an obvious and natural question after seeing this change is how\n>>> the original test passed with misspelled configuration. Is a test\n>>> that pushes into \"trash/wrong\" checking the right outcome? If the\n>>> reason why the existing tests passed without this patch is because\n>>> they do not test the right thing, then shouldn't they be corrected\n>>> together with the above fix?\n>>>\n>>\n>> Ha, I was look there, too, just today and was wondering the same.\n>>\n>> I guess the test wanted to make sure (among other things) that\n>> url.trash3/.pushInsteadOf does not affect the push to remote r (which\n>> has an explicit pushurl)...\n> \n> OK, so it makes sure the push goes to testrepo/ but it does not make\n> sure trash2 or trash3 are not touched. Makes sort of sense.\n> \n> Thanks.\n> \n> I am not sure how the intention of the commit 1c2eafb8 (Add\n> url..pushInsteadOf: URL rewriting for push only, 2009-09-07),\n> which introduced the behaviour verified by this test, interacts with\n> the desire to redefine what URL and pushURL mean in our recent past,\n> what e.g. e6196ae1 (remote: add --fetch and --both options to set-url,\n> 2014-11-25) wanted to do, though. Thoughts?\n> \n\nWow. That looks strange to me on first read. Sorry I didn't catch it\nback then. \"--fetch\" sets both url and pushurl??\n\nAlso, talking about these things (in the commit msg etc.) can be\nconfusing very quickly because there are the config names url and\npushurl, the struct members url und pushurl, and then suddenly the\nnotion of \"fetch URL\" appears.\n\nSo, for definiteness sake, I'll use \"url\" and \"pushurl\" for the config\nnames and struct members (which the config values end up in), and I'll\nuse \"URL for fetch\" and \"URL for push\" for the URLs that git will use\nfor fetch resp. push.\n\nIf there is no \"pushurl\":\n \"url\" will be used as the \"URL for fetch\" and as the \"URL for push\".\n if we are pushing and there is \"pushinsteadof\" for (part of) \"url\"\n subsitute that within \"url\"\n else if there is \"insteadof\" for (part of) \"url\"\n substitute that within \"url\"\n\nIf there is a \"pushurl\":\n \"url\" will be used as the \"URL for fetch\" and \"pushurl\" as the \"URL\nfor push\".\n if we are pushing and there is \"insteadof\" for (part of) \"pushurl\"\n substitute that within \"pushurl\"\n else if we are fetching and there is \"insteadof\" for (part of) \"url\"\n substiute that within \"url\"\n\nI don't know what \"remote set-url\" does, I think the whole remote\ncommand as it is does not fit in well with our UI, but that applies to\nother commands with non-option subcommands as well (or bad). I would\nhave hoped it is set-url and set-pushurl to set those two config\noptions. Maybe that's not useful.\n\nThe logic above may look a bit strange regarding pushinsteadof, but it's\nreally what makes this useful. An example:\n\n[url \"git://github.com/\"]\n insteadOf = github:\n[url \"github:\"]\n pushinsteadOf = github:\n\nThis allows me to use\ngit remote add github:/\n\nand have fetches over git protocol, pushes over ssh since my ssh_config has:\n\nhost github\nHostname github.com\nUser git\nIdentityFile ~/.ssh/repoor_dsa\n\n[url \"https://bitbucket.org/\"]\n insteadOf = bitbucket:\n[url \"https://grubix@bitbucket.org/\"]\n pushinsteadOf = bitbucket:\n\nThis allows me to use\n\ngit remote add bitbucket:/\n\nand have fetches over https, pushes over https with my username preset.\n\nI rarely use pushurl, but could use it to override url, or to do the\nabove magic on a case by case level, such as if I need to specify\ndifferent usernames for the same host.\n\nSo, that's my understanding of how these things are supposed to work and\nwhy it is useful.\n\nMichael\n"},{"id":"256982","messageId":"alpine.DEB.2.10.1503040439100.60859@buzzword-bingo.mit.edu","threadId":"38672","inReplyTo":"54F6D20D.903@drmicha.warpmail.net","subject":"Re: [PATCH] t5516-fetch-push: Correct misspelled pushInsteadOf","fromName":"Anders Kaseorg","fromEmail":"andersk@mit.edu","sentAt":"2015-03-04T09:43:23Z","receivedAt":"2015-03-04T09:43:23Z","isPatch":true,"sender":{"key":"andersk@mit.edu","avatar":"https://avatars.githubusercontent.com/u/26471?v=4"},"body":"On Wed, 4 Mar 2015, Michael J Gruber wrote:\n> If there is no \"pushurl\":\n> \"url\" will be used as the \"URL for fetch\" and as the \"URL for push\".\n> if we are pushing and there is \"pushinsteadof\" for (part of) \"url\"\n> subsitute that within \"url\"\n> else if there is \"insteadof\" for (part of) \"url\"\n> substitute that within \"url\"\n> \n> If there is a \"pushurl\":\n> \"url\" will be used as the \"URL for fetch\" and \"pushurl\" as the \"URL for push\".\n> if we are pushing and there is \"insteadof\" for (part of) \"pushurl\"\n> substitute that within \"pushurl\"\n> else if we are fetching and there is \"insteadof\" for (part of) \"url\"\n> substiute that within \"url\"\n\nSpeaking of that, I recently had to reimplement most of this logic for the \nopenstack git-review tool (https://review.openstack.org/160152), which is \nwhy I was staring too closely at t5516 in the first place. It would be \nnice to have a ‘git ls-remote --get-push-url’ analogous to the existing \n‘git ls-remote --get-url’.\n\nAnders\n"},{"id":"256987","messageId":"54F6DA06.7000602@drmicha.warpmail.net","threadId":"38672","inReplyTo":"alpine.DEB.2.10.1503040439100.60859@buzzword-bingo.mit.edu","subject":"Re: [PATCH] t5516-fetch-push: Correct misspelled pushInsteadOf","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2015-03-04T10:10:14Z","receivedAt":"2015-03-04T10:10:14Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Anders Kaseorg venit, vidit, dixit 04.03.2015 10:43:\n> On Wed, 4 Mar 2015, Michael J Gruber wrote:\n>> If there is no \"pushurl\":\n>> \"url\" will be used as the \"URL for fetch\" and as the \"URL for push\".\n>> if we are pushing and there is \"pushinsteadof\" for (part of) \"url\"\n>> subsitute that within \"url\"\n>> else if there is \"insteadof\" for (part of) \"url\"\n>> substitute that within \"url\"\n>>\n>> If there is a \"pushurl\":\n>> \"url\" will be used as the \"URL for fetch\" and \"pushurl\" as the \"URL for push\".\n>> if we are pushing and there is \"insteadof\" for (part of) \"pushurl\"\n>> substitute that within \"pushurl\"\n>> else if we are fetching and there is \"insteadof\" for (part of) \"url\"\n>> substiute that within \"url\"\n> \n> Speaking of that, I recently had to reimplement most of this logic for the \n> openstack git-review tool (https://review.openstack.org/160152), which is \n> why I was staring too closely at t5516 in the first place. It would be \n> nice to have a ‘git ls-remote --get-push-url’ analogous to the existing \n> ‘git ls-remote --get-url’.\n> \n> Anders\n> \n\nAnd vice-versa, it would be nice if \"git remote\" explained where the\nresulting URLs come from. I vaguely remember looking into this, but I\ndon't think it was simple.\n\nls-remote and remote don't share code, which is a pitty. GSOC...\n\nMichael\n"},{"id":"257014","messageId":"xmqqbnk88qql.fsf@gitster.dls.corp.google.com","threadId":"38672","inReplyTo":"54F6D20D.903@drmicha.warpmail.net","subject":"Re: [PATCH] t5516-fetch-push: Correct misspelled pushInsteadOf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-04T20:14:26Z","receivedAt":"2015-03-04T20:14:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n>> I am not sure how the intention of the commit 1c2eafb8 (Add\n>> url..pushInsteadOf: URL rewriting for push only, 2009-09-07),\n>> which introduced the behaviour verified by this test, interacts with\n>> the desire to redefine what URL and pushURL mean in our recent past,\n>> what e.g. e6196ae1 (remote: add --fetch and --both options to set-url,\n>> 2014-11-25) wanted to do, though. Thoughts?\n>> \n>\n> Wow. That looks strange to me on first read. Sorry I didn't catch it\n> back then. \"--fetch\" sets both url and pushurl??\n\nYeah, sounds crazy, no? Taken in isolation without consideration\naround the InsteadOf rewriting, the change sort-of makes sense,\nthough.\n\nBut I do not know if the combined whole makes much sense.\n\n> So, for definiteness sake, I'll use \"url\" and \"pushurl\" for the config\n> names and struct members (which the config values end up in), and I'll\n> use \"URL for fetch\" and \"URL for push\" for the URLs that git will use\n> for fetch resp. push.\n>\n> If there is no \"pushurl\":\n> \"url\" will be used as the \"URL for fetch\" and as the \"URL for push\".\n> if we are pushing and there is \"pushinsteadof\" for (part of) \"url\"\n> subsitute that within \"url\"\n> else if there is \"insteadof\" for (part of) \"url\"\n> substitute that within \"url\"\n>\n> If there is a \"pushurl\":\n> \"url\" will be used as the \"URL for fetch\" and \"pushurl\" as the \"URL\n> for push\".\n> if we are pushing and there is \"insteadof\" for (part of) \"pushurl\"\n> substitute that within \"pushurl\"\n> else if we are fetching and there is \"insteadof\" for (part of) \"url\"\n> substiute that within \"url\"\n\nThese make sense to me.\n\n> I don't know what \"remote set-url\" does, I think the whole remote\n> command as it is does not fit in well with our UI, but that applies to\n> other commands with non-option subcommands as well (or bad). I would\n> have hoped it is set-url and set-pushurl to set those two config\n> options.\n\nI would have hoped so, too, but I don't use \"git remote\" myself (as\nyou said, it is an odd man out and there is nothing it and only it\ncan do that is essential for my everyday workflow), so... ;-)\n\n> I rarely use pushurl, but could use it to override url, or to do the\n> above magic on a case by case level, such as if I need to specify\n> different usernames for the same host.\n>\n> So, that's my understanding of how these things are supposed to work and\n> why it is useful.\n\nYes, I am not questioning the usefulness of insteadof magic. I am\nwondering if that \"--fetch sets both?\" breaks the expectation of\nthose who rely on it.\n"}]}