[PATCH 0/8] some view cleanups and minor bugfixes

[PATCH 0/8] some view cleanups and minor bugfixes

[Eric Wong]

Pretty insignificant, but the diffstat makes me happy :>

Eric Wong (8):
  view: remove mhref arg from multipart_text_as_html
  view: single id="t" for multi-Subject messages
  view: dump_topics: better naming of top Subject
  view: cleanup topic accumulation and dumping
  view,searchview: avoid smsg method calls when using SQLite/Xapian
  view: escape Subject HTML directly
  view: escape ampersand in Message-IDs
  view: remove last Hval->new caller

 lib/PublicInbox/ExtMsg.pm        |  19 ++--
 lib/PublicInbox/Hval.pm          |  28 +-----
 lib/PublicInbox/Linkify.pm       |   9 +-
 lib/PublicInbox/Reply.pm         |   6 +-
 lib/PublicInbox/SearchView.pm    |  10 +--
 lib/PublicInbox/View.pm          | 144 ++++++++++++++-----------------
 lib/PublicInbox/WwwAtomStream.pm |  12 +--
 t/psgi_bad_mids.t                |   9 +-
 xt/perf-msgview.t                |   3 +-
 9 files changed, 104 insertions(+), 136 deletions(-)

[PATCH 1/8] view: remove mhref arg from multipart_text_as_html

[Eric Wong]

No point in passing something on stack only to stash it
into the $ctx which holds most other parameters used for
rendering the HTML.
---
 lib/PublicInbox/View.pm          | 14 +++++++-------
 lib/PublicInbox/WwwAtomStream.pm |  3 ++-
 xt/perf-msgview.t                |  3 ++-
 3 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/lib/PublicInbox/View.pm b/lib/PublicInbox/View.pm
index d097d8ab..dcaee1ae 100644
--- a/lib/PublicInbox/View.pm
+++ b/lib/PublicInbox/View.pm
@@ -29,8 +29,9 @@ sub msg_page_i {
 	my $more = $ctx->{more};
 	if ($nr == 1) {
 		# $more cannot be true w/o $smsg being defined:
-		my $upfx = $more ? '../'.mid_escape($ctx->{smsg}->mid).'/' : '';
-		multipart_text_as_html(delete $ctx->{mime}, $upfx, $ctx);
+		$ctx->{mhref} = $more ? '../'.mid_escape($ctx->{smsg}->mid).'/'
+				      : '';
+		multipart_text_as_html(delete $ctx->{mime}, $ctx);
 		${delete $ctx->{obuf}} .= '</pre><hr>';
 	} elsif ($more) {
 		++$ctx->{end_nr};
@@ -83,10 +84,10 @@ sub msg_page_more {
 	my $next = $ibx->over->next_by_mid($ctx->{mid}, \$id, \$prev);
 	$ctx->{more} = $next ? [ $id, $prev, $next ] : undef;
 	return '' unless $smsg;
-	my $upfx = '../' . mid_escape($smsg->mid) . '/';
+	$ctx->{mhref} = '../' . mid_escape($smsg->mid) . '/';
 	my $mime = delete $smsg->{mime};
 	_msg_page_prepare_obuf($mime->header_obj, $ctx, $nr);
-	multipart_text_as_html($mime, $upfx, $ctx);
+	multipart_text_as_html($mime, $ctx);
 	${delete $ctx->{obuf}} .= '</pre><hr>';
 }
 
@@ -486,11 +487,10 @@ sub thread_html_i { # PublicInbox::WwwStream::getline callback
 }
 
 sub multipart_text_as_html {
-	my (undef, $mhref, $ctx) = @_; # $mime = $_[0]
-	$ctx->{mhref} = $mhref;
+	# ($mime, $ctx) = @_; # msg_iter will do "$_[0] = undef"
 
 	# scan through all parts, looking for displayable text
-	msg_iter($_[0], \&add_text_body, $ctx, 1);
+	msg_iter($_[0], \&add_text_body, $_[1], 1);
 }
 
 sub flush_quote {
diff --git a/lib/PublicInbox/WwwAtomStream.pm b/lib/PublicInbox/WwwAtomStream.pm
index ea871fe5..658934a2 100644
--- a/lib/PublicInbox/WwwAtomStream.pm
+++ b/lib/PublicInbox/WwwAtomStream.pm
@@ -142,7 +142,8 @@ sub feed_entry {
 		qq{<div\nxmlns="http://www.w3.org/1999/xhtml">} .
 		qq(<pre\nstyle="white-space:pre-wrap">);
 	$ctx->{obuf} = \$s;
-	PublicInbox::View::multipart_text_as_html($mime, $href, $ctx);
+	$ctx->{mhref} = $href;
+	PublicInbox::View::multipart_text_as_html($mime, $ctx);
 	delete $ctx->{obuf};
 	$s .= '</pre></div></content></entry>';
 }
diff --git a/xt/perf-msgview.t b/xt/perf-msgview.t
index f676ca93..3ea92ec6 100644
--- a/xt/perf-msgview.t
+++ b/xt/perf-msgview.t
@@ -37,13 +37,14 @@ my $n = 0;
 my $t = timeit(1, sub {
 	my $obuf = '';
 	$ctx->{obuf} = \$obuf;
+	$ctx->{mhref} = '../';
 	while (<$fh>) {
 		($cmt, $type) = split / /;
 		next if $type ne 'blob';
 		++$n;
 		$str = $git->cat_file($cmt);
 		$mime = PublicInbox::MIME->new($str);
-		PublicInbox::View::multipart_text_as_html($mime, '../', $ctx);
+		PublicInbox::View::multipart_text_as_html($mime, $ctx);
 		$obuf = '';
 	}
 });

[PATCH 2/8] view: single id="t" for multi-Subject messages

[Eric Wong]

While multi-Subject messages are unfortunate, try not to
generate confusing/invalid HTML with multiple elements
having the same HTML id attribute.
---
 lib/PublicInbox/View.pm | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/lib/PublicInbox/View.pm b/lib/PublicInbox/View.pm
index dcaee1ae..45c191db 100644
--- a/lib/PublicInbox/View.pm
+++ b/lib/PublicInbox/View.pm
@@ -651,16 +651,15 @@ sub _msg_page_prepare_obuf {
 	}
 	my @subj = $hdr->header('Subject');
 	if (@subj) {
-		for my $v (@subj) {
+		my $v = ascii_html(shift @subj);
+		obfuscate_addrs($obfs_ibx, $v) if $obfs_ibx;
+		$rv .= 'Subject: ';
+		$rv .= $over ? qq(<a\nhref="#r"\nid=t>$v</a>\n) : "$v\n";
+		$title[0] = $v;
+		for $v (@subj) { # multi-Subject message :<
 			$v = ascii_html($v);
 			obfuscate_addrs($obfs_ibx, $v) if $obfs_ibx;
-			$rv .= 'Subject: ';
-			if ($over) {
-				$rv .= qq(<a\nhref="#r"\nid=t>$v</a>\n);
-			} else {
-				$rv .= "$v\n";
-			}
-			$title[0] //= $v;
+			$rv .= "Subject: $v\n";
 		}
 	} else { # dummy anchor for thread skeleton at bottom of page
 		$rv .= qq(<a\nhref="#r"\nid=t></a>) if $over;

[PATCH 3/8] view: dump_topics: better naming of top Subject

[Eric Wong]

We use `$top' in other places, so name it to `$top_subj'
consistently for `$subj' and `$prev_subj' comparisons down
the function.
---
 lib/PublicInbox/View.pm | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/PublicInbox/View.pm b/lib/PublicInbox/View.pm
index 45c191db..f0446723 100644
--- a/lib/PublicInbox/View.pm
+++ b/lib/PublicInbox/View.pm
@@ -1087,13 +1087,13 @@ sub dump_topics {
 
 	# sort by recency, this allows new posts to "bump" old topics...
 	foreach my $topic (sort { $b->[0] <=> $a->[0] } @$order) {
-		my ($ds, $n, $seen, $top, @ex) = @$topic;
+		my ($ds, $n, $seen, $top_subj, @ex) = @$topic;
 		@$topic = ();
-		next unless defined $top;  # ghost topic
-		my $mid = delete $seen->{$top};
+		next unless defined $top_subj;  # ghost topic
+		my $mid = delete $seen->{$top_subj};
 		my $href = mid_escape($mid);
-		my $prev_subj = [ split(/ /, $top) ];
-		$top = PublicInbox::Hval->new($top)->as_html;
+		my $prev_subj = [ split(/ /, $top_subj) ];
+		$top_subj = PublicInbox::Hval->new($top_subj)->as_html;
 		$ds = fmt_ts($ds);
 
 		# $n isn't the total number of posts on the topic,
@@ -1109,7 +1109,7 @@ sub dump_topics {
 
 		my $mbox = qq(<a\nhref="$href/t.mbox.gz">mbox.gz</a>);
 		my $atom = qq(<a\nhref="$href/t.atom">Atom</a>);
-		my $s = "<a\nhref=\"$href/T/$anchor\">$top</a>\n" .
+		my $s = "<a\nhref=\"$href/T/$anchor\">$top_subj</a>\n" .
 			" $ds UTC $n - $mbox / $atom\n";
 		for (my $i = 0; $i < scalar(@ex); $i += 2) {
 			my $level = $ex[$i];

[PATCH 4/8] view: cleanup topic accumulation and dumping

[Eric Wong]

Avoid needlessly normalizing the subject when dumping, since
it's pushed into the @$topic array during accumulation in
normalized form.

We can also safely treat $smsg as a hashref and avoid
calling "->ds" as a method since we know we've got that
loaded via Over||Search and won't have to use Email::MIME
header lookup methods.
---
 lib/PublicInbox/View.pm | 41 +++++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/lib/PublicInbox/View.pm b/lib/PublicInbox/View.pm
index f0446723..e586b068 100644
--- a/lib/PublicInbox/View.pm
+++ b/lib/PublicInbox/View.pm
@@ -1039,35 +1039,32 @@ sub sort_ds {
 sub acc_topic { # walk_thread callback
 	my ($ctx, $level, $node) = @_;
 	my $mid = $node->{id};
-	my $x = $node->{smsg} || $ctx->{-inbox}->smsg_by_mid($mid);
-	my ($subj, $ds);
-	my $topic;
-	if ($x) {
-		$subj = $x->subject;
-		$subj = subject_normalized($subj);
+	my $smsg = $node->{smsg} // $ctx->{-inbox}->smsg_by_mid($mid);
+	if ($smsg) {
+		my $subj = subject_normalized($smsg->subject);
 		$subj = '(no subject)' if $subj eq '';
-		$ds = $x->ds;
-		if ($level == 0) {
-			$topic = [ $ds, 1, { $subj => $mid }, $subj ];
+		my $ds = $smsg->{ds};
+		if ($level == 0) { # new, top-level topic
+			my $topic = [ $ds, 1, { $subj => $mid }, $subj ];
 			$ctx->{-cur_topic} = $topic;
 			push @{$ctx->{order}}, $topic;
 			return 1;
 		}
 
-		$topic = $ctx->{-cur_topic}; # should never be undef
+		# continue existing topic
+		my $topic = $ctx->{-cur_topic}; # should never be undef
 		$topic->[0] = $ds if $ds > $topic->[0];
-		$topic->[1]++;
+		$topic->[1]++; # bump N+ message counter
 		my $seen = $topic->[2];
 		if (scalar(@$topic) == 3) { # parent was a ghost
 			push @$topic, $subj;
-		} elsif (!$seen->{$subj}) {
-			push @$topic, $level, $subj;
+		} elsif (!defined($seen->{$subj})) {
+			push @$topic, $level, $subj; # @extra messages
 		}
 		$seen->{$subj} = $mid; # latest for subject
 	} else { # ghost message
 		return 1 if $level != 0; # ignore child ghosts
-		$topic = [ -666, 0, {} ];
-		$ctx->{-cur_topic} = $topic;
+		my $topic = $ctx->{-cur_topic} = [ -666, 0, {} ];
 		push @{$ctx->{order}}, $topic;
 	}
 	1;
@@ -1087,7 +1084,7 @@ sub dump_topics {
 
 	# sort by recency, this allows new posts to "bump" old topics...
 	foreach my $topic (sort { $b->[0] <=> $a->[0] } @$order) {
-		my ($ds, $n, $seen, $top_subj, @ex) = @$topic;
+		my ($ds, $n, $seen, $top_subj, @extra) = @$topic;
 		@$topic = ();
 		next unless defined $top_subj;  # ghost topic
 		my $mid = delete $seen->{$top_subj};
@@ -1111,19 +1108,19 @@ sub dump_topics {
 		my $atom = qq(<a\nhref="$href/t.atom">Atom</a>);
 		my $s = "<a\nhref=\"$href/T/$anchor\">$top_subj</a>\n" .
 			" $ds UTC $n - $mbox / $atom\n";
-		for (my $i = 0; $i < scalar(@ex); $i += 2) {
-			my $level = $ex[$i];
-			my $subj = $ex[$i + 1];
+		for (my $i = 0; $i < scalar(@extra); $i += 2) {
+			my $level = $extra[$i];
+			my $subj = $extra[$i + 1]; # already normalized
 			$mid = delete $seen->{$subj};
-			my @subj = split(/ /, subject_normalized($subj));
+			my @subj = split(/ /, $subj);
 			my @next_prev = @subj; # full copy
 			my $omit = dedupe_subject($prev_subj, \@subj, ' &#34;');
 			$prev_subj = \@next_prev;
-			$subj = ascii_html(join(' ', @subj));
+			$subj = ascii_html($subj);
 			obfuscate_addrs($obfs_ibx, $subj) if $obfs_ibx;
 			$href = mid_escape($mid);
 			$s .= indent_for($level) . TCHILD;
-			$s .= "<a\nhref=\"$href/T/#u\">$subj</a>$omit\n";
+			$s .= qq(<a\nhref="$href/T/#u">$subj</a>$omit\n);
 		}
 		push @out, $s;
 	}

[PATCH 5/8] view,searchview: avoid smsg method calls when using SQLite/Xapian

[Eric Wong]

We already pre-populate the hashref when loading $smsg
(PublicInbox::SearchMsg) objects out of over.sqlite3 or Xapian,
so making expensive method calls isn't necessary in those cases.

We only need to use the method calls when SQLite or Xapian are
not available or are being populated (such as during indexing).
---
 lib/PublicInbox/SearchView.pm |  8 ++++----
 lib/PublicInbox/View.pm       | 16 ++++++++--------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/lib/PublicInbox/SearchView.pm b/lib/PublicInbox/SearchView.pm
index 828926c7..7e508bb7 100644
--- a/lib/PublicInbox/SearchView.pm
+++ b/lib/PublicInbox/SearchView.pm
@@ -108,14 +108,14 @@ sub mset_summary {
 			};
 			next;
 		}
-		my $s = ascii_html($smsg->subject);
-		my $f = ascii_html($smsg->from_name);
+		my $s = ascii_html($smsg->{subject});
+		my $f = ascii_html($smsg->{from_name});
 		if ($obfs_ibx) {
 			obfuscate_addrs($obfs_ibx, $s);
 			obfuscate_addrs($obfs_ibx, $f);
 		}
-		my $date = PublicInbox::View::fmt_ts($smsg->ds);
-		my $mid = PublicInbox::Hval->new_msgid($smsg->mid)->{href};
+		my $date = PublicInbox::View::fmt_ts($smsg->{ds});
+		my $mid = PublicInbox::Hval->new_msgid($smsg->{mid})->{href};
 		$s = '(no subject)' if $s eq '';
 		$$res .= qq{$rank. <b><a\nhref="$mid/">}.
 			$s . "</a></b>\n";
diff --git a/lib/PublicInbox/View.pm b/lib/PublicInbox/View.pm
index e586b068..033a0c14 100644
--- a/lib/PublicInbox/View.pm
+++ b/lib/PublicInbox/View.pm
@@ -84,7 +84,7 @@ sub msg_page_more {
 	my $next = $ibx->over->next_by_mid($ctx->{mid}, \$id, \$prev);
 	$ctx->{more} = $next ? [ $id, $prev, $next ] : undef;
 	return '' unless $smsg;
-	$ctx->{mhref} = '../' . mid_escape($smsg->mid) . '/';
+	$ctx->{mhref} = '../' . mid_escape($smsg->{mid}) . '/';
 	my $mime = delete $smsg->{mime};
 	_msg_page_prepare_obuf($mime->header_obj, $ctx, $nr);
 	multipart_text_as_html($mime, $ctx);
@@ -423,7 +423,7 @@ sub stream_thread ($$) {
 	return missing_thread($ctx) unless $smsg;
 
 	$ctx->{-obfs_ibx} = $ibx->{obfuscate} ? $ibx : undef;
-	$ctx->{-title_html} = ascii_html($smsg->subject);
+	$ctx->{-title_html} = ascii_html($smsg->{subject});
 	$ctx->{-html_tip} = thread_index_entry($ctx, $level, $smsg);
 	$ctx->{-queue} = \@q;
 	PublicInbox::WwwStream->response($ctx, 200, \&stream_thread_i);
@@ -469,7 +469,7 @@ sub thread_html {
 		$smsg = $ibx->smsg_mime($m) and last;
 	}
 	return missing_thread($ctx) unless $smsg;
-	$ctx->{-title_html} = ascii_html($smsg->subject);
+	$ctx->{-title_html} = ascii_html($smsg->{subject});
 	$ctx->{-html_tip} = '<pre>'.index_entry($smsg, $ctx, scalar @$msgs);
 	$ctx->{msgs} = $msgs;
 	PublicInbox::WwwStream->response($ctx, 200, \&thread_html_i);
@@ -932,7 +932,7 @@ sub skel_dump { # walk_thread callback
 		$$skel .= delete($ctx->{sl_note}) || '';
 	}
 
-	my $f = ascii_html($smsg->from_name);
+	my $f = ascii_html($smsg->{from_name});
 	my $obfs_ibx = $ctx->{-obfs_ibx};
 	obfuscate_addrs($obfs_ibx, $f) if $obfs_ibx;
 
@@ -971,7 +971,7 @@ sub skel_dump { # walk_thread callback
 	# Subject is never undef, this mail was loaded from
 	# our Xapian which would've resulted in '' if it were
 	# really missing (and Filter rejects empty subjects)
-	my @subj = split(/ /, subject_normalized($smsg->subject));
+	my @subj = split(/ /, subject_normalized($smsg->{subject}));
 	# remove common suffixes from the subject if it matches the previous,
 	# so we do not show redundant text at the end.
 	my $prev_subj = $ctx->{prev_subj} || [];
@@ -1029,8 +1029,8 @@ sub _skel_ghost {
 
 sub sort_ds {
 	[ sort {
-		(eval { $a->topmost->{smsg}->ds } || 0) <=>
-		(eval { $b->topmost->{smsg}->ds } || 0)
+		(eval { $a->topmost->{smsg}->{ds} } || 0) <=>
+		(eval { $b->topmost->{smsg}->{ds} } || 0)
 	} @{$_[0]} ];
 }
 
@@ -1041,7 +1041,7 @@ sub acc_topic { # walk_thread callback
 	my $mid = $node->{id};
 	my $smsg = $node->{smsg} // $ctx->{-inbox}->smsg_by_mid($mid);
 	if ($smsg) {
-		my $subj = subject_normalized($smsg->subject);
+		my $subj = subject_normalized($smsg->{subject});
 		$subj = '(no subject)' if $subj eq '';
 		my $ds = $smsg->{ds};
 		if ($level == 0) { # new, top-level topic

[PATCH 6/8] view: escape Subject HTML directly

[Eric Wong]

No need to use the over-engineered Hval OO API when the subject
is already normalized and there's no trailing spaces because of
normalization.
---
 lib/PublicInbox/View.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/PublicInbox/View.pm b/lib/PublicInbox/View.pm
index 033a0c14..d4bfa62d 100644
--- a/lib/PublicInbox/View.pm
+++ b/lib/PublicInbox/View.pm
@@ -1090,7 +1090,7 @@ sub dump_topics {
 		my $mid = delete $seen->{$top_subj};
 		my $href = mid_escape($mid);
 		my $prev_subj = [ split(/ /, $top_subj) ];
-		$top_subj = PublicInbox::Hval->new($top_subj)->as_html;
+		$top_subj = ascii_html($top_subj);
 		$ds = fmt_ts($ds);
 
 		# $n isn't the total number of posts on the topic,

[PATCH 7/8] view: escape ampersand in Message-IDs

[Eric Wong]

We need to escape ampersands (and some other characters for href
attributes), so introduce a `mid_href' sub to do just that.

'<', '>' and '"'  were always escaped, so there's no risk of tag
or attribute injection, but creative Message-IDs could cause
confusion for some parsers and generate invalid URLs.

Start getting rid of the bloated, over-engineered OO Hval API
while we're at it, I only noticed this bug because I started
killing off Hval->new* callers.
---
 lib/PublicInbox/ExtMsg.pm        | 19 +++++-------
 lib/PublicInbox/Hval.pm          |  7 ++---
 lib/PublicInbox/Linkify.pm       |  9 +++---
 lib/PublicInbox/Reply.pm         |  6 ++--
 lib/PublicInbox/SearchView.pm    |  4 +--
 lib/PublicInbox/View.pm          | 51 ++++++++++++++------------------
 lib/PublicInbox/WwwAtomStream.pm |  9 +++---
 t/psgi_bad_mids.t                |  9 ++++--
 8 files changed, 53 insertions(+), 61 deletions(-)

diff --git a/lib/PublicInbox/ExtMsg.pm b/lib/PublicInbox/ExtMsg.pm
index c48c2db4..44884ad2 100644
--- a/lib/PublicInbox/ExtMsg.pm
+++ b/lib/PublicInbox/ExtMsg.pm
@@ -8,7 +8,7 @@
 package PublicInbox::ExtMsg;
 use strict;
 use warnings;
-use PublicInbox::Hval qw(ascii_html prurl);
+use PublicInbox::Hval qw(ascii_html prurl mid_href);
 use PublicInbox::WwwStream;
 our $MIN_PARTIAL_LEN = 16;
 
@@ -129,9 +129,8 @@ sub ext_msg {
 	}
 
 	my $code = 404;
-	my $h = PublicInbox::Hval->new_msgid($mid);
-	my $href = $h->{href};
-	my $html = $h->as_html;
+	my $href = mid_href($mid);
+	my $html = ascii_html($mid);
 	my $title = "&lt;$html&gt; not found";
 	my $s = "<pre>Message-ID &lt;$html&gt;\nnot found\n";
 	if ($n_partial) {
@@ -145,10 +144,9 @@ sub ext_msg {
 			my $env = $ctx->{env} if $ibx->{name} eq $cur_name;
 			my $u = $ibx->base_url($env) or next;
 			foreach my $m (@$res) {
-				my $p = PublicInbox::Hval->new_msgid($m);
-				my $r = $p->{href};
-				my $t = $p->as_html;
-				$s .= qq{<a\nhref="$u$r/">$u$t/</a>\n};
+				my $href = mid_href($m);
+				my $html = ascii_html($m);
+				$s .= qq{<a\nhref="$u$href/">$u$html/</a>\n};
 			}
 		}
 	}
@@ -183,9 +181,8 @@ sub ext_urls {
 
 sub exact {
 	my ($ctx, $found, $mid) = @_;
-	my $h = PublicInbox::Hval->new_msgid($mid);
-	my $href = $h->{href};
-	my $html = $h->as_html;
+	my $href = mid_href($mid);
+	my $html = ascii_html($mid);
 	my $title = "&lt;$html&gt; found in ";
 	my $end = @$found == 1 ? 'another inbox' : 'other inboxes';
 	$ctx->{-title_html} = $title . $end;
diff --git a/lib/PublicInbox/Hval.pm b/lib/PublicInbox/Hval.pm
index 2e883f81..40c97da4 100644
--- a/lib/PublicInbox/Hval.pm
+++ b/lib/PublicInbox/Hval.pm
@@ -10,7 +10,7 @@ use Encode qw(find_encoding);
 use PublicInbox::MID qw/mid_clean mid_escape/;
 use base qw/Exporter/;
 our @EXPORT_OK = qw/ascii_html obfuscate_addrs to_filename src_escape
-		to_attr prurl/;
+		to_attr prurl mid_href/;
 my $enc_ascii = find_encoding('us-ascii');
 
 # safe-ish acceptable filename pattern for portability
@@ -27,10 +27,7 @@ sub new {
 	}, $class;
 }
 
-sub new_msgid {
-	my ($class, $msgid) = @_;
-	$class->new($msgid, mid_escape($msgid));
-}
+sub mid_href { ascii_html(mid_escape($_[0])) }
 
 # some of these overrides are standard C escapes so they're
 # easy-to-understand when rendered.
diff --git a/lib/PublicInbox/Linkify.pm b/lib/PublicInbox/Linkify.pm
index d176a7cc..2bd8f64a 100644
--- a/lib/PublicInbox/Linkify.pm
+++ b/lib/PublicInbox/Linkify.pm
@@ -13,7 +13,7 @@ package PublicInbox::Linkify;
 use strict;
 use warnings;
 use Digest::SHA qw/sha1_hex/;
-use PublicInbox::Hval qw(ascii_html);
+use PublicInbox::Hval qw(ascii_html mid_href);
 
 my $SALT = rand;
 my $LINK_RE = qr{([\('!])?\b((?:ftps?|https?|nntps?|gopher)://
@@ -94,10 +94,9 @@ sub linkify_2 {
 sub linkify_mids {
 	my ($self, $pfx, $str, $raw) = @_;
 	$$str =~ s!<([^>]+)>!
-		my $msgid = PublicInbox::Hval->new_msgid($1);
-		my $html = $msgid->as_html;
-		my $href = $msgid->{href};
-		$href = ascii_html($href); # for IDN
+		my $mid = $1;
+		my $html = ascii_html($mid);
+		my $href = mid_href($mid);
 
 		# salt this, as this could be exploited to show
 		# links in the HTML which don't show up in the raw mail.
diff --git a/lib/PublicInbox/Reply.pm b/lib/PublicInbox/Reply.pm
index edeb1ac2..5058ff8c 100644
--- a/lib/PublicInbox/Reply.pm
+++ b/lib/PublicInbox/Reply.pm
@@ -6,9 +6,9 @@ package PublicInbox::Reply;
 use strict;
 use warnings;
 use URI::Escape qw/uri_escape_utf8/;
-use PublicInbox::Hval qw/ascii_html obfuscate_addrs/;
+use PublicInbox::Hval qw(ascii_html obfuscate_addrs mid_href);
 use PublicInbox::Address;
-use PublicInbox::MID qw/mid_clean mid_escape/;
+use PublicInbox::MID qw(mid_clean);
 
 sub squote_maybe ($) {
 	my ($val) = @_;
@@ -76,7 +76,7 @@ sub mailto_arg_link {
 	$subj = "Re: $subj" unless $subj =~ /\bRe:/i;
 	my $mid = $hdr->header_raw('Message-ID');
 	push @arg, '--in-reply-to='.squote_maybe(mid_clean($mid));
-	my $irt = mid_escape($mid);
+	my $irt = mid_href($mid);
 	delete $cc->{$to};
 	if ($obfs) {
 		my $arg_to = $to;
diff --git a/lib/PublicInbox/SearchView.pm b/lib/PublicInbox/SearchView.pm
index 7e508bb7..9b67b045 100644
--- a/lib/PublicInbox/SearchView.pm
+++ b/lib/PublicInbox/SearchView.pm
@@ -7,7 +7,7 @@ use strict;
 use warnings;
 use URI::Escape qw(uri_unescape uri_escape);
 use PublicInbox::SearchMsg;
-use PublicInbox::Hval qw/ascii_html obfuscate_addrs/;
+use PublicInbox::Hval qw(ascii_html obfuscate_addrs mid_href);
 use PublicInbox::View;
 use PublicInbox::WwwAtomStream;
 use PublicInbox::SearchThread;
@@ -115,7 +115,7 @@ sub mset_summary {
 			obfuscate_addrs($obfs_ibx, $f);
 		}
 		my $date = PublicInbox::View::fmt_ts($smsg->{ds});
-		my $mid = PublicInbox::Hval->new_msgid($smsg->{mid})->{href};
+		my $mid = mid_href($smsg->{mid});
 		$s = '(no subject)' if $s eq '';
 		$$res .= qq{$rank. <b><a\nhref="$mid/">}.
 			$s . "</a></b>\n";
diff --git a/lib/PublicInbox/View.pm b/lib/PublicInbox/View.pm
index d4bfa62d..14b7d81d 100644
--- a/lib/PublicInbox/View.pm
+++ b/lib/PublicInbox/View.pm
@@ -8,9 +8,9 @@ use strict;
 use warnings;
 use bytes (); # only for bytes::length
 use PublicInbox::MsgTime qw(msg_datestamp);
-use PublicInbox::Hval qw(ascii_html obfuscate_addrs prurl);
+use PublicInbox::Hval qw(ascii_html obfuscate_addrs prurl mid_href);
 use PublicInbox::Linkify;
-use PublicInbox::MID qw/id_compress mid_escape mids mids_for_index references/;
+use PublicInbox::MID qw/id_compress mids mids_for_index references/;
 use PublicInbox::MsgIter;
 use PublicInbox::Address;
 use PublicInbox::WwwStream;
@@ -29,7 +29,7 @@ sub msg_page_i {
 	my $more = $ctx->{more};
 	if ($nr == 1) {
 		# $more cannot be true w/o $smsg being defined:
-		$ctx->{mhref} = $more ? '../'.mid_escape($ctx->{smsg}->mid).'/'
+		$ctx->{mhref} = $more ? '../'.mid_href($ctx->{smsg}->{mid}).'/'
 				      : '';
 		multipart_text_as_html(delete $ctx->{mime}, $ctx);
 		${delete $ctx->{obuf}} .= '</pre><hr>';
@@ -84,7 +84,7 @@ sub msg_page_more {
 	my $next = $ibx->over->next_by_mid($ctx->{mid}, \$id, \$prev);
 	$ctx->{more} = $next ? [ $id, $prev, $next ] : undef;
 	return '' unless $smsg;
-	$ctx->{mhref} = '../' . mid_escape($smsg->{mid}) . '/';
+	$ctx->{mhref} = '../' . mid_href($smsg->{mid}) . '/';
 	my $mime = delete $smsg->{mime};
 	_msg_page_prepare_obuf($mime->header_obj, $ctx, $nr);
 	multipart_text_as_html($mime, $ctx);
@@ -220,7 +220,7 @@ sub index_entry {
 	obfuscate_addrs($obfs_ibx, $from) if $obfs_ibx;
 	$rv .= "From: $from @ ".fmt_ts($ds)." UTC";
 	my $upfx = $ctx->{-upfx};
-	my $mhref = $upfx . mid_escape($mid_raw) . '/';
+	my $mhref = $upfx . mid_href($mid_raw) . '/';
 	$rv .= qq{ (<a\nhref="$mhref">permalink</a> / };
 	$rv .= qq{<a\nhref="${mhref}raw">raw</a>)\n};
 	my $to = fold_addresses(_hdr_names_html($hdr, 'To'));
@@ -244,9 +244,8 @@ sub index_entry {
 
 	my $mapping = $ctx->{mapping};
 	if (!$mapping && (defined($irt) || defined($irt = in_reply_to($hdr)))) {
-		my $mirt = PublicInbox::Hval->new_msgid($irt);
-		my $href = $upfx . $mirt->{href}. '/';
-		my $html = $mirt->as_html;
+		my $href = $upfx . mid_href($irt) . '/';
+		my $html = ascii_html($irt);
 		$rv .= qq(In-Reply-To: &lt;<a\nhref="$href">$html</a>&gt;\n)
 	}
 	$rv .= "\n";
@@ -672,8 +671,7 @@ sub _msg_page_prepare_obuf {
 	}
 	$ctx->{-title_html} = join(' - ', @title);
 	if (scalar(@$mids) == 1) { # common case
-		my $mid = PublicInbox::Hval->new_msgid($mids->[0]);
-		my $mhtml = $mid->as_html;
+		my $mhtml = ascii_html($mids->[0]);
 		$rv .= "Message-ID: &lt;$mhtml&gt; ";
 		$rv .= "(<a\nhref=\"raw\">raw</a>)\n";
 	} else {
@@ -751,9 +749,8 @@ sub _parent_headers {
 		$refs = references($hdr);
 		my $irt = pop @$refs;
 		if (defined $irt) {
-			my $v = PublicInbox::Hval->new_msgid($irt);
-			my $html = $v->as_html;
-			my $href = $v->{href};
+			my $html = ascii_html($irt);
+			my $href = mid_href($irt);
 			$rv .= "In-Reply-To: &lt;";
 			$rv .= "<a\nhref=\"../$href/\">$html</a>&gt;\n";
 		}
@@ -787,17 +784,17 @@ sub html_footer {
 		$next = $prev = '    ';
 
 		if (my $n = $ctx->{next_msg}) {
-			$n = PublicInbox::Hval->new_msgid($n)->{href};
+			$n = mid_href($n);
 			$next = "<a\nhref=\"$upfx$n/\"\nrel=next>next</a>";
 		}
 		my $u;
 		my $par = $ctx->{parent_msg};
 		if ($par) {
-			$u = PublicInbox::Hval->new_msgid($par)->{href};
+			$u = mid_href($par);
 			$u = "$upfx$u/";
 		}
 		if (my $p = $ctx->{prev_msg}) {
-			$prev = PublicInbox::Hval->new_msgid($p)->{href};
+			$prev = mid_href($p);
 			if ($p && $par && $p eq $par) {
 				$prev = "<a\nhref=\"$upfx$prev/\"\n" .
 					'rel=prev>prev parent</a>';
@@ -819,9 +816,9 @@ sub html_footer {
 }
 
 sub linkify_ref_no_over {
-	my $v = PublicInbox::Hval->new_msgid($_[0]);
-	my $html = $v->as_html;
-	my $href = $v->{href};
+	my ($mid) = @_;
+	my $href = mid_href($mid);
+	my $html = ascii_html($mid);
 	"&lt;<a\nhref=\"../$href/\">$html</a>&gt;";
 }
 
@@ -833,9 +830,8 @@ sub anchor_for {
 sub ghost_parent {
 	my ($upfx, $mid) = @_;
 
-	$mid = PublicInbox::Hval->new_msgid($mid);
-	my $href = $mid->{href};
-	my $html = $mid->as_html;
+	my $href = mid_href($mid);
+	my $html = ascii_html($mid);
 	qq{[parent not found: &lt;<a\nhref="$upfx$href/">$html</a>&gt;]};
 }
 
@@ -996,7 +992,7 @@ sub skel_dump { # walk_thread callback
 		$map->[0] = "$d<a\nhref=\"$m\">$end";
 		$id = "\nid=r".$id;
 	} else {
-		$m = $ctx->{-upfx}.mid_escape($mid).'/';
+		$m = $ctx->{-upfx}.mid_href($mid).'/';
 	}
 	$$skel .=  $d . "<a\nhref=\"$m\"$id>" . $end;
 	1;
@@ -1010,9 +1006,8 @@ sub _skel_ghost {
 	$d .= '    '  if exists $ctx->{searchview};
 	$d .= indent_for($level) . th_pfx($level);
 	my $upfx = $ctx->{-upfx};
-	my $m = PublicInbox::Hval->new_msgid($mid);
-	my $href = $upfx . $m->{href} . '/';
-	my $html = $m->as_html;
+	my $href = $upfx . mid_href($mid) . '/';
+	my $html = ascii_html($mid);
 
 	my $mapping = $ctx->{mapping};
 	my $map = $mapping->{$mid} if $mapping;
@@ -1088,7 +1083,7 @@ sub dump_topics {
 		@$topic = ();
 		next unless defined $top_subj;  # ghost topic
 		my $mid = delete $seen->{$top_subj};
-		my $href = mid_escape($mid);
+		my $href = mid_href($mid);
 		my $prev_subj = [ split(/ /, $top_subj) ];
 		$top_subj = ascii_html($top_subj);
 		$ds = fmt_ts($ds);
@@ -1118,7 +1113,7 @@ sub dump_topics {
 			$prev_subj = \@next_prev;
 			$subj = ascii_html($subj);
 			obfuscate_addrs($obfs_ibx, $subj) if $obfs_ibx;
-			$href = mid_escape($mid);
+			$href = mid_href($mid);
 			$s .= indent_for($level) . TCHILD;
 			$s .= qq(<a\nhref="$href/T/#u">$subj</a>$omit\n);
 		}
diff --git a/lib/PublicInbox/WwwAtomStream.pm b/lib/PublicInbox/WwwAtomStream.pm
index 658934a2..aa917ed8 100644
--- a/lib/PublicInbox/WwwAtomStream.pm
+++ b/lib/PublicInbox/WwwAtomStream.pm
@@ -12,8 +12,7 @@ use warnings;
 use POSIX qw(strftime);
 use Digest::SHA qw(sha1_hex);
 use PublicInbox::Address;
-use PublicInbox::Hval qw(ascii_html);
-use PublicInbox::MID qw(mid_escape);
+use PublicInbox::Hval qw(ascii_html mid_href);
 use PublicInbox::MsgTime qw(msg_timestamp);
 
 # called by PSGI server after getline:
@@ -71,7 +70,7 @@ sub atom_header {
 	my $mid = $ctx->{mid};
 	my $page_id;
 	if (defined $mid) { # per-thread
-		$self_url .= mid_escape($mid).'/t.atom';
+		$self_url .= mid_href($mid).'/t.atom';
 		$page_id = to_uuid("t\n".$mid)
 	} elsif (defined $search_q) {
 		my $query = $search_q->{'q'};
@@ -109,13 +108,13 @@ sub feed_entry {
 	my $base = $ctx->{feed_base_url};
 	if (defined $irt) {
 		my $irt_uuid = to_uuid($irt);
-		$irt = mid_escape($irt);
+		$irt = mid_href($irt);
 		$irt = qq(<thr:in-reply-to\nref="$irt_uuid"\n).
 			qq(href="$base$irt/"/>);
 	} else {
 		$irt = '';
 	}
-	my $href = $base . mid_escape($mid) . '/';
+	my $href = $base . mid_href($mid) . '/';
 	my $t = msg_timestamp($hdr);
 	my @t = gmtime(defined $t ? $t : time);
 	my $updated = feed_updated(@t);
diff --git a/t/psgi_bad_mids.t b/t/psgi_bad_mids.t
index d86c90bc..43025a4d 100644
--- a/t/psgi_bad_mids.t
+++ b/t/psgi_bad_mids.t
@@ -28,6 +28,7 @@ $im->{parallel} = 0;
 my $msgs = <<'';
 F1V5OR6NMF.3M649JTLO9IXD@tux.localdomain/hehe1"'<foo
 F1V5NB0PTU.3U0DCVGAJ750Z@tux.localdomain"'<>/foo
+F1V5NB0PTU.3U0DCVGAJ750Z@tux&.ampersand
 F1V5MIHGCU.2ABINKW6WBE8N@tux.localdomain/raw
 F1V5LF9D9C.2QT5PGXZQ050E@tux.localdomain/t.atom
 F1V58X3CMU.2DCCVAKQZGADV@tux.localdomain/../../../../foo
@@ -70,9 +71,13 @@ test_psgi(sub { $www->call(@_) }, sub {
 		'got escaped links to all messages');
 
 	@xmids = reverse @xmids;
+	my %uxs = ( gt => '>', lt => '<' );
 	foreach my $i (0..$#xmids) {
-		$res = $cb->(GET("/bad-mids/$xmids[$i]/raw"));
-		is($res->code, 200, 'got 200 OK raw message');
+		my $uri = $xmids[$i];
+		$uri =~ s/&#([0-9]+);/sprintf("%c", $1)/sge;
+		$uri =~ s/&(lt|gt);/$uxs{$1}/sge;
+		$res = $cb->(GET("/bad-mids/$uri/raw"));
+		is($res->code, 200, 'got 200 OK raw message '.$uri);
 		like($res->content, qr/Message-ID: <\Q$mids[$i]\E>/s,
 			'retrieved correct message');
 	}

[PATCH 8/8] view: remove last Hval->new caller

[Eric Wong]

The object-oriented Hval API turned out to be less useful and
more clunky than I envisioned years ago, so get rid of it.
We'll no longer strip trailing whitespace from From: headers in
the HTML display, but I doubt anybody cares.
---
 lib/PublicInbox/Hval.pm | 21 ---------------------
 lib/PublicInbox/View.pm |  5 ++---
 2 files changed, 2 insertions(+), 24 deletions(-)

diff --git a/lib/PublicInbox/Hval.pm b/lib/PublicInbox/Hval.pm
index 40c97da4..5f7ab513 100644
--- a/lib/PublicInbox/Hval.pm
+++ b/lib/PublicInbox/Hval.pm
@@ -16,17 +16,6 @@ my $enc_ascii = find_encoding('us-ascii');
 # safe-ish acceptable filename pattern for portability
 our $FN = '[a-zA-Z0-9][a-zA-Z0-9_\-\.]+[a-zA-Z0-9]'; # needs \z anchor
 
-sub new {
-	my ($class, $raw, $href) = @_;
-
-	# we never care about trailing whitespace
-	$raw =~ s/\s*\z//;
-	bless {
-		raw => $raw,
-		href => defined $href ? $href : $raw,
-	}, $class;
-}
-
 sub mid_href { ascii_html(mid_escape($_[0])) }
 
 # some of these overrides are standard C escapes so they're
@@ -71,16 +60,6 @@ sub ascii_html {
 	$enc_ascii->encode($s, Encode::HTMLCREF);
 }
 
-sub as_html { ascii_html($_[0]->{raw}) }
-
-sub raw {
-	if (defined $_[1]) {
-		$_[0]->{raw} = $_[1];
-	} else {
-		$_[0]->{raw};
-	}
-}
-
 # returns a protocol-relative URL string
 sub prurl ($$) {
 	my ($env, $u) = @_;
diff --git a/lib/PublicInbox/View.pm b/lib/PublicInbox/View.pm
index 14b7d81d..980f265a 100644
--- a/lib/PublicInbox/View.pm
+++ b/lib/PublicInbox/View.pm
@@ -630,10 +630,9 @@ sub _msg_page_prepare_obuf {
 	}
 	my @title; # (Subject[0], From[0])
 	for my $v ($hdr->header('From')) {
-		$v = PublicInbox::Hval->new($v);
-		my @n = PublicInbox::Address::names($v->raw);
+		my @n = PublicInbox::Address::names($v);
+		$v = ascii_html($v);
 		$title[1] //= ascii_html(join(', ', @n));
-		$v = $v->as_html;
 		if ($obfs_ibx) {
 			obfuscate_addrs($obfs_ibx, $v);
 			obfuscate_addrs($obfs_ibx, $title[1]);