Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream
On Fri, Feb 28, 2014 at 04:28:38PM +0900, Brian Gesiak wrote: I would be in favor of using test_i18ngrep, but it seems like this test file overwhelmingly uses test_(i18n)cmp, even when inspecting stderr output. We generally prefer cmp checks to grep checks, because they are more rigorous. However, when testing human-readable output which may change, sometimes being too specific can simply make the tests brittle and annoying. Using a forgiving regex that matches keywords can be helpful. So there's definitely some room for judgement. I think what you posted as v2 looks OK. Making double-sure that all tests pass when run with sh -x seems like a larger endeavor. Of course, I'd be happy to submit several patches if there's support for such a change. But as Peff points out it will be a large diff. Yeah, I don't think it's worth the effort. If you feel like continuing on this series, converting the warning() into a die() would be a much more productive use of time (but if you don't, I do not see any reason not to take the patches you've posted). -Peff -- 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 1/2] t3200-branch: test setting branch as own upstream
If you feel like continuing on this series, converting the warning() into a die() would be a much more productive use of time (but if you don't, I do not see any reason not to take the patches you've posted). I'd be happy to keep working on this. In fact, I think I have a patch for this ready. But just to clarify: I notice that the warning comes from install_branch_config, which gets used both for branch -u, but also in the side effect case I mentioned above. Is it possible to trigger this as part of such a case? I think maybe git branch -f --track foo foo would do it. If so, we should perhaps include a test that it does not break if we upgrade the -u case to an error. Do you mean that install_branch_config should continue to emit a warning in the side effect case? I'm not sure I agree--how is git branch -f --track foo foo less erroneous than git branch -u foo refs/heads/foo? Perhaps I'm missing some insight on how --track is used. The tests appear to already cover all instances in which install_branch_config is called, and bumping the warning to an error does not cause any test failures. - Brian Gesiak -- 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 1/2] t3200-branch: test setting branch as own upstream
On Fri, Feb 28, 2014 at 07:44:10PM +0900, Brian Gesiak wrote: I notice that the warning comes from install_branch_config, which gets used both for branch -u, but also in the side effect case I mentioned above. Is it possible to trigger this as part of such a case? I think maybe git branch -f --track foo foo would do it. If so, we should perhaps include a test that it does not break if we upgrade the -u case to an error. Do you mean that install_branch_config should continue to emit a warning in the side effect case? I'm not sure I agree--how is git branch -f --track foo foo less erroneous than git branch -u foo refs/heads/foo? Perhaps I'm missing some insight on how --track is used. I'd be more worried about triggering it via the config. E.g.: git config branch.autosetupmerge always git branch -f foo foo Should the second command die? I admit I'm having a hard time coming up with a feasible reason why anyone would do branch -f foo foo in the first place. I just don't want to regress somebody else's workflow due to my lack of imagination. -Peff -- 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 1/2] t3200-branch: test setting branch as own upstream
I just don't want to regress somebody else's workflow due to my lack of imagination. This makes a lot of sense to me, although as-is the function emits a warning and returns immediately (although with a successful status code), so I'm also stumped as to what kind of workflow this would be included in. In any case, if the jury's out on this one, I suppose the two patches I submitted are good to merge? Part of me thinks the bump from warning to error belongs in its own patch anyway. - Brian Gesiak -- 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 1/2] t3200-branch: test setting branch as own upstream
On Fri, Feb 28, 2014 at 08:16:13PM +0900, Brian Gesiak wrote: I just don't want to regress somebody else's workflow due to my lack of imagination. This makes a lot of sense to me, although as-is the function emits a warning and returns immediately (although with a successful status code), so I'm also stumped as to what kind of workflow this would be included in. I'm cc-ing Matthieu, who wrote 85e2233, which introduces the check. Its commit message says: branch: warn and refuse to set a branch as a tracking branch of itself. Previous patch allows commands like git branch --set-upstream foo foo, which doesn't make much sense. Warn the user and don't change the configuration in this case. Don't die to let the caller finish its job in such case. For those just joining us, we are focused on the final statement, and deciding whether we should die() in this case. But I am not clear what job it would want to be finishing (creating the branch, I guess, but you cannot be doing anything useful, since by definition the branch already exists and you are not changing its tip). There wasn't any relevant discussion on the list[1]. Matthieu, can you remember anything else that led to that decision? In any case, if the jury's out on this one, I suppose the two patches I submitted are good to merge? Part of me thinks the bump from warning to error belongs in its own patch anyway. Yeah, it should definitely be a separate patch on top. -Peff [1] Threads are: http://thread.gmane.org/gmane.comp.version-control.git/137297/focus=137299 and http://thread.gmane.org/gmane.comp.version-control.git/137401/focus=137403 but the discussion focused on the first part of the series. -- 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 1/2] t3200-branch: test setting branch as own upstream
Jeff King p...@peff.net writes: Don't die to let the caller finish its job in such case. [...] Matthieu, can you remember anything else that led to that decision? Not at all, unfortunately. I don't remember if I did that in case there's something like some cleanup to do or because I had something more precise in mind. A case to be carefull about is if you're using the same git branch command for multiple actions (trying --set-upstream in combination with other options). But I do not see a case where this would be possible. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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 1/2] t3200-branch: test setting branch as own upstream
On Fri, Feb 28, 2014 at 01:27:15AM -0500, Jeff King wrote: On Fri, Feb 28, 2014 at 03:17:28PM +0900, Brian Gesiak wrote: Patch is on the way, just waiting for the tests to complete. Thanks for pointing that out! Also, sorry if it's in the Makefile somewhere, but is there an easy way to run just a single test file in the t directory? You can run ./t-sh explicitly. You can also use Perl's prove command, which provides some nice-to-have features, such as exiting abnormally if your tests abort prematurely. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
[PATCH 1/2] t3200-branch: test setting branch as own upstream
From: modocache modoca...@gmail.com No test asserts that git branch -u refs/heads/my-branch my-branch emits a warning. Add a test that does so. Signed-off-by: Brian Gesiak modoca...@gmail.com --- t/t3200-branch.sh | 8 1 file changed, 8 insertions(+) diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index fcdb867..f70b96f 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -507,6 +507,14 @@ EOF test_cmp expected actual ' +test_expect_success '--set-upstream-to shows warning if used to set branch as own upstream' ' + git branch --set-upstream-to refs/heads/my13 my13 2actual + cat expected EOF +warning: Not setting branch my13 as its own upstream. +EOF + test_cmp expected actual +' + # Keep this test last, as it changes the current branch cat expect EOF $_z40 $HEAD $GIT_COMMITTER_NAME $GIT_COMMITTER_EMAIL 1117150200 + branch: Created from master -- 1.8.3.4 (Apple Git-47) -- 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 1/2] t3200-branch: test setting branch as own upstream
On Fri, Feb 28, 2014 at 12:04:18PM +0900, Brian Gesiak wrote: No test asserts that git branch -u refs/heads/my-branch my-branch emits a warning. Add a test that does so. For an operation like git branch foo origin where setting up the tracking is a side effect, a warning makes sense. But the sole purpose of the command above is to set the upstream, and we didn't do it; should this warning actually be upgraded to an error? +test_expect_success '--set-upstream-to shows warning if used to set branch as own upstream' ' + git branch --set-upstream-to refs/heads/my13 my13 2actual + cat expected EOF +warning: Not setting branch my13 as its own upstream. +EOF + test_cmp expected actual +' This should use test_i18ncmp, as the string you are matching is internationalized. -Peff -- 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 1/2] t3200-branch: test setting branch as own upstream
For an operation like git branch foo origin where setting up the tracking is a side effect, a warning makes sense. But the sole purpose of the command above is to set the upstream, and we didn't do it; should this warning actually be upgraded to an error? I agree. I originally wrote the test using test_expect_failure--imagine my surprise when the exit status was 0, despite the fact that the upstream wasn't set! This should use test_i18ncmp, as the string you are matching is internationalized. Patch is on the way, just waiting for the tests to complete. Thanks for pointing that out! Also, sorry if it's in the Makefile somewhere, but is there an easy way to run just a single test file in the t directory? - Brian Gesiak -- 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 1/2] t3200-branch: test setting branch as own upstream
On Fri, Feb 28, 2014 at 03:17:28PM +0900, Brian Gesiak wrote: For an operation like git branch foo origin where setting up the tracking is a side effect, a warning makes sense. But the sole purpose of the command above is to set the upstream, and we didn't do it; should this warning actually be upgraded to an error? I agree. I originally wrote the test using test_expect_failure--imagine my surprise when the exit status was 0, despite the fact that the upstream wasn't set! For reference, you don't want test_expect_failure here; it is about we want this to succeed, but git is currently buggy and we know it, so mark it as a failing test. You want test_must_fail to indicate a command inside a test that must exit non-zero: test_expect_success 'pointing upstream to itself fails' ' test_must_fail git branch -u ... ' I notice that the warning comes from install_branch_config, which gets used both for branch -u, but also in the side effect case I mentioned above. Is it possible to trigger this as part of such a case? I think maybe git branch -f --track foo foo would do it. If so, we should perhaps include a test that it does not break if we upgrade the -u case to an error. Patch is on the way, just waiting for the tests to complete. Thanks for pointing that out! Also, sorry if it's in the Makefile somewhere, but is there an easy way to run just a single test file in the t directory? You can run ./t-sh explicitly. -Peff -- 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 1/2] t3200-branch: test setting branch as own upstream
Am 2/28/2014 6:37, schrieb Jeff King: On Fri, Feb 28, 2014 at 12:04:18PM +0900, Brian Gesiak wrote: No test asserts that git branch -u refs/heads/my-branch my-branch emits a warning. Add a test that does so. For an operation like git branch foo origin where setting up the tracking is a side effect, a warning makes sense. But the sole purpose of the command above is to set the upstream, and we didn't do it; should this warning actually be upgraded to an error? +test_expect_success '--set-upstream-to shows warning if used to set branch as own upstream' ' +git branch --set-upstream-to refs/heads/my13 my13 2actual +cat expected EOF +warning: Not setting branch my13 as its own upstream. +EOF +test_cmp expected actual +' This should use test_i18ncmp, as the string you are matching is internationalized. More generally, stderr output shouldn't be tested with test_cmp or test_i18ncmp at all, but with grep and test_i18ngrep. The reason is that when you run the test with 'sh -x t3200* -v -i', the trace output is also in stderr, and the test_cmp/test_i18ncmp fails due to the unexpected extra text. -- Hannes -- 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 1/2] t3200-branch: test setting branch as own upstream
On Fri, Feb 28, 2014 at 07:55:25AM +0100, Johannes Sixt wrote: This should use test_i18ncmp, as the string you are matching is internationalized. More generally, stderr output shouldn't be tested with test_cmp or test_i18ncmp at all, but with grep and test_i18ngrep. The reason is that when you run the test with 'sh -x t3200* -v -i', the trace output is also in stderr, and the test_cmp/test_i18ncmp fails due to the unexpected extra text. I didn't think we bothered to make sh -x work robustly. I don't mind if we do, but git grep -E 'test_(i18n)?cmp .*err shows many potential problem spots. Hmm. Looks like it is only a problem if you are calling a shell function (since it is the shell function's trace output you are seeing). So this test would be OK as-is, but testing for an error, like: test_must_fail git branch -u foo foo 2stderr would not be, because we see the trace from test_must_fail. So some of the callsites found by my grep are actually probably fine. -Peff -- 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 1/2] t3200-branch: test setting branch as own upstream
On Fri, Feb 28, 2014 at 02:14:01AM -0500, Jeff King wrote: I didn't think we bothered to make sh -x work robustly. I don't mind if we do, but git grep -E 'test_(i18n)?cmp .*err shows many potential problem spots. Just for fun: cd t make SHELL_PATH=sh -x prove causes 326 test failures across 43 scripts. That's slightly misleading, because 200 of the failures are all in t0008, and updating one function would probably clear up all of them. -Peff -- 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 1/2] t3200-branch: test setting branch as own upstream
Hmm. Looks like it is only a problem if you are calling a shell function (since it is the shell function's trace output you are seeing). So this test would be OK as-is Indeed, this test passes when run locally, even using sh -x. I would be in favor of using test_i18ngrep, but it seems like this test file overwhelmingly uses test_(i18n)cmp, even when inspecting stderr output. Making double-sure that all tests pass when run with sh -x seems like a larger endeavor. Of course, I'd be happy to submit several patches if there's support for such a change. But as Peff points out it will be a large diff. - Brian Gesiak On Fri, Feb 28, 2014 at 4:26 PM, Jeff King p...@peff.net wrote: On Fri, Feb 28, 2014 at 02:14:01AM -0500, Jeff King wrote: I didn't think we bothered to make sh -x work robustly. I don't mind if we do, but git grep -E 'test_(i18n)?cmp .*err shows many potential problem spots. Just for fun: cd t make SHELL_PATH=sh -x prove causes 326 test failures across 43 scripts. That's slightly misleading, because 200 of the failures are all in t0008, and updating one function would probably clear up all of them. -Peff -- 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 1/2] t3200-branch: test setting branch as own upstream
Am 2/28/2014 8:14, schrieb Jeff King: I didn't think we bothered to make sh -x work robustly. I don't mind if we do, but git grep -E 'test_(i18n)?cmp .*err shows many potential problem spots. Hmm. Looks like it is only a problem if you are calling a shell function (since it is the shell function's trace output you are seeing). So this test would be OK as-is, but testing for an error, like: test_must_fail git branch -u foo foo 2stderr would not be, because we see the trace from test_must_fail. So some of the callsites found by my grep are actually probably fine. Yeah, your assessment is correct: only shell function output is affected. Some time (years?) ago, I used to run the tests on Windows with sh -x, redirected to a log file to be able to trace intermittent failures. It was distracting to find many false positives. Today, I don't do that anymore, and it is not a big deal for me, just like for anybody else ;-) Consider the topic settled. -- Hannes -- 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
