From: Taylor Blau <me@ttaylorr.com>
Date: Wed, 14 Jul 2021 04:04:10 GMT
Subject: Re: [PATCH v3 3/5] tests: replace [de]packetize() shell+perl test-tool pkt-line
Message-ID: <YO5iOqWx46UxdhKX@nand.local>
In-Reply-To: <patch-3.5-92bfd8a87b-20210714T005115Z-avarab@gmail.com>

On Wed, Jul 14, 2021 at 02:54:03AM +0200, Ævar Arnfjörð Bjarmason wrote:
> -extract_haves () {
> -	depacketize | perl -lne '/^(\S+) \.have/ and print $1'
> -}
> -
>  test_expect_success 'with core.alternateRefsCommand' '
>  	write_script fork/alternate-refs <<-\EOF &&
>  		git --git-dir="$1" for-each-ref \
> @@ -27,18 +23,40 @@ test_expect_success 'with core.alternateRefsCommand' '
>  			refs/heads/public/
>  	EOF
>  	test_config -C fork core.alternateRefsCommand ./alternate-refs &&
> -	git rev-parse public/branch >expect &&
> -	printf "0000" | git receive-pack fork >actual &&
> -	extract_haves <actual >actual.haves &&
> -	test_cmp expect actual.haves
> +
> +	test-tool pkt-line pack >in <<-\EOF &&
> +	0000
> +	EOF
> +
> +	cat >expect <<-EOF &&
> +	$(git rev-parse main) refs/heads/main
> +	$(git rev-parse base) refs/tags/base
> +	$(git rev-parse public) .have
> +	0000
> +	EOF
> +
> +	git receive-pack fork >out <in &&
> +	test-tool pkt-line unpack <out >actual &&

I don't think extracting the haves themselves (and stripping ".have"
from the output) adds much verbosity at all. Wouldn't it be:

  test-tool pkt-line unpack <out >actual &&
  perl -lne '/^(\S+) \.have/ and print $1' <actual >actual.haves

(in fact, it might be quite nice to leave extract_haves as-is changing
depacketize for 'test-tool pkt-line unpack').

For what it's worth, I also don't care enough to argue about it, but I
am curious why the change here was more invasive compared to other
locations (where depacketize was just replaced with the earlier
incantation).

If memory serves, I am the original author of this test (via 89284c1d6c
(transport.c: introduce core.alternateRefsCommand, 2018-10-08)), but the
Perl bits are Peff's from the discussion in:

    https://lore.kernel.org/git/20180922195258.GA20983@sigill.intra.peff.net/

Thanks,
Taylor