Re: [PATCH v2 11/14] t/t5505-remote: test multiple push/pull in remotes-file
Ramkumar Ramachandra artag...@gmail.com writes: Extend the test migrate a remote from named file in $GIT_DIR/remotes to test that multiple Push: and Pull: lines in the remotes-file works as expected. Signed-off-by: Ramkumar Ramachandra artag...@gmail.com --- t/t5505-remote.sh | 16 ++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 229a89c..6a622fc 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -735,7 +735,9 @@ test_expect_success 'rename a remote with name prefix of other remote' ' cat remotes_origin -EOF URL: quux Push: refs/heads/master:refs/heads/upstream +Push: refs/heads/master:refs/heads/upstream2 Pull: refs/heads/master:refs/heads/origin +Pull: refs/heads/master:refs/heads/origin2 EOF I do not think we ever designed this to get 'master' pushed to update two separate destination branches or their 'master' to update our two separate tracking branches. If you want to make things more realistic like you did in 08/14, I do not see a point to change the tests that is already done for a useful and realistic case by making it deliberately less realistic. The same comment applies to the bogus quux URL from the patch 04/14. I'll reject 04/14 and tweak this patch to use 'next' for the new ref mappings, not duplicated 'master'. Patches 07/14, 12/14, 13/14, and 14/14 are bad idea (these will not be queued on tonight's pushout of 'pu'; neither 04/14 will be). We may not be encouraging, but that is very different from deprecating the original mechanisms. The tests that depend on them to work should be kept. Otherwise, we will never know when we break them like we did at df93e33c accidenally. If we want to have tests that exercise the equivalents spelled with the modern in-config mechanism, they should be added as new tests, not by replacing the existing ones. -- 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 v2 11/14] t/t5505-remote: test multiple push/pull in remotes-file
Junio C Hamano wrote: I'll reject 04/14 and tweak this patch to use 'next' for the new ref mappings, not duplicated 'master'. It's a matter of taste: I don't like realistic (albeit misleading) values when I'm testing configuration variables, while you think they make the tests more readable. Fine. Patches 07/14, 12/14, 13/14, and 14/14 are bad idea (these will not be queued on tonight's pushout of 'pu'; neither 04/14 will be). We may not be encouraging, but that is very different from deprecating the original mechanisms. The tests that depend on them to work should be kept. Otherwise, we will never know when we break them like we did at df93e33c accidenally. If we want to have tests that exercise the equivalents spelled with the modern in-config mechanism, they should be added as new tests, not by replacing the existing ones. I disagree. It is trivial to prove that the tests in t/remote will break if this fringe feature breaks: I don't know where we will never know when we break them is coming from. Why should I know about this fringe feature when I'm reading/writing tests for fetch-merge-logic? And what is _advantage_ of depending on this fringe feature when testing fetch-merge-logic? More tests break? But whatever. I've already spent more time discussing (bikeshedding?) this series than writing it. I regret having written a stupid cleanup series with no real consequence now; I'll be less likely to make the same mistake again in the future. Thanks. -- 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 v2 11/14] t/t5505-remote: test multiple push/pull in remotes-file
Ramkumar Ramachandra artag...@gmail.com writes: I disagree. It is trivial to prove that the tests in t/remote will break if this fringe feature breaks: I don't know where we will never know when we break them is coming from. If you remove tests that configure the remotes via the .git/branches and the .git/remotes mechanism to lose test coverage, and then remove the call to read_branches_file() or read_remotes_file() from remote_get_1(), how would you make sure these tests that originally expected these configuration mechanisms to work notice the change in behaviour? Why should I know about this fringe feature when I'm reading/writing tests for fetch-merge-logic? That is exactly why I asked you at the very beginning: I say minor only because I think the cost of keeping these old mechanisms alive is very low (if it is a heavy burden on the maintenance, please tell me and how). If we are not deprecating and keeping them alive, anybody who is touching the codepath to deal with struct remote should be at least aware of these mechanisms to keep them working. If it is a heavy burden to do so, then keeping them alive is not minor at all. I've been operating under the assumption, from your response to the message in the original discussion in v1, that we are in agreement that we are not deprecating or removing them. And if we are deliberately keeping them alive, there is no need for you to paint them fringe. You may not use it, and you can write in your blog or e-mail or whategver that you do not use it, but that label does not belong to our codebase and documentation as long as we are keeping them as supported feature. Of course, .git/branches, being a single liner format, will not get extended to support richer features that in-config remotes will learn, so you cannot build on existing tests that use the mechanism to show new features. You would need to introduce tests based on the in-config remotes to demonstrate new features such as the support for triangular workflow. Because the .git/remotes/ files are designed to be extensible by introducing new fields, theoretically it may be possible to extend it to support new features, but we will not. We will keep existing users' existing use cases before new features are introduced in recent Git alive, without adding anything new. People will be migrated out of these old mechanisms over time that way. -- 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 v2 11/14] t/t5505-remote: test multiple push/pull in remotes-file
Extend the test migrate a remote from named file in $GIT_DIR/remotes to test that multiple Push: and Pull: lines in the remotes-file works as expected. Signed-off-by: Ramkumar Ramachandra artag...@gmail.com --- t/t5505-remote.sh | 16 ++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 229a89c..6a622fc 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -735,7 +735,9 @@ test_expect_success 'rename a remote with name prefix of other remote' ' cat remotes_origin -EOF URL: quux Push: refs/heads/master:refs/heads/upstream +Push: refs/heads/master:refs/heads/upstream2 Pull: refs/heads/master:refs/heads/origin +Pull: refs/heads/master:refs/heads/origin2 EOF test_expect_success 'migrate a remote from named file in $GIT_DIR/remotes' ' @@ -748,8 +750,18 @@ test_expect_success 'migrate a remote from named file in $GIT_DIR/remotes' ' git remote rename origin origin test_path_is_missing .git/remotes/origin test $(git config remote.origin.url) = quux - test $(git config remote.origin.push) = refs/heads/master:refs/heads/upstream - test $(git config remote.origin.fetch) = refs/heads/master:refs/heads/origin + cat push_expected -\EOF + refs/heads/master:refs/heads/upstream + refs/heads/master:refs/heads/upstream2 + EOF + cat fetch_expected -\EOF + refs/heads/master:refs/heads/origin + refs/heads/master:refs/heads/origin2 + EOF + git config --get-all remote.origin.push push_actual + git config --get-all remote.origin.fetch fetch_actual + test_cmp push_expected push_actual + test_cmp fetch_expected fetch_actual ) ' -- 1.8.3.1.498.gacf2885 -- 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
