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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 22, 2017, 01:12 UTC
Message-ID: <xmqqvaj8dlyz.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171019123030.17338-5-chriscool@tuxfamily.org>

Christian Couder <christian.couder@gmail.com> writes:

Show 6 quoted lines

> +sub packet_initialize {
> +	my ($name, $version) = @_;
> +
> +	( packet_txt_read() eq ( 0, $name . "-client" ) )       || die "bad initialize";
> +	( packet_txt_read() eq ( 0, "version=" . $version ) )   || die "bad version";
> +	( packet_bin_read() eq ( 1, "" ) )                      || die "bad version end";

This is not a new issue and it is a bit surprising that nobody noticed when edcc8581 ("convert: add filter.<driver>.process option", 2016-10-16) was reviewed, but the above is quite broken. packet_txt_read() returns a 2-element list, and on the right hand side of "eq" also has a list with (two, elements), but this takes the last element of the list on each side, and compares them. The elading 0/1 we see above do not matter, which means we do not require to see a flush at the end of the version---a simple empty string or an EOF would do, which is definitely not what we want.

	#!/usr/bin/perl
	sub p {
		my ($res, $buf) = @_;
		return ($res, $buf);
	}
	if (p(0, "") eq (-1, 2, 3, "")) { print "ok\n"; }

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).

Thanks.