Re: [PATCH] t5516-fetch-push: Correct misspelled pushInsteadOf
Michael J Gruber g...@drmicha.warpmail.net writes: I am not sure how the intention of the commit 1c2eafb8 (Add url.base.pushInsteadOf: URL rewriting for push only, 2009-09-07), which introduced the behaviour verified by this test, interacts with the desire to redefine what URL and pushURL mean in our recent past, what e.g. e6196ae1 (remote: add --fetch and --both options to set-url, 2014-11-25) wanted to do, though. Thoughts? Wow. That looks strange to me on first read. Sorry I didn't catch it back then. --fetch sets both url and pushurl?? Yeah, sounds crazy, no? Taken in isolation without consideration around the InsteadOf rewriting, the change sort-of makes sense, though. But I do not know if the combined whole makes much sense. So, for definiteness sake, I'll use url and pushurl for the config names and struct members (which the config values end up in), and I'll use URL for fetch and URL for push for the URLs that git will use for fetch resp. push. If there is no pushurl: url will be used as the URL for fetch and as the URL for push. if we are pushing and there is pushinsteadof for (part of) url subsitute that within url else if there is insteadof for (part of) url substitute that within url If there is a pushurl: url will be used as the URL for fetch and pushurl as the URL for push. if we are pushing and there is insteadof for (part of) pushurl substitute that within pushurl else if we are fetching and there is insteadof for (part of) url substiute that within url These make sense to me. I don't know what remote set-url does, I think the whole remote command as it is does not fit in well with our UI, but that applies to other commands with non-option subcommands as well (or bad). I would have hoped it is set-url and set-pushurl to set those two config options. I would have hoped so, too, but I don't use git remote myself (as you said, it is an odd man out and there is nothing it and only it can do that is essential for my everyday workflow), so... ;-) I rarely use pushurl, but could use it to override url, or to do the above magic on a case by case level, such as if I need to specify different usernames for the same host. So, that's my understanding of how these things are supposed to work and why it is useful. Yes, I am not questioning the usefulness of insteadof magic. I am wondering if that --fetch sets both? breaks the expectation of those who rely on it. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t5516-fetch-push: Correct misspelled pushInsteadOf
Junio C Hamano venit, vidit, dixit 03.03.2015 21:06: Michael J Gruber g...@drmicha.warpmail.net writes: Junio C Hamano venit, vidit, dixit 02.03.2015 20:43: Anders Kaseorg ande...@mit.edu writes: Signed-off-by: Anders Kaseorg ande...@mit.edu --- t/t5516-fetch-push.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 85c7fec..594d7a6 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -238,7 +238,7 @@ test_expect_success 'push with pushInsteadOf' ' test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' ' mk_empty testrepo test_config url.trash2/.pushInsteadOf testrepo/ - test_config url.trash3/.pusnInsteadOf trash/wrong + test_config url.trash3/.pushInsteadOf trash/wrong test_config remote.r.url trash/wrong test_config remote.r.pushurl testrepo/ git push r refs/heads/master:refs/remotes/origin/master Interesting. Now an obvious and natural question after seeing this change is how the original test passed with misspelled configuration. Is a test that pushes into trash/wrong checking the right outcome? If the reason why the existing tests passed without this patch is because they do not test the right thing, then shouldn't they be corrected together with the above fix? Ha, I was look there, too, just today and was wondering the same. I guess the test wanted to make sure (among other things) that url.trash3/.pushInsteadOf does not affect the push to remote r (which has an explicit pushurl)... OK, so it makes sure the push goes to testrepo/ but it does not make sure trash2 or trash3 are not touched. Makes sort of sense. Thanks. I am not sure how the intention of the commit 1c2eafb8 (Add url.base.pushInsteadOf: URL rewriting for push only, 2009-09-07), which introduced the behaviour verified by this test, interacts with the desire to redefine what URL and pushURL mean in our recent past, what e.g. e6196ae1 (remote: add --fetch and --both options to set-url, 2014-11-25) wanted to do, though. Thoughts? Wow. That looks strange to me on first read. Sorry I didn't catch it back then. --fetch sets both url and pushurl?? Also, talking about these things (in the commit msg etc.) can be confusing very quickly because there are the config names url and pushurl, the struct members url und pushurl, and then suddenly the notion of fetch URL appears. So, for definiteness sake, I'll use url and pushurl for the config names and struct members (which the config values end up in), and I'll use URL for fetch and URL for push for the URLs that git will use for fetch resp. push. If there is no pushurl: url will be used as the URL for fetch and as the URL for push. if we are pushing and there is pushinsteadof for (part of) url subsitute that within url else if there is insteadof for (part of) url substitute that within url If there is a pushurl: url will be used as the URL for fetch and pushurl as the URL for push. if we are pushing and there is insteadof for (part of) pushurl substitute that within pushurl else if we are fetching and there is insteadof for (part of) url substiute that within url I don't know what remote set-url does, I think the whole remote command as it is does not fit in well with our UI, but that applies to other commands with non-option subcommands as well (or bad). I would have hoped it is set-url and set-pushurl to set those two config options. Maybe that's not useful. The logic above may look a bit strange regarding pushinsteadof, but it's really what makes this useful. An example: [url git://github.com/] insteadOf = github: [url github:] pushinsteadOf = github: This allows me to use git remote add remotename github:user/reponame and have fetches over git protocol, pushes over ssh since my ssh_config has: host github Hostname github.com User git IdentityFile ~/.ssh/repoor_dsa [url https://bitbucket.org/;] insteadOf = bitbucket: [url https://gru...@bitbucket.org/;] pushinsteadOf = bitbucket: This allows me to use git remote add remotename bitbucket:user/reponame and have fetches over https, pushes over https with my username preset. I rarely use pushurl, but could use it to override url, or to do the above magic on a case by case level, such as if I need to specify different usernames for the same host. So, that's my understanding of how these things are supposed to work and why it is useful. Michael -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t5516-fetch-push: Correct misspelled pushInsteadOf
Anders Kaseorg venit, vidit, dixit 04.03.2015 10:43: On Wed, 4 Mar 2015, Michael J Gruber wrote: If there is no pushurl: url will be used as the URL for fetch and as the URL for push. if we are pushing and there is pushinsteadof for (part of) url subsitute that within url else if there is insteadof for (part of) url substitute that within url If there is a pushurl: url will be used as the URL for fetch and pushurl as the URL for push. if we are pushing and there is insteadof for (part of) pushurl substitute that within pushurl else if we are fetching and there is insteadof for (part of) url substiute that within url Speaking of that, I recently had to reimplement most of this logic for the openstack git-review tool (https://review.openstack.org/160152), which is why I was staring too closely at t5516 in the first place. It would be nice to have a ‘git ls-remote --get-push-url’ analogous to the existing ‘git ls-remote --get-url’. Anders And vice-versa, it would be nice if git remote explained where the resulting URLs come from. I vaguely remember looking into this, but I don't think it was simple. ls-remote and remote don't share code, which is a pitty. GSOC... Michael -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t5516-fetch-push: Correct misspelled pushInsteadOf
On Wed, 4 Mar 2015, Michael J Gruber wrote: If there is no pushurl: url will be used as the URL for fetch and as the URL for push. if we are pushing and there is pushinsteadof for (part of) url subsitute that within url else if there is insteadof for (part of) url substitute that within url If there is a pushurl: url will be used as the URL for fetch and pushurl as the URL for push. if we are pushing and there is insteadof for (part of) pushurl substitute that within pushurl else if we are fetching and there is insteadof for (part of) url substiute that within url Speaking of that, I recently had to reimplement most of this logic for the openstack git-review tool (https://review.openstack.org/160152), which is why I was staring too closely at t5516 in the first place. It would be nice to have a ‘git ls-remote --get-push-url’ analogous to the existing ‘git ls-remote --get-url’. Anders -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t5516-fetch-push: Correct misspelled pushInsteadOf
Junio C Hamano venit, vidit, dixit 02.03.2015 20:43: Anders Kaseorg ande...@mit.edu writes: Signed-off-by: Anders Kaseorg ande...@mit.edu --- t/t5516-fetch-push.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 85c7fec..594d7a6 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -238,7 +238,7 @@ test_expect_success 'push with pushInsteadOf' ' test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' ' mk_empty testrepo test_config url.trash2/.pushInsteadOf testrepo/ -test_config url.trash3/.pusnInsteadOf trash/wrong +test_config url.trash3/.pushInsteadOf trash/wrong test_config remote.r.url trash/wrong test_config remote.r.pushurl testrepo/ git push r refs/heads/master:refs/remotes/origin/master Interesting. Now an obvious and natural question after seeing this change is how the original test passed with misspelled configuration. Is a test that pushes into trash/wrong checking the right outcome? If the reason why the existing tests passed without this patch is because they do not test the right thing, then shouldn't they be corrected together with the above fix? Ha, I was look there, too, just today and was wondering the same. I guess the test wanted to make sure (among other things) that url.trash3/.pushInsteadOf does not affect the push to remote r (which has an explicit pushurl)... Michael -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t5516-fetch-push: Correct misspelled pushInsteadOf
Michael J Gruber g...@drmicha.warpmail.net writes: Junio C Hamano venit, vidit, dixit 02.03.2015 20:43: Anders Kaseorg ande...@mit.edu writes: Signed-off-by: Anders Kaseorg ande...@mit.edu --- t/t5516-fetch-push.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 85c7fec..594d7a6 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -238,7 +238,7 @@ test_expect_success 'push with pushInsteadOf' ' test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' ' mk_empty testrepo test_config url.trash2/.pushInsteadOf testrepo/ - test_config url.trash3/.pusnInsteadOf trash/wrong + test_config url.trash3/.pushInsteadOf trash/wrong test_config remote.r.url trash/wrong test_config remote.r.pushurl testrepo/ git push r refs/heads/master:refs/remotes/origin/master Interesting. Now an obvious and natural question after seeing this change is how the original test passed with misspelled configuration. Is a test that pushes into trash/wrong checking the right outcome? If the reason why the existing tests passed without this patch is because they do not test the right thing, then shouldn't they be corrected together with the above fix? Ha, I was look there, too, just today and was wondering the same. I guess the test wanted to make sure (among other things) that url.trash3/.pushInsteadOf does not affect the push to remote r (which has an explicit pushurl)... OK, so it makes sure the push goes to testrepo/ but it does not make sure trash2 or trash3 are not touched. Makes sort of sense. Thanks. I am not sure how the intention of the commit 1c2eafb8 (Add url.base.pushInsteadOf: URL rewriting for push only, 2009-09-07), which introduced the behaviour verified by this test, interacts with the desire to redefine what URL and pushURL mean in our recent past, what e.g. e6196ae1 (remote: add --fetch and --both options to set-url, 2014-11-25) wanted to do, though. Thoughts? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t5516-fetch-push: Correct misspelled pushInsteadOf
Anders Kaseorg ande...@mit.edu writes: Signed-off-by: Anders Kaseorg ande...@mit.edu --- t/t5516-fetch-push.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 85c7fec..594d7a6 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -238,7 +238,7 @@ test_expect_success 'push with pushInsteadOf' ' test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' ' mk_empty testrepo test_config url.trash2/.pushInsteadOf testrepo/ - test_config url.trash3/.pusnInsteadOf trash/wrong + test_config url.trash3/.pushInsteadOf trash/wrong test_config remote.r.url trash/wrong test_config remote.r.pushurl testrepo/ git push r refs/heads/master:refs/remotes/origin/master Interesting. Now an obvious and natural question after seeing this change is how the original test passed with misspelled configuration. Is a test that pushes into trash/wrong checking the right outcome? If the reason why the existing tests passed without this patch is because they do not test the right thing, then shouldn't they be corrected together with the above fix? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] t5516-fetch-push: Correct misspelled pushInsteadOf
Signed-off-by: Anders Kaseorg ande...@mit.edu --- t/t5516-fetch-push.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 85c7fec..594d7a6 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -238,7 +238,7 @@ test_expect_success 'push with pushInsteadOf' ' test_expect_success 'push with pushInsteadOf and explicit pushurl (pushInsteadOf should not rewrite)' ' mk_empty testrepo test_config url.trash2/.pushInsteadOf testrepo/ - test_config url.trash3/.pusnInsteadOf trash/wrong + test_config url.trash3/.pushInsteadOf trash/wrong test_config remote.r.url trash/wrong test_config remote.r.pushurl testrepo/ git push r refs/heads/master:refs/remotes/origin/master -- 2.3.1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
