[PATCH 0/4] more tweaks and finalize WwwStream conversion

[PATCH 0/4] more tweaks and finalize WwwStream conversion

[Eric Wong]

Finally, we now have consistent HTML for all of our non-error
pages and we have fewer layering violations for cleanup.

Eric Wong (4):
      githttpbackend: avoid intermediate array creation from stat
      t/git-http-backend: check BSD::Resource availability
      inbox: cleanup and consolidate object weakening
      www: remove old footer generation code and normalize new.html

 lib/PublicInbox/Feed.pm           | 86 ++++++++++-----------------------------
 lib/PublicInbox/GitHTTPBackend.pm |  3 +-
 lib/PublicInbox/HTTP.pm           | 16 --------
 lib/PublicInbox/Inbox.pm          | 37 +++++++++++++++--
 lib/PublicInbox/NNTP.pm           | 17 --------
 lib/PublicInbox/View.pm           | 47 ++++++++++++++++-----
 lib/PublicInbox/WWW.pm            | 44 +-------------------
 t/cgi.t                           | 17 +-------
 t/git-http-backend.t              |  2 +-
 t/html_index.t                    | 16 --------
 t/plack.t                         |  4 +-
 11 files changed, 98 insertions(+), 191 deletions(-)

[PATCH 1/4] githttpbackend: avoid intermediate array creation from stat

[Eric Wong]

No need to keep an extra array around for this.
---
 lib/PublicInbox/GitHTTPBackend.pm | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/PublicInbox/GitHTTPBackend.pm b/lib/PublicInbox/GitHTTPBackend.pm
index a9c0e9c..b684988 100644
--- a/lib/PublicInbox/GitHTTPBackend.pm
+++ b/lib/PublicInbox/GitHTTPBackend.pm
@@ -96,8 +96,7 @@ sub serve_dumb {
 
 	my $f = (ref $git ? $git->{git_dir} : $git) . '/' . $path;
 	return r(404) unless -f $f && -r _; # just in case it's a FIFO :P
-	my @st = stat(_);
-	my $size = $st[7];
+	my $size = -s _;
 
 	# TODO: If-Modified-Since and Last-Modified?
 	open my $in, '<', $f or return r(404);
-- 
EW

[PATCH 2/4] t/git-http-backend: check BSD::Resource availability

[Eric Wong]

We should not fail tests when this is not available.
---
 t/git-http-backend.t | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/git-http-backend.t b/t/git-http-backend.t
index e879392..e506e77 100644
--- a/t/git-http-backend.t
+++ b/t/git-http-backend.t
@@ -12,7 +12,7 @@ use Cwd qw(getcwd);
 
 my $git_dir = $ENV{GIANT_GIT_DIR};
 plan 'skip_all' => 'GIANT_GIT_DIR not defined' unless $git_dir;
-foreach my $mod (qw(Danga::Socket
+foreach my $mod (qw(Danga::Socket BSD::Resource
 			Plack::Util Plack::Builder
 			HTTP::Date HTTP::Status Net::HTTP)) {
 	eval "require $mod";
-- 
EW

[PATCH 3/4] inbox: cleanup and consolidate object weakening

[Eric Wong]

This fixes some layering violations and consolidates
the cleanup into the inbox object itself.  Keeping in
mind weakening does not work at all without our PSGI
server.
---
 lib/PublicInbox/HTTP.pm  | 16 ----------------
 lib/PublicInbox/Inbox.pm | 37 +++++++++++++++++++++++++++++++++----
 lib/PublicInbox/NNTP.pm  | 17 -----------------
 lib/PublicInbox/WWW.pm   |  2 --
 4 files changed, 33 insertions(+), 39 deletions(-)

diff --git a/lib/PublicInbox/HTTP.pm b/lib/PublicInbox/HTTP.pm
index e19c592..abf648f 100644
--- a/lib/PublicInbox/HTTP.pm
+++ b/lib/PublicInbox/HTTP.pm
@@ -25,15 +25,6 @@ use constant {
 	CHUNK_MAX_HDR => 256,
 };
 
-# FIXME: duplicated code with NNTP.pm, layering violation
-my $WEAKEN = {}; # string(inbox) -> inbox
-my $weakt;
-sub weaken_task () {
-	$weakt = undef;
-	$_->weaken_all for values %$WEAKEN;
-	$WEAKEN = {};
-}
-
 my $pipelineq = [];
 my $pipet;
 sub process_pipelineq () {
@@ -252,13 +243,6 @@ sub response_done ($$) {
 	$self->{env} = undef;
 	$self->write("0\r\n\r\n") if $alive == 2;
 	$self->write(sub { $alive ? next_request($self) : $self->close });
-
-	# FIXME: layering violation
-	if (my $obj = $env->{'pi-httpd.inbox'}) {
-		# grace period for reaping resources
-		$WEAKEN->{"$obj"} = $obj;
-		PublicInbox::EvCleanup::later(*weaken_task);
-	}
 }
 
 sub getline_response {
diff --git a/lib/PublicInbox/Inbox.pm b/lib/PublicInbox/Inbox.pm
index 728caa0..dc9980b 100644
--- a/lib/PublicInbox/Inbox.pm
+++ b/lib/PublicInbox/Inbox.pm
@@ -9,6 +9,26 @@ use Scalar::Util qw(weaken isweak);
 use PublicInbox::Git;
 use PublicInbox::MID qw(mid2path);
 
+my $weakt;
+eval {
+	$weakt = 'disabled';
+	require PublicInbox::EvCleanup;
+	$weakt = undef; # OK if we get here
+};
+
+my $WEAKEN = {}; # string(inbox) -> inbox
+sub weaken_task () {
+	$weakt = undef;
+	_weaken_fields($_) for values %$WEAKEN;
+	$WEAKEN = {};
+}
+
+sub _weaken_later ($) {
+	my ($self) = @_;
+	$weakt ||= PublicInbox::EvCleanup::later(*weaken_task);
+	$WEAKEN->{"$self"} = $self;
+}
+
 sub new {
 	my ($class, $opts) = @_;
 	my $v = $opts->{address} ||= 'public-inbox@example.com';
@@ -17,7 +37,7 @@ sub new {
 	bless $opts, $class;
 }
 
-sub weaken_all {
+sub _weaken_fields {
 	my ($self) = @_;
 	foreach my $f (qw(git mm search)) {
 		isweak($self->{$f}) or weaken($self->{$f});
@@ -26,17 +46,26 @@ sub weaken_all {
 
 sub git {
 	my ($self) = @_;
-	$self->{git} ||= eval { PublicInbox::Git->new($self->{mainrepo}) };
+	$self->{git} ||= eval {
+		_weaken_later($self);
+		PublicInbox::Git->new($self->{mainrepo});
+	};
 }
 
 sub mm {
 	my ($self) = @_;
-	$self->{mm} ||= eval { PublicInbox::Msgmap->new($self->{mainrepo}) };
+	$self->{mm} ||= eval {
+		_weaken_later($self);
+		PublicInbox::Msgmap->new($self->{mainrepo});
+	};
 }
 
 sub search {
 	my ($self) = @_;
-	$self->{search} ||= eval { PublicInbox::Search->new($self->{mainrepo}) };
+	$self->{search} ||= eval {
+		_weaken_later($self);
+		PublicInbox::Search->new($self->{mainrepo});
+	};
 }
 
 sub try_cat {
diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index 56d0838..0b30d43 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -38,8 +38,6 @@ my %DISABLED; # = map { $_ => 1 } qw(xover list_overview_fmt newnews xhdr);
 my $EXPMAP; # fd -> [ idle_time, $self ]
 my $expt;
 our $EXPTIME = 180; # 3 minutes
-my $WEAKEN = {}; # string(nntpd) -> nntpd
-my $weakt;
 my $nextt;
 
 my $nextq = [];
@@ -64,16 +62,6 @@ sub update_idle_time ($) {
 	defined $fd and $EXPMAP->{$fd} = [ now(), $self ];
 }
 
-# reduce FD pressure by closing some "git cat-file --batch" processes
-# and unused FDs for msgmap and Xapian indices
-sub weaken_groups () {
-	$weakt = undef;
-	foreach my $nntpd (values %$WEAKEN) {
-		$_->weaken_all foreach (@{$nntpd->{grouplist}});
-	}
-	$WEAKEN = {};
-}
-
 sub expire_old () {
 	my $now = now();
 	my $exp = $EXPTIME;
@@ -92,15 +80,11 @@ sub expire_old () {
 	$EXPMAP = \%new;
 	if ($nr) {
 		$expt = PublicInbox::EvCleanup::later(*expire_old);
-		weaken_groups();
 	} else {
 		$expt = undef;
 		# noop to kick outselves out of the loop ASAP so descriptors
 		# really get closed
 		PublicInbox::EvCleanup::asap(sub {});
-
-		# grace period for reaping resources
-		$weakt ||= PublicInbox::EvCleanup::later(*weaken_groups);
 	}
 }
 
@@ -113,7 +97,6 @@ sub new ($$$) {
 	$self->{rbuf} = '';
 	$self->watch_read(1);
 	update_idle_time($self);
-	$WEAKEN->{"$nntpd"} = $nntpd;
 	$expt ||= PublicInbox::EvCleanup::later(*expire_old);
 	$self;
 }
diff --git a/lib/PublicInbox/WWW.pm b/lib/PublicInbox/WWW.pm
index 5425308..5de5ffe 100644
--- a/lib/PublicInbox/WWW.pm
+++ b/lib/PublicInbox/WWW.pm
@@ -148,8 +148,6 @@ sub invalid_inbox {
 	if (defined $obj) {
 		$ctx->{git_dir} = $obj->{mainrepo};
 		$ctx->{git} = $obj->git;
-		# for PublicInbox::HTTP::weaken_task:
-		$ctx->{env}->{'pi-httpd.inbox'} = $obj;
 		$ctx->{-inbox} = $obj;
 		$ctx->{inbox} = $inbox;
 		return;
-- 
EW

[PATCH 4/4] www: remove old footer generation code and normalize new.html

[Eric Wong]

We now generate all of our HTML using WwwStream which
forces us to have consistent headers and footers in
the HTML itself.

This also makes the search-capable vs search-less installs
go to the new.html endpoint to maintain consistency
(in case an admin decides to enable Xapian).
---
 lib/PublicInbox/Feed.pm | 86 +++++++++++++------------------------------------
 lib/PublicInbox/View.pm | 47 +++++++++++++++++++++------
 lib/PublicInbox/WWW.pm  | 42 +-----------------------
 t/cgi.t                 | 17 +---------
 t/html_index.t          | 16 ---------
 t/plack.t               |  4 +--
 6 files changed, 63 insertions(+), 149 deletions(-)

diff --git a/lib/PublicInbox/Feed.pm b/lib/PublicInbox/Feed.pm
index 026a069..65adf37 100644
--- a/lib/PublicInbox/Feed.pm
+++ b/lib/PublicInbox/Feed.pm
@@ -31,7 +31,19 @@ sub generate_thread_atom {
 
 sub generate_html_index {
 	my ($ctx) = @_;
-	sub { emit_html_index($_[0], $ctx) };
+	# if the 'r' query parameter is given, it is a legacy permalink
+	# which we must continue supporting:
+	my $qp = $ctx->{qp};
+	if ($qp && !$qp->{r} && $ctx->{srch}) {
+		return PublicInbox::View::index_topics($ctx);
+	}
+
+	my $env = $ctx->{env};
+	my $url = $ctx->{-inbox}->base_url($env) . 'new.html';
+	my $qs = $env->{QUERY_STRING};
+	$url .= "?$qs" if $qs ne '';
+	[302, [ 'Location', $url, 'Content-Type', 'text/plain'],
+		[ "Redirecting to $url\n" ] ];
 }
 
 sub new_html {
@@ -56,7 +68,7 @@ sub new_html {
 			$s .= '</pre>' unless $more;
 			return $s;
 		}
-		undef;
+		new_html_footer($ctx, $last);
 	});
 }
 
@@ -159,74 +171,20 @@ sub _html_index_top {
 		"</head><body>$top";
 }
 
-sub emit_html_index {
-	my ($res, $ctx) = @_;
-	my $feed_opts = get_feedopts($ctx);
-	my $fh = $res->([200,['Content-Type'=>'text/html; charset=UTF-8']]);
-
-	my $max = $ctx->{max} || MAX_PER_PAGE;
-	$ctx->{-upfx} = '';
-
-	my ($footer, $param, $last);
-	$ctx->{seen} = {};
-	$ctx->{anchor_idx} = 0;
-	$ctx->{fh} = $fh;
-	my $srch = $ctx->{srch};
-	$fh->write(_html_index_top($feed_opts, $srch));
-
-	# if the 'r' query parameter is given, it is a legacy permalink
-	# which we must continue supporting:
-	my $qp = $ctx->{qp};
-	if ($qp && !$qp->{r} && $srch) {
-		$last = PublicInbox::View::emit_index_topics($ctx);
-		$param = 'o';
-	} else {
-		$last = emit_index_nosrch($ctx);
-		$param = 'r';
-	}
-	$footer = nav_footer($ctx, $last, $feed_opts, $param);
-	if ($footer) {
-		my $list_footer = $ctx->{footer};
-		$footer .= "\n\n" . $list_footer if $list_footer;
-		$footer = "<hr><pre>$footer</pre>";
-	}
-	$fh->write("$footer</body></html>");
-	$fh->close;
-}
-
-sub emit_index_nosrch {
-	my ($ctx) = @_;
-	my $ibx = $ctx->{-inbox};
-	my $fh = $ctx->{fh};
-	my (undef, $last) = each_recent_blob($ctx, sub {
-		my ($path, $commit, $ts, $u, $subj) = @_;
-		$ctx->{first} ||= $commit;
-
-		my $mime = do_cat_mail($ibx, $path) or return 0;
-		$fh->write(PublicInbox::View::index_entry($mime, $ctx, 1));
-		1;
-	});
-	$last;
-}
-
-sub nav_footer {
-	my ($ctx, $last, $feed_opts, $param) = @_;
-	my $qp = $ctx->{qp} or return '';
-	my $old_r = $qp->{$param};
-	my $head = '    ';
+sub new_html_footer {
+	my ($ctx, $last) = @_;
+	my $qp = delete $ctx->{qp} or return;
+	my $old_r = $qp->{r};
+	my $latest = '';
 	my $next = '    ';
-	my $first = $ctx->{first};
-	my $anchor = $ctx->{anchor_idx};
 
 	if ($last) {
-		$next = qq!<a\nhref="?$param=$last"\nrel=next>next</a>!;
+		$next = qq!<a\nhref="?r=$last"\nrel=next>next</a>!;
 	}
 	if ($old_r) {
-		$head = $ctx->{env}->{PATH_INFO};
-		$head = qq!<a\nhref="$head">head</a>!;
+		$latest = qq! <a\nhref='./new.html'>latest</a>!;
 	}
-	my $atom = "<a\nhref=\"$feed_opts->{atomurl}\">Atom feed</a>";
-	"<a\nname=\"s$anchor\">page:</a> $next $head $atom";
+	"<hr><pre>page: $next$latest</pre>";
 }
 
 sub each_recent_blob {
diff --git a/lib/PublicInbox/View.pm b/lib/PublicInbox/View.pm
index 7ff3b40..0903550 100644
--- a/lib/PublicInbox/View.pm
+++ b/lib/PublicInbox/View.pm
@@ -24,7 +24,7 @@ sub th_pfx ($) { $_[0] == 0 ? '' : TCHILD };
 
 # public functions: (unstable)
 sub msg_html {
-	my ($ctx, $mime, $footer) = @_;
+	my ($ctx, $mime) = @_;
 	my $hdr = $mime->header_obj;
 	my $tip = _msg_html_prepare($hdr, $ctx);
 	PublicInbox::WwwStream->response($ctx, 200, sub {
@@ -853,13 +853,15 @@ sub add_topic {
 	}
 }
 
-sub emit_topics {
+sub topics {
 	my ($ctx) = @_;
 	my $order = $ctx->{order};
 	my $subjs = $ctx->{subjs};
 	my $latest = $ctx->{latest};
-	my $fh = $ctx->{fh};
-	return $fh->write("\n[No topics in range]</pre>") unless scalar @$order;
+	if (!@$order) {
+		$ctx->{-html_tip} = '<pre>[No topics in range]</pre>';
+		return 404;
+	}
 	my $pfx;
 	my $prev = 0;
 	my $prev_attr = '';
@@ -903,13 +905,38 @@ sub emit_topics {
 	}
 	push @recent, $cur if $cur;
 	@recent = map { $_->[1] } sort { $b->[0] <=> $a->[0] } @recent;
-	$fh->write(join('', @recent) . '</pre>');
+	$ctx->{-html_tip} = join('', '<pre>', @recent, '</pre>');
+	200;
 }
 
-sub emit_index_topics {
+sub index_nav { # callback for WwwStream
+	my (undef, $ctx) = @_;
+	delete $ctx->{qp} or return;
+	my ($next, $prev);
+	$next = $prev = '    ';
+	my $latest = '';
+
+	my $next_o = $ctx->{-next_o};
+	if ($next_o) {
+		$next = qq!<a\nhref="?o=$next_o"\nrel=next>next</a>!;
+	}
+	if (my $cur_o = $ctx->{-cur_o}) {
+		$latest = qq! <a\nhref=.>latest</a>!;
+
+		my $o = $cur_o - ($next_o - $cur_o);
+		if ($o > 0) {
+			$prev = qq!<a\nhref="?o=$o"\nrel=prev>prev</a>!;
+		} elsif ($o == 0) {
+			$prev = qq!<a\nhref=.\nrel=prev>prev</a>!;
+		}
+	}
+	"<hr><pre>page: $next $prev$latest</pre>";
+}
+
+sub index_topics {
 	my ($ctx) = @_;
 	my ($off) = (($ctx->{qp}->{o} || '0') =~ /(\d+)/);
-	$ctx->{order} = [];
+	my $order = $ctx->{order} = [];
 	$ctx->{subjs} = {};
 	$ctx->{latest} = {};
 	my $max = 25;
@@ -921,9 +948,9 @@ sub emit_index_topics {
 		walk_thread(thread_results($sres), $ctx, *add_topic);
 		$opts{offset} += $nr;
 	}
-
-	emit_topics($ctx);
-	$opts{offset};
+	$ctx->{-next_o} = $opts{offset};
+	$ctx->{-cur_o} = $off;
+	PublicInbox::WwwStream->response($ctx, topics($ctx), *index_nav);
 }
 
 sub thread_adj_level {
diff --git a/lib/PublicInbox/WWW.pm b/lib/PublicInbox/WWW.pm
index 5de5ffe..2c60d59 100644
--- a/lib/PublicInbox/WWW.pm
+++ b/lib/PublicInbox/WWW.pm
@@ -198,7 +198,6 @@ sub get_index {
 	my ($ctx) = @_;
 	require PublicInbox::Feed;
 	my $srch = searcher($ctx);
-	footer($ctx);
 	if ($ctx->{env}->{QUERY_STRING} =~ /(?:\A|[&;])q=/) {
 		require PublicInbox::SearchView;
 		PublicInbox::SearchView::sres_top_html($ctx);
@@ -227,11 +226,10 @@ sub get_mid_html {
 	my $x = mid2blob($ctx) or return r404($ctx);
 
 	require PublicInbox::View;
-	my $foot = footer($ctx);
 	require Email::MIME;
 	my $mime = Email::MIME->new($x);
 	searcher($ctx);
-	PublicInbox::View::msg_html($ctx, $mime, $foot);
+	PublicInbox::View::msg_html($ctx, $mime);
 }
 
 # /$INBOX/$MESSAGE_ID/t/
@@ -250,44 +248,6 @@ sub ctx_get {
 	$val;
 }
 
-sub footer {
-	my ($ctx) = @_;
-	return '' unless $ctx;
-	my $obj = $ctx->{-inbox} or return '';
-
-	# auto-generate a footer
-	chomp(my $desc = $obj->description);
-	$desc = PublicInbox::Hval::ascii_html($desc);
-
-	my $urls;
-	my @urls = @{$obj->cloneurl};
-	my %seen = map { $_ => 1 } @urls;
-	my $env = $ctx->{env};
-	my $http = $obj->base_url($env);
-	chop $http;
-	$seen{$http} or unshift @urls, $http;
-	my $ssoma_url = PublicInbox::Hval::prurl($env, SSOMA_URL);
-	if (scalar(@urls) == 1) {
-		$urls = "URL for <a\nhref=\"" . $ssoma_url .
-			qq(">ssoma</a> or <b>git clone --mirror $urls[0]</b>);
-	} else {
-		$urls = "URLs for <a\nhref=\"" . $ssoma_url .
-			qq(">ssoma</a> or <b>git clone --mirror</b>\n) .
-			join("\n", map { "\tgit clone --mirror $_" } @urls);
-	}
-
-	my $addr = $obj->{-primary_address};
-	$ctx->{footer} = join("\n",
-		'- ' . $desc,
-		"A <a\nhref=\"" .
-			PublicInbox::Hval::prurl($ctx->{env}, PI_URL) .
-			'">public-inbox</a>, ' .
-			'anybody may post in plain-text (not HTML):',
-		$addr,
-		$urls
-	);
-}
-
 # search support is optional, returns undef if Xapian is not installed
 # or not configured for the given GIT_DIR
 sub searcher {
diff --git a/t/cgi.t b/t/cgi.t
index 720925a..a0f09c5 100644
--- a/t/cgi.t
+++ b/t/cgi.t
@@ -131,21 +131,6 @@ EOF
 	like($res->{body}, qr/what\?/, "reply included");
 }
 
-# indices
-{
-	local $ENV{HOME} = $home;
-	my $res = cgi_run("/test/");
-	like($res->{head}, qr/Status: 200 OK/, "index returns 200");
-
-	my $idx = cgi_run("/test/index.html");
-	$idx->{body} =~ s!/index.html(\?r=)!/$1!g; # dirty...
-	$idx->{body} = [ split(/\n/, $idx->{body}) ];
-	$res->{body} = [ split(/\n/, $res->{body}) ];
-	is_deeply($res, $idx,
-		'/$INBOX/ and /$INBOX/index.html are nearly identical');
-	# more checks in t/feed.t
-}
-
 # message-id pages
 {
 	local $ENV{HOME} = $home;
@@ -187,7 +172,7 @@ EOF
 	$res = cgi_run("/test/blahblah\@example.con/");
 	like($res->{head}, qr/Status: 300 Multiple Choices/, "mid html miss");
 
-	$res = cgi_run("/test/");
+	$res = cgi_run("/test/new.html");
 	like($res->{body}, qr/slashy%2Fasdf%40example\.com/,
 		"slashy URL generated correctly");
 }
diff --git a/t/html_index.t b/t/html_index.t
index 100d21a..f29b442 100644
--- a/t/html_index.t
+++ b/t/html_index.t
@@ -26,11 +26,6 @@ my $im = PublicInbox::Import->new($git, 'tester', 'test@example');
 	my $prev = "";
 
 	foreach my $i (1..6) {
-		# my $pid = open(my $pipe, "|-");
-		# defined $pid or die "fork/pipe failed: $!\n";
-		# if ($pid == 0) {
-			# exec("ssoma-mda", $git_dir);
-		# }
 		my $mid = "<$i\@example.com>";
 		my $mid_line = "Message-ID: $mid";
 		if ($prev) {
@@ -58,15 +53,4 @@ EOF
 	$im->done;
 }
 
-# check HTML index
-{
-	use IO::File;
-	my $cb = PublicInbox::Feed::generate_html_index({
-		-inbox => $ibx,
-		max => 3
-	});
-	require 't/common.perl';
-	like(stream_to_string($cb), qr/html/, "feed is valid HTML :)");
-}
-
 done_testing();
diff --git a/t/plack.t b/t/plack.t
index 40298e5..db3a9b2 100644
--- a/t/plack.t
+++ b/t/plack.t
@@ -120,9 +120,9 @@ EOF
 	test_psgi($app, sub {
 		my ($cb) = @_;
 		my $atomurl = 'http://example.com/test/new.atom';
-		my $res = $cb->(GET('http://example.com/test/'));
+		my $res = $cb->(GET('http://example.com/test/new.html'));
 		is(200, $res->code, 'success response received');
-		like($res->content, qr!href="\Q$atomurl\E"!,
+		like($res->content, qr!href="new\.atom"!,
 			'atom URL generated');
 		like($res->content, qr!href="blah%40example\.com/"!,
 			'index generated');
-- 
EW