[PATCH 0/9] more small fixes and cleanups

[PATCH 0/9] more small fixes and cleanups

[Eric Wong]

One bugfix for solver, a couple of optimizations which will
make sense when dealing with many and larger code repos.
And some cleanups to reduce redundant code.

Eric Wong (9):
  config: do not slurp entire cgitrc at once
  git: modified: don't slurp `rev-parse --branches'
  git: packed_bytes: use GLOB_NOSORT
  solver: path_a may be undef from /dev/null
  cgit: drop cgit_parse_hdr wrapper
  xapcmd: use popen_rd for running xapian-compact
  xt/git_async_cmp: do not slurp large OID list into memory
  t/solver_git: avoid uninitialized warnings in hostname generation
  use popen_rd for bidirectional pipes

 lib/PublicInbox/Cgit.pm       |  9 ++-------
 lib/PublicInbox/Config.pm     |  4 ++--
 lib/PublicInbox/Git.pm        | 24 +++++++++++-------------
 lib/PublicInbox/Import.pm     |  8 +++-----
 lib/PublicInbox/SolverGit.pm  |  4 ++--
 lib/PublicInbox/V2Writable.pm | 11 +++--------
 lib/PublicInbox/Xapcmd.pm     | 16 +++++-----------
 t/solver_git.t                | 12 ++++++------
 xt/git_async_cmp.t            |  4 ++--
 9 files changed, 36 insertions(+), 56 deletions(-)

DELETE! DELETE! DELETE!

[PATCH 1/9] config: do not slurp entire cgitrc at once

[Eric Wong]

cgitrc files can have hundreds or thousands of lines in them and
slurping them into memory is a waste.  "while (<$fh>)" only
reads one line at a time, whereas "for (<$fh>)" reads the entire
contents of the file into a temporary array.
---
 lib/PublicInbox/Config.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/Config.pm b/lib/PublicInbox/Config.pm
index 56d146c2..1ba1225e 100644
--- a/lib/PublicInbox/Config.pm
+++ b/lib/PublicInbox/Config.pm
@@ -249,7 +249,7 @@ sub scan_projects_coderepo ($$$) {
 		warn "failed to open cgit projectlist=$list: $!\n";
 		return;
 	};
-	foreach (<$fh>) {
+	while (<$fh>) {
 		chomp;
 		scan_path_coderepo($self, $path, "$path/$_");
 	}
@@ -274,7 +274,7 @@ sub parse_cgitrc {
 
 	# FIXME: this doesn't support macro expansion via $VARS, yet
 	my $repo;
-	foreach (<$fh>) {
+	while (<$fh>) {
 		chomp;
 		if (m!\Arepo\.url=(.+?)/*\z!) {
 			my $nick = $1;

[PATCH 2/9] git: modified: don't slurp `rev-parse --branches'

[Eric Wong]

While v1 inboxes typically only have one branch, code repositories
may have dozens or even hundreds.  Slurping those into memory is
a waste.
---
 lib/PublicInbox/Git.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/PublicInbox/Git.pm b/lib/PublicInbox/Git.pm
index 8ee04e17..0ace907e 100644
--- a/lib/PublicInbox/Git.pm
+++ b/lib/PublicInbox/Git.pm
@@ -347,7 +347,7 @@ sub modified ($) {
 	my $fh = popen($self, qw(rev-parse --branches));
 	cat_async_begin($self);
 	local $/ = "\n";
-	foreach my $oid (<$fh>) {
+	while (my $oid = <$fh>) {
 		chomp $oid;
 		cat_async($self, $oid, \&extract_cmt_time, \$modified);
 	}

[PATCH 3/9] git: packed_bytes: use GLOB_NOSORT

[Eric Wong]

File::Glob is loaded by the perl for the "glob()" op, anyways,
so call bsd_glob with the GLOB_NOSORT to avoid needless sorting
of the output.
---
 lib/PublicInbox/Git.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/PublicInbox/Git.pm b/lib/PublicInbox/Git.pm
index 0ace907e..15f53495 100644
--- a/lib/PublicInbox/Git.pm
+++ b/lib/PublicInbox/Git.pm
@@ -11,6 +11,7 @@ use strict;
 use warnings;
 use POSIX qw(dup2);
 use IO::Handle; # ->autoflush
+use File::Glob qw(bsd_glob GLOB_NOSORT);
 use PublicInbox::Spawn qw(spawn popen_rd);
 use PublicInbox::Tmpfile;
 use base qw(Exporter);
@@ -276,7 +277,7 @@ sub packed_bytes {
 	my ($self) = @_;
 	my $n = 0;
 	my $pack_dir = git_path($self, 'objects/pack');
-	foreach my $p (glob("$pack_dir/*.pack")) {
+	foreach my $p (bsd_glob("$pack_dir/*.pack", GLOB_NOSORT)) {
 		$n += -s $p;
 	}
 	$n

[PATCH 4/9] solver: path_a may be undef from /dev/null

[Eric Wong]

This avoids uninitialized variable warnings when viewing
newly-created files.
---
 lib/PublicInbox/SolverGit.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/SolverGit.pm b/lib/PublicInbox/SolverGit.pm
index 8629f0da..62c925d4 100644
--- a/lib/PublicInbox/SolverGit.pm
+++ b/lib/PublicInbox/SolverGit.pm
@@ -169,7 +169,7 @@ sub extract_diff ($$) {
 	my $patch = $9;
 
 	# don't care for leading 'a/' and 'b/'
-	my (undef, @a) = split(m{/}, git_unquote($path_a));
+	my (undef, @a) = split(m{/}, git_unquote($path_a)) if defined($path_a);
 	my (undef, @b) = split(m{/}, git_unquote($path_b));
 
 	# get rid of path-traversal attempts and junk patches:
@@ -177,7 +177,7 @@ sub extract_diff ($$) {
 	state $bad_component = { map { $_ => 1 } ('', '.', '..') };
 	foreach (@a, @b) { return if $bad_component->{$_} }
 
-	$di->{path_a} = join('/', @a);
+	$di->{path_a} = join('/', @a) if @a;
 	$di->{path_b} = join('/', @b);
 
 	my $path = ++$self->{tot};

[PATCH 5/9] cgit: drop cgit_parse_hdr wrapper

[Eric Wong]

Unlike PublicInbox::GitHTTPBackend::git_parse_hdr,
cgit_parse_hdr does nothing interesting besides calling
parse_cgi_headers.  So just make a reference to
PublicInbox::GitHTTPBackend::parse_cgi_headers and call it.
---
 lib/PublicInbox/Cgit.pm | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/lib/PublicInbox/Cgit.pm b/lib/PublicInbox/Cgit.pm
index c42f8847..d9b7831f 100644
--- a/lib/PublicInbox/Cgit.pm
+++ b/lib/PublicInbox/Cgit.pm
@@ -11,7 +11,6 @@ use PublicInbox::GitHTTPBackend;
 use PublicInbox::Git;
 # not bothering with Exporter for a one-off
 *input_prepare = *PublicInbox::GitHTTPBackend::input_prepare;
-*parse_cgi_headers = *PublicInbox::GitHTTPBackend::parse_cgi_headers;
 *serve = *PublicInbox::GitHTTPBackend::serve;
 use warnings;
 use PublicInbox::Qspawn;
@@ -94,11 +93,7 @@ my @PASS_ENV = qw(
 );
 # XXX: cgit filters may care about more variables...
 
-sub cgit_parse_hdr { # {parse_hdr} for Qspawn
-	my ($r, $bref) = @_;
-	my $res = parse_cgi_headers($r, $bref) or return; # incomplete
-	$res;
-}
+my $parse_cgi_headers = \&PublicInbox::GitHTTPBackend::parse_cgi_headers;
 
 sub call {
 	my ($self, $env) = @_;
@@ -127,7 +122,7 @@ sub call {
 	my $rdr = input_prepare($env) or return r(500);
 	my $qsp = PublicInbox::Qspawn->new($self->{cmd}, $cgi_env, $rdr);
 	my $limiter = $self->{pi_config}->limiter('-cgit');
-	$qsp->psgi_return($env, $limiter, \&cgit_parse_hdr);
+	$qsp->psgi_return($env, $limiter, $parse_cgi_headers);
 }
 
 1;

[PATCH 6/9] xapcmd: use popen_rd for running xapian-compact

[Eric Wong]

public-inbox-compact wrapper displays progress by default,
anyways, and there's not a lot of output, so simplify our
code by using popen_rd instead of spawn + optional pipe.

While we're at it use "while (<HANDLE>)" to display
progress as it happens, since "foreach (<$HANDLE>)"
slurps the contents into an array, first.
---
 lib/PublicInbox/Xapcmd.pm | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/lib/PublicInbox/Xapcmd.pm b/lib/PublicInbox/Xapcmd.pm
index 4871378e..de2ef5c6 100644
--- a/lib/PublicInbox/Xapcmd.pm
+++ b/lib/PublicInbox/Xapcmd.pm
@@ -3,7 +3,7 @@
 package PublicInbox::Xapcmd;
 use strict;
 use warnings;
-use PublicInbox::Spawn qw(which spawn);
+use PublicInbox::Spawn qw(which popen_rd);
 use PublicInbox::Over;
 use PublicInbox::SearchIdx;
 use File::Temp 0.19 (); # ->newdir
@@ -277,7 +277,6 @@ sub compact ($$) {
 	my ($args, $opt) = @_;
 	my ($src, $newdir) = @$args;
 	my $dst = ref($newdir) ? $newdir->dirname : $newdir;
-	my ($r, $w);
 	my $pfx = $opt->{-progress_pfx} ||= progress_pfx($src);
 	my $pr = $opt->{-progress};
 	my $rdr = {};
@@ -286,7 +285,6 @@ sub compact ($$) {
 		defined(my $dfd = $opt->{$fd}) or next;
 		$rdr->{$fd} = $dfd;
 	}
-	$rdr->{1} = $w if $pr && pipe($r, $w);
 
 	# we rely on --no-renumber to keep docids synched to NNTP
 	my $cmd = [ $XAPIAN_COMPACT, '--no-renumber' ];
@@ -299,18 +297,14 @@ sub compact ($$) {
 	}
 	$pr->("$pfx `".join(' ', @$cmd)."'\n") if $pr;
 	push @$cmd, $src, $dst;
-	my $pid = spawn($cmd, undef, $rdr);
-	if ($pr) {
-		close $w or die "close: \$w: $!";
-		foreach (<$r>) {
+	my $rd = popen_rd($cmd, undef, $rdr);
+	while (<$rd>) {
+		if ($pr) {
 			s/\r/\r$pfx /g;
 			$pr->("$pfx $_");
 		}
 	}
-	my $rp = waitpid($pid, 0);
-	if ($? || $rp != $pid) {
-		die join(' ', @$cmd)." failed: $? (pid=$pid, reaped=$rp)\n";
-	}
+	close $rd or die join(' ', @$cmd)." failed: $?n";
 }
 
 sub cpdb_loop ($$$;$$) {

[PATCH 7/9] xt/git_async_cmp: do not slurp large OID list into memory

[Eric Wong]

I somehow thought "foreach (<$cat>)" could work like
"while (<$cat>)" when it came to iterating over file
handles...
---
 xt/git_async_cmp.t | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xt/git_async_cmp.t b/xt/git_async_cmp.t
index f8ffe3d9..18fce3db 100644
--- a/xt/git_async_cmp.t
+++ b/xt/git_async_cmp.t
@@ -27,7 +27,7 @@ my $async = timeit($nr, sub {
 	my $cat = $git->popen(@cat);
 	$git->cat_async_begin;
 
-	foreach (<$cat>) {
+	while (<$cat>) {
 		my ($oid, undef, undef) = split(/ /);
 		$git->cat_async($oid, $cb);
 	}
@@ -39,7 +39,7 @@ my $async = timeit($nr, sub {
 my $sync = timeit($nr, sub {
 	my $dig = Digest::SHA->new(1);
 	my $cat = $git->popen(@cat);
-	foreach (<$cat>) {
+	while (<$cat>) {
 		my ($oid, undef, undef) = split(/ /);
 		my $bref = $git->cat_file($oid);
 		$dig->add($$bref);

[PATCH 8/9] t/solver_git: avoid uninitialized warnings in hostname generation

[Eric Wong]

Outside of tests, this is only relevant for non-PSGI use, which
may happen someday...

Fixes: cb1c874520153f5c ("inbox: use PublicInbox::Git::host_prefix_url for base_url")
---
 t/solver_git.t | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/solver_git.t b/t/solver_git.t
index 77fa1e09..98317fae 100644
--- a/t/solver_git.t
+++ b/t/solver_git.t
@@ -42,7 +42,8 @@ $ibx->{-repo_objs} = [ $git ];
 my $res;
 my $solver = PublicInbox::SolverGit->new($ibx, sub { $res = $_[0] });
 open my $log, '+>>', "$inboxdir/solve.log" or die "open: $!";
-my $psgi_env = { 'psgi.errors' => *STDERR, 'psgi.url_scheme' => 'http' };
+my $psgi_env = { 'psgi.errors' => *STDERR, 'psgi.url_scheme' => 'http',
+		'HTTP_HOST' => 'example.com' };
 $solver->solve($psgi_env, $log, '69df7d5', {});
 ok($res, 'solved a blob!');
 my $wt_git = $res->[0];

[PATCH 9/9] use popen_rd for bidirectional pipes

[Eric Wong]

popen_rd accepts arbitrary redirects, so we can reuse its
code to setup the pipe end we want to read, saving each
caller a few lines of code compared to calling pipe+spawn.
---
 lib/PublicInbox/Git.pm        | 19 ++++++++-----------
 lib/PublicInbox/Import.pm     |  8 +++-----
 lib/PublicInbox/V2Writable.pm | 11 +++--------
 t/solver_git.t                |  9 ++++-----
 4 files changed, 18 insertions(+), 29 deletions(-)

diff --git a/lib/PublicInbox/Git.pm b/lib/PublicInbox/Git.pm
index 15f53495..4f230197 100644
--- a/lib/PublicInbox/Git.pm
+++ b/lib/PublicInbox/Git.pm
@@ -12,7 +12,7 @@ use warnings;
 use POSIX qw(dup2);
 use IO::Handle; # ->autoflush
 use File::Glob qw(bsd_glob GLOB_NOSORT);
-use PublicInbox::Spawn qw(spawn popen_rd);
+use PublicInbox::Spawn qw(popen_rd);
 use PublicInbox::Tmpfile;
 use base qw(Exporter);
 our @EXPORT_OK = qw(git_unquote git_quote);
@@ -104,27 +104,24 @@ sub _bidi_pipe {
 		}
 		return;
 	}
-	my ($in_r, $in_w, $out_r, $out_w);
-
-	pipe($in_r, $in_w) or fail($self, "pipe failed: $!");
+	my ($out_r, $out_w);
 	pipe($out_r, $out_w) or fail($self, "pipe failed: $!");
-	if ($^O eq 'linux') { # 1031: F_SETPIPE_SZ
-		fcntl($out_w, 1031, 4096);
-		fcntl($in_w, 1031, 4096) if $batch eq '--batch-check';
-	}
-
 	my @cmd = (qw(git), "--git-dir=$self->{git_dir}",
 			qw(-c core.abbrev=40 cat-file), $batch);
-	my $redir = { 0 => $out_r, 1 => $in_w };
+	my $redir = { 0 => $out_r };
 	if ($err) {
 		my $id = "git.$self->{git_dir}$batch.err";
 		my $fh = tmpfile($id) or fail($self, "tmpfile($id): $!");
 		$self->{$err} = $fh;
 		$redir->{2} = $fh;
 	}
-	my $p = spawn(\@cmd, undef, $redir);
+	my ($in_r, $p) = popen_rd(\@cmd, undef, $redir);
 	$self->{$pid} = $p;
 	$out_w->autoflush(1);
+	if ($^O eq 'linux') { # 1031: F_SETPIPE_SZ
+		fcntl($out_w, 1031, 4096);
+		fcntl($in_r, 1031, 4096) if $batch eq '--batch-check';
+	}
 	$self->{$out} = $out_w;
 	$self->{$in} = $in_r;
 }
diff --git a/lib/PublicInbox/Import.pm b/lib/PublicInbox/Import.pm
index 6ac43d37..d279fea1 100644
--- a/lib/PublicInbox/Import.pm
+++ b/lib/PublicInbox/Import.pm
@@ -9,7 +9,7 @@ package PublicInbox::Import;
 use strict;
 use warnings;
 use base qw(PublicInbox::Lock);
-use PublicInbox::Spawn qw(spawn);
+use PublicInbox::Spawn qw(spawn popen_rd);
 use PublicInbox::MID qw(mids mid2path);
 use PublicInbox::Address;
 use PublicInbox::MsgTime qw(msg_timestamp msg_datestamp);
@@ -46,8 +46,7 @@ sub gfi_start {
 
 	return ($self->{in}, $self->{out}) if $self->{pid};
 
-	my ($in_r, $in_w, $out_r, $out_w);
-	pipe($in_r, $in_w) or die "pipe failed: $!";
+	my ($out_r, $out_w);
 	pipe($out_r, $out_w) or die "pipe failed: $!";
 	my $git = $self->{git};
 
@@ -66,8 +65,7 @@ sub gfi_start {
 	my $git_dir = $git->{git_dir};
 	my @cmd = ('git', "--git-dir=$git_dir", qw(fast-import
 			--quiet --done --date-format=raw));
-	my $rdr = { 0 => $out_r, 1 => $in_w };
-	my $pid = spawn(\@cmd, undef, $rdr);
+	my ($in_r, $pid) = popen_rd(\@cmd, undef, { 0 => $out_r });
 	$out_w->autoflush(1);
 	$self->{in} = $in_r;
 	$self->{out} = $out_w;
diff --git a/lib/PublicInbox/V2Writable.pm b/lib/PublicInbox/V2Writable.pm
index 51794326..9073d9ef 100644
--- a/lib/PublicInbox/V2Writable.pm
+++ b/lib/PublicInbox/V2Writable.pm
@@ -16,7 +16,7 @@ use PublicInbox::ContentId qw(content_id content_digest);
 use PublicInbox::Inbox;
 use PublicInbox::OverIdx;
 use PublicInbox::Msgmap;
-use PublicInbox::Spawn qw(spawn);
+use PublicInbox::Spawn qw(spawn popen_rd);
 use PublicInbox::SearchIdx;
 use IO::Handle; # ->autoflush
 use File::Temp qw(tempfile);
@@ -471,17 +471,12 @@ sub git_hash_raw ($$) {
 	print $tmp_fh $$raw or die "print \$tmp_fh: $!";
 	sysseek($tmp_fh, 0, 0) or die "seek failed: $!";
 
-	my ($r, $w);
-	pipe($r, $w) or die "failed to create pipe: $!";
-	my $rdr = { 0 => $tmp_fh, 1 => $w };
 	my $git_dir = $self->{-inbox}->git->{git_dir};
 	my $cmd = ['git', "--git-dir=$git_dir", qw(hash-object --stdin)];
-	my $pid = spawn($cmd, undef, $rdr);
-	close $w;
+	my $r = popen_rd($cmd, undef, { 0 => $tmp_fh });
 	local $/ = "\n";
 	chomp(my $oid = <$r>);
-	waitpid($pid, 0) == $pid or die "git hash-object did not finish";
-	die "git hash-object failed: $?" if $?;
+	close $r or die "git hash-object failed: $?";
 	$oid =~ /\A[a-f0-9]{40}\z/ or die "OID not expected: $oid";
 	$oid;
 }
diff --git a/t/solver_git.t b/t/solver_git.t
index 98317fae..92402c3a 100644
--- a/t/solver_git.t
+++ b/t/solver_git.t
@@ -6,7 +6,7 @@ use Test::More;
 use Cwd qw(abs_path);
 use PublicInbox::TestCommon;
 require_git(2.6);
-use PublicInbox::Spawn qw(spawn);
+use PublicInbox::Spawn qw(popen_rd);
 require_mods(qw(DBD::SQLite Search::Xapian Plack::Util));
 chomp(my $git_dir = `git rev-parse --git-dir 2>/dev/null`);
 plan skip_all => "$0 must be run from a git working tree" if $?;
@@ -120,14 +120,13 @@ SKIP: {
 	my $cmd = [ qw(git hash-object -w --stdin) ];
 	my $env = { GIT_DIR => $binfoo };
 	while (my ($label, $size) = each %bin) {
-		pipe(my ($rout, $wout)) or die;
 		pipe(my ($rin, $win)) or die;
-		my $rdr = { 0 => $rin, 1 => $wout };
-		my $pid = spawn($cmd , $env, $rdr);
-		$wout = $rin = undef;
+		my $rout = popen_rd($cmd , $env, { 0 => $rin });
+		$rin = undef;
 		print { $win } ("\0" x $size) or die;
 		close $win or die;
 		chomp($oid{$label} = <$rout>);
+		close $rout or die "$?";
 	}
 
 	# ensure the PSGI frontend (ViewVCS) works: