Re: [PATCH 4/6] t0021/rot13-filter: add packet_initialize()

From: Lars Schneider <larsxschneider@gmail.com>
Date: Oct 28, 2017, 14:59 UTC
Message-ID: <09393EDB-19A3-4244-A88F-68EDA8470898@gmail.com>
In-Reply-To: <xmqqr2tpcn6g.fsf@gitster.mtv.corp.google.com>

Show 18 quoted lines

> On 27 Oct 2017, at 04:57, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> It is fine to leave the original code broken at this step while we
>> purely move the lines around, and hopefully this will be corrected
>> in a later step in the series (I am responding as I read on, so I do
>> not yet know at which patch that happens in this series).
> 
> Actually, I think you'd probably be better off if you fixed these
> broken comparisions that does (@list1 eq @list2) very early in the
> series, perhaps as [PATCH 0.8/6].  
> 
> I am sure Perl experts among us on the list can come up with a
> cleaner and better implementation of compare_lists sub I am adding
> here, but in the meantime, here is what I would start with if I were
> working on this topic.
>

Thanks for spotting this! I had some trouble applying the patch in this email. I got an error about "corrupt patch at line 54"

BTW: I am using this little snippet to apply patches from the mailing:

    PATCH=$(curl -L --silent https://public-inbox.org/git/xmqqr2tpcn6g.fsf@gitster.mtv.corp.google.com/raw); 
    ((printf '%s' "$PATCH" | git am -3) || (git am --abort; printf '%s' "$PATCH" | git apply)) && 
    echo && echo "Patch successfully applied"

Does this look sensible to you?

I applied the patch "manually" and the result looks good to me. The patch looks good to me, too, but I am by no means a Perl expert.

Thanks, Lars

Show 59 quoted lines

> Thanks.
> 
> t/t0021/rot13-filter.pl | 35 ++++++++++++++++++++++++++++-------
> 1 file changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
> index ad685d92f8..9bf5a756af 100644
> --- a/t/t0021/rot13-filter.pl
> +++ b/t/t0021/rot13-filter.pl
> @@ -107,21 +107,42 @@ sub packet_flush {
> 	STDOUT->flush();
> }
> 
> +sub compare_lists {
> +	my ($expect, @result) = @_;
> +	my $ix;
> +	if (scalar @$expect != scalar @result) {
> +		return undef;
> +	}
> +	for ($ix = 0; $ix < $#result; $ix++) {
> +		if ($expect->[$ix] ne $result[$ix]) {
> +			return undef;
> +		}
> +	}
> +	return 1;
> +}
> +
> print $debug "START\n";
> $debug->flush();
> 
> -( packet_txt_read() eq ( 0, "git-filter-client" ) ) || die "bad initialize";
> -( packet_txt_read() eq ( 0, "version=2" ) )         || die "bad version";
> -( packet_bin_read() eq ( 1, "" ) )                  || die "bad version end";
> +compare_lists([0, "git-filter-client"], packet_txt_read()) ||
> +	die "bad initialize";
> +compare_lists([0, "version=2"], packet_txt_read()) ||
> +	die "bad version";
> +compare_lists([1, ""], packet_bin_read()) ||
> +	die "bad version end";
> 
> packet_txt_write("git-filter-server");
> packet_txt_write("version=2");
> packet_flush();
> 
> -( packet_txt_read() eq ( 0, "capability=clean" ) )  || die "bad capability";
> -( packet_txt_read() eq ( 0, "capability=smudge" ) ) || die "bad capability";
> -( packet_txt_read() eq ( 0, "capability=delay" ) )  || die "bad capability";
> -( packet_bin_read() eq ( 1, "" ) )                  || die "bad capability end";
> +compare_lists([0, "capability=clean"], packet_txt_read()) ||
> +	die "bad capability";
> +compare_lists([0, "capability=smudge"], packet_txt_read()) ||
> +	die "bad capability";
> +compare_lists([0, "capability=delay"], packet_txt_read()) ||
> +	die "bad capability";
> +compare_lists([1, ""], packet_bin_read()) ||
> +	die "bad capability end";
> 
> foreach (@capabilities) {
> 	packet_txt_write( "capability=" . $_ );