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

From: Christian Couder <christian.couder@gmail.com>
Date: Nov 7, 2017, 06:34 UTC
Message-ID: <CAP8UFD2vk4jV7jEBx3Axd-dhfcsGSJVFFt+pumdT1j8GD_oM_w@mail.gmail.com>
In-Reply-To: <xmqqo9oehotn.fsf@gitster.mtv.corp.google.com>

On Tue, Nov 7, 2017 at 2:15 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

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

Show 7 quoted lines

> 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'";