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

[Eric Wong]

Ævar Arnfjörð Bjarmason <ava...@gmail.com> wrote:
> On Fri, Apr 06 2018, Eric Wong wrote:
> > Ævar Arnfjörð Bjarmason <ava...@gmail.com> wrote:
> >
> >> --- 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.

I've always known chomp to respect the value of $/; so chomp($rec)
whould only cut out whatever $rs is, and be a no-op if $rs is undef.

> 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));

Subtle difference, that chomp() still sees $/ as "\n".
$/ is only undef inside get_record.

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

I think the chomp is necessary. In git-svn.perl /^sub get_commit_entry {:

        # <snip>
        open my $log_fh, '>', $commit_editmsg or croak $!;

        # <snip>
                $msgbuf =~ s/\s+$//s;

        # <snip>

                print $log_fh $msgbuf or croak $!;

        # <snip>
        close $log_fh or croak $!;

# Above, we ensured the contents of $commit_editmsg has no trailing newline

        if ($_edit || ($type eq 'tree')) {
                chomp(my $editor = command_oneline(qw(var GIT_EDITOR)));
                system('sh', '-c', $editor.' "$@"', $editor, $commit_editmsg);
        }

# However, $editor is likely to introduce a trailing newline

        rename $commit_editmsg, $commit_msg or croak $!;
        {
                require Encode;
                # SVN requires messages to be UTF-8 when entering the repo
                open $log_fh, '<', $commit_msg or croak $!;
                binmode $log_fh;

# chomp trailing newline introduced by $editor:

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