Re: [PATCH v2] t1410-reflog.sh: avoid suppressing git's exit code in pipelines

[Junio C Hamano <gitster@pobox.com>]

Gatla Vishweshwar Reddy <gatlavishweshwarreddy26@gmail.com> writes:

> Piping git commands directly to wc -l suppresses the exit code of
> git, hiding potential failures from the test suite. Capture the
> output to a temporary file first, then count the lines separately
> to preserve the exit code. Where the expected count is known ahead
> of time, use test_stdout_line_count instead.
>
> Signed-off-by: Gatla Vishweshwar Reddy <gatlavishweshwarreddy26@gmail.com>
> ---
>  t/t1410-reflog.sh | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)

The above descripotion looks reasonble.

By the way, Documentation/SubmittingPatches has this:

    Before sending another version, make sure you have answered
    meaningful review comments in the existing discussion.  Also
    give reviewers enough time to comment before sending another
    version.

> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
> index ce71f9a30a..8e018d172b 100755
> --- a/t/t1410-reflog.sh
> +++ b/t/t1410-reflog.sh
> @@ -244,26 +244,30 @@ test_expect_success 'delete' '
>  	test_tick &&
>  	git commit -m tiger C &&
>  
> -	HEAD_entry_count=$(git reflog | wc -l) &&
> -	main_entry_count=$(git reflog show main | wc -l) &&
> -
> -	test $HEAD_entry_count = 5 &&
> -	test $main_entry_count = 5 &&
> -
> +	test_stdout_line_count = 5 git reflog &&
> +	git reflog >reflog_output &&
> +	HEAD_entry_count=$(wc -l <reflog_output) &&
> +	test_stdout_line_count = 5 git reflog show main &&
> +	git reflog show main >reflog_main_output &&
> +	main_entry_count=$(wc -l <reflog_main_output) &&
>  
>  	git reflog delete main@{1} &&
>  	git reflog show main > output &&
>  	test_line_count = $(($main_entry_count - 1)) output &&
> -	test $HEAD_entry_count = $(git reflog | wc -l) &&
> +	git reflog >reflog_output &&
> +	test $HEAD_entry_count = $(wc -l <reflog_output) &&
>  	! grep ox < output &&

Now, you no longer have new consecutive blank lines in the above,
but the above shares the same "what did the author meant to convey
with this blank line?" puzzlement.

The updated code somehow wanders around in many directions like a
drunken man.  Let's comment on each line.

> +	test_stdout_line_count = 5 git reflog &&

This is "Does the reflog for HEAD have exactly 5 entries?" test.

> +	git reflog >reflog_output &&
> +	HEAD_entry_count=$(wc -l <reflog_output) &&

As we already saw that HEAD_entry_count variable is exactly equal to
5, it is puzzling why we want to perform this computation again and
assign the result to the variable.

> +	test_stdout_line_count = 5 git reflog show main &&

And then we check "Does the reflog for 'main' have exactly 5
entries?"

> +	git reflog show main >reflog_main_output &&
> +	main_entry_count=$(wc -l <reflog_main_output) &&

And recompute what we already know and asssign to main_entry_count
variable, which shares the same puzzlement.

>  
>  	git reflog delete main@{1} &&
>  	git reflog show main > output &&
>  	test_line_count = $(($main_entry_count - 1)) output &&

Now, after a blank line, it goes on to test a completely different
thing, which is "after deleting an entry in main's reflog, can we
count how many there is, and does it match what we expect, which is
the previous count minus 1"?  Why should we even need to do so, when

	git reflog delete main@{1} &&
	test_stdout_line_count = 4 git reflog show main &&

would do just fine?

> -	test $HEAD_entry_count = $(git reflog | wc -l) &&
> +	git reflog >reflog_output &&
> +	test $HEAD_entry_count = $(wc -l <reflog_output) &&

And then it comes back to test what we already know, i.e. "does the
reflog for HEAD have 5 entries?".  Which we tested earlier already.

Are we interested in checking that "reflog delete main@{1}" does
not affect the reflog for HEAD?  If so, doing

	test_stdout_line_count = 5 git reflog &&

again here would be simpler, no?  That way, there is no need to
recompute and assign to the {HEAD,main}_entry_count variables in the
earlier part of the tests.

I guess the same comment applies to the remainder of this test,
where it is checked that a removal from HEAD reflog does not affect
the reflog of main.



>  	main_entry_count=$(wc -l < output) &&
>  
>  	git reflog delete HEAD@{1} &&
> -	test $(($HEAD_entry_count -1)) = $(git reflog | wc -l) &&
> -	test $main_entry_count = $(git reflog show main | wc -l) &&
> +	git reflog >reflog_output &&
> +	test $(($HEAD_entry_count -1)) = $(wc -l <reflog_output) &&
> +	git reflog show main >reflog_main_output &&
> +	test $main_entry_count = $(wc -l <reflog_main_output) &&
>  
> -	HEAD_entry_count=$(git reflog | wc -l) &&
> +	git reflog >reflog_output &&
> +	HEAD_entry_count=$(wc -l <reflog_output) &&
>  
>  	git reflog delete main@{07.04.2005.15:15:00.-0700} &&
>  	git reflog show main > output &&
> @@ -319,13 +323,12 @@ test_expect_success 'git reflog expire unknown reference' '
>  	test_must_fail git reflog expire does-not-exist 2>stderr &&
>  	test_grep "error: reflog could not be found: ${SQ}does-not-exist${SQ}" stderr
>  '
> -
>  test_expect_success 'checkout should not delete log for packed ref' '
> -	test $(git reflog main | wc -l) = 4 &&
> +	test_stdout_line_count = 4 git reflog main &&
>  	git branch foo &&
>  	git pack-refs --all &&
>  	git checkout foo &&
> -	test $(git reflog main | wc -l) = 4
> +	test_stdout_line_count = 4 git reflog main
>  '
>  
>  test_expect_success 'stale dirs do not cause d/f conflicts (reflogs on)' '