Re: [PATCH 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE
Ramkumar Ramachandra wrote: I've never really found the outputs from earlier tests enlightening. If the test suite would automatically use set -x when appropriate so output for each command was preceded by the command being run, that would presumably make the verbose output more useful. If you are not in a situation where it's difficult to debug interactively, I doubt you'll find anything better than reproducing the bug by hand and exploring. I suppose an option Run up to this test, stop, and tell me the $TRASH_DIRECTORY and next test so I can try it manually could be a way to simplify that workflow. Regards, Jonathan -- 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 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE
Junio C Hamano wrote: But the output from passing -v before the test that breaks is not very useful for two reasons. I sometimes checkout the Good branch in a different worktree, compare the output/ state of the passing test with the failing one. I've never really found the outputs from earlier tests enlightening. From my experience, the failure is often due to an earlier test not imposing tighter passing conditions: but because it's shell, the debugging time is very small. I always just patch-locally and run. I'm not sure how to make the testing framework more useful. -- 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 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE
A couple of tests execute 'git rebase' with GIT_TRACE set to 1, but this trace output is not used anywhere. Remove it, since it is not relevant to what we are testing. Signed-off-by: Ramkumar Ramachandra artag...@gmail.com --- t/t3400-rebase.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 0841a12..d0d9442 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -137,13 +137,13 @@ test_expect_success 'rebase a single mode change' ' test_chmod +x A test_tick git commit -m modechange - GIT_TRACE=1 git rebase master + git rebase master ' test_expect_success 'rebase is not broken by diff.renames' ' test_config diff.renames copies git checkout filemove - GIT_TRACE=1 git rebase force-3way + git rebase force-3way ' test_expect_success 'setup: recover' ' -- 1.8.3.rc1.52.gc14258d -- 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 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE
Ramkumar Ramachandra artag...@gmail.com writes: A couple of tests execute 'git rebase' with GIT_TRACE set to 1, but this trace output is not used anywhere. Isn't it shown in t4300-*.sh -v output to help the debugger? relevant to what we are testing. Signed-off-by: Ramkumar Ramachandra artag...@gmail.com --- t/t3400-rebase.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 0841a12..d0d9442 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -137,13 +137,13 @@ test_expect_success 'rebase a single mode change' ' test_chmod +x A test_tick git commit -m modechange - GIT_TRACE=1 git rebase master + git rebase master ' test_expect_success 'rebase is not broken by diff.renames' ' test_config diff.renames copies git checkout filemove - GIT_TRACE=1 git rebase force-3way + git rebase force-3way ' test_expect_success 'setup: recover' ' -- 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 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE
Junio C Hamano wrote: Ramkumar Ramachandra artag...@gmail.com writes: A couple of tests execute 'git rebase' with GIT_TRACE set to 1, but this trace output is not used anywhere. Isn't it shown in t4300-*.sh -v output to help the debugger? Um, but why the GIT_TRACE in just these two places? Can't I set GIT_TRACE=1 when executing the shell script if I really wanted that? -- 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 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE
Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: Ramkumar Ramachandra artag...@gmail.com writes: A couple of tests execute 'git rebase' with GIT_TRACE set to 1, but this trace output is not used anywhere. Isn't it shown in t4300-*.sh -v output to help the debugger? Um, but why the GIT_TRACE in just these two places? Can't I set GIT_TRACE=1 when executing the shell script if I really wanted that? Perhaps because this is a test about rebase and a typical debugger does not want to trace other git things while debugging this? -- 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 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE
Junio C Hamano wrote: Perhaps because this is a test about rebase and a typical debugger does not want to trace other git things while debugging this? Okay, let's drop this 4-part series: it's too minor. -- 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 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE
Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: Perhaps because this is a test about rebase and a typical debugger does not want to trace other git things while debugging this? Okay, let's drop this 4-part series: it's too minor. Why throw the baby with bathwater? To me, most of them look like responses to valid issues, and that holds true even for [PATCH 1/4]. Even though your response may have been an incorrect one, the issue that triggered the response is still valid---the setting of these variables without explanation invites curiosities and a mistake similar to what you made in that patch. If the patch were to consistently remove GIT_TRACE=1 placed on git rebase from all test scripts that do not check the trace output consistently (i.e. 3400, 3402, and 7402), with a different justification, e.g. whoever wants to debug a specific part of the test can add GIT_TRACE=1 before running the test with -v, but it should not be committed, the change would be very reasonable, I would think. -- 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 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE
Junio C Hamano wrote: To me, most of them look like responses to valid issues, and that holds true even for [PATCH 1/4]. Even though your response may have been an incorrect one, the issue that triggered the response is still valid---the setting of these variables without explanation invites curiosities and a mistake similar to what you made in that patch. I like comments, but lately I've seen nothing but reviews stripping my comments. If the patch were to consistently remove GIT_TRACE=1 placed on git rebase from all test scripts that do not check the trace output consistently (i.e. 3400, 3402, and 7402), with a different justification, e.g. whoever wants to debug a specific part of the test can add GIT_TRACE=1 before running the test with -v, but it should not be committed, the change would be very reasonable, I would think. Quite frankly, I think -v is completely useless; who likes to scroll through pages of terminal output? If there are really users of this thing, why haven't they got it to auto-pager yet? I just comment out the test_expect_success and close-quote, and put a test_done after it. I would never advocate this GIT_TRACE thing anywhere, because I want to put GIT_TRACE=1 (and possibly other modifications) where I want it. Locally. -- 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 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE
Ramkumar Ramachandra wrote: I just comment out the test_expect_success and close-quote, and put a test_done after it. I would never advocate this GIT_TRACE thing anywhere, because I want to put GIT_TRACE=1 (and possibly other modifications) where I want it. Locally. On that note, I'd really like a switch that runs until the first failing test and re-runs just that failing test with -v. -- 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 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE
Ramkumar Ramachandra wrote: Quite frankly, I think -v is completely useless; who likes to scroll through pages of terminal output? I use -v -i together quite frequently when debugging. I also use -v automatically to debug test failures when tests are invoked automatically on machines I do not have access to. Please don't break it. No particular opinion about the use of GIT_TRACE here: I haven't looked at it closely (and in particular I didn't look up the purpose of the commit that added it, so I can't really judge). Thanks, Jonathan -- 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 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE
Jonathan Nieder wrote: I use -v -i together quite frequently when debugging. I also use -v automatically to debug test failures when tests are invoked automatically on machines I do not have access to. Yeah, it makes sense on remote machines. I just found out about -i, and the -v -i combination is useful (and the autopager makes no sense in this case); no doubt. Can we do better by not printing the -v output of the passing tests though? -- 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 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE
Ramkumar Ramachandra wrote: Can we do better by not printing the -v output of the passing tests though? Not for my use. The output from comprable tests before is often useful for comparison. I wouldn't be against such an option for people who want it, though. Thanks, Jonathan -- 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 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE
Jonathan Nieder jrnie...@gmail.com writes: Ramkumar Ramachandra wrote: Can we do better by not printing the -v output of the passing tests though? Not for my use. The output from comprable tests before is often useful for comparison. Perhaps. But the output from passing -v before the test that breaks is not very useful for two reasons. - Often they are identical between passing and failing runs because the resulting states in their trash directories are truly the same, in which case the output is just being too noisy. - Some other times, the output from passing and failing runs look identical but only because earlier tests are sloppy and not showing differences that happens to matter as precondition to the failing test, in which case the output is too sketchy to be useful. In the latter case, where a breakage needs deeper inspection, I haven't found a way better than to stop the test _before_ the broken one (by temporarily inserting exit just before it), run it once again so that the trash directory just before the failing test is left behind, and go there to manually inspect the states in the trash directories of passing and failing ones to compare where in the earlier tests that passed left a subtle differences that were not detected but still caused to make a difference to the broken test. I am not saying it is not very useful, so remove it but it is not very useful, and I wish somebody clever thinks of a way to make it more useful, though ;-) -- 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
