Re: [PATCH 5/6] t0021/rot13-filter: add capability functions

[Junio C Hamano <gitster@pobox.com>]

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

> Add functions to help read and write capabilities.
> These functions will be reused in following patches.

One more thing that is more noteworthy (read: do not forget to
describe it in the proposed log message) is that the original used
to require capabilities to come in a fixed order.

The new code allows these capabilities to be declared in any order,
it even allows duplicates (intended? shouldn't we be flagging it as
an error?), the helper can require a set of capabilities we do want
to see and fail if the remote doesn't declare any one of them
(good).

It does not check if the remote declares any capability we do not
know about (intended? the original noticed this situation and error
out---shouldn't the more generalized helper that is meant to be
reusable allow us to do so, too?).

	Side note: my answer to the last question is "it is OK and
	even desirable to ignore the fact that they claim to support
	a capability we do not know about", but I may be mistaken.
	The reasoning and the conclusion that is consistent with
	what the code does should be described in any case.

> +sub packet_read_capabilities {
> +	my @cap;
> +	while (1) {
> +		my ( $res, $buf ) = packet_bin_read();
> +		return ( $res, @cap ) if ( $res != 0 );

The original had the same "'list eq list' does not do what you may
think it does" issue.  This one corrects it, which is good.

I am not sure if ($res != 0) is correct though.  What should happen
when you get an unexpected EOF at this point?  The original would
have died; this ignores and continues.

> +		unless ( $buf =~ s/\n$// ) {
> +			die "A non-binary line MUST be terminated by an LF.\n"
> +			    . "Received: '$buf'";
> +		}

It may make sense to extract this in a small helper and call it from
here and from packet_txt_read().

> +		die "bad capability buf: '$buf'" unless ( $buf =~ s/capability=// );

This may merely be a style thing, but I somehow find statement
modifiers hard to follow, unless its condition is almost always
true.  If you follow the logic in a loop and see "die" at the
beginning, a normal thing to expect is that there were conditionals
that said "continue" (eh, 'next' or 'redo') to catch the normal case
and the control would reach "die" only under exceptional error
cases, but hiding a rare error condition after 'unless' statement
modifier breaks that expectation.

> +		push @cap, $buf;
> +	}
> +}
> +
> +sub packet_read_and_check_capabilities {
> +	my @local_caps = @_;
> +	my @remote_res_caps = packet_read_capabilities();
> +	my $res = shift @remote_res_caps;
> +	my %remote_caps = map { $_ => 1 } @remote_res_caps;

FYI:

	my ($res, @remote_caps) = packet_read_capabilities();
	my %remote_caps = map { $_ => 1 } @remote_caps;

may be more conventional way.

> +	foreach (@local_caps) {
> +        	die "'$_' capability not available" unless (exists($remote_caps{$_}));
> +	}

It is good that we can now accept capabilities in any order and
still enforce all the required capabilities are supported by the
other side.  It deserves a mention in the proposed log message.

> +	return $res;
> +}
> +
> +sub packet_write_capabilities {
> +	packet_txt_write( "capability=" . $_ ) foreach (@_);
> +	packet_flush();
> +}
> +
>  print $debug "START\n";
>  $debug->flush();
>  
>  packet_initialize("git-filter", 2);
>  
> -( 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";
> +packet_read_and_check_capabilities("clean", "smudge", "delay");
> +packet_write_capabilities(@capabilities);

Neither the original nor the rewrite ensures that @capabilities we
ask to the other side to activate is a subset of what the other side
actually declared.

Fixing this is a bit more involved than "refactor what we have", but
probably is part of "refactor so that we can reuse in other
situations".  You'd want to return the list of caps received from
packet_read_and_check_capabilities() and make sure @capabilities is
a subset of that before writing them out to the other side to
request.

Thanks.