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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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

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