[PATCH 01/18] spawn: croak directly in C pi_fork_exec

[PATCH 01/18] spawn: croak directly in C pi_fork_exec

[Eric Wong]

This saves us some Perl code in the wrapper, since the SpawnPP
implementation also dies directly.
---
 lib/PublicInbox/Spawn.pm | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/lib/PublicInbox/Spawn.pm b/lib/PublicInbox/Spawn.pm
index 1fa7a41f..5740ee58 100644
--- a/lib/PublicInbox/Spawn.pm
+++ b/lib/PublicInbox/Spawn.pm
@@ -89,7 +89,7 @@ int pi_fork_exec(SV *redirref, SV *file, SV *cmdref, SV *envref, SV *rlimref,
 	AV *env = (AV *)SvRV(envref);
 	AV *rlim = (AV *)SvRV(rlimref);
 	const char *filename = SvPV_nolen(file);
-	pid_t pid;
+	pid_t pid = -1;
 	char **argv, **envp;
 	sigset_t set, old;
 	int ret, perrnum;
@@ -100,17 +100,17 @@ int pi_fork_exec(SV *redirref, SV *file, SV *cmdref, SV *envref, SV *rlimref,
 	AV2C_COPY(argv, cmd);
 	AV2C_COPY(envp, env);
 
-	if (sigfillset(&set)) return -1;
-	if (sigdelset(&set, SIGABRT)) return -1;
-	if (sigdelset(&set, SIGBUS)) return -1;
-	if (sigdelset(&set, SIGFPE)) return -1;
-	if (sigdelset(&set, SIGILL)) return -1;
-	if (sigdelset(&set, SIGSEGV)) return -1;
+	if (sigfillset(&set)) goto out;
+	if (sigdelset(&set, SIGABRT)) goto out;
+	if (sigdelset(&set, SIGBUS)) goto out;
+	if (sigdelset(&set, SIGFPE)) goto out;
+	if (sigdelset(&set, SIGILL)) goto out;
+	if (sigdelset(&set, SIGSEGV)) goto out;
 	/* no XCPU/XFSZ here */
-	if (sigprocmask(SIG_SETMASK, &set, &old)) return -1;
+	if (sigprocmask(SIG_SETMASK, &set, &old)) goto out;
 	chld_is_member = sigismember(&old, SIGCHLD);
-	if (chld_is_member < 0) return -1;
-	if (chld_is_member > 0 && sigdelset(&old, SIGCHLD)) return -1;
+	if (chld_is_member < 0) goto out;
+	if (chld_is_member > 0 && sigdelset(&old, SIGCHLD)) goto out;
 
 	pid = vfork();
 	if (pid == 0) {
@@ -171,6 +171,8 @@ int pi_fork_exec(SV *redirref, SV *file, SV *cmdref, SV *envref, SV *rlimref,
 	} else if (perrnum) {
 		errno = perrnum;
 	}
+out:
+	if (pid < 0) croak("E: fork_exec %s: %s\n", filename, strerror(errno));
 	return (int)pid;
 }
 
@@ -369,9 +371,7 @@ sub spawn ($;$$) {
 	}
 	my $cd = $opt->{'-C'} // ''; # undef => NULL mapping doesn't work?
 	my $pgid = $opt->{pgid} // -1;
-	my $pid = pi_fork_exec(\@rdr, $f, $cmd, \@env, $rlim, $cd, $pgid);
-	die "fork_exec @$cmd failed: $!\n" unless $pid > 0;
-	$pid;
+	pi_fork_exec(\@rdr, $f, $cmd, \@env, $rlim, $cd, $pgid);
 }
 
 sub popen_rd {

[PATCH 02/18] spawnpp: use autodie for syscall failures

[Eric Wong]

We lose a little info for fork failures, but I don't think it
matters.
---
 lib/PublicInbox/SpawnPP.pm | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/lib/PublicInbox/SpawnPP.pm b/lib/PublicInbox/SpawnPP.pm
index e7174d6f..f89d37d4 100644
--- a/lib/PublicInbox/SpawnPP.pm
+++ b/lib/PublicInbox/SpawnPP.pm
@@ -7,6 +7,7 @@
 package PublicInbox::SpawnPP;
 use v5.12;
 use POSIX qw(dup2 _exit setpgid :signal_h);
+use autodie qw(chdir close fork pipe);
 # this is loaded by PublicInbox::Spawn, so we can't use/require it, here
 
 # Pure Perl implementation for folks that do not use Inline::C
@@ -20,9 +21,8 @@ sub pi_fork_exec ($$$$$$$) {
 		$set->delset($_) or die "delset($_): $!";
 	}
 	sigprocmask(SIG_SETMASK, $set, $old) or die "SIG_SETMASK(set): $!";
-	my $syserr;
-	pipe(my ($r, $w)) or die "pipe: $!";
-	my $pid = fork // die "fork (+exec) @$cmd: $!\n";
+	pipe(my $r, my $w);
+	my $pid = fork;
 	if ($pid == 0) {
 		close $r;
 		$SIG{__DIE__} = sub {
@@ -40,9 +40,7 @@ sub pi_fork_exec ($$$$$$$) {
 			die "setpgid(0, $pgid): $!";
 		}
 		$SIG{$_} = 'DEFAULT' for grep(!/\A__/, keys %SIG);
-		if ($cd ne '') {
-			chdir $cd or die "cd $cd: $!";
-		}
+		chdir($cd) if $cd ne '';
 		while (@$rlim) {
 			my ($r, $soft, $hard) = splice(@$rlim, 0, 3);
 			BSD::Resource::setrlimit($r, $soft, $hard) or

[PATCH 03/18] spawn: avoid alloca in C pi_fork_exec

[Eric Wong]

We don't have thread-safety to worry about, so just leave a few
allocations at process exit at worst.  We'll also update some
comments about usage while we're at it.
---
 lib/PublicInbox/Spawn.pm | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/lib/PublicInbox/Spawn.pm b/lib/PublicInbox/Spawn.pm
index 5740ee58..8382c4d2 100644
--- a/lib/PublicInbox/Spawn.pm
+++ b/lib/PublicInbox/Spawn.pm
@@ -6,10 +6,8 @@
 # is explicitly defined in the environment (and writable).
 # Under Linux, vfork can make a big difference in spawning performance
 # as process size increases (fork still needs to mark pages for CoW use).
-# Currently, we only use this for code intended for long running
-# daemons (inside the PSGI code (-httpd) and -nntpd).  The short-lived
-# scripts (-mda, -index, -learn, -init) either use IPC::run or standard
-# Perl routines.
+# None of this is intended to be thread-safe since Perl5 maintainers
+# officially discourage the use of threads.
 #
 # There'll probably be more OS-level C stuff here, down the line.
 # We don't want too many DSOs: https://udrepper.livejournal.com/8790.html
@@ -39,12 +37,6 @@ BEGIN {
 #include <time.h>
 #include <stdio.h>
 #include <string.h>
-
-/* some platforms need alloca.h, but some don't */
-#if defined(__GNUC__) && !defined(alloca)
-#  define alloca(sz) __builtin_alloca(sz)
-#endif
-
 #include <signal.h>
 #include <assert.h>
 
@@ -56,11 +48,17 @@ BEGIN {
  *   This is unlike "sv_len", which returns what you would expect.
  */
 #define AV2C_COPY(dst, src) do { \
+	static size_t dst##__capa; \
 	I32 i; \
 	I32 top_index = av_len(src); \
 	I32 real_len = top_index + 1; \
 	I32 capa = real_len + 1; \
-	dst = alloca(capa * sizeof(char *)); \
+	if (capa > dst##__capa) { \
+		dst##__capa = 0; /* in case Newx croaks */ \
+		Safefree(dst); \
+		Newx(dst, capa, char *); \
+		dst##__capa = capa; \
+	} \
 	for (i = 0; i < real_len; i++) { \
 		SV **sv = av_fetch(src, i, 0); \
 		dst[i] = SvPV_nolen(*sv); \
@@ -90,7 +88,7 @@ int pi_fork_exec(SV *redirref, SV *file, SV *cmdref, SV *envref, SV *rlimref,
 	AV *rlim = (AV *)SvRV(rlimref);
 	const char *filename = SvPV_nolen(file);
 	pid_t pid = -1;
-	char **argv, **envp;
+	static char **argv, **envp;
 	sigset_t set, old;
 	int ret, perrnum;
 	volatile int cerrnum = 0; /* shared due to vfork */
@@ -172,7 +170,8 @@ int pi_fork_exec(SV *redirref, SV *file, SV *cmdref, SV *envref, SV *rlimref,
 		errno = perrnum;
 	}
 out:
-	if (pid < 0) croak("E: fork_exec %s: %s\n", filename, strerror(errno));
+	if (pid < 0)
+		croak("E: fork_exec %s: %s\n", filename, strerror(errno));
 	return (int)pid;
 }
 

[PATCH 04/18] git: use run_qx to read `git --version'

[Eric Wong]

It exists, now, so save us a few lines of code.
---
 lib/PublicInbox/Git.pm | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/lib/PublicInbox/Git.pm b/lib/PublicInbox/Git.pm
index f4a24f2a..b5adc1f4 100644
--- a/lib/PublicInbox/Git.pm
+++ b/lib/PublicInbox/Git.pm
@@ -18,7 +18,7 @@ use Errno qw(EINTR EAGAIN);
 use File::Glob qw(bsd_glob GLOB_NOSORT);
 use File::Spec ();
 use Time::HiRes qw(stat);
-use PublicInbox::Spawn qw(spawn popen_rd which);
+use PublicInbox::Spawn qw(spawn popen_rd run_qx which);
 use PublicInbox::ProcessIONBF;
 use PublicInbox::Tmpfile;
 use IO::Poll qw(POLLIN);
@@ -61,9 +61,8 @@ sub check_git_exe () {
 	my @st = stat($GIT_EXE) or die "stat($GIT_EXE): $!";
 	my $st = pack('dd', $st[10], $st[7]);
 	if ($st ne $EXE_ST) {
-		my $rd = popen_rd([ $GIT_EXE, '--version' ]);
-		my $v = readline($rd);
-		CORE::close($rd) or die "$GIT_EXE --version: $?";
+		my $v = run_qx([ $GIT_EXE, '--version' ]);
+		die "$GIT_EXE --version: \$?=$?" if $?;
 		$v =~ /\b([0-9]+(?:\.[0-9]+){2})/ or die
 			"$GIT_EXE --version output: $v # unparseable";
 		$GIT_VER = eval("v$1") // die "BUG: bad vstring: $1 ($v)";

[PATCH 05/18] git: avoid extra stat(2) for git version

[Eric Wong]

No sane installer will update executable files in place due to
ETXTBSY, SIGBUS, or similar errors on execve.  So save ourselves
a stat(2) call by relying on the special `CORE::stat(_)' case to
reuse the cached result from the `-x FILE' filetest in which().
---
 lib/PublicInbox/Git.pm | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/PublicInbox/Git.pm b/lib/PublicInbox/Git.pm
index b5adc1f4..a1d52118 100644
--- a/lib/PublicInbox/Git.pm
+++ b/lib/PublicInbox/Git.pm
@@ -17,7 +17,6 @@ use PublicInbox::Syscall qw(EPOLLIN EPOLLET);
 use Errno qw(EINTR EAGAIN);
 use File::Glob qw(bsd_glob GLOB_NOSORT);
 use File::Spec ();
-use Time::HiRes qw(stat);
 use PublicInbox::Spawn qw(spawn popen_rd run_qx which);
 use PublicInbox::ProcessIONBF;
 use PublicInbox::Tmpfile;
@@ -53,13 +52,13 @@ my %GIT_ESC = (
 );
 my %ESC_GIT = map { $GIT_ESC{$_} => $_ } keys %GIT_ESC;
 
-my $EXE_ST = ''; # pack('dd', st_ctime, st_size);
+my $EXE_ST = ''; # pack('dd', st_dev, st_ino); # no `q' in some 32-bit builds
 my ($GIT_EXE, $GIT_VER);
 
 sub check_git_exe () {
 	$GIT_EXE = which('git') // die "git not found in $ENV{PATH}";
-	my @st = stat($GIT_EXE) or die "stat($GIT_EXE): $!";
-	my $st = pack('dd', $st[10], $st[7]);
+	my @st = stat(_) or die "stat($GIT_EXE): $!"; # can't do HiRes w/ _
+	my $st = pack('dd', $st[0], $st[1]);
 	if ($st ne $EXE_ST) {
 		my $v = run_qx([ $GIT_EXE, '--version' ]);
 		die "$GIT_EXE --version: \$?=$?" if $?;
@@ -117,6 +116,7 @@ sub git_path ($$) {
 sub alternates_changed {
 	my ($self) = @_;
 	my $alt = git_path($self, 'objects/info/alternates');
+	use Time::HiRes qw(stat);
 	my @st = stat($alt) or return 0;
 
 	# can't rely on 'q' on some 32-bit builds, but `d' works

[PATCH 06/18] import: use run_qx

[Eric Wong]

---
 lib/PublicInbox/Import.pm | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/PublicInbox/Import.pm b/lib/PublicInbox/Import.pm
index fb70b91b..6eee8774 100644
--- a/lib/PublicInbox/Import.pm
+++ b/lib/PublicInbox/Import.pm
@@ -8,7 +8,7 @@
 package PublicInbox::Import;
 use v5.12;
 use parent qw(PublicInbox::Lock);
-use PublicInbox::Spawn qw(run_die popen_rd spawn);
+use PublicInbox::Spawn qw(run_die run_qx spawn);
 use PublicInbox::MID qw(mids mid2path);
 use PublicInbox::Address;
 use PublicInbox::Smsg;
@@ -25,10 +25,8 @@ use PublicInbox::Git qw(read_all);
 
 sub default_branch () {
 	state $default_branch = do {
-		my $r = popen_rd([qw(git config --global init.defaultBranch)],
+		my $h = run_qx([qw(git config --global init.defaultBranch)],
 				 { GIT_CONFIG => undef });
-		chomp(my $h = <$r> // '');
-		CORE::close $r;
 		$h eq '' ? 'refs/heads/master' : "refs/heads/$h";
 	}
 }

[PATCH 07/18] treewide: use ->close method rather than CORE::close

[Eric Wong]

It's easier-to-read and should open the door for us to get rid
of `tie' for ProcessIO without performance penalties for
more frequently-used perlop calls and `stat' usage.
---
 lib/PublicInbox/CodeSearchIdx.pm | 6 +++---
 lib/PublicInbox/DS.pm            | 4 ++--
 lib/PublicInbox/DirIdle.pm       | 2 +-
 lib/PublicInbox/Gcf2.pm          | 2 +-
 lib/PublicInbox/Git.pm           | 6 +++---
 lib/PublicInbox/HTTP.pm          | 4 ++--
 lib/PublicInbox/LeiMirror.pm     | 6 +++---
 7 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/lib/PublicInbox/CodeSearchIdx.pm b/lib/PublicInbox/CodeSearchIdx.pm
index bf410734..c1a1ee90 100644
--- a/lib/PublicInbox/CodeSearchIdx.pm
+++ b/lib/PublicInbox/CodeSearchIdx.pm
@@ -480,11 +480,11 @@ sub partition_refs ($$$) {
 			$seen = 0;
 		}
 		if ($DO_QUIT) {
-			CORE::close($rfh);
+			$rfh->close;
 			return ();
 		}
 	}
-	CORE::close($rfh);
+	$rfh->close;
 	return () if $DO_QUIT;
 	if (!$? || (($? & 127) == POSIX::SIGPIPE && $seen > $SEEN_MAX)) {
 		my $n = $NCHANGE - $n0;
@@ -887,7 +887,7 @@ sub associate {
 			++$score{"$ibx_id $_"} for @root_ids;
 		}
 	}
-	CORE::close $rd or die "@join failed: $?=$?";
+	$rd->close or die "fatal: @join failed: \$?=$?";
 	my $min = $self->{-opt}->{'assoc-min'} // 10;
 	progress($self, scalar(keys %score).' potential pairings...');
 	for my $k (keys %score) {
diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index 6041c6b5..d8c84884 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -361,8 +361,8 @@ sub greet {
 	my $ev = EPOLLIN;
 	my $wbuf;
 	if ($sock->can('accept_SSL') && !$sock->accept_SSL) {
-		return CORE::close($sock) if $! != EAGAIN;
-		$ev = PublicInbox::TLS::epollbit() or return CORE::close($sock);
+		return $sock->close if $! != EAGAIN;
+		$ev = PublicInbox::TLS::epollbit() or return $sock->close;
 		$wbuf = [ \&accept_tls_step, $self->can('do_greet')];
 	}
 	new($self, $sock, $ev | EPOLLONESHOT);
diff --git a/lib/PublicInbox/DirIdle.pm b/lib/PublicInbox/DirIdle.pm
index de6f229b..e6a326ab 100644
--- a/lib/PublicInbox/DirIdle.pm
+++ b/lib/PublicInbox/DirIdle.pm
@@ -89,7 +89,7 @@ sub force_close {
 	my ($self) = @_;
 	my $inot = delete $self->{inot} // return;
 	if ($inot->can('fh')) { # Linux::Inotify2 2.3+
-		CORE::close($inot->fh) or warn "CLOSE ERROR: $!";
+		$inot->fh->close or warn "CLOSE ERROR: $!";
 	} elsif ($inot->isa('Linux::Inotify2')) {
 		require PublicInbox::LI2Wrap;
 		PublicInbox::LI2Wrap::wrapclose($inot);
diff --git a/lib/PublicInbox/Gcf2.pm b/lib/PublicInbox/Gcf2.pm
index 4f163cde..81a1bae2 100644
--- a/lib/PublicInbox/Gcf2.pm
+++ b/lib/PublicInbox/Gcf2.pm
@@ -34,7 +34,7 @@ BEGIN {
 	for my $k (@switches) {
 		my $rd = popen_rd([$pc, "--$k", 'libgit2'], undef, $rdr);
 		chomp(my $val = do { local $/; <$rd> });
-		CORE::close($rd) or last; # checks for error and sets $?
+		$rd->close or last; # checks for error and sets $?
 		$vals->{$k} = $val;
 	}
 	if (!$?) {
diff --git a/lib/PublicInbox/Git.pm b/lib/PublicInbox/Git.pm
index a1d52118..1f8ce1f0 100644
--- a/lib/PublicInbox/Git.pm
+++ b/lib/PublicInbox/Git.pm
@@ -441,12 +441,12 @@ sub qx {
 	my $fh = popen(@_);
 	if (wantarray) {
 		my @ret = <$fh>;
-		CORE::close $fh; # caller should check $?
+		$fh->close; # caller should check $?
 		@ret;
 	} else {
 		local $/;
 		my $ret = <$fh>;
-		CORE::close $fh; # caller should check $?
+		$fh->close; # caller should check $?
 		$ret;
 	}
 }
@@ -620,7 +620,7 @@ sub manifest_entry {
 		}
 	}
 	$ent->{fingerprint} = sha_all(1, $sr)->hexdigest;
-	CORE::close $sr or return; # empty, uninitialized git repo
+	$sr->close or return; # empty, uninitialized git repo
 	$ent->{modified} = modified(undef, $mod);
 	chomp($buf = <$own> // '');
 	utf8::decode($buf);
diff --git a/lib/PublicInbox/HTTP.pm b/lib/PublicInbox/HTTP.pm
index edc88fe8..85991ae7 100644
--- a/lib/PublicInbox/HTTP.pm
+++ b/lib/PublicInbox/HTTP.pm
@@ -63,8 +63,8 @@ sub new ($$$) {
 	my $ev = EPOLLIN;
 	my $wbuf;
 	if ($sock->can('accept_SSL') && !$sock->accept_SSL) {
-		return CORE::close($sock) if $! != EAGAIN;
-		$ev = PublicInbox::TLS::epollbit() or return CORE::close($sock);
+		return $sock->close if $! != EAGAIN;
+		$ev = PublicInbox::TLS::epollbit() or return $sock->close;
 		$wbuf = [ \&PublicInbox::DS::accept_tls_step ];
 	}
 	$self->{wbuf} = $wbuf if $wbuf;
diff --git a/lib/PublicInbox/LeiMirror.pm b/lib/PublicInbox/LeiMirror.pm
index 43e59e6c..49b38d43 100644
--- a/lib/PublicInbox/LeiMirror.pm
+++ b/lib/PublicInbox/LeiMirror.pm
@@ -59,7 +59,7 @@ sub try_scrape {
 	my $opt = { 0 => $lei->{0}, 2 => $lei->{2} };
 	my $fh = popen_rd($cmd, undef, $opt);
 	my $html = do { local $/; <$fh> } // die "read(curl $uri): $!";
-	CORE::close($fh) or return $lei->child_error($?, "@$cmd failed");
+	$fh->close or return $lei->child_error($?, "@$cmd failed");
 
 	# we grep with URL below, we don't want Subject/From headers
 	# making us clone random URLs.  This assumes remote instances
@@ -335,7 +335,7 @@ sub fgrp_update {
 			upr($lei, $w, 'create', $ref, $oid);
 		}
 	}
-	CORE::close($w) or upref_warn();
+	$w->close or upref_warn();
 }
 
 sub satellite_done {
@@ -345,7 +345,7 @@ sub satellite_done {
 		while (my ($ref, $oid) = each %$create) {
 			upr($fgrp->{lei}, $w, 'create', $ref, $oid);
 		}
-		CORE::close($w) or upref_warn();
+		$w->close or upref_warn();
 	} else {
 		pack_refs($fgrp, $fgrp->{cur_dst});
 		run_puh($fgrp);

[PATCH 08/18] fetch: use run_qx

[Eric Wong]

---
 lib/PublicInbox/Fetch.pm | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/lib/PublicInbox/Fetch.pm b/lib/PublicInbox/Fetch.pm
index e41dd448..b0f1437c 100644
--- a/lib/PublicInbox/Fetch.pm
+++ b/lib/PublicInbox/Fetch.pm
@@ -5,7 +5,7 @@ package PublicInbox::Fetch;
 use v5.12;
 use parent qw(PublicInbox::IPC);
 use URI ();
-use PublicInbox::Spawn qw(popen_rd run_wait);
+use PublicInbox::Spawn qw(popen_rd run_qx run_wait);
 use PublicInbox::Admin;
 use PublicInbox::LEI;
 use PublicInbox::LeiCurl;
@@ -20,9 +20,8 @@ sub remote_url ($$) {
 	my $rn = $lei->{opt}->{'try-remote'} // [ 'origin', '_grokmirror' ];
 	for my $r (@$rn) {
 		my $cmd = [ qw(git config), "remote.$r.url" ];
-		my $fh = popen_rd($cmd, undef, { -C => $dir, 2 => $lei->{2} });
-		my $url = <$fh>;
-		close $fh or next;
+		my $url = run_qx($cmd, undef, { -C => $dir, 2 => $lei->{2} });
+		next if $?;
 		$url =~ s!/*\n!!s;
 		return $url;
 	}

[PATCH 09/18] git_credential: use IO::Handle->close

[Eric Wong]

---
 lib/PublicInbox/GitCredential.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/GitCredential.pm b/lib/PublicInbox/GitCredential.pm
index 10114a10..a4444e2c 100644
--- a/lib/PublicInbox/GitCredential.pm
+++ b/lib/PublicInbox/GitCredential.pm
@@ -30,7 +30,7 @@ sub run ($$;$) {
 	close $in_w or die "close (git credential $op): $!";
 	return $out_r if $op eq 'fill';
 	<$out_r> and die "unexpected output from `git credential $op'\n";
-	close $out_r or die "`git credential $op' failed: \$!=$! \$?=$?\n";
+	$out_r->close or die "`git credential $op' failed: \$!=$! \$?=$?\n";
 }
 
 sub check_netrc {
@@ -61,7 +61,7 @@ sub fill {
 		/\A([^=]+)=(.*)\z/ or die "bad line: $_\n";
 		$self->{$1} = $2;
 	}
-	close $out_r or die "git credential fill failed: \$!=$! \$?=$?\n";
+	$out_r->close or die "git credential fill failed: \$!=$! \$?=$?\n";
 	$self->{filled} = 1;
 }
 

[PATCH 10/18] cindex: drop redundant close on regular FH

[Eric Wong]

There's no need to waste optree space on close() statements for
file handles which are (effectively) read-only on their last
use.  Instead, let Perl refcounting take care of it so we have
less code to wade through when focusing on close statements
which actually matter.
---
 lib/PublicInbox/CodeSearchIdx.pm | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/PublicInbox/CodeSearchIdx.pm b/lib/PublicInbox/CodeSearchIdx.pm
index c1a1ee90..9117ec3b 100644
--- a/lib/PublicInbox/CodeSearchIdx.pm
+++ b/lib/PublicInbox/CodeSearchIdx.pm
@@ -268,7 +268,6 @@ sub shard_index { # via wq_io_do in IDX_SHARDS
 	my $cmd = $git->cmd(@LOG_STDIN);
 	my $rd = popen_rd($cmd, undef, { 0 => $in },
 				\&cidx_reap_log, $cmd, $self, $op_p);
-	close $in;
 	PublicInbox::CidxLogP->new($rd, $self, $git, $roots);
 	# CidxLogP->event_step will call cidx_read_log_p once there's input
 }
@@ -457,7 +456,6 @@ sub partition_refs ($$$) {
 	my ($self, $git, $refs) = @_; # show-ref --heads --tags --hash output
 	sysseek($refs, 0, SEEK_SET);
 	my $rfh = $git->popen(qw(rev-list --stdin), undef, { 0 => $refs });
-	close $refs;
 	my $seen = 0;
 	my @shard_in = map {
 		$_->reopen;

[PATCH 11/18] admin: run_qx

[Eric Wong]

---
 lib/PublicInbox/Admin.pm | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/lib/PublicInbox/Admin.pm b/lib/PublicInbox/Admin.pm
index 3140afad..893f4a1b 100644
--- a/lib/PublicInbox/Admin.pm
+++ b/lib/PublicInbox/Admin.pm
@@ -9,7 +9,7 @@ use parent qw(Exporter);
 our @EXPORT_OK = qw(setup_signals);
 use PublicInbox::Config;
 use PublicInbox::Inbox;
-use PublicInbox::Spawn qw(popen_rd);
+use PublicInbox::Spawn qw(run_qx);
 use PublicInbox::Eml;
 *rel2abs_collapsed = \&PublicInbox::Config::rel2abs_collapsed;
 
@@ -67,9 +67,8 @@ sub resolve_git_dir {
 	my ($cd) = @_;
 	# try v1 bare git dirs
 	my $cmd = [ qw(git rev-parse --git-dir) ];
-	my $fh = popen_rd($cmd, undef, {-C => $cd});
-	my $dir = do { local $/; <$fh> };
-	close $fh or die "error in @$cmd (cwd:${\($cd // '.')}): $?\n";
+	my $dir = run_qx($cmd, undef, {-C => $cd});
+	die "error in @$cmd (cwd:${\($cd // '.')}): $?\n" if $?;
 	chomp $dir;
 	# --absolute-git-dir requires git v2.13.0+
 	$dir = rel2abs_collapsed($dir, $cd) if $dir !~ m!\A/!;

[PATCH 12/18] config: use ->close for popen_rd pipe

[Eric Wong]

---
 lib/PublicInbox/Config.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/PublicInbox/Config.pm b/lib/PublicInbox/Config.pm
index d156b2d3..e2e87cad 100644
--- a/lib/PublicInbox/Config.pm
+++ b/lib/PublicInbox/Config.pm
@@ -192,7 +192,7 @@ sub git_config_dump {
 	push(@cmd, '-f', $file) if !@opt_c && defined($file);
 	my $fh = popen_rd(\@cmd, \%env, $opt);
 	my $rv = config_fh_parse($fh, "\0", "\n");
-	close $fh or die "@cmd failed: \$?=$?\n";
+	$fh->close or die "@cmd failed: \$?=$?\n";
 	$rv->{-opt_c} = \@opt_c if @opt_c; # for ->urlmatch
 	$rv->{-f} = $file;
 	bless $rv, $class;

[PATCH 13/18] import: use ->close for fast-import socket

[Eric Wong]

---
 lib/PublicInbox/Import.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/Import.pm b/lib/PublicInbox/Import.pm
index 6eee8774..5e2e8a30 100644
--- a/lib/PublicInbox/Import.pm
+++ b/lib/PublicInbox/Import.pm
@@ -476,7 +476,7 @@ sub done {
 	my $io = delete $self->{io} or return;
 	eval {
 		print $io "done\n" or wfail;
-		close $io; # reaps and dies on error
+		$io->close or croak "close fast-import \$?=$?"; # reaps
 	};
 	my $wait_err = $@;
 	my $nchg = delete $self->{nchg};
@@ -489,7 +489,7 @@ sub done {
 	die $wait_err if $wait_err;
 }
 
-sub atfork_child { close(delete($_[0]->{io}) // return) }
+sub atfork_child { (delete($_[0]->{io}) // return)->close }
 
 sub digest2mid ($$;$) {
 	my ($dig, $hdr, $fallback_time) = @_;

[PATCH 14/18] lei_mirror: use run_qx

[Eric Wong]

---
 lib/PublicInbox/LeiMirror.pm | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/lib/PublicInbox/LeiMirror.pm b/lib/PublicInbox/LeiMirror.pm
index 49b38d43..413d869c 100644
--- a/lib/PublicInbox/LeiMirror.pm
+++ b/lib/PublicInbox/LeiMirror.pm
@@ -7,7 +7,7 @@ use v5.12;
 use parent qw(PublicInbox::IPC);
 use IO::Uncompress::Gunzip qw(gunzip $GunzipError);
 use IO::Compress::Gzip qw(gzip $GzipError);
-use PublicInbox::Spawn qw(popen_rd spawn run_wait run_die);
+use PublicInbox::Spawn qw(popen_rd spawn run_wait run_die run_qx);
 use File::Path ();
 use File::Temp ();
 use File::Spec ();
@@ -57,9 +57,8 @@ sub try_scrape {
 	my $curl = $self->{curl} //= PublicInbox::LeiCurl->new($lei) or return;
 	my $cmd = $curl->for_uri($lei, $uri, '--compressed');
 	my $opt = { 0 => $lei->{0}, 2 => $lei->{2} };
-	my $fh = popen_rd($cmd, undef, $opt);
-	my $html = do { local $/; <$fh> } // die "read(curl $uri): $!";
-	$fh->close or return $lei->child_error($?, "@$cmd failed");
+	my $html = run_qx($cmd, undef, $opt);
+	return $lei->child_error($?, "@$cmd failed") if $?;
 
 	# we grep with URL below, we don't want Subject/From headers
 	# making us clone random URLs.  This assumes remote instances

[PATCH 15/18] lei_rediff: ->close for popen_wr

[Eric Wong]

---
 lib/PublicInbox/LeiRediff.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/PublicInbox/LeiRediff.pm b/lib/PublicInbox/LeiRediff.pm
index 230f3e83..fdff4b4b 100644
--- a/lib/PublicInbox/LeiRediff.pm
+++ b/lib/PublicInbox/LeiRediff.pm
@@ -126,7 +126,7 @@ EOM
 			qw(fast-import --quiet --done --date-format=raw)],
 			$lei->{env}, { 2 => $lei->{2} });
 	print $w $ta, "\n", $tb, "\ndone\n" or die "print fast-import: $!";
-	close $w or die "close w fast-import: \$?=$? \$!=$!";
+	$w->close or die "close w fast-import: \$?=$? \$!=$!";
 
 	my $cmd = [ 'diff' ];
 	_lei_diff_prepare($lei, $cmd);

[PATCH 16/18] spamcheck/spamc: use run_qx

[Eric Wong]

---
 lib/PublicInbox/Spamcheck/Spamc.pm | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/lib/PublicInbox/Spamcheck/Spamc.pm b/lib/PublicInbox/Spamcheck/Spamc.pm
index cba33a66..798de218 100644
--- a/lib/PublicInbox/Spamcheck/Spamc.pm
+++ b/lib/PublicInbox/Spamcheck/Spamc.pm
@@ -4,7 +4,7 @@
 # Default spam filter class for wrapping spamc(1)
 package PublicInbox::Spamcheck::Spamc;
 use v5.12;
-use PublicInbox::Spawn qw(popen_rd run_wait);
+use PublicInbox::Spawn qw(run_qx run_wait);
 use IO::Handle;
 use Fcntl qw(SEEK_SET);
 
@@ -20,14 +20,9 @@ sub new {
 sub spamcheck {
 	my ($self, $msg, $out) = @_;
 
+	$out = \(my $buf = '') unless ref($out);
 	my $rdr = { 0 => _msg_to_fh($self, $msg) };
-	my $fh = popen_rd($self->{checkcmd}, undef, $rdr);
-	unless (ref $out) {
-		my $buf = '';
-		$out = \$buf;
-	}
-	$$out = do { local $/; <$fh> };
-	close $fh; # PublicInbox::ProcessIO::CLOSE
+	$$out = run_qx($self->{checkcmd}, undef, $rdr);
 	($? || $$out eq '') ? 0 : 1;
 }
 

[PATCH 17/18] lei_xsearch: use ->close

[Eric Wong]

---
 lib/PublicInbox/LeiXSearch.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/PublicInbox/LeiXSearch.pm b/lib/PublicInbox/LeiXSearch.pm
index 241b9dab..9cad3642 100644
--- a/lib/PublicInbox/LeiXSearch.pm
+++ b/lib/PublicInbox/LeiXSearch.pm
@@ -353,7 +353,7 @@ print STDERR $_;
 						$lei, $each_smsg);
 		$lei->sto_done_request if delete($self->{-sto_imported});
 		my $nr = delete $lei->{-nr_remote_eml} // 0;
-		close $cfh;
+		$cfh->close;
 		my $code = $?;
 		if (!$code) { # don't update if no results, maybe MTA is down
 			$lei->{lss}->cfg_set($key, $start) if $key && $nr;

[PATCH 18/18] searchidx: ->close

[Eric Wong]

---
 lib/PublicInbox/SearchIdx.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/PublicInbox/SearchIdx.pm b/lib/PublicInbox/SearchIdx.pm
index 3c64c715..78519b22 100644
--- a/lib/PublicInbox/SearchIdx.pm
+++ b/lib/PublicInbox/SearchIdx.pm
@@ -980,7 +980,7 @@ sub log2stack ($$$) {
 			$stk->push_rec('m', $at, $ct, $oid, $cmt);
 		}
 	}
-	close $fh or die "git log failed: \$?=$?";
+	$fh->close or die "git log failed: \$?=$?";
 	$stk //= PublicInbox::IdxStack->new;
 	$stk->read_prepare;
 }