[PATCH 0/6] a few random small fixes and improvements

[PATCH 0/6] a few random small fixes and improvements

[Eric Wong]

Some minor things I noticed while working on other things.

Eric Wong (6):
  tests: fix missing modules under TEST_RUN_MODE=0
  test_common: disable fsync in git(1) commands
  nntp: improve protocol error messages
  nntp: integerize {article} to save memory
  view: reduce ops for <b> encasement
  view: fix obfuscation in message/* attachments

 lib/PublicInbox/NNTP.pm       | 40 +++++++++++++++++++++--------------
 lib/PublicInbox/TestCommon.pm |  2 ++
 lib/PublicInbox/View.pm       | 10 ++++-----
 t/extsearch.t                 |  1 +
 t/www_static.t                |  1 +
 5 files changed, 33 insertions(+), 21 deletions(-)

[PATCH 1/6] tests: fix missing modules under TEST_RUN_MODE=0

[Eric Wong]

By default, we rely heavily on preload to speed up tests and
missing modules were always present by the time we hit some
tests.  However, the more realistic (and significantly slower)
TEST_RUN_MODE=0 doesn't preload so we must explicitly load
missing modules.
---
 t/extsearch.t  | 1 +
 t/www_static.t | 1 +
 2 files changed, 2 insertions(+)

diff --git a/t/extsearch.t b/t/extsearch.t
index 28c43763..1a212bc8 100644
--- a/t/extsearch.t
+++ b/t/extsearch.t
@@ -609,6 +609,7 @@ if ('indexheader support') {
 	$es = PublicInbox::Config->new($cfg_path)->ALL;
 	my $mset = $es->mset('xarchiveshash:deadbeefcafe');
 	is $mset->size, 1, 'extindex.*.indexheader works';
+	require PublicInbox::XapClient;
 	local $PublicInbox::Search::XHC =
 			PublicInbox::XapClient::start_helper('-j0') or
 			xbail "no XHC: $@";
diff --git a/t/www_static.t b/t/www_static.t
index 8fb86a82..e258be1c 100644
--- a/t/www_static.t
+++ b/t/www_static.t
@@ -16,6 +16,7 @@ my $psgi_env = sub { # @_ is passed to WwwStatic->new
 	write_file '>', $ret, <<EOM;
 use v5.12;
 use Plack::Builder;
+use PublicInbox::WwwStatic;
 my \$ws = PublicInbox::WwwStatic->new(docroot => "$tmpdir" @_);
 builder { sub { \$ws->call(shift) } }
 EOM

[PATCH 2/6] test_common: disable fsync in git(1) commands

[Eric Wong]

As with git itself, fsync(2) results in needless overhead and
storage wear in test cases where data integrity is not an issue.
I normally point TMPDIR to tmpfs when running tests, but this
still affects initial setup of data for stuff in t/data-gen as
well as improving life for users with too little RAM for a tmpfs
TMPDIR.
---
 lib/PublicInbox/TestCommon.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/PublicInbox/TestCommon.pm b/lib/PublicInbox/TestCommon.pm
index 6c3677d2..b863ad6b 100644
--- a/lib/PublicInbox/TestCommon.pm
+++ b/lib/PublicInbox/TestCommon.pm
@@ -18,6 +18,7 @@ our $tail_cmd = $ENV{TAIL};
 our ($lei_opt, $lei_out, $lei_err);
 use autodie qw(chdir close fcntl mkdir open opendir seek unlink);
 $ENV{XDG_CACHE_HOME} //= "$ENV{HOME}/.cache"; # reuse C++ xap_helper builds
+$ENV{GIT_TEST_FSYNC} = 0; # hopefully reduce wear
 
 $_ = File::Spec->rel2abs($_) for (grep(!m!^/!, @INC));
 our $CURRENT_DAEMON;
@@ -87,6 +88,7 @@ sub tmpdir (;$) {
 	my ($base) = @_;
 	require File::Temp;
 	($base) = ($0 =~ m!\b([^/]+)\.[^\.]+\z!) unless defined $base;
+	($base) = ($0 =~ m!\b([^/]+)\z!) unless defined $base;
 	my $tmpdir = File::Temp->newdir("pi-$base-$$-XXXX", TMPDIR => 1);
 	wantarray ? ($tmpdir->dirname, $tmpdir) : $tmpdir;
 }

[PATCH 3/6] nntp: improve protocol error messages

[Eric Wong]

NNTP clients (e.g. lei :P) may display the message from the
server, so it's helpful to tell them the article number or
Message-ID that's missing.
---
 lib/PublicInbox/NNTP.pm | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index 603cf094..1749a755 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -26,8 +26,8 @@ use constant {
 	r502 => "502 Command unavailable\r\n",
 	r221 => "221 Header follows\r\n",
 	r225 =>	"225 Headers follow (multi-line)\r\n",
-	r430 => "430 No article with that message-id\r\n",
 };
+sub r430 ($) { "430 No article with that message-id <$_[0]>\r\n" }
 use Errno qw(EAGAIN);
 my $ONE_MSGID = qr/\A$MID_EXTRACT\z/;
 my @OVERVIEW = qw(Subject From Date Message-ID References);
@@ -466,17 +466,16 @@ sub set_nntp_headers ($$) {
 
 sub art_lookup ($$$) {
 	my ($self, $art, $code) = @_;
-	my ($ibx, $n);
-	my $err;
+	my ($ibx, $n, $mid, $err);
 	if (defined $art) {
 		if ($art =~ /\A[0-9]+\z/) {
-			$err = \"423 no such article number in this group\r\n";
+			$err = \"423 no such article (#$art) in this group\r\n";
 			$n = int($art);
 			goto find_ibx;
 		} elsif ($art =~ $ONE_MSGID) {
-			($ibx, $n) = mid_lookup($self, $1);
+			($ibx, $n) = mid_lookup($self, $mid = $1);
 			goto found if $ibx;
-			return \r430;
+			return \r430 $mid;
 		} else {
 			return \r501;
 		}
@@ -488,7 +487,13 @@ find_ibx:
 			return \"412 no newsgroup has been selected\r\n";
 	}
 found:
-	my $smsg = $ibx->over(1)->get_art($n) or return $err;
+	my $smsg = $ibx->over(1)->get_art($n) or do {
+		if (defined $mid) {
+			warn "BUG: $ibx->{newsgroup} <$mid> #$n missing";
+			return r430 $mid;
+		}
+		return $err;
+	};
 	$smsg->{-ibx} = $ibx;
 	if ($code == 223) { # STAT
 		set_art($self, $n);
@@ -632,8 +637,8 @@ sub hdr_message_id ($$$) { # optimize XHDR Message-ID [range] for slrnpull.
 	my ($self, $xhdr, $range) = @_;
 
 	if (defined $range && $range =~ $ONE_MSGID) {
-		my ($ibx, $n) = mid_lookup($self, $1);
-		return r430 unless $n;
+		my ($ibx, $n) = mid_lookup($self, my $mid = $1);
+		return r430 $mid unless $n;
 		hdr_mid_response($self, $xhdr, $ibx, $n, $range, $range);
 	} else { # numeric range
 		$range = $self->{article} unless defined $range;
@@ -703,7 +708,7 @@ sub hdr_xref ($$$) { # optimize XHDR Xref [range] for rtin
 	if (defined $range && $range =~ $ONE_MSGID) {
 		my $mid = $1;
 		my ($ibx, $n) = mid_lookup($self, $mid);
-		return r430 unless $n;
+		return r430 $mid unless defined $n;
 		my $smsg = $ibx->over(1)->get_art($n) or return;
 		hdr_mid_response($self, $xhdr, $ibx, $n, $range,
 				xref($self, $ibx, $smsg));
@@ -747,8 +752,8 @@ sub smsg_range_i {
 sub hdr_smsg ($$$$) {
 	my ($self, $xhdr, $field, $range) = @_;
 	if (defined $range && $range =~ $ONE_MSGID) {
-		my ($ibx, $n) = mid_lookup($self, $1);
-		return r430 unless defined $n;
+		my ($ibx, $n) = mid_lookup($self, my $mid = $1);
+		return r430 $mid unless defined $n;
 		my $v = over_header_for($ibx, $n, $field);
 		hdr_mid_response($self, $xhdr, $ibx, $n, $range, $v);
 	} else { # numeric range
@@ -849,9 +854,12 @@ sub over_line ($$$) {
 sub cmd_over ($;$) {
 	my ($self, $range) = @_;
 	if ($range && $range =~ $ONE_MSGID) {
-		my ($ibx, $n) = mid_lookup($self, $1);
-		defined $n or return r430;
-		my $smsg = $ibx->over(1)->get_art($n) or return r430;
+		my ($ibx, $n) = mid_lookup($self, my $mid = $1);
+		defined $n or return r430 $mid;
+		my $smsg = $ibx->over(1)->get_art($n) or do {
+			warn "BUG: $ibx->{newsgroup} <$mid> #$n missing";
+			return r430 $mid;
+		};
 		$self->msg_more(
 			"224 Overview information follows (multi-line)\r\n");
 

[PATCH 4/6] nntp: integerize {article} to save memory

[Eric Wong]

The NNTP article number is always an integer, so ensure it's
stored as one to avoid malloc overhead since NNTP clients may
linger for minutes at a time.
---
 lib/PublicInbox/NNTP.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index 1749a755..6fa9159f 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -520,7 +520,7 @@ sub msg_body_write ($$) {
 
 sub set_art {
 	my ($self, $art) = @_;
-	$self->{article} = $art if defined $art && $art =~ /\A[0-9]+\z/;
+	$self->{article} = $art + 0 if defined $art && $art =~ /\A[0-9]+\z/;
 }
 
 sub msg_hdr_write ($$) {

[PATCH 5/6] view: reduce ops for <b> encasement

[Eric Wong]

We can rely on print to concatenate its args and
reduce the amount of needless copies and string ops before the
print.
---
 lib/PublicInbox/View.pm | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/lib/PublicInbox/View.pm b/lib/PublicInbox/View.pm
index 7ca85a85..ad259253 100644
--- a/lib/PublicInbox/View.pm
+++ b/lib/PublicInbox/View.pm
@@ -272,13 +272,12 @@ sub emit_eml ($$) {
 	my $root_anchor = $ctx->{root_anchor} || '';
 	my $irt;
 	my $obfs_ibx = $ctx->{-obfs_ibx};
-	$subj = '(no subject)' if $subj eq '';
-	$subj = '<b>'.ascii_html($subj).'</b>';
+	$subj = $subj eq '' ? '(no subject)' : ascii_html($subj);
 	obfuscate_addrs($obfs_ibx, $subj) if $obfs_ibx;
 	$subj = "<u\nid=u>$subj</u>" if $root_anchor eq $id_m;
 	my $zfh = $ctx->{zfh} // die 'BUG: no {zfh}';
-	print $zfh "<a\nhref=#e", $id, "\nid=m", $id, '>*</a> ',
-		$subj, "\n", _th_index_lite($mid_raw, \$irt, $id, $ctx);
+	print $zfh "<a\nhref=#e", $id, "\nid=m", $id, '>*</a> <b>',
+		$subj, "</b>\n", _th_index_lite($mid_raw, \$irt, $id, $ctx);
 	my @tocc;
 	my $ds = delete $smsg->{ds}; # for v1 non-Xapian/SQLite users
 

[PATCH 6/6] view: fix obfuscation in message/* attachments

[Eric Wong]

Our address obfuscation currently relies on HTML-escaped output,
so we need to call obfuscate_addrs() after ascii_html().  This
bug only affected rare messages which include another message/*
attachment.  Without this fix it didn't fail to obfuscate, but
rather showed the showed `•' in the HTML instead of the
entity it represents.
---
 lib/PublicInbox/View.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/PublicInbox/View.pm b/lib/PublicInbox/View.pm
index ad259253..21cee63a 100644
--- a/lib/PublicInbox/View.pm
+++ b/lib/PublicInbox/View.pm
@@ -592,8 +592,9 @@ sub submsg_hdr ($$) {
 	for my $h (qw(From To Cc Subject Date Message-ID X-Alt-Message-ID)) {
 		$s .= "$h: $_\n" for $eml->header($h);
 	}
+	$s = ascii_html($s);
 	obfuscate_addrs($ctx->{-obfs_ibx}, $s) if $ctx->{-obfs_ibx};
-	ascii_html($s);
+	$s;
 }
 
 sub attach_link ($$$$;$) {