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

[Junio C Hamano]

Christian Couder  writes:

>>> + 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.
>
> Well if there is an unexpected EOF, then packet_bin_read() returns
> (-1, ""), so packet_read_capabilities() returns (-1, @cap) where @cap
> contains the capabilities already received. Then
> packet_read_and_check_capabilities() checks that we received all the
> capabilities we expect and dies if that is not the case. If we did
> receive all the capabilities, then
> packet_read_and_check_capabilities() still returns -1, so the caller
> may check that and die.

In other words, it happens, by accident, to stop before going very
far.  I think we'd be better off making it an explicitly checked
error.

> But yeah we could also just die in packet_read_capabilities() if $res
> is -1. I will make this change.

Good.

Thanks.

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

[Christian Couder]

On Sun, Oct 22, 2017 at 3:46 AM, Junio C Hamano  wrote:
> Christian Couder  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,

Yeah and I think it is good.

> it even allows duplicates (intended? shouldn't we be flagging it as
> an error?),

I think allowing duplicates is ok, as we allow duplicates in many
cases already, like duplicate command line options.
Or perhaps we should just warn?

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

Yeah.

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

I think that it is ok in general to just ignore capabilities we don't
know (as long as we don't advertise them). The protocol should work
using only the capabilities that both ends support.

Now if we just talk about testing, we might sometimes want to check
that one end sends only some specific capabilities. So in this case it
could be good if we could error out. On the other hand, if we test how
Git behaves when we advertise some specific capabilities, it might not
be a good idea to error out as it would break tests when Git learns a
new capability and advertise it.

In the specific case of rot13-filter.pl I think we are more in the
later case than in the former.

So I think it is ok to wait until we would really want to check that
one end sends only some specific capabilities, before we improve the
Packet.pm module to make it support that.

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

Yeah I agree.

> The reasoning and the conclusion that is consistent with
> what the code does should be described in any case.

Ok I will document all the above in the commit message.

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

Well if there is an unexpected EOF, then packet_bin_read() returns
(-1, ""), so packet_read_capabilities() returns (-1, @cap) where @cap
contains the capabilities already received. Then
packet_read_and_check_capabilities() checks that we received all the
capabilities we expect and dies if that is not the case. If we did
receive all the capabilities, then
packet_read_and_check_capabilities() still returns -1, so the caller
may check that and die.

But yeah we could also just die in packet_read_capabilities() if $res
is -1. I will make this change.

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

Ok, I have done this in my current version, that I plan to send soon.

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

Ok, I will use:

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

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

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

[Junio C Hamano]

Christian Couder  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.

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

[Christian Couder]

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

Signed-off-by: Christian Couder 
---
 t/t0021/rot13-filter.pl | 40 
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index 278fc6f534..ba18b207c6 100644
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -116,20 +116,44 @@ sub packet_initialize {
packet_flush();
 }
 
+sub packet_read_capabilities {
+   my @cap;
+   while (1) {
+   my ( $res, $buf ) = packet_bin_read();
+   return ( $res, @cap ) if ( $res != 0 );
+   unless ( $buf =~ s/\n$// ) {
+   die "A non-binary line MUST be terminated by an LF.\n"
+   . "Received: '$buf'";
+   }
+   die "bad capability buf: '$buf'" unless ( $buf =~ 
s/capability=// );
+   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;
+   foreach (@local_caps) {
+   die "'$_' capability not available" unless 
(exists($remote_caps{$_}));
+   }
+   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);
 
-foreach (@capabilities) {
-   packet_txt_write( "capability=" . $_ );
-}
-packet_flush();
 print $debug "init handshake complete\n";
 $debug->flush();
 
-- 
2.15.0.rc1.106.g7e97f58a41