On Tue, Nov 7, 2017 at 2:15 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Christian Couder <christian.couder@xxxxxxxxx> writes:
>
>> +sub packet_required_key_val_read {
>> + my ( $key ) = @_;
>> + my ( $res, $buf ) = packet_txt_read();
>> + unless ( $res == -1 or ( $buf =~ s/^$key=// and $buf ne '' ) ) {
>> + die "bad $key: '$buf'";
>> + }
>> + return ( $res, $buf );
>> +}
>
> The function calls itself "required", but it does not die when it
> sees an unexpected EOF or an empty line.
If $res is not -1, it will die if $buf is empty.
> Neither of these cases
> gives it a key (nor val), but it is not treated as an error.
>
> That is curious, isn't it?
Yeah it is a bit strange for the unexpected EOF case.
I think I will remove "required" from the name and add a comment
before the function.
> By the way, is it just me who finds this "unless" even less
> unerstandable than the original in packet_txt_read() you modeled
> this one after? The original depended on packet_bin_read() to die
> on an unexpected EOF, so its "unless" made some sense (we know we
> got some input, and it is an error for the input not to end with LF
> unless it is an empty string). Negating the condition and making it
> "if" may make it easier to understand, perhaps. I dunno.
I can remove the "unless" and make it easier to understand like this:
if ( $res == -1 ) {
return ( $res, $buf );
}
if ( $buf =~ s/^$key=// and $buf ne '' ) ) {
return ( $res, $buf );
}
die "bad $key: '$buf'";
Or to keep it short just:
if ( $res == -1 or ( $buf =~ s/^$key=// and $buf ne '' ) ) {
return ( $res, $buf );
}
die "bad $key: '$buf'";
