git 2.17.0: uninitialized value $rec in scalar chomp at ...Git.pm line 557

PATCH In review Joseph Mingrone 6 participants 11 messages Apr 6, 2018 – Apr 9, 2018

lore

Share & tools

lore

Joseph Mingrone

author

2018-04-06 12:01 UTC #0 lore raw

Hello,

After upgrading to version 2.17.0, this message appears repeatedly when running git svn rebase:

Use of uninitialized value $rec in scalar chomp at /usr/local/lib/perl5/site_perl/Git.pm line 557, <$fh> chunk 1.

The value of chunk varies. For example the message above may end with '...<$fh> chunk 5.' This is with the FreeBSD packages git-2.17.0 and perl 5.26.1.

Regards,

Johannes Schindelin

2018-04-06 12:22 UTC #1 lore raw

Hi Joseph,

Quoted 8 lines · Joseph Mingrone

After upgrading to version 2.17.0, this message appears repeatedly when running git svn rebase:
Use of uninitialized value $rec in scalar chomp at /usr/local/lib/perl5/site_perl/Git.pm line 557, <$fh> chunk 1.
The value of chunk varies. For example the message above may end with '...<$fh> chunk 5.' This is with the FreeBSD packages git-2.17.0 and perl 5.26.1.

Does this stop the rebase?

Also: could you publish the unrebased branch so that others can reproduce the error? (I am not claiming that I have enough time; I don't...)

Thanks,
Johannes

Joseph Mingrone

author

2018-04-06 12:32 UTC #2 lore raw

Quoted 13 lines · Johannes Schindelin

Hi Joseph,
Quoted 8 lines · Joseph Mingrone
After upgrading to version 2.17.0, this message appears repeatedly when running git svn rebase:
Use of uninitialized value $rec in scalar chomp at /usr/local/lib/perl5/site_perl/Git.pm line 557, <$fh> chunk 1.
The value of chunk varies. For example the message above may end with '...<$fh> chunk 5.' This is with the FreeBSD packages git-2.17.0 and perl 5.26.1.

Quoted 7 lines

Does this stop the rebase?
Also: could you publish the unrebased branch so that others can reproduce the error? (I am not claiming that I have enough time; I don't...)
Thanks,
Johannes

Hi Johannes,

No, this does not stop the svn rebase. This is with the FreeBSD ports tree, svn.freebsd.org/ports, following the setup described at https://wiki.freebsd.org/GitWorkflow/GitSvn .

Ævar Arnfjörð Bjarmason

2018-04-06 13:15 UTC #3 lore raw

PATCH 1/1 git-svn: avoid warning on undef readline()

In reply to Joseph Mingrone

Change code in Git.pm that sometimes calls chomp() on undef to only do so the value is defined.

This code has been chomping undef values ever since it was added in b26098fc2f ("git-svn: reduce scope of input record separator change", 2016-10-14), but started warning due to the introduction of "use warnings" to Git.pm in my f0e19cb7ce ("Git.pm: add the "use warnings" pragma", 2018-02-25) released with 2.17.0.

Since this function will return undef in those cases it's still possible that the code using it will warn if it does a chomp of its own, as the code added in b26098fc2f ("git-svn: reduce scope of input record separator change", 2016-10-14) might do, but since git-svn has "use warnings" already that's clearly not a codepath that's going to warn.

See https://public-inbox.org/git/[email protected]/ for the original report.

Reported-by Joseph Mingrone <[email protected]>

Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>

---
 perl/Git.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

perl/Git.pm +1 -1

index 16ebcc612c..6b6079c456 100644
@@ -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;
 	$rec;
 }

Signature · 1 line

2.17.0.rc1.321.gba9d0f2565

Johannes Schindelin

2018-04-06 14:31 UTC #4 lore raw

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

Hi Ævar,

Quoted 30 lines · Ævar Arnfjörð Bjarmason

Change code in Git.pm that sometimes calls chomp() on undef to only do so the value is defined.
This code has been chomping undef values ever since it was added in b26098fc2f ("git-svn: reduce scope of input record separator change", 2016-10-14), but started warning due to the introduction of "use warnings" to Git.pm in my f0e19cb7ce ("Git.pm: add the "use warnings" pragma", 2018-02-25) released with 2.17.0.
Since this function will return undef in those cases it's still possible that the code using it will warn if it does a chomp of its own, as the code added in b26098fc2f ("git-svn: reduce scope of input record separator change", 2016-10-14) might do, but since git-svn has "use warnings" already that's clearly not a codepath that's going to warn.
See https://public-inbox.org/git/[email protected]/ for the original report.
Reported-by Joseph Mingrone <[email protected]>
Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>
---
 perl/Git.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
perl/Git.pm +0 -0
index 16ebcc612c..6b6079c456 100644

perl/Git.pm sub get_record { · line 559

Quoted 7 lines

perl/Git.pm +1 -1

@@ -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;
 	$rec;
 }

The explanation as well as the patch make a total lot of sense to me.

Ciao,
Dscho

Eric Wong

2018-04-06 16:56 UTC #5 lore raw

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

In reply to Ævar Arnfjörð Bjarmason

Quoted 2 lines · Ævar Arnfjörð Bjarmason

See https://public-inbox.org/git/[email protected]/ for the original report.

Thanks for taking a look at this. Also https://bugs.debian.org/894997

perl/Git.pm sub get_record { · line 557

Quoted 8 lines

perl/Git.pm +1 -1

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

Ævar Arnfjörð Bjarmason

2018-04-06 18:23 UTC #6 lore raw

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

Quoted 18 lines · Eric Wong

Quoted 2 lines · Ævar Arnfjörð Bjarmason
See https://public-inbox.org/git/[email protected]/ for the original report.
Thanks for taking a look at this. Also https://bugs.debian.org/894997
perl/Git.pm sub get_record { · line 557
Quoted 8 lines
perl/Git.pm +1 -1
@@ -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:

Quoted 1 line

	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.

Eric Wong

2018-04-06 20:49 UTC #7 lore raw

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

Quoted 17 lines · Ævar Arnfjörð Bjarmason

Quoted 14 lines · Eric Wong
perl/Git.pm sub get_record { · line 557
Quoted 8 lines · Ævar Arnfjörð Bjarmason
perl/Git.pm +1 -1
@@ -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:
Quoted 1 line
	chomp $rec if defined $rec;

Quoted 8 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

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

Ævar Arnfjörð Bjarmason

2018-04-06 21:05 UTC #8 lore raw

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

Quoted 22 lines · Eric Wong

Quoted 21 lines · Ævar Arnfjörð Bjarmason
Quoted 14 lines · Eric Wong
perl/Git.pm sub get_record { · line 557
Quoted 8 lines · Ævar Arnfjörð Bjarmason
perl/Git.pm +1 -1
@@ -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:
Quoted 1 line
	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:

Quoted 7 lines

Quoted 4 lines
=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.

brian m. carlson

2018-04-07 19:20 UTC #10 lore raw

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

In reply to Ævar Arnfjörð Bjarmason

Quoted 9 lines · Ævar Arnfjörð Bjarmason

Change code in Git.pm that sometimes calls chomp() on undef to only do so the value is defined.
This code has been chomping undef values ever since it was added in b26098fc2f ("git-svn: reduce scope of input record separator change", 2016-10-14), but started warning due to the introduction of "use warnings" to Git.pm in my f0e19cb7ce ("Git.pm: add the "use warnings" pragma", 2018-02-25) released with 2.17.0.

Quoted 6 lines

Since this function will return undef in those cases it's still possible that the code using it will warn if it does a chomp of its own, as the code added in b26098fc2f ("git-svn: reduce scope of input record separator change", 2016-10-14) might do, but since git-svn has "use warnings" already that's clearly not a codepath that's going to warn.

Thanks for this patch. I ran into this earlier this week (with git svn fetch) and had intended to send a patch, but you clearly got to it before I did.

Signature · 2 lines

brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

Junio C Hamano

2018-04-09 02:09 UTC #9 lore raw

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

In reply to Eric Wong

Quoted 18 lines · Eric Wong

Quoted 2 lines · Ævar Arnfjörð Bjarmason
See https://public-inbox.org/git/[email protected]/ for the original report.
Thanks for taking a look at this. Also https://bugs.debian.org/894997
perl/Git.pm sub get_record { · line 557
Quoted 8 lines
perl/Git.pm +1 -1
@@ -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:

Quoted 1 line

	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.