Re: [PATCH 2/2] t0000: avoid masking git exit value through pipes

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 16, 2021, 10:45 UTC
Message-ID: <871r5ohiy7.fsf@evledraar.gmail.com>
In-Reply-To: <20210916023706.55760-3-carenas@gmail.com>

On Wed, Sep 15 2021, Carlo Marcelo Arenas Belón wrote:

Show 56 quoted lines

> 9af0b8dbe2 (t0000-basic: more commit-tree tests., 2006-04-26) adds
> tets for commit-tree that mask the return exit from git as described
> in a378fee5b07.
>
> Fix the tests, to avoid pipes by using instead a temporary file.
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  t/t0000-basic.sh | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> index cb87768513..545ff5af13 100755
> --- a/t/t0000-basic.sh
> +++ b/t/t0000-basic.sh
> @@ -1270,26 +1270,31 @@ test_expect_success 'no diff after checkout and git update-index --refresh' '
>  P=$(test_oid root)
>  
>  test_expect_success 'git commit-tree records the correct tree in a commit' '
> -	commit0=$(echo NO | git commit-tree $P) &&
> -	tree=$(git show --pretty=raw $commit0 |
> -		 sed -n -e "s/^tree //p" -e "/^author /q") &&
> +	echo NO | git commit-tree $P >out &&
> +	commit0=$(cat out) &&
> +	git show --pretty=raw $commit0 >out &&
> +	tree=$(cat out | sed -n -e "s/^tree //p" -e "/^author /q") &&
>  	test "z$tree" = "z$P"
>  '
>  
>  test_expect_success 'git commit-tree records the correct parent in a commit' '
> -	commit1=$(echo NO | git commit-tree $P -p $commit0) &&
> -	parent=$(git show --pretty=raw $commit1 |
> -		sed -n -e "s/^parent //p" -e "/^author /q") &&
> +	echo NO | git commit-tree $P -p $commit0 >out &&
> +	commit1=$(cat out) &&
> +	git show --pretty=raw $commit1 >out &&
> +	parent=$(cat out | sed -n -e "s/^parent //p" -e "/^author /q") &&
>  	test "z$commit0" = "z$parent"
>  '
>  
>  test_expect_success 'git commit-tree omits duplicated parent in a commit' '
> -	commit2=$(echo NO | git commit-tree $P -p $commit0 -p $commit0) &&
> -	     parent=$(git show --pretty=raw $commit2 |
> +	echo NO | git commit-tree $P -p $commit0 -p $commit0 >out &&
> +	commit2=$(cat out) &&
> +	git show --pretty=raw $commit2 >out &&
> +	parent=$(cat out |
>  		sed -n -e "s/^parent //p" -e "/^author /q" |
>  		sort -u) &&
>  	test "z$commit0" = "z$parent" &&
> -	numparent=$(git show --pretty=raw $commit2 |
> +	git show --pretty=raw $commit2 >out &&
> +	numparent=$(cat out |
>  		sed -n -e "s/^parent //p" -e "/^author /q" |
>  		wc -l) &&
>  	test $numparent = 1

Well spotted. This looks good to me sans the cat v.s. pipe to sed that was already pointed out. In addition to that (Taylor may have meant this, but not said so explicitly) it looks like you can also e.g.:

    v=$(echo foo | ...) &&
    git show ... $v

Instead of:

    echo foo | ... >out &&
    v=$(cat out) &&
    git show ... $v

But that's a small nit either way.

On the change as a whole:

For what it's worth two ways we could have avoided this sort of edge case is if my SANITIZE=leak series would e.g. save the log of leaks somewhere and scour it later, i.e. something like what Jeff King suggested in[1]. I just re-rolled it at [2], but not with that approach (but response to your comments on another thread).

I don't think that's worth doing for an intial implementation of that feature for the reasons argued in its 2/2, just say'n.

The other (and more general) way would be to resurrect my GIT_TEST_PIPEFAIL mode[3]. I just tried it now in combination with the SANITIZE=leak test mode, and it would have caught this issue[4]!

I'll see if I can re-poke the bash maintainer (Chet Ramey) about some way forward for that mode. I had an off-list discussion with him about my proposed "set -o pipefail" change back in January and he rightly pointed out that it's intended behavior, meant to catch the sort of thing that was discussed here on-list in the thread around pagers and pipefail [5].

So since writing that WIP patch I've come around to his view that "set -o pipefail" can't be changed like that in general, but perhaps he'd accept a patch for an optional configuration on top of that. I'll contact him.

1. https://lore.kernel.org/git/cover-v4-0.3-00000000000-20210907T151855Z-avarab@gmail.com/
2. https://lore.kernel.org/git/cover-v6-0.2-00000000000-20210916T085311Z-avarab@gmail.com/
3. https://lore.kernel.org/git/20210116153554.12604-12-avarab@gmail.com/
4. https://lore.kernel.org/git/cover-v4-0.3-00000000000-20210907T151855Z-avarab@gmail.com/
5. https://lore.kernel.org/git/YAG%2FvzctP4JwSp5x@zira.vinc17.org/