Re: [PATCH v2 2/3] t3501: don't ignore "git <cmd>" exit code

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

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

> without this change this test will become flaky e.g under
> SANITIZE=leak if some (but not all) memory leaks revealed by
> these commands, according to c419562860e

The body of the proposed log message is *not* a continuation (the
latter half) of a sentence that the title started, but should be a
full sentence on its own.  Capitalize the first word as usual.

Also, the leak check should not be the primary reason to do this
change.  We do not want to miss "git rev-parse" used in these tests
starts to fail, regardless of why they fail.  Perhaps like this
instead?

    Otherwise, 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.

> +test_cmp_rev_parse () {
> +	git rev-parse $1 >expect &&
> +	git rev-parse $2 >actual &&
> +	test_cmp expect actual
> +}

To me, it looks like we can just use test_cmp_rev that already
exists, but am I missing some subtlety?

If not (then we need to explain why in the proposed commit log
message), at least we should place $1 and $2 inside a pair of
double-quotes.  For the current callers, what they pass happen not
to need such quoting, but once we write a helper function, we are
helping _future_ callers as well, and we should be reasonably
prepared for them.

>  test_expect_success 'cherry-pick --nonsense' '
>  
>  	pos=$(git rev-parse HEAD) &&
> @@ -66,7 +72,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_cmp_rev_parse HEAD^ rename2 &&

OK.

>  	test_path_is_file opos &&
>  	grep "Add extra line at the end" opos &&
>  	git reflog -1 | grep cherry-pick
> @@ -77,7 +83,7 @@ test_expect_success 'revert after renaming branch' '
>  
>  	git checkout rename1 &&
>  	git revert added &&
> -	test $(git rev-parse HEAD^) = $(git rev-parse rename1) &&
> +	test_cmp_rev_parse HEAD^ rename1 &&

OK.

>  	test_path_is_file spoo &&
>  	! grep "Add extra line at the end" spoo &&
>  	git reflog -1 | grep revert

Thanks.