[PATCH 0/6] misc. cleanups and simplifications

[PATCH 0/6] misc. cleanups and simplifications

[Eric Wong]

More stuff noticed while working on other stuff...

Eric Wong (6):
  devel/sysdefs-list: explain why we don't autodie, here
  multi_git: remove redundant ops
  init: reduce git-config execve for idempotent cases
  msgmap: use v5.12
  lei_mirror: use write_file to append configs
  git_http_backend: input_prepare: die on unrecoverable errors

 devel/sysdefs-list                |  1 +
 lib/PublicInbox/Cgit.pm           |  2 +-
 lib/PublicInbox/GitHTTPBackend.pm | 20 +++++++++-----------
 lib/PublicInbox/LeiMirror.pm      | 14 +++++---------
 lib/PublicInbox/Msgmap.pm         |  3 +--
 lib/PublicInbox/MultiGit.pm       |  6 +++---
 script/public-inbox-init          | 24 ++++++++++++------------
 7 files changed, 32 insertions(+), 38 deletions(-)

[PATCH 1/6] devel/sysdefs-list: explain why we don't autodie, here

[Eric Wong]

Some distros split out core modules like autodie, and
sysdefs-list has higher portability requirements than the rest
of our code since it's used for uncommon platforms.
---
 devel/sysdefs-list | 1 +
 1 file changed, 1 insertion(+)

diff --git a/devel/sysdefs-list b/devel/sysdefs-list
index 3a2e60d2..39a56323 100755
--- a/devel/sysdefs-list
+++ b/devel/sysdefs-list
@@ -6,6 +6,7 @@
 eval 'exec perl -S $0 ${1+"$@"}' # no shebang
 	if 0; # running under some shell
 use v5.12;
+# no autodie here since some CFarm machines don't have it :<
 use File::Temp 0.19;
 use POSIX qw(uname);
 use Config;

[PATCH 2/6] multi_git: remove redundant ops

[Eric Wong]

read_all already returns an array in array context so the
`split' op is unnecessary, and we don't need to use
`join("", ...)' on arrays that are destined for `print'
or `say'.
---
 lib/PublicInbox/MultiGit.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/PublicInbox/MultiGit.pm b/lib/PublicInbox/MultiGit.pm
index 32bb3588..9be3a876 100644
--- a/lib/PublicInbox/MultiGit.pm
+++ b/lib/PublicInbox/MultiGit.pm
@@ -34,7 +34,7 @@ sub read_alternates {
 			qr!\A\Q../../$self->{epfx}\E/([0-9]+)\.git/objects\z! :
 			undef;
 		$$moderef = (stat($fh))[2] & 07777;
-		for my $rel (split(/^/m, read_all($fh, -s _))) {
+		for my $rel (read_all($fh, -s _)) {
 			chomp(my $dir = $rel);
 			my $score;
 			if (defined($is_edir) && $dir =~ $is_edir) {
@@ -67,10 +67,10 @@ sub write_alternates {
 	my ($self, $mode, $alt, @new) = @_;
 	my $all_dir = "$self->{topdir}/$self->{all}";
 	PublicInbox::Import::init_bare($all_dir);
-	my $out = join('', sort { $alt->{$b} <=> $alt->{$a} } keys %$alt);
+	my @out = sort { $alt->{$b} <=> $alt->{$a} } keys %$alt;
 	my $info_dir = "$all_dir/objects/info";
 	my $fh = File::Temp->new(TEMPLATE => 'alt-XXXX', DIR => $info_dir);
-	print $fh $out, @new;
+	print $fh @out, @new;
 	chmod($mode, $fh);
 	close $fh;
 	rename($fh->filename, "$info_dir/alternates");

[PATCH 3/6] init: reduce git-config execve for idempotent cases

[Eric Wong]

This probably saves us a few cycles in some cases since spawning
new processes is expensive.
---
 script/public-inbox-init | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/script/public-inbox-init b/script/public-inbox-init
index b3f60bad..b9e234e3 100755
--- a/script/public-inbox-init
+++ b/script/public-inbox-init
@@ -125,7 +125,7 @@ sysopen($lockfh, $lockfile, O_RDWR|O_CREAT|O_EXCL) or do {
 require PublicInbox::OnDestroy;
 my $auto_unlink = PublicInbox::OnDestroy::on_destroy(sub { unlink $lockfile });
 my $perm = 0644 & ~umask;
-my %seen;
+my (%seen, $old_ibx);
 if (-e $pi_config) {
 	require PublicInbox::IO;
 	open(my $oh, '<', $pi_config);
@@ -156,8 +156,8 @@ if (-e $pi_config) {
 
 	exit(1) if $conflict;
 
-	my $ibx = $cfg->lookup_name($name);
-	$indexlevel //= $ibx->{indexlevel} if $ibx;
+	$old_ibx = $cfg->lookup_name($name);
+	$indexlevel //= $old_ibx->{indexlevel} if $old_ibx;
 }
 my $pi_config_tmp = $fh->filename;
 close($fh);
@@ -224,17 +224,17 @@ umask(0077);
 require PublicInbox::Spawn;
 PublicInbox::Spawn->import(qw(run_die));
 
-foreach my $addr (@address) {
-	next if $seen{lc($addr)};
+for my $addr (grep { !$seen{lc $_} } @address) {
 	run_die([@x, "--add", "$pfx.address", $addr]);
 }
-run_die([@x, "$pfx.url", $http_url]);
-run_die([@x, "$pfx.inboxdir", $inboxdir]);
-
-if (defined($indexlevel)) {
-	run_die([@x, "$pfx.indexlevel", $indexlevel]);
-}
-run_die([@x, "$pfx.newsgroup", $ng]) if $ng ne '';
+run_die([@x, "$pfx.url", $http_url]) if
+	(!$old_ibx || !grep(/\Q$http_url\E/, @{$old_ibx->{url} // []}));
+run_die([@x, "$pfx.inboxdir", $inboxdir]) if
+	(!$old_ibx || ($old_ibx->{inboxdir} ne $inboxdir));
+run_die([@x, "$pfx.indexlevel", $indexlevel]) if defined($indexlevel) &&
+	(!$old_ibx || (($old_ibx->{indexlevel} // '') ne $indexlevel));
+run_die([@x, "$pfx.newsgroup", $ng]) if $ng ne '' &&
+	(!$old_ibx || (($old_ibx->{newsgroup} // '') ne $ng));
 
 for my $kv (@c_extra) {
 	my ($k, $v) = split(/=/, $kv, 2);

[PATCH 4/6] msgmap: use v5.12

[Eric Wong]

No unicode_string dependencies, here, so we can use v5.12 and
perhaps to speed up loading one day if everything drops `strict'
in favor of v5.12 (or higher).
---
 lib/PublicInbox/Msgmap.pm | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/PublicInbox/Msgmap.pm b/lib/PublicInbox/Msgmap.pm
index 3101fd7d..71415feb 100644
--- a/lib/PublicInbox/Msgmap.pm
+++ b/lib/PublicInbox/Msgmap.pm
@@ -8,8 +8,7 @@
 #
 # This is maintained by ::SearchIdx (v1) and ::V2Writable (v2)
 package PublicInbox::Msgmap;
-use strict;
-use v5.10.1;
+use v5.12;
 use DBI;
 use DBD::SQLite;
 use DBD::SQLite::Constants qw(SQLITE_CONSTRAINT);

[PATCH 5/6] lei_mirror: use write_file to append configs

[Eric Wong]

It saves us a few lines of code and reduces unnecessary
error checking since the close done by write_file aleady
does autodie for error checking.
---
 lib/PublicInbox/LeiMirror.pm | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/lib/PublicInbox/LeiMirror.pm b/lib/PublicInbox/LeiMirror.pm
index f87cdc51..0f14ceba 100644
--- a/lib/PublicInbox/LeiMirror.pm
+++ b/lib/PublicInbox/LeiMirror.pm
@@ -447,21 +447,17 @@ sub fgrp_fetch_all {
 			# update the config atomically via O_APPEND while
 			# respecting git-config locking
 			sysopen(my $lk, "$f.lock", O_CREAT|O_EXCL|O_WRONLY);
-			open my $fh, '>>', $f;
-			$fh->autoflush(1);
-			my $buf = '';
+			my @buf;
 			if (@$old) {
-				$buf = "[fetch]\n\thideRefs = refs\n";
-				$buf .= join('', map {
+				@buf = ("[fetch]\n\thideRefs = refs\n", map {
 					"\thideRefs = !refs/remotes/" .
 						"$_->{-remote}/\n";
 				} @$old);
 			}
-			$buf .= join('', "[remotes]\n",
+			push @buf, "[remotes]\n",
 				(map { "\t$grp = $_->{-remote}\n" } @$old),
-				(map { "\t$grp = $_->{-remote}\n" } @$new));
-			print $fh $buf or die "print($f): $!";
-			close $fh;
+				(map { "\t$grp = $_->{-remote}\n" } @$new);
+			write_file '>>', $f, @buf;
 			unlink("$f.lock");
 		}
 		$cmd  = [ @git, "--git-dir=$osdir", @fetch, $grp ];

[PATCH 6/6] git_http_backend: input_prepare: die on unrecoverable errors

[Eric Wong]

We shouldn't be guarding against errors at this level to return
500 errors, but rather we can rely on existing eval wrappers in
the stack (e.g. Plack::Util::run_app and Qspawn->parse_hdr_done)
to capture errors and return the HTTP 500 status.
---
 lib/PublicInbox/Cgit.pm           |  2 +-
 lib/PublicInbox/GitHTTPBackend.pm | 20 +++++++++-----------
 2 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/lib/PublicInbox/Cgit.pm b/lib/PublicInbox/Cgit.pm
index 480d1df6..a7ee48b9 100644
--- a/lib/PublicInbox/Cgit.pm
+++ b/lib/PublicInbox/Cgit.pm
@@ -105,7 +105,7 @@ sub call {
 	@cgi_env{@PASS_ENV} = @$env{@PASS_ENV}; # spawn ignores undef vals
 	$cgi_env{HTTPS} = 'on' if $env->{'psgi.url_scheme'} eq 'https';
 
-	my $rdr = input_prepare($env) or return r(500);
+	my $rdr = { 0 => input_prepare($env) };
 	my $qsp = PublicInbox::Qspawn->new($self->{cmd}, \%cgi_env, $rdr);
 	my $limiter = $self->{pi_cfg}->limiter('-cgit');
 	$qsp->psgi_yield($env, $limiter, $parse_cgi_headers, $ctx);
diff --git a/lib/PublicInbox/GitHTTPBackend.pm b/lib/PublicInbox/GitHTTPBackend.pm
index 57d2ec7b..3dce4dbb 100644
--- a/lib/PublicInbox/GitHTTPBackend.pm
+++ b/lib/PublicInbox/GitHTTPBackend.pm
@@ -7,6 +7,7 @@ package PublicInbox::GitHTTPBackend;
 use strict;
 use v5.10.1;
 use Fcntl qw(:seek);
+use autodie qw(sysseek);
 use IO::Handle; # ->flush
 use HTTP::Date qw(time2str);
 use PublicInbox::Limiter;
@@ -41,8 +42,6 @@ sub serve {
 	serve_dumb($env, $git, $path);
 }
 
-sub ucarp { Carp::carp(@_); undef }
-
 my $prev = 0;
 my $exp;
 sub cache_one_year {
@@ -103,8 +102,7 @@ sub serve_smart ($$$;$) {
 		($pi_cfg ? $pi_cfg->limiter('-httpbackend', 32) : undef);
 	$env{GIT_HTTP_EXPORT_ALL} = '1';
 	$env{PATH_TRANSLATED} = "$git->{git_dir}/$path";
-	my $rdr = input_prepare($env) or return r(500);
-	$rdr->{quiet} = 1;
+	my $rdr = { 0 => input_prepare($env), quiet => 1 };
 	my $cmd = $git->cmd('http-backend');
 	splice @$cmd, 1, 0, '-c', 'safe.directory=*';
 	my $qsp = PublicInbox::Qspawn->new($cmd, \%env, $rdr);
@@ -116,19 +114,19 @@ sub input_prepare {
 
 	my $input = $env->{'psgi.input'};
 	my $fd = eval { fileno($input) };
-	return { 0 => $fd } if (defined $fd && $fd >= 0);
+	return $fd if (defined $fd && $fd >= 0);
 	my $id = "git-http.input.$env->{REMOTE_ADDR}:$env->{REMOTE_PORT}";
-	my $in = tmpfile($id) // return ucarp("tmpfile: $!");
+	my $in = tmpfile($id) // die "tmpfile: $!";
 	my $buf;
 	while (1) {
-		my $r = $input->read($buf, 8192) // return ucarp("read $!");
+		my $r = $input->read($buf, 8192) // die "read: $!";
 		last if $r == 0;
-		print $in $buf // return ucarp("print: $!");
+		print $in $buf;
 	}
 	# ensure it's visible to git-http-backend(1):
-	$in->flush // return ucarp("flush: $!");
-	sysseek($in, 0, SEEK_SET) // return ucarp($env, "seek: $!");
-	{ 0 => $in };
+	$in->flush // die "flush: $!";
+	sysseek $in, 0, SEEK_SET;
+	$in;
 }
 
 sub parse_cgi_headers { # {parse_hdr} for Qspawn