[PATCH 0/3] MboxReader usage and changes

[PATCH 0/3] MboxReader usage and changes

[Eric Wong]

*** BLURB HERE ***

Eric Wong (3):
  filter/vger: kill trailing newlines aggressively
  import_mbox: use MboxReader
  mbox_reader: do not chomp non-blank EOL

 lib/PublicInbox/Filter/Vger.pm   |  2 +-
 lib/PublicInbox/InboxWritable.pm | 44 +++++++-------------------------
 lib/PublicInbox/MboxReader.pm    |  2 +-
 t/mbox_reader.t                  | 22 +++++++++++++++-
 4 files changed, 32 insertions(+), 38 deletions(-)

[PATCH 1/3] filter/vger: kill trailing newlines aggressively

[Eric Wong]

PublicInbox::MboxReader->(mboxrd|mboxo) only deletes the last
trailing newline, not every single trailing newline like
InboxWritable->import_mbox does.

Testing PublicInbox::MboxReader->mboxrd (next commit) with
scripts/import_vger_from_mbox on the LKML archive I got 2018 for
v2 development; this difference was responsible for a single
spam message(*) from out of 2722831 not being filtered correctly
and returning a different result.

(*) dated 2014-08-25
---
 lib/PublicInbox/Filter/Vger.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/PublicInbox/Filter/Vger.pm b/lib/PublicInbox/Filter/Vger.pm
index 0b1f5dd3..5b3c0277 100644
--- a/lib/PublicInbox/Filter/Vger.pm
+++ b/lib/PublicInbox/Filter/Vger.pm
@@ -24,7 +24,7 @@ sub scrub {
 	# the vger appender seems to only work on the raw string,
 	# so in multipart (e.g. GPG-signed) messages, the list trailer
 	# becomes invisible to MIME-aware email clients.
-	if ($s =~ s/$l0\n$l1\n$l2\n$l3\n($l4\n)?\z//os) {
+	if ($s =~ s/$l0\n$l1\n$l2\n$l3\n(?:$l4\n)?\n*\z//os) {
 		$mime = PublicInbox::Eml->new(\$s);
 	}
 	$self->ACCEPT($mime);

[PATCH 2/3] import_mbox: use MboxReader

[Eric Wong]

It supports more mbox variants and it's trailing newline
behavior is probably more correct despite the previous change
to PublicInbox::Filter::Vger.
---
 lib/PublicInbox/InboxWritable.pm | 44 +++++++-------------------------
 1 file changed, 9 insertions(+), 35 deletions(-)

diff --git a/lib/PublicInbox/InboxWritable.pm b/lib/PublicInbox/InboxWritable.pm
index c3acc4f9..d4a9040f 100644
--- a/lib/PublicInbox/InboxWritable.pm
+++ b/lib/PublicInbox/InboxWritable.pm
@@ -158,49 +158,23 @@ sub import_maildir {
 	$im->done;
 }
 
-# asctime: From example@example.com Fri Jun 23 02:56:55 2000
-my $from_strict = qr/^From \S+ +\S+ \S+ +\S+ [^:]+:[^:]+:[^:]+ [^:]+/;
-
-sub mb_add ($$$$) {
-	my ($im, $variant, $filter, $msg) = @_;
-	$$msg =~ s/(\r?\n)+\z/$1/s;
-	if ($variant eq 'mboxrd') {
-		$$msg =~ s/^>(>*From )/$1/gms;
-	} elsif ($variant eq 'mboxo') {
-		$$msg =~ s/^>From /From /gms;
-	}
-	my $mime = PublicInbox::Eml->new($msg);
+sub _mbox_eml_cb { # MboxReader->mbox* callback
+	my ($eml, $im, $filter) = @_;
 	if ($filter) {
-		my $ret = $filter->scrub($mime) or return;
+		my $ret = $filter->scrub($eml) or return;
 		return if $ret == REJECT();
-		$mime = $ret;
+		$eml = $ret;
 	}
-	$im->add($mime)
+	$im->add($eml);
 }
 
 sub import_mbox {
 	my ($self, $fh, $variant) = @_;
-	if ($variant !~ /\A(?:mboxrd|mboxo)\z/) {
-		die "variant must be 'mboxrd' or 'mboxo'\n";
-	}
+	require PublicInbox::MboxReader;
+	my $cb = PublicInbox::MboxReader->can($variant) or
+		die "$variant not supported\n";
 	my $im = $self->importer(1);
-	my $prev = undef;
-	my $msg = '';
-	my $filter = $self->filter;
-	while (defined(my $l = <$fh>)) {
-		if ($l =~ /$from_strict/o) {
-			if (!defined($prev) || $prev =~ /^\r?$/) {
-				mb_add($im, $variant, $filter, \$msg) if $msg;
-				$msg = '';
-				$prev = $l;
-				next;
-			}
-			warn "W[$.] $l\n";
-		}
-		$prev = $l;
-		$msg .= $l;
-	}
-	mb_add($im, $variant, $filter, \$msg) if $msg;
+	$cb->(undef, $fh, \&_mbox_eml_cb, $im, $self->filter);
 	$im->done;
 }
 

[PATCH 3/3] mbox_reader: do not chomp non-blank EOL

[Eric Wong]

It's conceivable some cases won't generate an empty line before
an mboxrd or mboxo From_ line.  Ensure we can handle that case
and don't leave the Eml->{bdy} without a trailing LF character.

And drop an unnecessary alarm import while we're in the area.
---
 lib/PublicInbox/MboxReader.pm |  2 +-
 t/mbox_reader.t               | 22 +++++++++++++++++++++-
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/MboxReader.pm b/lib/PublicInbox/MboxReader.pm
index 59ce4fb6..df7c78fa 100644
--- a/lib/PublicInbox/MboxReader.pm
+++ b/lib/PublicInbox/MboxReader.pm
@@ -26,7 +26,7 @@ sub _mbox_from {
 		}
 		@raw = grep /[^ \t\r\n]/s, @raw; # skip empty messages
 		while (defined(my $raw = shift @raw)) {
-			$raw =~ s/\r?\n\z//s;
+			$raw =~ s/^\r?\n\z//ms;
 			$raw =~ s/$from_re/$1/gms;
 			my $eml = PublicInbox::Eml->new(\$raw);
 			$eml_cb->($eml, @arg);
diff --git a/t/mbox_reader.t b/t/mbox_reader.t
index 30a5e6e3..18d0fd68 100644
--- a/t/mbox_reader.t
+++ b/t/mbox_reader.t
@@ -74,9 +74,29 @@ for my $fmt (@mbox) { $check_fmt->($fmt) }
 s/\n/\r\n/sg for (values %raw);
 for my $fmt (@mbox) { $check_fmt->($fmt) }
 
+{
+	my $no_blank_eom = <<'EOM';
+From x@y Fri Oct  2 00:00:00 1993
+a: b
+
+body1
+From x@y Fri Oct  2 00:00:00 1993
+c: d
+
+body2
+EOM
+	# chop($no_blank_eom) eq "\n" or BAIL_OUT 'broken LF';
+	for my $variant (qw(mboxrd mboxo)) {
+		my @x;
+		open my $fh, '<', \$no_blank_eom or BAIL_OUT 'PerlIO::scalar';
+		$reader->$variant($fh, sub { push @x, shift });
+		is_deeply($x[0]->{bdy}, \"body1\n", 'LF preserved in 1st');
+		is_deeply($x[1]->{bdy}, \"body2\n", 'no LF added in 2nd');
+	}
+}
+
 SKIP: {
 	use PublicInbox::Spawn qw(popen_rd);
-	use Time::HiRes qw(alarm);
 	my $fh = popen_rd([ $^X, '-E', <<'' ]);
 say "From x@y Fri Oct  2 00:00:00 1993";
 print "a: b\n\n", "x" x 70000, "\n\n";