[PATCH 0/7] some low impact changes and cleanups

[PATCH 0/7] some low impact changes and cleanups

[Eric Wong]

Some other changes coming in the next few days, I hope;
but the straightforward stuff, first...

Eric Wong (7):
  t: localize the PI_CONFIG env
  t/common: set $0 when running script w/o fork
  ds: ->Reset initializes $nextq
  TODO: update and add a few more items
  tests: don't repeatly validate NEWS.atom
  spawn: remove support for clearing the env
  import: (cleanup) drop redundant env arg to run_die

 .gitignore                |  1 +
 Documentation/include.mk  |  5 +++--
 TODO                      | 12 +++++++++---
 lib/PublicInbox/DS.pm     |  5 +++--
 lib/PublicInbox/Import.pm |  4 ++--
 lib/PublicInbox/Spawn.pm  | 12 +-----------
 t/common.perl             |  1 +
 t/indexlevels-mirror.t    |  1 +
 t/spawn.t                 | 10 ----------
 t/xcpdb-reshard.t         |  1 +
 10 files changed, 22 insertions(+), 30 deletions(-)

[PATCH 1/7] t: localize the PI_CONFIG env

[Eric Wong]

We don't want the user's ~/.public-inbox/config to be read from
during tests.  I only noticed this because I had a non-existent
pathname for one of my inboxes :x

I've also verified this change by running "inotifywait
~/.public-inbox/config -m" in another terminal while running
"make check"; (perhaps a portable solution could make it
into the test suite).
---
 t/indexlevels-mirror.t | 1 +
 t/xcpdb-reshard.t      | 1 +
 2 files changed, 2 insertions(+)

diff --git a/t/indexlevels-mirror.t b/t/indexlevels-mirror.t
index f1c338e1..3d4813be 100644
--- a/t/indexlevels-mirror.t
+++ b/t/indexlevels-mirror.t
@@ -32,6 +32,7 @@ sub import_index_incremental {
 	my ($v, $level) = @_;
 	my $this = "pi-$v-$level-indexlevels";
 	my ($tmpdir, $for_destroy) = tmpdir();
+	local $ENV{PI_CONFIG} = "$tmpdir/config";
 	my $ibx = PublicInbox::Inbox->new({
 		inboxdir => "$tmpdir/testbox",
 		name => $this,
diff --git a/t/xcpdb-reshard.t b/t/xcpdb-reshard.t
index ebf156a3..a4ab35d6 100644
--- a/t/xcpdb-reshard.t
+++ b/t/xcpdb-reshard.t
@@ -25,6 +25,7 @@ my $mime = PublicInbox::MIME->create(
 
 my ($this) = (split('/', $0))[-1];
 my ($tmpdir, $for_destroy) = tmpdir();
+local $ENV{PI_CONFIG} = "$tmpdir/config";
 my $ibx = PublicInbox::Inbox->new({
 	inboxdir => "$tmpdir/testbox",
 	name => $this,

[PATCH 2/7] t/common: set $0 when running script w/o fork

[Eric Wong]

We can localize changes to $0 so $0 is restored when the
"script" sub is done.  This will be helpful when we encounter
a stuck/slow processes during our tests (hopefully never!)
---
 t/common.perl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/common.perl b/t/common.perl
index 0ff5de4a..288a0a19 100644
--- a/t/common.perl
+++ b/t/common.perl
@@ -171,6 +171,7 @@ sub run_script ($;$$) {
 		local *STDERR = *STDERR;
 		local %ENV = $env ? (%ENV, %$env) : %ENV;
 		local %SIG = %SIG;
+		local $0 = join(' ', @$cmd);
 		_prepare_redirects($fhref);
 		_run_sub($sub, $key, \@argv);
 	}

[PATCH 3/7] ds: ->Reset initializes $nextq

[Eric Wong]

I haven't noticed this being a problem in practice, but
be consistent with the rest of the singleton stuff.
Since we always call Reset() at load time, only do
initialization in that sub and not at declaration.
---
 lib/PublicInbox/DS.pm | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index 17c640f4..301ec057 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -39,8 +39,8 @@ use Errno  qw(EAGAIN EINVAL);
 use Carp   qw(croak confess carp);
 require File::Spec;
 
-my $nextq = []; # queue for next_tick
-my $WaitPids = [];               # list of [ pid, callback, callback_arg ]
+my $nextq; # queue for next_tick
+my $WaitPids; # list of [ pid, callback, callback_arg ]
 my $reap_timer;
 our (
      %DescriptorMap,             # fd (num) -> PublicInbox::DS object
@@ -69,6 +69,7 @@ Reset all state
 =cut
 sub Reset {
     %DescriptorMap = ();
+    $nextq = [];
     $WaitPids = [];
     $reap_timer = undef;
     @ToClose = ();

[PATCH 4/7] TODO: update and add a few more items

[Eric Wong]

SpamAssassin has used re2c (via sa-compile) for many years, now,
and it seems to work fine, there.  GMime also looks promising
when combined with Inline::C since GMime can operate on mmap-ed
regions.

Given the inevitable demise of many .orgs when price rise;
supporting a URL rewriter similar to .mailmap makes sense.

And HTTP CONNECT seems like something our -httpd can support
to let firewalled users read over NNTP.
---
 TODO | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/TODO b/TODO
index 922163f8..369fc56e 100644
--- a/TODO
+++ b/TODO
@@ -6,9 +6,13 @@ all need to be considered for everything we introduce)
 
 * general performance improvements, but without relying on
   XS or pre-built modules any more than we currently do.
+  (Optional Inline::C and user-compiled re2c acceptable)
 
 * mailmap support (same as git) for remapping expired email addresses
 
+* support remapping of expired URLs similar to mailmap
+  (coordinate with git.git with this?)
+
 * POP3 server, since some webmail providers support external POP3:
   https://public-inbox.org/meta/20160411034104.GA7817@dcvr.yhbt.net/
   Perhaps make this depend solely the NNTP server and work as a proxy.
@@ -21,6 +25,9 @@ all need to be considered for everything we introduce)
   yet storing large amounts of data on computers without a
   public IP behind a home Internet connection.
 
+* support HTTP(S) CONNECT proxying to NNTP for users with
+  firewall problems
+
 * DHT (distributed hash table) for mapping Message-IDs to various
   archive locations to avoid SPOF.
 
@@ -68,10 +75,9 @@ all need to be considered for everything we introduce)
 * linkify thread skeletons better
   https://public-inbox.org/git/6E3699DEA672430CAEA6DEFEDE6918F4@PhilipOakley/
 
-* streaming Email::MIME replacement: currently we generate many
+* low-memory Email::MIME replacement: currently we generate many
   allocations/strings for headers we never look at and slurp
-  entire message bodies into memory.
-  (this is pie-in-the-sky territory...)
+  entire message bodies into memory.  GMime+Inline::C could work.
 
 * use REQUEST_URI properly for CGI / mod_perl2 compatibility
   with Message-IDs which include '%' (done?)

[PATCH 5/7] tests: don't repeatly validate NEWS.atom

[Eric Wong]

We can create a stamp to avoid rerunning the check unless
NEWS.atom changes (and it will, soon, I hope :>).
---
 .gitignore               | 1 +
 Documentation/include.mk | 5 +++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/.gitignore b/.gitignore
index 9eb97751..bdb8cf15 100644
--- a/.gitignore
+++ b/.gitignore
@@ -15,6 +15,7 @@
 *.html
 *.gz
 .*.cols
+.*.check
 /NEWS.html
 /NEWS.atom
 /NEWS
diff --git a/Documentation/include.mk b/Documentation/include.mk
index 651fdf30..624f07f4 100644
--- a/Documentation/include.mk
+++ b/Documentation/include.mk
@@ -134,11 +134,12 @@ NEWS NEWS.atom NEWS.html : Documentation/include.mk
 	$(PERL) -I lib -w Documentation/mknews.perl $@ $(RELEASES)
 
 # check for internal API changes:
-check :: NEWS check-NEWS.atom NEWS.html
+check :: NEWS .NEWS.atom.check NEWS.html
 
-check-NEWS.atom: NEWS.atom
+.NEWS.atom.check: NEWS.atom
 	$(XMLSTARLET) val NEWS.atom || \
 		{ e=$$?; test $$e -eq 0 || test $$e -eq 127; }
+	>$@
 
 Documentation/%.html: Documentation/%.txt
 	$(txt2pre)

[PATCH 6/7] spawn: remove support for clearing the env

[Eric Wong]

It's unnecessary code which I'm not sure we ever used.  In
retrospect, completely clearing the environment doesn't make
sense for the processes we spawn.  We don't need to clobber
individual environment variables in our code, either
(and if we did for tests, we can use 'local').
---
 lib/PublicInbox/Spawn.pm | 12 +-----------
 t/spawn.t                | 10 ----------
 2 files changed, 1 insertion(+), 21 deletions(-)

diff --git a/lib/PublicInbox/Spawn.pm b/lib/PublicInbox/Spawn.pm
index b946a663..6493ad38 100644
--- a/lib/PublicInbox/Spawn.pm
+++ b/lib/PublicInbox/Spawn.pm
@@ -193,17 +193,7 @@ sub spawn ($;$$) {
 	my @env;
 	$opts ||= {};
 
-	my %env = $opts->{-env} ? () : %ENV;
-	if ($env) {
-		foreach my $k (keys %$env) {
-			my $v = $env->{$k};
-			if (defined $v) {
-				$env{$k} = $v;
-			} else {
-				delete $env{$k};
-			}
-		}
-	}
+	my %env = $env ? (%ENV, %$env) : %ENV;
 	while (my ($k, $v) = each %env) {
 		push @env, "$k=$v";
 	}
diff --git a/t/spawn.t b/t/spawn.t
index ebebfb57..2e409157 100644
--- a/t/spawn.t
+++ b/t/spawn.t
@@ -38,16 +38,6 @@ use PublicInbox::Spawn qw(which spawn popen_rd);
 	is($?, 0, 'sh exited successfully');
 }
 
-{
-	my ($r, $w);
-	pipe $r, $w or die "pipe failed: $!";
-	my $pid = spawn(['env'], {}, { -env => 1, 1 => fileno($w) });
-	close $w or die "close pipe[1] failed: $!";
-	ok(!defined(<$r>), 'read stdout of spawned from pipe');
-	is(waitpid($pid, 0), $pid, 'waitpid succeeds on spawned process');
-	is($?, 0, 'env(1) exited successfully');
-}
-
 {
 	my $fh = popen_rd([qw(echo hello)]);
 	ok(fileno($fh) >= 0, 'tied fileno works');

[PATCH 7/7] import: (cleanup) drop redundant env arg to run_die

[Eric Wong]

run_die() doesn't require an $env arg, so there's no
point passing "undef" to it.
---
 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 8a369ee4..46de09c4 100644
--- a/lib/PublicInbox/Import.pm
+++ b/lib/PublicInbox/Import.pm
@@ -178,14 +178,14 @@ sub _update_git_info ($$) {
 		my $env = { GIT_INDEX_FILE => $index };
 		run_die([@cmd, qw(read-tree -m -v -i), $self->{ref}], $env);
 	}
-	run_die([@cmd, 'update-server-info'], undef);
+	run_die([@cmd, 'update-server-info']);
 	my $ibx = $self->{-inbox};
 	($ibx && $self->{path_type} eq '2/38') and eval {
 		require PublicInbox::SearchIdx;
 		my $s = PublicInbox::SearchIdx->new($ibx);
 		$s->index_sync({ ref => $self->{ref} });
 	};
-	eval { run_die([@cmd, qw(gc --auto)], undef) } if $do_gc;
+	eval { run_die([@cmd, qw(gc --auto)]) } if $do_gc;
 }
 
 sub barrier {