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

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

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.

Show 5 quoted lines

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

Show 9 quoted lines

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

Show 19 quoted lines

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