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

[Eric Wong <e@xxxxxxxxx>]

Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote:
> On Fri, Apr 06 2018, Eric Wong wrote:
> > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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));