Re: [PATCH] git-svn: avoid warning on undef readline()

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 6, 2018, 18:23 UTC
Message-ID: <87y3i0yz1t.fsf@evledraar.gmail.com>
In-Reply-To: <20180406165618.GA6367@80x24.org>

On Fri, Apr 06 2018, Eric Wong wrote:

Show 19 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> See https://public-inbox.org/git/86h8oobl36.fsf@phe.ftfl.ca/ for the
>> original report.
>
> Thanks for taking a look at this.  Also https://bugs.debian.org/894997
>
>> --- a/perl/Git.pm
>> +++ b/perl/Git.pm
>> @@ -554,7 +554,7 @@ sub get_record {
>>  	my ($fh, $rs) = @_;
>>  	local $/ = $rs;
>>  	my $rec = <$fh>;
>> -	chomp $rec if defined $rs;
>> +	chomp $rec if defined $rs and defined $rec;
>
> I'm struggling to understand the reason for the "defined $rs"
> check.  I think it was a braino on my part and meant to use:
>
> 	chomp $rec if defined $rec;

Whether this makes any sense is another question, but you seem to have explicitly meant this at the time. The full function definition with documentation:

    =item get_record ( FILEHANDLE, INPUT_RECORD_SEPARATOR )

    Read one record from FILEHANDLE delimited by INPUT_RECORD_SEPARATOR,
    removing any trailing INPUT_RECORD_SEPARATOR.

    =cut

    sub get_record {
    	my ($fh, $rs) = @_;
    	local $/ = $rs;
    	my $rec = <$fh>;
    	chomp $rec if defined $rs;
    	$rec;
    }

It doesn't make to remove the trailing record separator if it's not defined, otherwise we'd be coercing undef to "\n" while at the same time returning multiple records. But then of course the only user of this with an "undef" argument just does:

    chomp($log_entry{log} = get_record($log_fh, undef));

So we could also remove that chomp(), adn not check defined $rs, but IMO it's cleaner & more consistent this way.