Re: [PATCH v2 2/8] t0021/rot13-filter: refactor packet reading functions

[Christian Couder <christian.couder@gmail.com>]

On Tue, Nov 7, 2017 at 2:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> 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'";