From: Eric Wong <e@80x24.org>
Date: Thu, 27 Mar 2025 23:20:45 +0000 (+0000)
Subject: nntp: avoid modifying $_[0] in RE conversions
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3988b91d29ea037df1f1790ca18d4457e833d87b;p=thirdparty%2Fpublic-inbox.git

nntp: avoid modifying $_[0] in RE conversions

I've been getting occasional t/nntp.t warnings about
uninitialized variables which I'm not able to reproduce
reliably.  My current theory is that modifying $_[0] may get
wonky when it's happening across several layers of the call
stack, so stop doing it since it's unlikely to yield any
real world speedups and only made the code more difficult
to understand, here.
---

diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index 457e0fb7c..eab6d301d 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -121,8 +121,8 @@ sub names2ibx ($;$) {
 
 sub _list_groups (@) {
 	my ($cb, $self, $wildmat) = @_;
-	wildmat2re($wildmat);
-	my @names = grep /$wildmat/, @{$self->{nntpd}->{groupnames}};
+	my $re = wildmat2re($wildmat);
+	my @names = grep /$re/, @{$self->{nntpd}->{groupnames}};
 	$self->long_response($cb, names2ibx($self, \@names));
 }
 
@@ -274,11 +274,11 @@ sub cmd_newgroups ($$$;$$) {
 	$self->long_response(\&newgroups_i, $ts, names2ibx($self));
 }
 
-sub wildmat2re (;$) {
-	return $_[0] = qr/.*/ if (!defined $_[0] || $_[0] eq '*');
+sub wildmat2re ($) {
+	my $tmp = $_[0] // '*';
+	return qr/.*/ if $tmp eq '*';
 	my %keep;
 	my $salt = rand;
-	my $tmp = $_[0];
 
 	$tmp =~ s#(?<!\\)\[(.+)(?<!\\)\]#
 		my $orig = $1;
@@ -295,14 +295,14 @@ sub wildmat2re (;$) {
 			defined $orig ? $orig : $1;
 			#ge;
 	}
-	$_[0] = qr/\A$tmp\z/;
+	qr/\A$tmp\z/;
 }
 
-sub ngpat2re (;$) {
-	return $_[0] = qr/\A\z/ unless defined $_[0];
+sub ngpat2re ($) {
+	my $tmp = $_[0] // return qr/\A\z/;
 	my %map = ('*' => '.*', ',' => '|');
-	$_[0] =~ s!(.)!$map{$1} || "\Q$1"!ge;
-	$_[0] = qr/\A(?:$_[0])\z/;
+	$tmp =~ s!(.)!$map{$1} || "\Q$1\E"!ge;
+	qr/\A(?:$tmp)\z/;
 }
 
 sub newnews_i {
@@ -332,8 +332,8 @@ sub cmd_newnews ($$$$;$$) {
 	return r501 if $@;
 	$self->msg_more("230 list of new articles by message-id follows\r\n");
 	my ($keep, $skip) = split(/!/, $newsgroups, 2);
-	ngpat2re($keep);
-	ngpat2re($skip);
+	$keep = ngpat2re $keep;
+	$skip = ngpat2re $skip;
 	my @names = grep(/$keep/, @{$self->{nntpd}->{groupnames}});
 	@names = grep(!/$skip/, @names);
 	return \".\r\n" unless scalar(@names);
diff --git a/t/nntp.t b/t/nntp.t
index f1801cfe0..01c55d2e5 100644
--- a/t/nntp.t
+++ b/t/nntp.t
@@ -12,15 +12,15 @@ use POSIX qw(strftime);
 	my $wm_prepare = sub {
 		my ($wm) = @_;
 		my $orig = qq{'$wm'};
-		PublicInbox::NNTP::wildmat2re($_[0]);
-		my $new = explain($_[0]);
-		($orig, $new);
+		my $re = PublicInbox::NNTP::wildmat2re($wm);
+		my $new = explain($re);
+		($orig, $new, $re);
 	};
 
 	my $wildmat_like = sub {
 		my ($str, $wm) = @_;
-		my ($orig, $new) = $wm_prepare->($wm);
-		like($str, $wm, "$orig matches '$str' using $new");
+		my ($orig, $new, $re) = $wm_prepare->($wm);
+		like($str, $re, "$orig matches '$str' using $new");
 	};
 
 	my $wildmat_unlike = sub {
@@ -30,8 +30,8 @@ use POSIX qw(strftime);
 			my $re = qr/$wm/;
 			like($str, $re, "normal re with $wm matches, but ...");
 		}
-		my ($orig, $new) = $wm_prepare->($wm);
-		unlike($str, $wm, "$orig does not match '$str' using $new");
+		my ($orig, $new, $re) = $wm_prepare->($wm);
+		unlike($str, $re, "$orig does not match '$str' using $new");
 	};
 
 	$wildmat_like->('[foo]', '[\[foo\]]');
@@ -47,8 +47,8 @@ use POSIX qw(strftime);
 	my $ngpat_like = sub {
 		my ($str, $pat) = @_;
 		my $orig = $pat;
-		PublicInbox::NNTP::ngpat2re($pat);
-		like($str, $pat, "'$orig' matches '$str' using $pat");
+		my $re = PublicInbox::NNTP::ngpat2re($pat);
+		like($str, $re, "'$orig' matches '$str' using $re");
 	};
 
 	$ngpat_like->('any', '*');