From: Taylor Blau <me@ttaylorr.com>
Date: Tue, 25 Jul 2023 22:38:28 GMT
Subject: Re: [PATCH v3 1/8] pack-objects: allow `--filter` without `--stdout`
Message-ID: <ZMBO5A0WxWystYyv@nand.local>
In-Reply-To: <20230724085909.3831831-2-christian.couder@gmail.com>

On Mon, Jul 24, 2023 at 10:59:02AM +0200, Christian Couder wrote:
> diff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh
> index b26d476c64..2ff3eef9a3 100755
> --- a/t/t5317-pack-objects-filter-objects.sh
> +++ b/t/t5317-pack-objects-filter-objects.sh
> @@ -53,6 +53,14 @@ test_expect_success 'verify blob:none packfile has no blobs' '
>  	! grep blob verify_result
>  '
>
> +test_expect_success 'verify blob:none packfile without --stdout' '
> +	git -C r1 pack-objects --revs --filter=blob:none mypackname >packhash <<-EOF &&
> +	HEAD
> +	EOF
> +	git -C r1 verify-pack -v "mypackname-$(cat packhash).pack" >verify_result &&
> +	! grep blob verify_result
> +'

Just a couple of style nits here. It's a little strange (for me, at
least) to see the heredoc into a git process. I wonder if it might be
clearer to write something like:

    echo HEAD >in &&
    git -C r1 pack-objects --revs --filter=blob:none $packdir/pack <in

, but I could certainly go either way on that one. I am less certain
about redirecting the output into a file "packhash", only to cat it back
out.

Do later tests depend on the existence of this file? If so, then what
you have makes sense. If not, I would recommend storing the output in a
variable, which avoids both the I/O operation, and the unnecessary "cat"
sub-process.

Thanks,
Taylor