Bug#894997: [PATCH] git-svn: avoid warning on undef readline()
Eric Wong writes:
> Ævar Arnfjörð Bjarmason wrote:
>> See https://public-inbox.org/git/86h8oobl36@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;
That sounds quite plausible, so if there is no further discussion,
let me queue a version that does s/rs/rec/ on that line myself
(bypassing a pull from your repository at bogomips.org/).
Thanks.
Bug#894997: [PATCH] git-svn: avoid warning on undef readline()
On Fri, Apr 06 2018, Eric Wong wrote:
> Ævar Arnfjörð Bjarmason wrote:
>> On Fri, Apr 06 2018, Eric Wong wrote:
>> > Ævar Arnfjörð Bjarmason 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.
Yup, you're right. I missed that.
Bug#894997: [PATCH] git-svn: avoid warning on undef readline()
Ævar Arnfjörð Bjarmason wrote:
> On Fri, Apr 06 2018, Eric Wong wrote:
> > Ævar Arnfjörð Bjarmason 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 {:
#
open my $log_fh, '>', $commit_editmsg or croak $!;
#
$msgbuf =~ s/\s+$//s;
#
print $log_fh $msgbuf or croak $!;
#
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));
Bug#894997: [PATCH] git-svn: avoid warning on undef readline()
On Fri, Apr 06 2018, Eric Wong wrote:
> Ævar Arnfjörð Bjarmason wrote:
>> See https://public-inbox.org/git/86h8oobl36@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.
Bug#894997: [PATCH] git-svn: avoid warning on undef readline()
Ævar Arnfjörð Bjarmason wrote:
> See https://public-inbox.org/git/86h8oobl36@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;
