Re: [PATCH v3 1/1] t3501: remove redundant file check and stop ignoring git <cmd> exit code

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

Labnann <khalid.masum.92@gmail.com> writes:

> In the test 'cherry-pick after renaming branch', stop checking for
> the presence of a file (opos) because we are going to "grep" in it in
> the same test and the lack of it will be noticed as a failure anyway.

OK.

> In the test 'revert after renaming branch', instead of allowing any
> random contents as long as a known phrase is not there in it, we can
> expect the exact outcome---after the successful revert of
> "added", the contents of file "spoo" should become identical to
> what was in file "oops" in the "initial" commit.

Makes sense.  The code removes "test -f spoo", which is not
explained in the above, and I do not think we want to.  We'd
notice a breakage where revert leaves the right result in HEAD
but fails to match the working tree files if we leave it there.

> In both tests, we would not notice when "git rev-parse" starts
> segfaulting without emitting any output.  The 'test' command would
> end up being just "test =", which yields success. Therefore we could
> use test_cmp_rev

The two sentences are right.  The conclusion is a bit iffy; it is
not "we could", which implies that the current one is OK but it is
OK to also rewrite it to use test_cmp_rev.

    Use the 'test_cmp_rev' helper to make sure we will notice such a
    breakage.

or something like that, perhaps?

> Signed-off-by: Labnann <khalid.masum.92@gmail.com>

Is Labnann your name?  I do not know where you are from so forgive
me if it is.  We prefer people to sign with their real names (cf.
Documentation/SubmittingPatches[[real-name]]) on this line (and
because the name/e-mail on this line must match the author's ident,
the same preference applies there, too).

> ---
>  t/t3501-revert-cherry-pick.sh | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index 8617efaaf1..ad8f0cae5a 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -66,8 +66,7 @@ test_expect_success 'cherry-pick after renaming branch' '
>  
>  	git checkout rename2 &&
>  	git cherry-pick added &&
> -	test $(git rev-parse HEAD^) = $(git rev-parse rename2) &&
> -	test -f opos &&
> +	test_cmp_rev rename2 HEAD^ &&
>  	grep "Add extra line at the end" opos &&
>  	git reflog -1 | grep cherry-pick
>  
> @@ -77,9 +76,8 @@ test_expect_success 'revert after renaming branch' '
>  
>  	git checkout rename1 &&
>  	git revert added &&
> -	test $(git rev-parse HEAD^) = $(git rev-parse rename1) &&
> -	test -f spoo &&
> -	! grep "Add extra line at the end" spoo &&
> +	test_cmp_rev rename1 HEAD^ &&
> +	test_cmp_rev initial:oops HEAD:spoo &&
>  	git reflog -1 | grep revert
>  
>  '

The diff looks almost perfect here (modulo that "test -f spoo" is
better kept, I would think).

Thanks.