From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Thu, 23 Sep 2010 23:16:06 GMT
Subject: Re: [PATCH V2] git-send-email.perl: Add --to-cmd
Message-ID: <AANLkTin2YvPxnbCXzWsugfaGvbUUcm6n5LtwkNVxhJfC@mail.gmail.com>
In-Reply-To: <7v62xwqe7i.fsf@alter.siamese.dyndns.org>

On Thu, Sep 23, 2010 at 22:37, Junio C Hamano <gitster@pobox.com> wrote:
> Joe Perches <joe@perches.com> writes:
>
>> +     if (defined $to_cmd) {
>> +             open(F, "$to_cmd \Q$t\E |")
>> +                     or die "(to-cmd) Could not execute '$to_cmd'";
>> +             while(<F>) {
>> +                     my $t = $_;
>
> "my $t" masks another $t in the outer scope; technically not a bug, but
> questionable as a style.
>
>> +                     $t =~ s/^\s*//g;
>> +                     $t =~ s/\n$//g;
>> +                     next if ($t eq $sender and $suppress_from);
>> +                     push @to, parse_address_line($t)
>> +                         if defined $t; # sanitized/validated later
>
> This "if defined $t" makes my head hurt.  Why?
>
>  * The "while (<F>)" loop wouldn't have given you an undef in $t in the
>   first place;
>
>  * You would have got "Use of uninitialized value" warning at these two
>   s/// statements if $t were undef; and
>
>  * Even if $t were undef, these two s/// statements would have made $t a
>   defined, empty string.

Well spotted. Also it *can't* be undef here by definition. Since $t is
taken from the $_ value which comes from the <> operator. That just
wraps readline(), which will exit the loop when it hit EOF (at which
point readline() *would* return undef).

So this whole business of checking for the definedness of $t doesn't
make any sense.