[PATCH 0/4] Trivial cleanups to t3400 (rebase)

[PATCH 0/4] Trivial cleanups to t3400 (rebase)

[Ramkumar Ramachandra]

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

-- 
1.8.3.rc1.52.gc14258d

[PATCH 1/4] t3400 (rebase): don't set GIT_AUTHOR_{NAME,EMAIL}

[Ramkumar Ramachandra]

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

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index b58fa1a..a7ca2f1 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -10,10 +10,6 @@ among other things.
 '
 . ./test-lib.sh
 
-GIT_AUTHOR_NAME=author@name
-GIT_AUTHOR_EMAIL=bogus@email@address
-export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL
-
 test_expect_success 'prepare repository with topic branches' '
 	git config core.logAllRefUpdates true &&
 	echo First >A &&
-- 
1.8.3.rc1.52.gc14258d

Re: [PATCH 1/4] t3400 (rebase): don't set GIT_AUTHOR_{NAME,EMAIL}

[Eric Sunshine]

On Fri, May 10, 2013 at 10:29 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> 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(-)
>
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index b58fa1a..a7ca2f1 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -10,10 +10,6 @@ among other things.
>  '
>  . ./test-lib.sh
>
> -GIT_AUTHOR_NAME=author@name
> -GIT_AUTHOR_EMAIL=bogus@email@address

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

> -export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL
> -
>  test_expect_success 'prepare repository with topic branches' '
>         git config core.logAllRefUpdates true &&
>         echo First >A &&

Re: [PATCH 1/4] t3400 (rebase): don't set GIT_AUTHOR_{NAME,EMAIL}

[Ramkumar Ramachandra]

Eric Sunshine 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)?

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

Thanks for catching.

Re: [PATCH 1/4] t3400 (rebase): don't set GIT_AUTHOR_{NAME,EMAIL}

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Fri, May 10, 2013 at 10:29 AM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:
>> 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(-)
>>
>> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
>> index b58fa1a..a7ca2f1 100755
>> --- a/t/t3400-rebase.sh
>> +++ b/t/t3400-rebase.sh
>> @@ -10,10 +10,6 @@ among other things.
>>  '
>>  . ./test-lib.sh
>>
>> -GIT_AUTHOR_NAME=author@name
>> -GIT_AUTHOR_EMAIL=bogus@email@address
>
> 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)?
>
>> -export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL
>> -

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

[PATCH 2/4] t3400 (rebase): downcase a couple of test titles

[Ramkumar Ramachandra]

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

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index a7ca2f1..cb4234a 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -152,7 +152,7 @@ test_expect_success 'setup: recover' '
 	git checkout modechange
 '
 
-test_expect_success 'Show verbose error when HEAD could not be detached' '
+test_expect_success 'show verbose error when HEAD could not be detached' '
 	>B &&
 	test_must_fail git rebase topic 2>output.err >output.out &&
 	grep "The following untracked working tree files would be overwritten by checkout:" output.err &&
@@ -184,7 +184,7 @@ test_expect_success 'rebase -q is quiet' '
 	test ! -s output.out
 '
 
-test_expect_success 'Rebase a commit that sprinkles CRs in' '
+test_expect_success 'rebase a commit that sprinkles CRs in' '
 	(
 		echo "One"
 		echo "TwoQ"
-- 
1.8.3.rc1.52.gc14258d

[PATCH 3/4] t3400 (rebase): move lone statement into a test

[Ramkumar Ramachandra]

'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(-)

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index cb4234a..0841a12 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -154,11 +154,11 @@ test_expect_success 'setup: recover' '
 
 test_expect_success 'show verbose error when HEAD could not be detached' '
 	>B &&
+	test_when_finished rm -f B &&
 	test_must_fail git rebase topic 2>output.err >output.out &&
 	grep "The following untracked working tree files would be overwritten by checkout:" output.err &&
 	grep B output.err
 '
-rm -f B
 
 test_expect_success 'fail when upstream arg is missing and not on branch' '
 	git checkout topic &&
-- 
1.8.3.rc1.52.gc14258d

[PATCH 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE

[Ramkumar Ramachandra]

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

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

Re: [PATCH 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@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 <artagnon@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' '

Re: [PATCH 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@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?

Re: [PATCH 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> Ramkumar Ramachandra <artagnon@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?

Re: [PATCH 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE

[Ramkumar Ramachandra]

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.

Re: [PATCH 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE

[Junio C Hamano]

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

Re: [PATCH 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE

[Ramkumar Ramachandra]

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.

Re: [PATCH 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE

[Ramkumar Ramachandra]

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.

Re: [PATCH 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE

[Jonathan Nieder]

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

Re: [PATCH 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE

[Ramkumar Ramachandra]

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?

Re: [PATCH 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE

[Jonathan Nieder]

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

Re: [PATCH 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE

[Junio C Hamano]

Jonathan Nieder <jrnieder@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 ;-)

Re: [PATCH 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE

[Ramkumar Ramachandra]

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.

Re: [PATCH 4/4] t4300 (rebase): don't unnecessarily set GIT_TRACE

[Jonathan Nieder]

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