[PATCH 00/11] viewvcs: more fixes

[PATCH 00/11] viewvcs: more fixes

[Eric Wong]

Been hammering away with check-www-inbox on git@vger without problems,
so I'll probably deploy this to the non-onion site soon...

And maybe I'll set the non-.onion to use 216dark, too...

Eric Wong (11):
  hval: add src_escape for highlight post-processing
  t/check-www-inbox: replace IPC::Run with PublicInbox::Spawn
  t/check-www-inbox: don't follow mboxes
  t/check-www-inbox: disable history
  solvergit: do not solve blobs twice
  viewvcs: avoid segfault with highlight.pm at shutdown
  css/216dark: add comments and tweak highlight colors
  solvergit: do not show full path names to "git apply"
  solvergit: avoid "Wide character" warnings
  solvergit: extract mode from diff headers properly
  solvergit: deal with alternative diff prefixes

 contrib/css/216dark.css        | 14 ++++++------
 lib/PublicInbox/Hval.pm        |  8 ++++++-
 lib/PublicInbox/SolverGit.pm   | 18 ++++++++++-----
 lib/PublicInbox/UserContent.pm | 14 ++++++------
 lib/PublicInbox/ViewVCS.pm     |  9 +++++++-
 t/check-www-inbox.perl         | 42 +++++++++++++++++++++++++++++-----
 t/hval.t                       |  3 +++
 7 files changed, 80 insertions(+), 28 deletions(-)

-- 
EW

[PATCH 01/11] hval: add src_escape for highlight post-processing

[Eric Wong]

We need to post-process "highlight" output to ensure it doesn't
contain odd bytes which cause "wide character" warnings or
require odd glyphs in source form.
---
 lib/PublicInbox/Hval.pm    | 8 +++++++-
 lib/PublicInbox/ViewVCS.pm | 4 +++-
 t/hval.t                   | 3 +++
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/Hval.pm b/lib/PublicInbox/Hval.pm
index 4d70d5e..53810b3 100644
--- a/lib/PublicInbox/Hval.pm
+++ b/lib/PublicInbox/Hval.pm
@@ -9,7 +9,7 @@ use warnings;
 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/;
+our @EXPORT_OK = qw/ascii_html obfuscate_addrs to_filename src_escape/;
 
 my $enc_ascii = find_encoding('us-ascii');
 
@@ -63,6 +63,12 @@ my %xhtml_map = (
 $xhtml_map{chr($_)} = sprintf('\\x%02x', $_) for (0..31);
 %xhtml_map = (%xhtml_map, %escape_sequence);
 
+sub src_escape ($) {
+	$_[0] =~ s/\r\n/\n/sg;
+	$_[0] =~ s/([\x7f\x00-\x1f])/$xhtml_map{$1}/sge;
+	$_[0] = $enc_ascii->encode($_[0], Encode::HTMLCREF);
+}
+
 sub ascii_html {
 	my ($s) = @_;
 	$s =~ s/\r\n/\n/sg; # fixup bad line endings
diff --git a/lib/PublicInbox/ViewVCS.pm b/lib/PublicInbox/ViewVCS.pm
index a8aa0b6..63e503d 100644
--- a/lib/PublicInbox/ViewVCS.pm
+++ b/lib/PublicInbox/ViewVCS.pm
@@ -20,7 +20,7 @@ use Encode qw(find_encoding);
 use PublicInbox::SolverGit;
 use PublicInbox::WwwStream;
 use PublicInbox::Linkify;
-use PublicInbox::Hval qw(ascii_html to_filename);
+use PublicInbox::Hval qw(ascii_html to_filename src_escape);
 my $hl = eval {
 	require PublicInbox::HlMod;
 	PublicInbox::HlMod->new;
@@ -96,6 +96,8 @@ sub solve_result {
 	$l->linkify_1($$blob);
 	my $ok = $hl->do_hl($blob, $path) if $hl;
 	if ($ok) {
+		$$ok = $enc_utf8->decode($$ok);
+		src_escape($$ok);
 		$blob = $ok;
 	} else {
 		$$blob = ascii_html($$blob);
diff --git a/t/hval.t b/t/hval.t
index a193c29..bfc9a85 100644
--- a/t/hval.t
+++ b/t/hval.t
@@ -43,5 +43,8 @@ is('foo-bar', PublicInbox::Hval::to_filename("foo   bar\nanother line\n"),
 is('foo.bar', PublicInbox::Hval::to_filename("foo....bar"),
 	'to_filename squeezes -');
 
+my $s = "\0\x07\n";
+PublicInbox::Hval::src_escape($s);
+is($s, "\\0\\a\n", 'src_escape works as intended');
 
 done_testing();
-- 
EW

[PATCH 02/11] t/check-www-inbox: replace IPC::Run with PublicInbox::Spawn

[Eric Wong]

Because WWW::Mechanize uses truckload of memory, fork
needs to prepare all that memory for CoW, which ends up
bailing with ENOMEM.
---
 t/check-www-inbox.perl | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/t/check-www-inbox.perl b/t/check-www-inbox.perl
index 1e88e95..933362a 100644
--- a/t/check-www-inbox.perl
+++ b/t/check-www-inbox.perl
@@ -14,6 +14,12 @@ use POSIX qw(:sys_wait_h);
 use Time::HiRes qw(gettimeofday tv_interval);
 use WWW::Mechanize;
 use Data::Dumper;
+
+# we want to use vfork+exec with spawn, WWW::Mechanize can use too much
+# memory and fork(2) fails
+use PublicInbox::Spawn qw(spawn which);
+$ENV{PERL_INLINE_DIRECTORY} or warn "PERL_INLINE_DIRECTORY unset, may OOM\n";
+
 our $tmp_owner = $$;
 my $nproc = 4;
 my $slow = 0.5;
@@ -24,13 +30,35 @@ my %opts = (
 GetOptions(%opts) or die "bad command-line args\n$usage";
 my $root_url = shift or die $usage;
 
-chomp(my $xmlstarlet = `which xmlstarlet 2>/dev/null`);
+chomp(my $xmlstarlet = which('xmlstarlet'));
 my $atom_check = eval {
-	require IPC::Run;
 	my $cmd = [ qw(xmlstarlet val -e -) ];
 	sub {
 		my ($in, $out, $err) = @_;
-		IPC::Run::run($cmd, $in, $out, $err);
+		use autodie;
+		open my $in_fh, '+>', undef;
+		open my $out_fh, '+>', undef;
+		open my $err_fh, '+>', undef;
+		print $in_fh $$in;
+		$in_fh->flush;
+		sysseek($in_fh, 0, 0);
+		my $rdr = {
+			0 => fileno($in_fh),
+			1 => fileno($out_fh),
+			2 => fileno($err_fh),
+		};
+		my $pid = spawn($cmd, undef, $rdr);
+		defined $pid or die "spawn failure: $!";
+		while (waitpid($pid, 0) != $pid) {
+			next if $!{EINTR};
+			warn "waitpid(xmlstarlet, $pid) $!";
+			return $!;
+		}
+		sysseek($out_fh, 0, 0);
+		sysread($out_fh, $$out, -s $out_fh);
+		sysseek($err_fh, 0, 0);
+		sysread($err_fh, $$err, -s $err_fh);
+		$?
 	}
 } if $xmlstarlet;
 
@@ -120,6 +148,7 @@ while (keys %workers) { # reacts to SIGCHLD
 
 sub worker_loop {
 	my ($todo_rd, $done_wr) = @_;
+	$SIG{CHLD} = 'DEFAULT';
 	my $m = WWW::Mechanize->new(autocheck => 0);
 	my $cc = LWP::ConnCache->new;
 	$m->conn_cache($cc);
@@ -164,8 +193,8 @@ sub worker_loop {
 		if ($atom_check && $ct =~ m!\bapplication/atom\+xml\b!) {
 			my $raw = $r->decoded_content;
 			my ($out, $err) = ('', '');
-			$atom_check->(\$raw, \$out, \$err) and
-				warn "Atom ($?) - $u - <1:$out> <2:$err>\n";
+			my $fail = $atom_check->(\$raw, \$out, \$err);
+			warn "Atom ($fail) - $u - <1:$out> <2:$err>\n" if $fail;
 		}
 
 		next if $ct !~ m!\btext/html\b!;
-- 
EW

[PATCH 03/11] t/check-www-inbox: don't follow mboxes

[Eric Wong]

They can be extremely large with no limit, so can lead to OOM
errors.
---
 t/check-www-inbox.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/check-www-inbox.perl b/t/check-www-inbox.perl
index 933362a..0a6d61b 100644
--- a/t/check-www-inbox.perl
+++ b/t/check-www-inbox.perl
@@ -175,7 +175,7 @@ sub worker_loop {
 		my $s;
 		# blocking
 		foreach my $l (@links, "DONE\t$u") {
-			next if $l eq '';
+			next if $l eq '' || $l =~ /\.mbox(?:\.gz)\z/;
 			do {
 				$s = $done_wr->send($l, MSG_EOR);
 			} while (!defined $s && $!{EINTR});
-- 
EW

[PATCH 04/11] t/check-www-inbox: disable history

[Eric Wong]

WWW::Mechanize keeps an infinitely large stack, which was
leading to OOM errors on my system.
---
 t/check-www-inbox.perl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/check-www-inbox.perl b/t/check-www-inbox.perl
index 0a6d61b..db292c5 100644
--- a/t/check-www-inbox.perl
+++ b/t/check-www-inbox.perl
@@ -151,6 +151,7 @@ sub worker_loop {
 	$SIG{CHLD} = 'DEFAULT';
 	my $m = WWW::Mechanize->new(autocheck => 0);
 	my $cc = LWP::ConnCache->new;
+	$m->stack_depth(0); # no history
 	$m->conn_cache($cc);
 	while (1) {
 		$todo_rd->recv(my $u, 65535, 0);
-- 
EW

[PATCH 05/11] solvergit: do not solve blobs twice

[Eric Wong]

In some cases, a file may ping-pong between blob IDs in the same
message when reverts occur.  So break out of this early.

This doesn't account for different abbreviations, but the
limited variations of abbreviations should alleviate the
problem.
---
 lib/PublicInbox/SolverGit.pm | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/PublicInbox/SolverGit.pm b/lib/PublicInbox/SolverGit.pm
index 39acbe4..24d9da2 100644
--- a/lib/PublicInbox/SolverGit.pm
+++ b/lib/PublicInbox/SolverGit.pm
@@ -439,6 +439,9 @@ sub resolve_patch ($$) {
 
 	# see if we can find the blob in an existing git repo:
 	my $cur_want = $want->{oid_b};
+	if ($self->{seen_oid}->{$cur_want}++) {
+		die "Loop detected solving $cur_want\n";
+	}
 	if (my $existing = solve_existing($self, $want)) {
 		dbg($self, "found $cur_want in " .
 			join("\n", $existing->[0]->pub_urls));
@@ -504,6 +507,7 @@ sub solve ($$$$$) {
 
 	$self->{oid_want} = $oid_want;
 	$self->{out} = $out;
+	$self->{seen_oid} = {};
 	$self->{tot} = 0;
 	$self->{psgi_env} = $env;
 	$self->{todo} = [ { %$hints, oid_b => $oid_want } ];
-- 
EW

[PATCH 06/11] viewvcs: avoid segfault with highlight.pm at shutdown

[Eric Wong]

Proper ordering of destruction seems required to avoid segfaults
at shutdown.
---
 lib/PublicInbox/ViewVCS.pm | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/PublicInbox/ViewVCS.pm b/lib/PublicInbox/ViewVCS.pm
index 63e503d..85edf22 100644
--- a/lib/PublicInbox/ViewVCS.pm
+++ b/lib/PublicInbox/ViewVCS.pm
@@ -26,6 +26,11 @@ my $hl = eval {
 	PublicInbox::HlMod->new;
 };
 
+# we need to trigger highlight::CodeGenerator::deleteInstance
+# in HlMod::DESTROY before the rest of Perl shuts down to avoid
+# a segfault at shutdown
+END { $hl = undef };
+
 my %QP_MAP = ( A => 'oid_a', B => 'oid_b', a => 'path_a', b => 'path_b' );
 my $max_size = 1024 * 1024; # TODO: configurable
 my $enc_utf8 = find_encoding('UTF-8');
-- 
EW

[PATCH 07/11] css/216dark: add comments and tweak highlight colors

[Eric Wong]

Overkill, but "highlight" supports single-line comments (slc)
independently of multi-line comments (com); but we'll use the
same color for that.

We'll also use #0f0 instead of #0ff for "kwb" (keyword class "b")
since blue shades are prevalent in <a> links and comments, while
green was unused.
---
 contrib/css/216dark.css        | 14 +++++++-------
 lib/PublicInbox/UserContent.pm | 14 +++++++-------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/contrib/css/216dark.css b/contrib/css/216dark.css
index 35ef7aa..882fbc4 100644
--- a/contrib/css/216dark.css
+++ b/contrib/css/216dark.css
@@ -30,17 +30,17 @@ a:visited { color:#96f }
  * this doesn't use most of the colors available (I find too many
  * colors overwhelming).  So the #ccc default is commented out.
  */
-.hl.num { color:#f30 }
-.hl.esc { color:#f0f }
-.hl.str { color:#f30 }
-.hl.pps { color:#f30 }
-/* .hl.slc { color:#ccc } */
+.hl.num { color:#f30 } /* number */
+.hl.esc { color:#f0f } /* escape character */
+.hl.str { color:#f30 } /* string */
+.hl.ppc { color:#f0f } /* preprocessor */
+.hl.pps { color:#f30 } /* preprocessor string */
+.hl.slc { color:#09f } /* single-line comment */
 .hl.com { color:#09f }
-.hl.ppc { color:#f0f }
 /* .hl.opt { color:#ccc } */
 /* .hl.ipl { color:#ccc } */
 /* .hl.lin { color:#ccc } */
 .hl.kwa { color:#ff0 }
-.hl.kwb { color:#0ff }
+.hl.kwb { color:#0f0 }
 .hl.kwc { color:#ff0 }
 /* .hl.kwd { color:#ccc } */
diff --git a/lib/PublicInbox/UserContent.pm b/lib/PublicInbox/UserContent.pm
index 514cd95..df0429c 100644
--- a/lib/PublicInbox/UserContent.pm
+++ b/lib/PublicInbox/UserContent.pm
@@ -42,18 +42,18 @@ sub CSS () {
 	 * this doesn't use most of the colors available (I find too many
 	 * colors overwhelming).  So the #ccc default is commented out.
 	 */
-	.hl.num { color:#f30 }
-	.hl.esc { color:#f0f }
-	.hl.str { color:#f30 }
-	.hl.pps { color:#f30 }
-	/* .hl.slc { color:#ccc } */
+	.hl.num { color:#f30 } /* number */
+	.hl.esc { color:#f0f } /* escape character */
+	.hl.str { color:#f30 } /* string */
+	.hl.ppc { color:#f0f } /* preprocessor */
+	.hl.pps { color:#f30 } /* preprocessor string */
+	.hl.slc { color:#09f } /* single-line comment */
 	.hl.com { color:#09f }
-	.hl.ppc { color:#f0f }
 	/* .hl.opt { color:#ccc } */
 	/* .hl.ipl { color:#ccc } */
 	/* .hl.lin { color:#ccc } */
 	.hl.kwa { color:#ff0 }
-	.hl.kwb { color:#0ff }
+	.hl.kwb { color:#0f0 }
 	.hl.kwc { color:#ff0 }
 	/* .hl.kwd { color:#ccc } */
 _
-- 
EW

[PATCH 08/11] solvergit: do not show full path names to "git apply"

[Eric Wong]

"git apply" will warn about whitespace with the full path of the
patch, which will expose the $TMPDIR environment to users over
HTTP(S).

This change breaks compatibility with git pre-1.8.5, again;
but that was released in late-2013; so hopefully everybody
is on newer versions.
---
 lib/PublicInbox/SolverGit.pm | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/lib/PublicInbox/SolverGit.pm b/lib/PublicInbox/SolverGit.pm
index 24d9da2..891cde2 100644
--- a/lib/PublicInbox/SolverGit.pm
+++ b/lib/PublicInbox/SolverGit.pm
@@ -389,8 +389,8 @@ sub do_git_apply ($) {
 	my $patches = $self->{patches};
 
 	# we need --ignore-whitespace because some patches are CRLF
-	my @cmd = qw(git apply --cached --ignore-whitespace
-			--whitespace=warn --verbose);
+	my @cmd = (qw(git -C), $dn, qw(apply --cached --ignore-whitespace
+			--whitespace=warn --verbose));
 	my $len = length(join(' ', @cmd));
 	my $total = $self->{tot};
 	my $di; # keep track of the last one for "git ls-files"
@@ -400,8 +400,7 @@ sub do_git_apply ($) {
 		$di = shift @$patches;
 		dbg($self, "\napplying [$i/$total] " . di_url($self, $di) .
 			"\n" . join('', @{$di->{hdr_lines}}));
-		my $pn = $total + 1 - $i;
-		my $path = "$dn/$pn";
+		my $path = $total + 1 - $i;
 		$len += length($path) + 1;
 		push @cmd, $path;
 	} while (@$patches && $len < $ARG_SIZE_MAX);
-- 
EW

[PATCH 09/11] solvergit: avoid "Wide character" warnings

[Eric Wong]

Just quiet Perl down, since we don't know or care about the
encoding of the patch we hand off to git-apply.
---
 lib/PublicInbox/SolverGit.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/PublicInbox/SolverGit.pm b/lib/PublicInbox/SolverGit.pm
index 891cde2..1b1951b 100644
--- a/lib/PublicInbox/SolverGit.pm
+++ b/lib/PublicInbox/SolverGit.pm
@@ -125,6 +125,7 @@ sub extract_diff ($$$$$) {
 
 			push @$hdr_lines, $l;
 			$di->{hdr_lines} = $hdr_lines;
+			utf8::encode($_) for @$hdr_lines;
 			print $tmp @$hdr_lines or die "print(tmp): $!";
 
 			# for debugging/diagnostics:
@@ -153,6 +154,7 @@ sub extract_diff ($$$$$) {
 			$di->{path_b} = join('/', @b);
 			$hdr_lines = [ $l ];
 		} elsif ($tmp) {
+			utf8::encode($l);
 			print $tmp $l or die "print(tmp): $!";
 		} elsif ($hdr_lines) {
 			push @$hdr_lines, $l;
-- 
EW

[PATCH 10/11] solvergit: extract mode from diff headers properly

[Eric Wong]

grep() won't set $1, so use "=~", instead.
---
 lib/PublicInbox/SolverGit.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/PublicInbox/SolverGit.pm b/lib/PublicInbox/SolverGit.pm
index 1b1951b..d9b312c 100644
--- a/lib/PublicInbox/SolverGit.pm
+++ b/lib/PublicInbox/SolverGit.pm
@@ -286,7 +286,8 @@ EOF
 
 sub extract_old_mode ($) {
 	my ($di) = @_;
-	if (grep(/\Aold mode (100644|100755|120000)$/, @{$di->{hdr_lines}})) {
+	if (join('', @{$di->{hdr_lines}}) =~
+			/^old mode (100644|100755|120000)\b/) {
 		return $1;
 	}
 	'100644';
-- 
EW

[PATCH 11/11] solvergit: deal with alternative diff prefixes

[Eric Wong]

At least, without extra directory levels, since
git-diff supports --src-prefix and --dst-prefix,
and /git/6aa8857a11/s/ uses it...
---
 lib/PublicInbox/SolverGit.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/PublicInbox/SolverGit.pm b/lib/PublicInbox/SolverGit.pm
index d9b312c..d787533 100644
--- a/lib/PublicInbox/SolverGit.pm
+++ b/lib/PublicInbox/SolverGit.pm
@@ -131,7 +131,7 @@ sub extract_diff ($$$$$) {
 			# for debugging/diagnostics:
 			$di->{ibx} = $ibx;
 			$di->{smsg} = $smsg;
-		} elsif ($l =~ m!\Adiff --git ("?a/.+) ("?b/.+)$!) {
+		} elsif ($l =~ m!\Adiff --git ("?[^/]+/.+) ("?[^/]+/.+)$!) {
 			last if $tmp; # got our blob, done!
 
 			my ($path_a, $path_b) = ($1, $2);
-- 
EW