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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 1, 2022, 18:10 UTC
Message-ID: <xmqqilrswu76.fsf@gitster.g>
In-Reply-To: <20220331191525.17927-3-khalid.masum.92@gmail.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.

Show 5 quoted lines

> +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.

Show 9 quoted lines

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

Show 9 quoted lines

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