patch, 4 partst3400 (rebase): don't set GIT_AUTHOR_{NAME,EMAIL}

Hi,

I was just going through t3400 when wondering where to put the tests for rebase.autostash. Here's a set of trivial patches.

Thanks.

Ramkumar Ramachandra (4):
  t3400 (rebase): don't set GIT_AUTHOR_{NAME,EMAIL}
  t3400 (rebase): downcase a couple of test titles
  t3400 (rebase): move lone statement into a test
  t4300 (rebase): don't unnecessarily set GIT_TRACE

 t/t3400-rebase.sh | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

test-lib.sh already sets a sane GIT_AUTHOR_{NAME,EMAIL} for all test scripts to use. Don't unnecessarily duplicate the work.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t3400-rebase.sh | 4 ----
 1 file changed, 4 deletions(-)

On Fri, May 10, 2013 at 10:29 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

These values are intentionally bogus. Doesn't this change defeat the purpose of 43c2325 (am: use get_author_ident_from_commit instead of mailinfo when rebasing; 2010-06-16)?

Eric Sunshine wrote:

Oh, oops.  I paid too little attention to this series.

Thanks for catching.

Eric Sunshine <sunshine@sunshineco.com> writes:

Then perhaps there needs an in-code comment before these three lines to explain why they are deliberately set to bogus values, no?

Otherwise they stick out like sore thumbs in the test output, where all the other titles begin with a lowercase letter.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t3400-rebase.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

'rm -f B' is a lone statement that isn't contained inside any test_expect_* block. Use test_when_finished to execute it after the preceding test has finished.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t3400-rebase.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 <artagnon@gmail.com>
---
 t/t3400-rebase.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Isn't it shown in "t4300-*.sh -v" output to help the debugger?

Junio C Hamano wrote:

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?

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Perhaps because this is a test about "rebase" and a typical debugger does not want to trace other "git" things while debugging this?

Junio C Hamano wrote:

Okay, let's drop this 4-part series: it's too minor.

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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.

Junio C Hamano wrote:

I like comments, but lately I've seen nothing but reviews stripping my comments.

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.

Ramkumar Ramachandra wrote:

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.

Ramkumar Ramachandra 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.

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

Jonathan Nieder wrote:

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?

Ramkumar Ramachandra wrote:

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

Jonathan Nieder <jrnieder@gmail.com> writes:

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 ;-)

Junio C Hamano wrote:

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.

Ramkumar Ramachandra wrote:

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