[PATCH 00/14] IO/IPC-related cleanups

unofficial mirror of meta@public-inbox.org
 help / color / mirror / Atom feed

* [PATCH 00/14] IO/IPC-related cleanups
@ 2023-11-02  9:35 Eric Wong
  2023-11-02  9:35 ` [PATCH 01/14] xap_helper.pm: use do_fork to Reset and reseed Eric Wong
                   ` (13 more replies)
  0 siblings, 14 replies; 17+ messages in thread
From: Eric Wong @ 2023-11-02  9:35 UTC (permalink / raw)
  To: meta

8/14 to replace ProcessIO is a major change that I've been
hammering away at for a bit, and many preceding patches to
eliminate ->close calls make it easier to review.

11-12/14 - Error checking for buffered readline is a PITA
(IO::Handle->error was insufficient until Perl 5.34), and I may
switch to sysread eventually to avoid the double-copy overhead
of buffered bulk I/O, anyways.  The only place we really benefit
from userspace buffered disk reads is IdxStack, I think...

The new write_file sub in 9/14 seems long overdue.

Eric Wong (14):
  xap_helper.pm: use do_fork to Reset and reseed
  ds: replace FD map hash table with array
  treewide: use ->close method rather than CORE::close
  cindex: drop redundant close on regular FH
  treewide: use ->close to call ProcessIO->CLOSE
  multi_git: use autodie
  git_credential: use autodie where appropriate
  replace ProcessIO with untied PublicInbox::IO
  io: introduce write_file helper sub
  spawn: support PerlIO layer in scalar redirects
  treewide: check alternates writes with eof + autodie
  treewide: use eof and close to detect readline errors
  move read_all, try_cat, and poll_in to PublicInbox::IO
  t/cindex+extsearch: use write_file, autodie, etc.

 MANIFEST                         |  4 +-
 lib/PublicInbox/CodeSearchIdx.pm | 14 +++---
 lib/PublicInbox/Config.pm        |  2 +-
 lib/PublicInbox/DS.pm            | 22 ++++-----
 lib/PublicInbox/DirIdle.pm       |  2 +-
 lib/PublicInbox/Gcf2.pm          |  4 +-
 lib/PublicInbox/Gcf2Client.pm    |  7 ++-
 lib/PublicInbox/Git.pm           | 36 ++++----------
 lib/PublicInbox/GitCredential.pm | 16 +++----
 lib/PublicInbox/HTTP.pm          |  4 +-
 lib/PublicInbox/IO.pm            | 80 ++++++++++++++++++++++++++++++++
 lib/PublicInbox/IdxStack.pm      |  2 +-
 lib/PublicInbox/Import.pm        | 18 ++++---
 lib/PublicInbox/Inbox.pm         |  2 +-
 lib/PublicInbox/InboxWritable.pm |  2 +-
 lib/PublicInbox/InputPipe.pm     |  1 -
 lib/PublicInbox/LEI.pm           |  8 ++--
 lib/PublicInbox/LeiALE.pm        |  6 +--
 lib/PublicInbox/LeiBlob.pm       |  3 +-
 lib/PublicInbox/LeiConfig.pm     |  2 +-
 lib/PublicInbox/LeiInput.pm      |  5 +-
 lib/PublicInbox/LeiMailSync.pm   |  3 +-
 lib/PublicInbox/LeiMirror.pm     | 57 +++++++++--------------
 lib/PublicInbox/LeiRediff.pm     | 11 ++---
 lib/PublicInbox/LeiStoreErr.pm   |  2 +-
 lib/PublicInbox/LeiSucks.pm      |  2 +-
 lib/PublicInbox/LeiToMail.pm     | 34 +++++++-------
 lib/PublicInbox/LeiViewText.pm   |  2 +-
 lib/PublicInbox/LeiXSearch.pm    |  8 ++--
 lib/PublicInbox/MailDiff.pm      | 21 ++++-----
 lib/PublicInbox/MboxReader.pm    |  4 +-
 lib/PublicInbox/MultiGit.pm      | 13 +++---
 lib/PublicInbox/ProcessIO.pm     | 75 ------------------------------
 lib/PublicInbox/ProcessIONBF.pm  | 25 ----------
 lib/PublicInbox/Qspawn.pm        |  5 +-
 lib/PublicInbox/SearchIdx.pm     |  4 +-
 lib/PublicInbox/SolverGit.pm     | 38 ++++++---------
 lib/PublicInbox/Spawn.pm         | 42 ++++++++++-------
 lib/PublicInbox/TestCommon.pm    |  4 +-
 lib/PublicInbox/V2Writable.pm    |  2 +-
 lib/PublicInbox/ViewVCS.pm       |  6 +--
 lib/PublicInbox/WWW.pm           |  2 +-
 lib/PublicInbox/XapHelper.pm     | 14 ++----
 lib/PublicInbox/XapHelperCxx.pm  |  3 +-
 script/public-inbox-convert      |  6 +--
 script/public-inbox-edit         |  3 +-
 script/public-inbox-init         |  4 +-
 t/cindex.t                       | 17 ++-----
 t/clone-coderepo.t               | 10 ++--
 t/extsearch.t                    | 48 ++++++-------------
 t/httpd-corner.t                 |  2 +-
 t/init.t                         |  2 +-
 t/io.t                           | 33 +++++++++++++
 t/lei-mirror.t                   | 10 ++--
 t/lei-q-kw.t                     |  4 +-
 t/spawn.t                        | 36 +++++++-------
 xt/check-run.t                   |  2 +-
 xt/git_async_cmp.t               |  4 +-
 xt/httpd-async-stream.t          |  2 +-
 59 files changed, 368 insertions(+), 432 deletions(-)
 create mode 100644 lib/PublicInbox/IO.pm
 delete mode 100644 lib/PublicInbox/ProcessIO.pm
 delete mode 100644 lib/PublicInbox/ProcessIONBF.pm
 create mode 100644 t/io.t

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 01/14] xap_helper.pm: use do_fork to Reset and reseed
  2023-11-02  9:35 [PATCH 00/14] IO/IPC-related cleanups Eric Wong
@ 2023-11-02  9:35 ` Eric Wong
  2023-11-02  9:35 ` [PATCH 02/14] ds: replace FD map hash table with array Eric Wong
                   ` (12 subsequent siblings)
  13 siblings, 0 replies; 17+ messages in thread
From: Eric Wong @ 2023-11-02  9:35 UTC (permalink / raw)
  To: meta

We may start using rand() in the worker someday if we need
to seed a hash function for caching.  It saves us some LoC
in the meantime.
---
 lib/PublicInbox/XapHelper.pm | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/lib/PublicInbox/XapHelper.pm b/lib/PublicInbox/XapHelper.pm
index f7640f4c..41c66a12 100644
--- a/lib/PublicInbox/XapHelper.pm
+++ b/lib/PublicInbox/XapHelper.pm
@@ -216,14 +216,9 @@ sub reap_worker { # awaitpid CB
 
 sub start_worker ($) {
 	my ($nr) = @_;
-	my $pid = fork;
-	if (!defined($pid)) {
-		warn("fork: $!");
-		return undef;
-	};
+	my $pid = eval { PublicInbox::DS::do_fork } // return(warn($@));
 	if ($pid == 0) {
 		undef %WORKERS;
-		PublicInbox::DS::Reset();
 		$SIG{TTIN} = $SIG{TTOU} = 'IGNORE';
 		$SIG{CHLD} = 'DEFAULT'; # Xapian may use this
 		recv_loop();

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 02/14] ds: replace FD map hash table with array
  2023-11-02  9:35 [PATCH 00/14] IO/IPC-related cleanups Eric Wong
  2023-11-02  9:35 ` [PATCH 01/14] xap_helper.pm: use do_fork to Reset and reseed Eric Wong
@ 2023-11-02  9:35 ` Eric Wong
  2023-11-02  9:35 ` [PATCH 03/14] treewide: use ->close method rather than CORE::close Eric Wong
                   ` (11 subsequent siblings)
  13 siblings, 0 replies; 17+ messages in thread
From: Eric Wong @ 2023-11-02  9:35 UTC (permalink / raw)
  To: meta

FDs are array indices into the kernel, anyways, so we can
take advantage of space savings and speedups because the
majority of FDs a big process has is going to end up in
the array, anyways.
---
 lib/PublicInbox/DS.pm          | 18 +++++++++---------
 lib/PublicInbox/LeiStoreErr.pm |  2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index ca1f0f81..8331da95 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -42,7 +42,7 @@ my $reap_armed;
 my @active; # FDs (or objs) returned by epoll/kqueue
 our (%AWAIT_PIDS, # pid => [ $callback, @args ]
 	$cur_runq, # only set inside next_tick
-     %DescriptorMap,             # fd (num) -> PublicInbox::DS object
+     @FD_MAP, # fd (num) -> PublicInbox::DS object
      $Poller, # global Select, Epoll, DSPoll, or DSKQXS ref
 
      @post_loop_do,              # subref + args to call at the end of each loop
@@ -74,7 +74,7 @@ sub Reset {
 		# clobbering $Poller may call DSKQXS::DESTROY,
 		# we must always have this set to something to avoid
 		# needing branches before ep_del/ep_mod calls (via ->close).
-		%DescriptorMap = (); # likely to call ep_del
+		@FD_MAP = ();
 		@Timers = ();
 		%UniqTimer = ();
 		@post_loop_do = ();
@@ -85,7 +85,7 @@ sub Reset {
 		$nextq = undef; # may call ep_del
 		%AWAIT_PIDS = ();
 	} while (@Timers || $nextq || keys(%AWAIT_PIDS) ||
-		@active || keys(%DescriptorMap) ||
+		@active || @FD_MAP ||
 		@post_loop_do || keys(%UniqTimer) ||
 		scalar(@{$cur_runq // []})); # do not vivify cur_runq
 
@@ -226,7 +226,7 @@ sub in_loop () { $in_loop }
 # use inside @post_loop_do, returns number of busy clients
 sub close_non_busy () {
 	my $n = 0;
-	for my $s (values %DescriptorMap) {
+	for my $s (grep defined, @FD_MAP) {
 		# close as much as possible, early as possible
 		($s->busy ? ++$n : $s->close) if $s->can('busy');
 	}
@@ -291,7 +291,7 @@ sub event_loop (;$$) {
 		$Poller->ep_wait($timeout, \@active);
 
 		# map all FDs to their associated Perl object
-		@active = @DescriptorMap{@active};
+		@active = @FD_MAP[@active];
 
 		while (my $obj = shift @active) {
 			$obj->event_step;
@@ -329,10 +329,10 @@ retry:
         }
         die "EPOLL_CTL_ADD $self/$sock/$fd: $!";
     }
-    croak("FD:$fd in use by $DescriptorMap{$fd} (for $self/$sock)")
-        if defined($DescriptorMap{$fd});
+    defined($FD_MAP[$fd]) and
+		croak("BUG: FD:$fd in use by $FD_MAP[$fd] (for $self/$sock)");
 
-    $DescriptorMap{$fd} = $self;
+    $FD_MAP[$fd] = $self;
 }
 
 # for IMAP, NNTP, and POP3 which greet clients upon connect
@@ -366,7 +366,7 @@ sub close {
 	# be self-referential closures (sub { $client->close })
 	# preventing the object from being destroyed
 	delete $self->{wbuf};
-	delete $DescriptorMap{fileno($sock)};
+	$FD_MAP[fileno($sock)] = undef;
 
 	!$Poller->ep_del($sock); # stop getting notifications
 }
diff --git a/lib/PublicInbox/LeiStoreErr.pm b/lib/PublicInbox/LeiStoreErr.pm
index fe4af51e..c8bc72b6 100644
--- a/lib/PublicInbox/LeiStoreErr.pm
+++ b/lib/PublicInbox/LeiStoreErr.pm
@@ -47,7 +47,7 @@ sub event_step {
 	return ($!{EAGAIN} ? 0 : $self->close) if !defined($n);
 	return $self->close if !$n;
 	my $printed;
-	for my $lei (values %PublicInbox::DS::DescriptorMap) {
+	for my $lei (grep defined, @PublicInbox::DS::FD_MAP) {
 		my $cb = $lei->can('store_path') // next;
 		next if $cb->($lei) ne $self->{store_path};
 		emit($lei->{2} // next, $buf) and $printed = 1;

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 03/14] treewide: use ->close method rather than CORE::close
  2023-11-02  9:35 [PATCH 00/14] IO/IPC-related cleanups Eric Wong
  2023-11-02  9:35 ` [PATCH 01/14] xap_helper.pm: use do_fork to Reset and reseed Eric Wong
  2023-11-02  9:35 ` [PATCH 02/14] ds: replace FD map hash table with array Eric Wong
@ 2023-11-02  9:35 ` Eric Wong
  2023-11-02 21:35   ` [PATCH 15/14] ds: don't try ->close after ->accept_SSL failure Eric Wong
  2023-11-02  9:35 ` [PATCH 04/14] cindex: drop redundant close on regular FH Eric Wong
                   ` (10 subsequent siblings)
  13 siblings, 1 reply; 17+ messages in thread
From: Eric Wong @ 2023-11-02  9:35 UTC (permalink / raw)
  To: meta

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 ability to do `stat' directly
on the object instead of the awkward `tied' thing.
---
 lib/PublicInbox/CodeSearchIdx.pm | 6 +++---
 lib/PublicInbox/DS.pm            | 4 ++--
 lib/PublicInbox/DirIdle.pm       | 2 +-
 lib/PublicInbox/Git.pm           | 6 +++---
 lib/PublicInbox/HTTP.pm          | 4 ++--
 lib/PublicInbox/LeiMirror.pm     | 4 ++--
 6 files changed, 13 insertions(+), 13 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 8331da95..33f80087 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -341,8 +341,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/Git.pm b/lib/PublicInbox/Git.pm
index 191e4eea..3dac32be 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;
 	}
 }
@@ -621,7 +621,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 0f378768..71f41a11 100644
--- a/lib/PublicInbox/LeiMirror.pm
+++ b/lib/PublicInbox/LeiMirror.pm
@@ -334,7 +334,7 @@ sub fgrp_update {
 			upr($lei, $w, 'create', $ref, $oid);
 		}
 	}
-	CORE::close($w) or upref_warn();
+	$w->close or upref_warn();
 }
 
 sub satellite_done {
@@ -344,7 +344,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);

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 15/14] ds: don't try ->close after ->accept_SSL failure
  2023-11-02  9:35 ` [PATCH 03/14] treewide: use ->close method rather than CORE::close Eric Wong
@ 2023-11-02 21:35   ` Eric Wong
  0 siblings, 0 replies; 17+ messages in thread
From: Eric Wong @ 2023-11-02 21:35 UTC (permalink / raw)
  To: meta

Eric Wong <e@80x24.org> wrote:
> --- a/lib/PublicInbox/DS.pm
> +++ b/lib/PublicInbox/DS.pm
> @@ -341,8 +341,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);

Noticed this on deploy:

-----8<-----
Subject: [PATCH] ds: don't try ->close after ->accept_SSL failure

->accept_SSL failures leaves the socket ref as a GLOB (not
IO::Handle) and unable to respond to the ->close method.
Calling close in any form isn't actually necessary at all,
so just let refcounting destroy the socket.
---
 lib/PublicInbox/DS.pm | 3 +--
 t/nntpd-tls.t         | 5 +++++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index 33f80087..da26efc4 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -341,8 +341,7 @@ sub greet {
 	my $ev = EPOLLIN;
 	my $wbuf;
 	if ($sock->can('accept_SSL') && !$sock->accept_SSL) {
-		return $sock->close if $! != EAGAIN;
-		$ev = PublicInbox::TLS::epollbit() or return $sock->close;
+		return if $! != EAGAIN || !($ev = PublicInbox::TLS::epollbit());
 		$wbuf = [ \&accept_tls_step, $self->can('do_greet')];
 	}
 	new($self, $sock, $ev | EPOLLONESHOT);
diff --git a/t/nntpd-tls.t b/t/nntpd-tls.t
index cf3c95c9..a11a0dd9 100644
--- a/t/nntpd-tls.t
+++ b/t/nntpd-tls.t
@@ -185,6 +185,10 @@ for my $args (
 		is($x, undef, 'no BSD accept filter for plain NNTP');
 	};
 
+	my $s = tcp_connect($nntps);
+	syswrite($s, '->accept_SSL_ will fail on this!');
+	ok(!sysread($s, my $rbuf, 128), 'EOF or ECONNRESET on ->accept_SSL fail');
+
 	$c = undef;
 	$td->kill;
 	$td->join;
@@ -195,6 +199,7 @@ for my $args (
 		<$fh>;
 	};
 	unlike($eout, qr/wide/i, 'no Wide character warnings');
+	unlike($eout, qr/^E:/, 'no other errors');
 }
 done_testing();
 

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 04/14] cindex: drop redundant close on regular FH
  2023-11-02  9:35 [PATCH 00/14] IO/IPC-related cleanups Eric Wong
                   ` (2 preceding siblings ...)
  2023-11-02  9:35 ` [PATCH 03/14] treewide: use ->close method rather than CORE::close Eric Wong
@ 2023-11-02  9:35 ` Eric Wong
  2023-11-02  9:35 ` [PATCH 05/14] treewide: use ->close to call ProcessIO->CLOSE Eric Wong
                   ` (9 subsequent siblings)
  13 siblings, 0 replies; 17+ messages in thread
From: Eric Wong @ 2023-11-02  9:35 UTC (permalink / raw)
  To: meta

There's no need to waste optree space on close() statements for
file handles which are (effectively) read-only on their last use
and incapable of error checking in our Perl code (since they're
only read by git).

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 | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/lib/PublicInbox/CodeSearchIdx.pm b/lib/PublicInbox/CodeSearchIdx.pm
index c1a1ee90..ad915fa2 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;
@@ -971,7 +969,7 @@ sub dump_git_commits { # run_await cb
 	(defined($pid) && $?) and die "E: @PRUNE_BATCH: \$?=$?";
 	return if $DO_QUIT;
 	my ($hexlen) = keys(%ALT_FH) or return; # done
-	close(delete $ALT_FH{$hexlen});
+	delete $ALT_FH{$hexlen};
 
 	$PRUNE_BATCH[1] = "--git-dir=$TMPDIR/hexlen$hexlen.git";
 	run_await(\@PRUNE_BATCH, undef, $batch_opt, \&dump_git_commits);

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 05/14] treewide: use ->close to call ProcessIO->CLOSE
  2023-11-02  9:35 [PATCH 00/14] IO/IPC-related cleanups Eric Wong
                   ` (3 preceding siblings ...)
  2023-11-02  9:35 ` [PATCH 04/14] cindex: drop redundant close on regular FH Eric Wong
@ 2023-11-02  9:35 ` Eric Wong
  2023-11-02  9:35 ` [PATCH 06/14] multi_git: use autodie Eric Wong
                   ` (8 subsequent siblings)
  13 siblings, 0 replies; 17+ messages in thread
From: Eric Wong @ 2023-11-02  9:35 UTC (permalink / raw)
  To: meta

This will open the door for us to drop `tie' usage from
ProcessIO completely in favor of OO method dispatch.  While
OO method dispatches (e.g. `$fh->close') are slower than normal
subroutine calls, it hardly matters in this case since process
teardown is a fairly rare operation and we continue to use
`close($fh)' for Maildir writes.
---
 lib/PublicInbox/Config.pm        |  2 +-
 lib/PublicInbox/GitCredential.pm |  4 ++--
 lib/PublicInbox/Import.pm        |  6 +++---
 lib/PublicInbox/LEI.pm           |  8 ++++----
 lib/PublicInbox/LeiInput.pm      |  2 +-
 lib/PublicInbox/LeiRediff.pm     |  2 +-
 lib/PublicInbox/LeiViewText.pm   |  2 +-
 lib/PublicInbox/LeiXSearch.pm    |  8 ++++----
 lib/PublicInbox/MboxReader.pm    |  4 ++--
 lib/PublicInbox/SearchIdx.pm     |  2 +-
 lib/PublicInbox/Spawn.pm         |  2 +-
 lib/PublicInbox/V2Writable.pm    |  2 +-
 script/public-inbox-convert      |  4 ++--
 t/httpd-corner.t                 |  2 +-
 t/lei-q-kw.t                     |  4 ++--
 t/spawn.t                        | 10 +++++-----
 xt/git_async_cmp.t               |  4 ++--
 xt/httpd-async-stream.t          |  2 +-
 18 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/lib/PublicInbox/Config.pm b/lib/PublicInbox/Config.pm
index 0a572103..01cb536d 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;
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;
 }
 
diff --git a/lib/PublicInbox/Import.pm b/lib/PublicInbox/Import.pm
index 6eee8774..e12a56e8 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) = @_;
@@ -598,7 +598,7 @@ sub replace_oids {
 			push @buf, $_;
 		}
 	}
-	close $rd;
+	$rd->close or die "E: git @export (\$?=$?)";
 	if (@buf) {
 		print $io @buf or wfail;
 	}
diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm
index 0f6f7f6f..2832db63 100644
--- a/lib/PublicInbox/LEI.pm
+++ b/lib/PublicInbox/LEI.pm
@@ -501,7 +501,7 @@ sub err ($;@) {
 	my @eor = (substr($_[-1]//'', -1, 1) eq "\n" ? () : ("\n"));
 	print $err @_, @eor and return;
 	my $old_err = delete $self->{2};
-	close($old_err) if $! == EPIPE && $old_err;
+	$old_err->close if $! == EPIPE && $old_err;
 	$err = $self->{2} = ($self->{pgr} // [])->[2] // *STDERR{GLOB};
 	print $err @_, @eor or print STDERR @_, @eor;
 }
@@ -516,7 +516,7 @@ sub qfin { # show message on finalization (LeiFinmsg)
 
 sub fail_handler ($;$$) {
 	my ($lei, $code, $io) = @_;
-	close($io) if $io; # needed to avoid warnings on SIGPIPE
+	$io->close if $io; # needed to avoid warnings on SIGPIPE
 	_drop_wq($lei);
 	x_it($lei, $code // (1 << 8));
 }
@@ -565,7 +565,7 @@ sub child_error { # passes non-fatal curl exit codes to user
 
 sub note_sigpipe { # triggers sigpipe_handler
 	my ($self, $fd) = @_;
-	close(delete($self->{$fd})); # explicit close silences Perl warning
+	delete($self->{$fd})->close; # explicit close silences Perl warning
 	$self->{pkt_op_p}->pkt_do('sigpipe_handler') if $self->{pkt_op_p};
 	x_it($self, 13);
 }
@@ -1129,7 +1129,7 @@ sub stop_pager {
 	my ($self) = @_;
 	my $pgr = delete($self->{pgr}) or return;
 	$self->{2} = $pgr->[2];
-	close(delete($self->{1})) if $self->{1};
+	delete($self->{1})->close if $self->{1};
 	$self->{1} = $pgr->[1];
 }
 
diff --git a/lib/PublicInbox/LeiInput.pm b/lib/PublicInbox/LeiInput.pm
index 28b73ca9..f7c3f573 100644
--- a/lib/PublicInbox/LeiInput.pm
+++ b/lib/PublicInbox/LeiInput.pm
@@ -246,7 +246,7 @@ sub input_path_url {
 			my $fh = popen_rd($fp, undef, $rdr);
 			eval { $self->input_fh('eml', $fh, $input, @args) };
 			my @err = ($@ ? $@ : ());
-			close($fh) or push @err, "\$?=$?";
+			$fh->close or push @err, "\$?=$?";
 			$lei->child_error($?, "@$fp failed: @err") if @err;
 		} else {
 			$self->folder_missing("$ifmt:$input");
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);
diff --git a/lib/PublicInbox/LeiViewText.pm b/lib/PublicInbox/LeiViewText.pm
index 70441867..ce9f248e 100644
--- a/lib/PublicInbox/LeiViewText.pm
+++ b/lib/PublicInbox/LeiViewText.pm
@@ -75,7 +75,7 @@ sub new {
 	my @cmd = qw(git config -z --includes -l); # reuse normal git config
 	my $r = popen_rd(\@cmd, undef, { 2 => $lei->{2} });
 	my $cfg = PublicInbox::Config::config_fh_parse($r, "\0", "\n");
-	if (!close($r)) {
+	if ($r->close) {
 		warn "# @cmd failed, no color (non-fatal \$?=$?)\n";
 		return $self;
 	}
diff --git a/lib/PublicInbox/LeiXSearch.pm b/lib/PublicInbox/LeiXSearch.pm
index 241b9dab..5443188d 100644
--- a/lib/PublicInbox/LeiXSearch.pm
+++ b/lib/PublicInbox/LeiXSearch.pm
@@ -353,14 +353,14 @@ 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;
 			mset_progress($lei, $lei->{-current_url}, $nr, $nr);
 			next;
 		}
-		close(delete($rdr->{2})) if @lbf_tee;
+		delete($rdr->{2})->close if @lbf_tee;
 		seek($cerr, 0, SEEK_SET);
 		read($cerr, my $err, -s $cerr);
 		truncate($cerr, 0);
@@ -396,8 +396,8 @@ sub query_done { # EOF callback for main daemon
 	$lei->{ovv}->ovv_end($lei);
 	if ($l2m) { # close() calls LeiToMail reap_compress
 		if (my $out = delete $lei->{old_1}) {
-			if (my $mbout = $lei->{1}) {
-				close($mbout) or die <<"";
+			if (my $mbout = $lei->{1}) { # compressor pipe process
+				$mbout->close or die <<"";
 Error closing $lei->{ovv}->{dst}: \$!=$! \$?=$?
 
 			}
diff --git a/lib/PublicInbox/MboxReader.pm b/lib/PublicInbox/MboxReader.pm
index d67fb4eb..3d78ca23 100644
--- a/lib/PublicInbox/MboxReader.pm
+++ b/lib/PublicInbox/MboxReader.pm
@@ -29,7 +29,7 @@ sub _mbox_from {
 	my @raw;
 	while (defined(my $r = read($mbfh, $buf, 65536, length($buf)))) {
 		if ($r == 0) { # close here to check for "curl --fail"
-			close($mbfh) or die "error closing mbox: \$?=$? $!";
+			$mbfh->close or die "error closing mbox: \$?=$? $!";
 			@raw = ($buf);
 		} else {
 			@raw = split(/$from_strict/mos, $buf, -1);
@@ -88,7 +88,7 @@ sub _mbox_cl ($$$;@) {
 	my $buf = '';
 	while (defined(my $r = read($mbfh, $buf, 65536, length($buf)))) {
 		if ($r == 0) { # detect "curl --fail"
-			close($mbfh) or
+			$mbfh->close or
 				die "error closing mboxcl/mboxcl2: \$?=$? $!";
 			undef $mbfh;
 		}
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;
 }
diff --git a/lib/PublicInbox/Spawn.pm b/lib/PublicInbox/Spawn.pm
index e7d2c6ea..b4f37bea 100644
--- a/lib/PublicInbox/Spawn.pm
+++ b/lib/PublicInbox/Spawn.pm
@@ -421,7 +421,7 @@ sub run_qx {
 		local $/;
 		$ret[0] = <$fh>;
 	}
-	close $fh; # caller should check $?
+	$fh->close; # caller should check $?
 	read_out_err($opt);
 	wantarray ? @ret : $ret[0];
 }
diff --git a/lib/PublicInbox/V2Writable.pm b/lib/PublicInbox/V2Writable.pm
index 191c5588..4d606dfe 100644
--- a/lib/PublicInbox/V2Writable.pm
+++ b/lib/PublicInbox/V2Writable.pm
@@ -1070,7 +1070,7 @@ sub unindex_todo ($$$) {
 		/\A:\d{6} 100644 $OID ($OID) [AM]\tm$/o or next;
 		$self->git->cat_async($1, $unindex_oid, { %$sync, oid => $1 });
 	}
-	close $fh or die "git log failed: \$?=$?";
+	$fh->close or die "git log failed: \$?=$?";
 	$self->git->async_wait_all;
 
 	return unless $sync->{-opt}->{prune};
diff --git a/script/public-inbox-convert b/script/public-inbox-convert
index 96f6d2ea..d8186809 100755
--- a/script/public-inbox-convert
+++ b/script/public-inbox-convert
@@ -1,5 +1,5 @@
 #!/usr/bin/perl -w
-# Copyright (C) 2018-2021 all contributors <meta@public-inbox.org>
+# Copyright (C) all contributors <meta@public-inbox.org>
 # License: AGPL-3.0+ <http://www.gnu.org/licenses/agpl-3.0.txt>
 use strict;
 use v5.10.1;
@@ -158,7 +158,7 @@ while (<$rd>) {
 	last if $_ eq "done\n";
 	print $io $_ or $im->wfail;
 }
-close $rd or die "fast-export: \$?=$? \$!=$!\n";
+$rd->close or die "fast-export: \$?=$? \$!=$!\n";
 $io = undef;
 $v2w->done;
 if (my $old_mm = $old->mm) {
diff --git a/t/httpd-corner.t b/t/httpd-corner.t
index 2d2d1061..da1c24b9 100644
--- a/t/httpd-corner.t
+++ b/t/httpd-corner.t
@@ -368,7 +368,7 @@ SKIP: {
 		$n += $r;
 		$buf =~ /\A\0+\z/ or $non_zero++;
 	}
-	close $fh or die "close curl pipe: $!";
+	$fh->close or die "close curl pipe: $!";
 	is($?, 0, 'curl succesful');
 	is($n, 30 * 1024 * 1024, 'got expected output from curl');
 	is($non_zero, 0, 'read all zeros');
diff --git a/t/lei-q-kw.t b/t/lei-q-kw.t
index 4edee72a..06e1df6c 100644
--- a/t/lei-q-kw.t
+++ b/t/lei-q-kw.t
@@ -1,5 +1,5 @@
 #!perl -w
-# Copyright (C) 2020-2021 all contributors <meta@public-inbox.org>
+# Copyright (C) all contributors <meta@public-inbox.org>
 # License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
 use strict; use v5.10.1; use PublicInbox::TestCommon;
 use POSIX qw(mkfifo);
@@ -51,7 +51,7 @@ SKIP: {
 		'--import-before fails on non-seekable output');
 	like($lei_err, qr/not seekable/, 'unseekable noted in error');
 	is(do { local $/; <$cat> }, '', 'no output on FIFO');
-	close $cat;
+	$cat->close;
 	$cat = popen_rd(['cat', $o]);
 	lei_ok(qw(q m:qp@example.com -o), "mboxrd:$o");
 	my $buf = do { local $/; <$cat> };
diff --git a/t/spawn.t b/t/spawn.t
index 4b3baae4..938a2e5e 100644
--- a/t/spawn.t
+++ b/t/spawn.t
@@ -80,7 +80,7 @@ EOF
 	is($rdy, 'RDY', 'got ready signal, waitpid(-1) works in child');
 	ok(kill('CHLD', $pid), 'sent SIGCHLD to child');
 	is(readline($rd), "HI\n", '$SIG{CHLD} works in child');
-	ok(close $rd, 'popen_rd close works');
+	ok($rd->close, 'popen_rd close works');
 	PublicInbox::DS::sig_setmask($oldset);
 }
 
@@ -133,13 +133,13 @@ EOF
 	is($buf, "hello\n", 'tied gets works');
 	is(sysread($fh, $buf, 6), 0, 'sysread got EOF');
 	$? = 1;
-	ok(close($fh), 'close succeeds');
+	ok($fh->close, 'close succeeds');
 	is($?, 0, '$? set properly');
 }
 
 {
 	my $fh = popen_rd([qw(false)]);
-	ok(!close($fh), 'close fails on false');
+	ok(!$fh->close, 'close fails on false');
 	isnt($?, 0, '$? set properly: '.$?);
 }
 
@@ -152,7 +152,7 @@ EOF
 { # ->CLOSE vs ->DESTROY waitpid caller distinction
 	my @c;
 	my $fh = popen_rd(['true'], undef, undef, sub { @c = caller });
-	ok(close($fh), '->CLOSE fired and successful');
+	ok($fh->close, '->CLOSE fired and successful');
 	ok(scalar(@c), 'callback fired by ->CLOSE');
 	ok(grep(!m[/PublicInbox/DS\.pm\z], @c), 'callback not invoked by DS');
 
@@ -183,7 +183,7 @@ EOF
 	my @w;
 	local $SIG{__WARN__} = sub { push @w, @_ };
 	close $w;
-	close $fh;
+	$fh->close; # may set $?
 	is($?, 0, 'cat exited');
 	is(scalar(@arg), 2, 'callback got args');
 	is($arg[1], 'hi', 'passed arg');
diff --git a/xt/git_async_cmp.t b/xt/git_async_cmp.t
index 9edc1f37..4038898b 100644
--- a/xt/git_async_cmp.t
+++ b/xt/git_async_cmp.t
@@ -31,7 +31,7 @@ my $async = timeit($nr, sub {
 		my ($oid, undef, undef) = split(/ /);
 		$git->cat_async($oid, $cb);
 	}
-	close $cat or die "cat: $?";
+	$cat->close or xbail "cat: $?";
 	$git->async_wait_all;
 	push @dig, ['async', $dig->hexdigest ];
 });
@@ -44,7 +44,7 @@ my $sync = timeit($nr, sub {
 		my $bref = $git->cat_file($oid);
 		$dig->add($$bref);
 	}
-	close $cat or die "cat: $?";
+	$cat->close or xbail "cat: $?";
 	push @dig, ['sync', $dig->hexdigest ];
 });
 
diff --git a/xt/httpd-async-stream.t b/xt/httpd-async-stream.t
index 904f2ae9..099ceb79 100644
--- a/xt/httpd-async-stream.t
+++ b/xt/httpd-async-stream.t
@@ -67,7 +67,7 @@ my $do_get_all = sub {
 	}
 	my $res = $dig->hexdigest;
 	my $elapsed = sprintf('%0.3f', now() - $t0);
-	close $rd or die "close curl failed: $! \$?=$?\n";
+	$rd->close or xbail "close curl failed: $! \$?=$?\n";
 	print STDERR "# $job $$ ($?) $res (${elapsed}s) $bytes bytes\n";
 	$res;
 };

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 06/14] multi_git: use autodie
  2023-11-02  9:35 [PATCH 00/14] IO/IPC-related cleanups Eric Wong
                   ` (4 preceding siblings ...)
  2023-11-02  9:35 ` [PATCH 05/14] treewide: use ->close to call ProcessIO->CLOSE Eric Wong
@ 2023-11-02  9:35 ` Eric Wong
  2023-11-02  9:35 ` [PATCH 07/14] git_credential: use autodie where appropriate Eric Wong
                   ` (7 subsequent siblings)
  13 siblings, 0 replies; 17+ messages in thread
From: Eric Wong @ 2023-11-02  9:35 UTC (permalink / raw)
  To: meta

Trying to move away from half my code being "or die" statements...
---
 lib/PublicInbox/MultiGit.pm | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/lib/PublicInbox/MultiGit.pm b/lib/PublicInbox/MultiGit.pm
index 9074758a..1e8eb47a 100644
--- a/lib/PublicInbox/MultiGit.pm
+++ b/lib/PublicInbox/MultiGit.pm
@@ -10,6 +10,7 @@ use PublicInbox::Import;
 use File::Temp 0.19;
 use List::Util qw(max);
 use PublicInbox::Git qw(read_all);
+use autodie qw(chmod close rename);
 
 sub new {
 	my ($cls, $topdir, $all, $epfx) = @_;
@@ -68,12 +69,10 @@ sub write_alternates {
 	my $out = join('', 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);
-	my $f = $fh->filename;
-	print $fh $out, @new or die "print($f): $!";
-	chmod($mode, $fh) or die "fchmod($f): $!";
-	close $fh or die "close($f): $!";
-	my $fn = "$info_dir/alternates";
-	rename($f, $fn) or die "rename($f, $fn): $!";
+	print $fh $out, @new;
+	chmod($mode, $fh);
+	close $fh;
+	rename($fh->filename, "$info_dir/alternates");
 	$fh->unlink_on_destroy(0);
 }
 

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 07/14] git_credential: use autodie where appropriate
  2023-11-02  9:35 [PATCH 00/14] IO/IPC-related cleanups Eric Wong
                   ` (5 preceding siblings ...)
  2023-11-02  9:35 ` [PATCH 06/14] multi_git: use autodie Eric Wong
@ 2023-11-02  9:35 ` Eric Wong
  2023-11-02  9:35 ` [PATCH 08/14] replace ProcessIO with untied PublicInbox::IO Eric Wong
                   ` (6 subsequent siblings)
  13 siblings, 0 replies; 17+ messages in thread
From: Eric Wong @ 2023-11-02  9:35 UTC (permalink / raw)
  To: meta

We can also rely on `say' in Perl 5.10+ to save us the trouble
of printing a newline.
---
 lib/PublicInbox/GitCredential.pm | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/PublicInbox/GitCredential.pm b/lib/PublicInbox/GitCredential.pm
index a4444e2c..bb225ff3 100644
--- a/lib/PublicInbox/GitCredential.pm
+++ b/lib/PublicInbox/GitCredential.pm
@@ -5,19 +5,20 @@
 package PublicInbox::GitCredential;
 use v5.12;
 use PublicInbox::Spawn qw(popen_rd);
+use autodie qw(close pipe);
 
 sub run ($$;$) {
 	my ($self, $op, $lei) = @_;
 	my ($in_r, $in_w, $out_r);
 	my $cmd = [ qw(git credential), $op ];
-	pipe($in_r, $in_w) or die "pipe: $!";
+	pipe($in_r, $in_w);
 	if ($lei) { # we'll die if disconnected:
-		pipe($out_r, my $out_w) or die "pipe: $!";
+		pipe($out_r, my $out_w);
 		$lei->send_exec_cmd([ $in_r, $out_w ], $cmd, {});
 	} else {
 		$out_r = popen_rd($cmd, undef, { 0 => $in_r });
 	}
-	close $in_r or die "close in_r: $!";
+	close $in_r;
 
 	my $out = '';
 	for my $k (qw(url protocol host username password)) {
@@ -25,9 +26,8 @@ sub run ($$;$) {
 		die "`$k' contains `\\n' or `\\0'\n" if $v =~ /[\n\0]/;
 		$out .= "$k=$v\n";
 	}
-	$out .= "\n";
-	print $in_w $out or die "print (git credential $op): $!";
-	close $in_w or die "close (git credential $op): $!";
+	say $in_w $out;
+	close $in_w;
 	return $out_r if $op eq 'fill';
 	<$out_r> and die "unexpected output from `git credential $op'\n";
 	$out_r->close or die "`git credential $op' failed: \$!=$! \$?=$?\n";

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 08/14] replace ProcessIO with untied PublicInbox::IO
  2023-11-02  9:35 [PATCH 00/14] IO/IPC-related cleanups Eric Wong
                   ` (6 preceding siblings ...)
  2023-11-02  9:35 ` [PATCH 07/14] git_credential: use autodie where appropriate Eric Wong
@ 2023-11-02  9:35 ` Eric Wong
  2023-11-02  9:35 ` [PATCH 09/14] io: introduce write_file helper sub Eric Wong
                   ` (5 subsequent siblings)
  13 siblings, 0 replies; 17+ messages in thread
From: Eric Wong @ 2023-11-02  9:35 UTC (permalink / raw)
  To: meta

This fixes two major problems with the use of tie for filehandles:

* no way to do fcntl, stat, etc. calls directly on the tied handle,
  forcing callers to use the `tied' perlop to access the underlying
  IO::Handle

* needing separate classes to handle blocking and non-blocking I/O

As a result, Git->cleanup_if_unlinked, InputPipe->consume,
and Qspawn->_yield_start have fewer bizzare bits and we
can call `$io->blocking(0)' directly instead of
`(tied *$io)->{fh}->blocking(0)'

Having a PublicInbox::IO class will also allow us to support
custom read buffering which allows inspecting the current state.
---
 MANIFEST                        |  3 +-
 lib/PublicInbox/Gcf2Client.pm   |  7 ++-
 lib/PublicInbox/Git.pm          |  8 ++--
 lib/PublicInbox/IO.pm           | 54 ++++++++++++++++++++++++
 lib/PublicInbox/Import.pm       |  4 +-
 lib/PublicInbox/InputPipe.pm    |  1 -
 lib/PublicInbox/LeiToMail.pm    |  5 +--
 lib/PublicInbox/ProcessIO.pm    | 75 ---------------------------------
 lib/PublicInbox/ProcessIONBF.pm | 25 -----------
 lib/PublicInbox/Qspawn.pm       |  5 +--
 lib/PublicInbox/Spawn.pm        |  6 +--
 t/spawn.t                       | 26 ++++++------
 xt/check-run.t                  |  2 +-
 13 files changed, 85 insertions(+), 136 deletions(-)
 create mode 100644 lib/PublicInbox/IO.pm
 delete mode 100644 lib/PublicInbox/ProcessIO.pm
 delete mode 100644 lib/PublicInbox/ProcessIONBF.pm

diff --git a/MANIFEST b/MANIFEST
index 3df48667..479c09de 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -218,6 +218,7 @@ lib/PublicInbox/IMAPClient.pm
 lib/PublicInbox/IMAPD.pm
 lib/PublicInbox/IMAPTracker.pm
 lib/PublicInbox/IMAPsearchqp.pm
+lib/PublicInbox/IO.pm
 lib/PublicInbox/IPC.pm
 lib/PublicInbox/IdxStack.pm
 lib/PublicInbox/Import.pm
@@ -319,8 +320,6 @@ lib/PublicInbox/OverIdx.pm
 lib/PublicInbox/POP3.pm
 lib/PublicInbox/POP3D.pm
 lib/PublicInbox/PktOp.pm
-lib/PublicInbox/ProcessIO.pm
-lib/PublicInbox/ProcessIONBF.pm
 lib/PublicInbox/Qspawn.pm
 lib/PublicInbox/Reply.pm
 lib/PublicInbox/RepoAtom.pm
diff --git a/lib/PublicInbox/Gcf2Client.pm b/lib/PublicInbox/Gcf2Client.pm
index f63a0335..5220c474 100644
--- a/lib/PublicInbox/Gcf2Client.pm
+++ b/lib/PublicInbox/Gcf2Client.pm
@@ -10,7 +10,7 @@ use PublicInbox::Gcf2; # fails if Inline::C or libgit2-dev isn't available
 use PublicInbox::Spawn qw(spawn);
 use Socket qw(AF_UNIX SOCK_STREAM);
 use PublicInbox::Syscall qw(EPOLLIN);
-use PublicInbox::ProcessIO;
+use PublicInbox::IO;
 use autodie qw(socketpair);
 
 # fields:
@@ -32,11 +32,10 @@ sub new  {
 	$opt->{0} = $opt->{1} = $s2;
 	my $cmd = [$^X, $^W ? ('-w') : (),
 			qw[-MPublicInbox::Gcf2 -e PublicInbox::Gcf2::loop]];
-	my $pid = spawn($cmd, $env, $opt);
-	my $sock = PublicInbox::ProcessIO->maybe_new($pid, $s1);
+	PublicInbox::IO::attach_pid($s1, spawn($cmd, $env, $opt));
 	$self->{inflight} = [];
 	$self->{epwatch} = \undef; # for Git->cleanup
-	$self->SUPER::new($sock, EPOLLIN);
+	$self->SUPER::new($s1, EPOLLIN);
 }
 
 sub gcf2_async ($$$;$) {
diff --git a/lib/PublicInbox/Git.pm b/lib/PublicInbox/Git.pm
index 3dac32be..d00f576e 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 PublicInbox::Spawn qw(spawn popen_rd run_qx which);
-use PublicInbox::ProcessIONBF;
+use PublicInbox::IO;
 use PublicInbox::Tmpfile;
 use IO::Poll qw(POLLIN);
 use Carp qw(croak carp);
@@ -146,6 +146,7 @@ sub _sock_cmd {
 	my ($self, $batch, $err_c) = @_;
 	$self->{sock} and Carp::confess('BUG: {sock} exists');
 	socketpair(my $s1, my $s2, AF_UNIX, SOCK_STREAM, 0);
+	$s1->blocking(0);
 	my $opt = { pgid => 0, 0 => $s2, 1 => $s2 };
 	my $gd = $self->{git_dir};
 	if ($gd =~ s!/([^/]+/[^/]+)\z!/!) {
@@ -164,7 +165,7 @@ sub _sock_cmd {
 						$self->fail("tmpfile($id): $!");
 	}
 	my $pid = spawn(\@cmd, undef, $opt);
-	$self->{sock} = PublicInbox::ProcessIONBF->new($pid, $s1);
+	$self->{sock} = PublicInbox::IO::attach_pid($s1, $pid);
 }
 
 sub poll_in ($) { IO::Poll::_poll($RDTIMEO, fileno($_[0]), my $ev = POLLIN) }
@@ -638,8 +639,7 @@ sub cleanup_if_unlinked {
 	my $ret = 0;
 	for my $obj ($self, ($self->{ck} // ())) {
 		my $sock = $obj->{sock} // next;
-		my PublicInbox::ProcessIONBF $p = tied *$sock; # ProcessIONBF
-		my $pid = $p->{pid} // next;
+		my $pid = $sock->attached_pid // next;
 		open my $fh, '<', "/proc/$pid/maps" or return cleanup($self, 1);
 		while (<$fh>) {
 			# n.b. we do not restart for unlinked multi-pack-index
diff --git a/lib/PublicInbox/IO.pm b/lib/PublicInbox/IO.pm
new file mode 100644
index 00000000..63850a52
--- /dev/null
+++ b/lib/PublicInbox/IO.pm
@@ -0,0 +1,54 @@
+# Copyright (C) all contributors <meta@public-inbox.org>
+# License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
+
+# supports reaping of children tied to a pipe or socket
+package PublicInbox::IO;
+use v5.12;
+use parent qw(IO::Handle);
+use PublicInbox::DS qw(awaitpid);
+
+# TODO: this can probably be the new home for read_all, try_cat
+# and maybe even buffered read/readline...
+
+sub waitcb { # awaitpid callback
+	my ($pid, $errref, $cb, @args) = @_;
+	$$errref = $?; # sets .cerr for _close
+	$cb->($pid, @args) if $cb;
+}
+
+sub attach_pid ($$;@) {
+	my ($io, $pid, @cb_arg) = @_;
+	bless $io, __PACKAGE__;
+	# we share $err (and not $self) with awaitpid to avoid a ref cycle
+	${*$io}{pi_io_reap} = [ $$, $pid, \(my $err) ];
+	awaitpid($pid, \&waitcb, \$err, @cb_arg);
+	$io;
+}
+
+sub attached_pid {
+	my ($io) = @_;
+	${${*$io}{pi_io_reap} // []}[1];
+}
+
+# caller cares about error result if they call close explicitly
+# reap->[2] may be set before this is called via waitcb
+sub close {
+	my ($io) = @_;
+	my $ret = $io->SUPER::close;
+	my $reap = delete ${*$io}{pi_io_reap};
+	return $ret unless $reap && $reap->[0] == $$;
+	${$reap->[2]} // (my $w = awaitpid($reap->[1])); # sets [2]
+	($? = ${$reap->[2]}) ? '' : $ret;
+}
+
+sub DESTROY {
+	my ($io) = @_;
+	my $reap = delete ${*$io}{pi_io_reap};
+	if ($reap && $reap->[0] == $$) {
+		$io->SUPER::close;
+		awaitpid($reap->[1]);
+	}
+	$io->SUPER::DESTROY;
+}
+
+1;
diff --git a/lib/PublicInbox/Import.pm b/lib/PublicInbox/Import.pm
index e12a56e8..dfba34b9 100644
--- a/lib/PublicInbox/Import.pm
+++ b/lib/PublicInbox/Import.pm
@@ -16,7 +16,7 @@ use PublicInbox::MsgTime qw(msg_datestamp);
 use PublicInbox::ContentHash qw(content_digest);
 use PublicInbox::MDA;
 use PublicInbox::Eml;
-use PublicInbox::ProcessIO;
+use PublicInbox::IO;
 use POSIX qw(strftime);
 use autodie qw(read close socketpair);
 use Carp qw(croak);
@@ -77,7 +77,7 @@ sub gfi_start {
 				--quiet --done --date-format=raw) ];
 		my $pid = spawn($gfi, undef, { 0 => $s2, 1 => $s2 });
 		$self->{nchg} = 0;
-		$self->{io} = PublicInbox::ProcessIO->maybe_new($pid, $io);
+		$self->{io} = PublicInbox::IO::attach_pid($io, $pid);
 	};
 	if ($@) {
 		$self->lock_release;
diff --git a/lib/PublicInbox/InputPipe.pm b/lib/PublicInbox/InputPipe.pm
index f4d57e7d..232f20e8 100644
--- a/lib/PublicInbox/InputPipe.pm
+++ b/lib/PublicInbox/InputPipe.pm
@@ -39,7 +39,6 @@ sub consume {
 	if ($@) { # regular file (but not w/ select|IO::Poll backends)
 		$self->{-need_rq} = 1;
 		$self->requeue;
-	} elsif (do { no warnings 'unopened'; !stat($in) }) { # ProcessIONBF
 	} elsif (-p _ || -S _) { # O_NONBLOCK for sockets and pipes
 		$in->blocking(0);
 	} elsif (-t $in) { # isatty(3) can't use `_' stat cache
diff --git a/lib/PublicInbox/LeiToMail.pm b/lib/PublicInbox/LeiToMail.pm
index ead60b38..b07c2c90 100644
--- a/lib/PublicInbox/LeiToMail.pm
+++ b/lib/PublicInbox/LeiToMail.pm
@@ -7,7 +7,7 @@ use strict;
 use v5.10.1;
 use parent qw(PublicInbox::IPC);
 use PublicInbox::Eml;
-use PublicInbox::ProcessIO;
+use PublicInbox::IO;
 use PublicInbox::Spawn qw(spawn);
 use IO::Handle; # ->autoflush
 use Fcntl qw(SEEK_SET SEEK_END O_CREAT O_EXCL O_WRONLY);
@@ -160,8 +160,7 @@ sub _post_augment_mbox { # open a compressor process from top-level lei-daemon
 	my $cmd = PublicInbox::MboxReader::zsfx2cmd($zsfx, undef, $lei);
 	my ($r, $w) = @{delete $lei->{zpipe}};
 	my $rdr = { 0 => $r, 1 => $lei->{1}, 2 => $lei->{2}, pgid => 0 };
-	my $pid = spawn($cmd, undef, $rdr);
-	$lei->{1} = PublicInbox::ProcessIO->maybe_new($pid, $w,
+	$lei->{1} = PublicInbox::IO::attach_pid($w, spawn($cmd, undef, $rdr),
 				\&reap_compress, $lei, $cmd, $lei->{1});
 }
 
diff --git a/lib/PublicInbox/ProcessIO.pm b/lib/PublicInbox/ProcessIO.pm
deleted file mode 100644
index ea5d3e6c..00000000
--- a/lib/PublicInbox/ProcessIO.pm
+++ /dev/null
@@ -1,75 +0,0 @@
-# Copyright (C) all contributors <meta@public-inbox.org>
-# License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
-
-# a tied handle for auto reaping of children tied to a pipe or socket,
-# see perltie(1) for details.
-package PublicInbox::ProcessIO;
-use v5.12;
-use PublicInbox::DS qw(awaitpid);
-use Symbol qw(gensym);
-use bytes qw(length);
-
-sub maybe_new {
-	my ($cls, $pid, $fh, @cb_arg) = @_;
-	return ($fh, $pid) if wantarray;
-	my $s = gensym;
-	tie *$s, $cls, $pid, $fh, @cb_arg;
-	$s;
-}
-
-sub waitcb { # awaitpid callback
-	my ($pid, $err_ref, $cb, @args) = @_;
-	$$err_ref = $?; # sets >{pp_chld_err} for _close
-	$cb->($pid, @args) if $cb;
-}
-
-sub TIEHANDLE {
-	my ($cls, $pid, $fh, @cb_arg) = @_;
-	my $self = bless { pid => $pid, fh => $fh, ppid => $$ }, $cls;
-	# we share $err (and not $self) with awaitpid to avoid a ref cycle
-	$self->{pp_chld_err} = \(my $err);
-	awaitpid($pid, \&waitcb, \$err, @cb_arg);
-	$self;
-}
-
-# for IO::Uncompress::Gunzip and PublicInbox::LeiRediff
-sub BINMODE { @_ == 1 ? binmode($_[0]->{fh}) : binmode($_[0]->{fh}, $_[1]) }
-
-sub READ { read($_[0]->{fh}, $_[1], $_[2], $_[3] || 0) }
-
-sub READLINE { readline($_[0]->{fh}) }
-
-sub WRITE { syswrite($_[0]->{fh}, $_[1], $_[2] // length($_[1]), $_[3] // 0) }
-
-sub PRINT { print { $_[0]->{fh} } @_[1..$#_] }
-
-sub FILENO { fileno($_[0]->{fh}) }
-
-sub _close ($;$) {
-	my ($self, $wait) = @_;
-	my ($fh, $pid) = delete(@$self{qw(fh pid)});
-	my $ret = (defined($fh) && $wait) ? close($fh) : ($fh = '');
-	return $ret unless defined($pid) && $self->{ppid} == $$;
-	if ($wait) { # caller cares about the exit status:
-		# synchronous wait via defined(wantarray) on awaitpid:
-		defined(${$self->{pp_chld_err}}) or $wait = awaitpid($pid);
-		($? = ${$self->{pp_chld_err}}) and $ret = '';
-	} else {
-		awaitpid($pid); # depends on $in_loop or not
-	}
-	$ret;
-}
-
-# if caller uses close(), assume they want to check $? immediately so
-# we'll waitpid() synchronously.  n.b. wantarray doesn't seem to
-# propagate `undef' down to tied methods, otherwise I'd rely on that.
-sub CLOSE { _close($_[0], 1) }
-
-# if relying on DESTROY, assume the caller doesn't care about $? and
-# we can let the event loop call waitpid() whenever it gets SIGCHLD
-sub DESTROY {
-	_close($_[0]);
-	undef;
-}
-
-1;
diff --git a/lib/PublicInbox/ProcessIONBF.pm b/lib/PublicInbox/ProcessIONBF.pm
deleted file mode 100644
index 490e200a..00000000
--- a/lib/PublicInbox/ProcessIONBF.pm
+++ /dev/null
@@ -1,25 +0,0 @@
-# Copyright (C) all contributors <meta@public-inbox.org>
-# License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
-
-# used to support unbuffered partial reads
-package PublicInbox::ProcessIONBF;
-use v5.12;
-use parent qw(PublicInbox::ProcessIO);
-use IO::Handle; # ->blocking
-
-sub new {
-	my ($cls, $pid, $fh, @cb_arg) = @_;
-	$fh->blocking(0) // die "$fh->blocking(0): $!";
-	my $io = $cls->SUPER::maybe_new($pid, $fh, @cb_arg);
-}
-
-sub replace {
-	my ($cls, $orig) = @_;
-	my $pio = tied *$orig; # ProcessIO
-	$pio->{fh}->blocking(0) // die "$pio->{fh}->blocking(0): $!";
-	bless $pio, $cls;
-}
-
-sub READ { sysread($_[0]->{fh}, $_[1], $_[2], $_[3] // 0) }
-
-1;
diff --git a/lib/PublicInbox/Qspawn.pm b/lib/PublicInbox/Qspawn.pm
index a03e1b01..0bf857c6 100644
--- a/lib/PublicInbox/Qspawn.pm
+++ b/lib/PublicInbox/Qspawn.pm
@@ -149,7 +149,7 @@ sub finish ($;$) {
 
 	# we can safely finalize if pipe was closed before, or if
 	# {_err} is defined by waitpid_err.  Deleting {rpipe} will
-	# trigger PublicInbox::ProcessIO::DESTROY -> waitpid_err,
+	# trigger PublicInbox::IO::DESTROY -> waitpid_err,
 	# but it may not fire right away if inside the event loop.
 	my $closed_before = !delete($self->{rpipe});
 	finalize($self) if $closed_before || defined($self->{_err});
@@ -244,9 +244,8 @@ sub ipipe_cb { # InputPipe callback
 sub _yield_start { # may run later, much later...
 	my ($self) = @_;
 	if ($self->{psgi_env}->{'pi-httpd.async'}) {
-		require PublicInbox::ProcessIONBF;
 		my $rpipe = $self->{rpipe};
-		PublicInbox::ProcessIONBF->replace($rpipe);
+		$rpipe->blocking(0);
 		PublicInbox::InputPipe::consume($rpipe, \&ipipe_cb, $self);
 	} else {
 		require PublicInbox::GetlineResponse;
diff --git a/lib/PublicInbox/Spawn.pm b/lib/PublicInbox/Spawn.pm
index b4f37bea..d3b7ef6f 100644
--- a/lib/PublicInbox/Spawn.pm
+++ b/lib/PublicInbox/Spawn.pm
@@ -19,7 +19,7 @@ use PublicInbox::Lock;
 use Fcntl qw(SEEK_SET);
 use IO::Handle ();
 use Carp qw(croak);
-use PublicInbox::ProcessIO;
+use PublicInbox::IO;
 our @EXPORT_OK = qw(which spawn popen_rd popen_wr run_die run_wait run_qx);
 our @RLIMITS = qw(RLIMIT_CPU RLIMIT_CORE RLIMIT_DATA);
 use autodie qw(open pipe seek sysseek truncate);
@@ -377,7 +377,7 @@ sub popen_rd {
 	my ($cmd, $env, $opt, @cb_arg) = @_;
 	pipe(my $r, local $opt->{1});
 	my $pid = spawn($cmd, $env, $opt);
-	PublicInbox::ProcessIO->maybe_new($pid, $r, @cb_arg);
+	wantarray ? ($r, $pid) : PublicInbox::IO::attach_pid($r, $pid, @cb_arg)
 }
 
 sub popen_wr {
@@ -385,7 +385,7 @@ sub popen_wr {
 	pipe(local $opt->{0}, my $w);
 	$w->autoflush(1);
 	my $pid = spawn($cmd, $env, $opt);
-	PublicInbox::ProcessIO->maybe_new($pid, $w, @cb_arg)
+	wantarray ? ($w, $pid) : PublicInbox::IO::attach_pid($w, $pid, @cb_arg)
 }
 
 sub read_out_err ($) {
diff --git a/t/spawn.t b/t/spawn.t
index 938a2e5e..3479b6b3 100644
--- a/t/spawn.t
+++ b/t/spawn.t
@@ -107,34 +107,35 @@ EOF
 
 {
 	my $fh = popen_rd([qw(echo hello)]);
-	ok(fileno($fh) >= 0, 'tied fileno works');
+	ok(fileno($fh) >= 0, 'fileno works');
 	my $l = <$fh>;
-	is($l, "hello\n", 'tied readline works');
+	is($l, "hello\n", 'readline works');
 	$l = <$fh>;
-	ok(!$l, 'tied readline works for EOF');
+	ok(!$l, 'readline works for EOF');
 }
 
 {
 	my $fh = popen_rd([qw(printf foo\nbar)]);
-	ok(fileno($fh) >= 0, 'tied fileno works');
-	my $tfh = (tied *$fh)->{fh};
-	is($tfh->blocking(0), 1, '->blocking was true');
-	is($tfh->blocking, 0, '->blocking is false');
-	is($tfh->blocking(1), 0, '->blocking was true');
-	is($tfh->blocking, 1, '->blocking is true');
+	ok(fileno($fh) >= 0, 'fileno works');
+	is($fh->blocking(0), 1, '->blocking was true');
+	is($fh->blocking, 0, '->blocking is false');
+	is($fh->blocking(1), 0, '->blocking was true');
+	is($fh->blocking, 1, '->blocking is true');
 	my @line = <$fh>;
 	is_deeply(\@line, [ "foo\n", 'bar' ], 'wantarray works on readline');
 }
 
 {
 	my $fh = popen_rd([qw(echo hello)]);
+	like($fh->attached_pid, qr/\A[0-9]+\z/, 'have a PID');
 	my $buf;
 	is(sysread($fh, $buf, 6), 6, 'sysread got 6 bytes');
-	is($buf, "hello\n", 'tied gets works');
+	is($buf, "hello\n", 'sysread works');
 	is(sysread($fh, $buf, 6), 0, 'sysread got EOF');
 	$? = 1;
 	ok($fh->close, 'close succeeds');
 	is($?, 0, '$? set properly');
+	is($fh->attached_pid, undef, 'attached_pid cleared after close');
 }
 
 {
@@ -160,8 +161,8 @@ EOF
 	$fh = popen_rd(['true'], undef, undef, sub { @c = caller });
 	undef $fh; # ->DESTROY
 	ok(scalar(@c), 'callback fired by ->DESTROY');
-	ok(grep(!m[/PublicInbox/ProcessIO\.pm\z], @c),
-		'callback not invoked by ProcessIO');
+	ok(grep(!m[/PublicInbox/IO\.pm\z], @c),
+		'callback not invoked by PublicInbox::IO');
 }
 
 { # children don't wait on siblings
@@ -170,7 +171,6 @@ EOF
 	my @arg;
 	my $fh = popen_rd(['cat'], undef, { 0 => $r },
 			sub { @arg = @_; warn "x=$$\n" }, 'hi');
-	my $pp = tied *$fh;
 	my $pid = fork // BAIL_OUT $!;
 	local $SIG{__WARN__} = sub { _exit(1) };
 	if ($pid == 0) {
diff --git a/xt/check-run.t b/xt/check-run.t
index cda839fe..d12b925d 100755
--- a/xt/check-run.t
+++ b/xt/check-run.t
@@ -14,7 +14,7 @@ use v5.12;
 use IO::Handle; # ->autoflush
 use PublicInbox::TestCommon;
 use PublicInbox::Spawn;
-use PublicInbox::DS; # already loaded by Spawn via ProcessIO
+use PublicInbox::DS; # already loaded by Spawn via PublicInbox::IO
 use Getopt::Long qw(:config gnu_getopt no_ignore_case auto_abbrev);
 use Errno qw(EINTR);
 use Fcntl qw(:seek);

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 09/14] io: introduce write_file helper sub
  2023-11-02  9:35 [PATCH 00/14] IO/IPC-related cleanups Eric Wong
                   ` (7 preceding siblings ...)
  2023-11-02  9:35 ` [PATCH 08/14] replace ProcessIO with untied PublicInbox::IO Eric Wong
@ 2023-11-02  9:35 ` Eric Wong
  2023-11-02  9:35 ` [PATCH 10/14] spawn: support PerlIO layer in scalar redirects Eric Wong
                   ` (4 subsequent siblings)
  13 siblings, 0 replies; 17+ messages in thread
From: Eric Wong @ 2023-11-02  9:35 UTC (permalink / raw)
  To: meta

This is pretty convenient way to create files for diff
generation in both WWW and lei.  The test suite should also be
able to take advantage of it.
---
 MANIFEST                     |  1 +
 lib/PublicInbox/IO.pm        | 10 +++++++++-
 lib/PublicInbox/Import.pm    |  6 ++----
 lib/PublicInbox/LeiMirror.pm | 26 +++++++++---------------
 lib/PublicInbox/LeiRediff.pm |  9 +++------
 lib/PublicInbox/MailDiff.pm  | 18 ++++++++---------
 lib/PublicInbox/SolverGit.pm | 38 ++++++++++++------------------------
 t/io.t                       | 33 +++++++++++++++++++++++++++++++
 8 files changed, 78 insertions(+), 63 deletions(-)
 create mode 100644 t/io.t

diff --git a/MANIFEST b/MANIFEST
index 479c09de..51dcffaf 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -488,6 +488,7 @@ t/index-git-times.t
 t/indexlevels-mirror-v1.t
 t/indexlevels-mirror.t
 t/init.t
+t/io.t
 t/ipc.t
 t/iso-2202-jp.eml
 t/kqnotify.t
diff --git a/lib/PublicInbox/IO.pm b/lib/PublicInbox/IO.pm
index 63850a52..4c92566d 100644
--- a/lib/PublicInbox/IO.pm
+++ b/lib/PublicInbox/IO.pm
@@ -4,8 +4,9 @@
 # supports reaping of children tied to a pipe or socket
 package PublicInbox::IO;
 use v5.12;
-use parent qw(IO::Handle);
+use parent qw(IO::Handle Exporter);
 use PublicInbox::DS qw(awaitpid);
+our @EXPORT_OK = qw(write_file);
 
 # TODO: this can probably be the new home for read_all, try_cat
 # and maybe even buffered read/readline...
@@ -51,4 +52,11 @@ sub DESTROY {
 	$io->SUPER::DESTROY;
 }
 
+sub write_file ($$@) { # mode, filename, LIST (for print)
+	use autodie qw(open close);
+	open(my $fh, shift, shift);
+	print $fh @_;
+	defined(wantarray) && !wantarray ? $fh : close $fh;
+}
+
 1;
diff --git a/lib/PublicInbox/Import.pm b/lib/PublicInbox/Import.pm
index dfba34b9..5b0201c6 100644
--- a/lib/PublicInbox/Import.pm
+++ b/lib/PublicInbox/Import.pm
@@ -18,7 +18,7 @@ use PublicInbox::MDA;
 use PublicInbox::Eml;
 use PublicInbox::IO;
 use POSIX qw(strftime);
-use autodie qw(read close socketpair);
+use autodie qw(socketpair);
 use Carp qw(croak);
 use Socket qw(AF_UNIX SOCK_STREAM);
 use PublicInbox::Git qw(read_all);
@@ -462,9 +462,7 @@ EOM
 	while (my ($fn, $contents) = splice(@fn_contents, 0, 2)) {
 		my $f = $dir.'/'.$fn;
 		next if -f $f;
-		open my $fh, '>', $f;
-		print $fh $contents;
-		close $fh;
+		PublicInbox::IO::write_file '>', $f, $contents;
 	}
 }
 
diff --git a/lib/PublicInbox/LeiMirror.pm b/lib/PublicInbox/LeiMirror.pm
index 71f41a11..8542c587 100644
--- a/lib/PublicInbox/LeiMirror.pm
+++ b/lib/PublicInbox/LeiMirror.pm
@@ -8,6 +8,7 @@ use parent qw(PublicInbox::IPC);
 use IO::Uncompress::Gunzip qw(gunzip $GunzipError);
 use IO::Compress::Gzip qw(gzip $GzipError);
 use PublicInbox::Spawn qw(spawn run_wait run_die run_qx);
+use PublicInbox::IO qw(write_file);
 use File::Path ();
 use File::Temp ();
 use File::Spec ();
@@ -481,21 +482,18 @@ sub forkgroup_prep {
 	my $dir = "$os/$fg.git";
 	if (!-d $dir && !$self->{dry_run}) {
 		PublicInbox::Import::init_bare($dir);
-		open my $fh, '+>>', "$dir/config";
-		print $fh <<EOM;
+		write_file '+>>', "$dir/config", <<EOM;
 [repack]
 	useDeltaIslands = true
 [pack]
 	island = refs/remotes/([^/]+)/
 EOM
-		close $fh;
 	}
 	my $key = $self->{-key} // die 'BUG: no -key';
 	my $rn = substr(sha256_hex($key), 0, 16);
 	if (!-d $self->{cur_dst} && !$self->{dry_run}) {
 		PublicInbox::Import::init_bare($self->{cur_dst});
-		open my $fh, '+>>', "$self->{cur_dst}/config";
-		print $fh <<EOM;
+		write_file '+>>', "$self->{cur_dst}/config", <<EOM;
 ; rely on the "$rn" remote in the
 ; $fg fork group for fetches
 ; only uncomment the following iff you detach from fork groups
@@ -504,7 +502,6 @@ EOM
 ;	fetch = +refs/*:refs/*
 ;	mirror = true
 EOM
-		close $fh;
 	}
 	if (!$self->{dry_run}) {
 		my $alt = File::Spec->rel2abs("$dir/objects");
@@ -691,9 +688,11 @@ EOM
 sub init_placeholder ($$$) {
 	my ($src, $edst, $ent) = @_;
 	PublicInbox::Import::init_bare($edst);
-	my $f = "$edst/config";
-	open my $fh, '>>', $f;
-	print $fh <<EOM;
+	my @owner = defined($ent->{owner}) ? (<<EOM) : ();
+[gitweb]
+	owner = $ent->{owner}
+EOM
+	write_file '>>', "$edst/config", <<EOM, @owner;
 [remote "origin"]
 	url = $src
 	fetch = +refs/*:refs/*
@@ -703,18 +702,11 @@ sub init_placeholder ($$$) {
 ; will not fetch updates for it unless write permission is added.
 ; Hint: chmod +w $edst
 EOM
-	print $fh <<EOM if defined($ent->{owner});
-[gitweb]
-	owner = $ent->{owner}
-EOM
-	close $fh;
 	my %map = (head => 'HEAD', description => undef);
 	while (my ($key, $fn) = each %map) {
 		my $val = $ent->{$key} // next;
 		$fn //= $key;
-		open $fh, '>', "$edst/$fn";
-		say $fh $val;
-		close $fh;
+		write_file '>', "$edst/$fn", $val;
 	}
 }
 
diff --git a/lib/PublicInbox/LeiRediff.pm b/lib/PublicInbox/LeiRediff.pm
index fdff4b4b..35728330 100644
--- a/lib/PublicInbox/LeiRediff.pm
+++ b/lib/PublicInbox/LeiRediff.pm
@@ -114,12 +114,9 @@ EOM
 	if (!$rw->{-tmp}) {
 		my $d = "$self->{rdtmp}/for_tree.git";
 		-d $d or PublicInbox::Import::init_bare($d);
-		my $f = "$d/objects/info/alternates"; # always overwrite
-		open my $fh, '>', $f or die "open $f: $!";
-		for my $git (@{$self->{gits}}) {
-			print $fh $git->git_path('objects'),"\n";
-		}
-		close $fh or die "close $f: $!";
+		# always overwrite
+		PublicInbox::IO::write_file '>', "$d/objects/info/alternates",
+			map { $_->git_path('objects')."\n" } @{$self->{gits}};
 		$rw = PublicInbox::Git->new($d);
 	}
 	my $w = popen_wr(['git', "--git-dir=$rw->{git_dir}",
diff --git a/lib/PublicInbox/MailDiff.pm b/lib/PublicInbox/MailDiff.pm
index 908f223c..c7b991f1 100644
--- a/lib/PublicInbox/MailDiff.pm
+++ b/lib/PublicInbox/MailDiff.pm
@@ -9,14 +9,14 @@ use PublicInbox::ViewDiff qw(flush_diff);
 use PublicInbox::GitAsyncCat;
 use PublicInbox::ContentDigestDbg;
 use PublicInbox::Qspawn;
+use PublicInbox::IO qw(write_file);
+use autodie qw(close mkdir);
 
 sub write_part { # Eml->each_part callback
 	my ($ary, $self) = @_;
 	my ($part, $depth, $idx) = @$ary;
 	if ($idx ne '1' || $self->{-raw_hdr}) { # lei mail-diff --raw-header
-		open my $fh, '>', "$self->{curdir}/$idx.hdr" or die "open: $!";
-		print $fh ${$part->{hdr}} or die "print $!";
-		close $fh or die "close $!";
+		write_file '>', "$self->{curdir}/$idx.hdr", ${$part->{hdr}};
 	}
 	my $ct = $part->content_type || 'text/plain';
 	my ($s, $err) = msg_part_text($part, $ct);
@@ -24,22 +24,20 @@ sub write_part { # Eml->each_part callback
 	$s //= $part->body;
 	$s =~ s/\r\n/\n/gs; # TODO: consider \r+\n to match View
 	$s =~ s/\s*\z//s;
-	open my $fh, '>:utf8', "$self->{curdir}/$idx.$sfx" or die "open: $!";
-	print $fh $s or die "print $!";
-	close $fh or die "close $!";
+	write_file '>:utf8', "$self->{curdir}/$idx.$sfx", $s;
 }
 
 # public
 sub dump_eml ($$$) {
 	my ($self, $dir, $eml) = @_;
 	local $self->{curdir} = $dir;
-	mkdir $dir or die "mkdir($dir): $!";
+	mkdir $dir;
 	$eml->each_part(\&write_part, $self);
-	open my $fh, '>', "$dir/content_digest" or die "open: $!";
+	my $fh = write_file '>', "$dir/content_digest";
 	my $dig = PublicInbox::ContentDigestDbg->new($fh);
 	content_digest($eml, $dig);
-	print $fh "\n", $dig->hexdigest, "\n" or die "print $!";
-	close $fh or die "close: $!";
+	say $fh "\n", $dig->hexdigest;
+	close $fh;
 }
 
 # public
diff --git a/lib/PublicInbox/SolverGit.pm b/lib/PublicInbox/SolverGit.pm
index 23d4d3d1..ba3c94cb 100644
--- a/lib/PublicInbox/SolverGit.pm
+++ b/lib/PublicInbox/SolverGit.pm
@@ -11,8 +11,10 @@ package PublicInbox::SolverGit;
 use strict;
 use v5.10.1;
 use File::Temp 0.19 (); # 0.19 for ->newdir
+use autodie qw(mkdir);
 use Fcntl qw(SEEK_SET);
 use PublicInbox::Git qw(git_unquote git_quote);
+use PublicInbox::IO qw(write_file);
 use PublicInbox::MsgIter qw(msg_part_text);
 use PublicInbox::Qspawn;
 use PublicInbox::Tmpfile;
@@ -199,9 +201,7 @@ sub extract_diff ($$) {
 	my $path = ++$self->{tot};
 	$di->{n} = $path;
 	my $f = _tmp($self)->dirname."/$path";
-	open(my $tmp, '>:utf8', $f) or die "open($f): $!";
-	print $tmp $di->{hdr_lines}, $patch or die "print(tmp): $!";
-	close $tmp or die "close(tmp): $!";
+	write_file '>:utf8', $f, $di->{hdr_lines}, $patch;
 
 	# for debugging/diagnostics:
 	$di->{ibx} = $want->{cur_ibx};
@@ -291,36 +291,24 @@ sub do_git_init ($) {
 	my ($self) = @_;
 	my $git_dir = _tmp($self)->dirname.'/git';
 
-	foreach ('', qw(objects refs objects/info refs/heads)) {
-		mkdir("$git_dir/$_") or die "mkdir $_: $!";
-	}
-	open my $fh, '>', "$git_dir/config" or die "open git/config: $!";
+	mkdir("$git_dir/$_") for ('', qw(objects refs objects/info refs/heads));
 	my $first = $self->{gits}->[0];
 	my $fmt = $first->object_format;
-	my $v = defined($$fmt) ? 1 : 0;
-	print $fh <<EOF or die "print git/config $!";
+	my ($v, @ext) = defined($$fmt) ? (1, <<EOM) : (0);
+[extensions]
+	objectformat = $$fmt
+EOM
+	write_file '>', "$git_dir/config", <<EOF, @ext;
 [core]
 	repositoryFormatVersion = $v
 	filemode = true
 	bare = false
 	logAllRefUpdates = false
 EOF
-	print $fh <<EOM if defined($$fmt);
-[extensions]
-	objectformat = $$fmt
-EOM
-	close $fh or die "close git/config: $!";
-
-	open $fh, '>', "$git_dir/HEAD" or die "open git/HEAD: $!";
-	print $fh "ref: refs/heads/master\n" or die "print git/HEAD: $!";
-	close $fh or die "close git/HEAD: $!";
-
-	my $f = 'objects/info/alternates';
-	open $fh, '>', "$git_dir/$f" or die "open: $f: $!";
-	foreach my $git (@{$self->{gits}}) {
-		print $fh $git->git_path('objects'),"\n" or die "print $f: $!";
-	}
-	close $fh or die "close: $f: $!";
+	write_file '>', "$git_dir/HEAD", "ref: refs/heads/master\n";
+	write_file '>', "$git_dir/objects/info/alternates", map {
+			$_->git_path('objects')."\n"
+		} @{$self->{gits}};
 	my $tmp_git = $self->{tmp_git} = PublicInbox::Git->new($git_dir);
 	$tmp_git->{-tmp} = $self->{tmp};
 	$self->{git_env} = {
diff --git a/t/io.t b/t/io.t
new file mode 100644
index 00000000..4c7a97a3
--- /dev/null
+++ b/t/io.t
@@ -0,0 +1,33 @@
+#!perl -w
+# Copyright (C) all contributors <meta@public-inbox.org>
+# License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
+use v5.12;
+use PublicInbox::TestCommon;
+my $tmpdir = tmpdir;
+use_ok 'PublicInbox::IO';
+use PublicInbox::Spawn qw(which run_qx);
+
+# only test failures
+SKIP: {
+skip 'linux only test' if $^O ne 'linux';
+my $strace = which('strace') or skip 'strace missing for test';
+my $v = run_qx([$strace, '--version']);
+$v =~ m!version\s+([1-9]+\.[0-9]+)! or xbail "no strace --version: $v";
+$v = eval("v$1");
+$v ge v4.16 or skip "$strace too old for syscall injection (".
+		sprintf('v%vd', $v). ' < v4.16)';
+my $env = { PERL5LIB => join(':', @INC) };
+my $opt = { 1 => \my $out, 2 => \my $err };
+my $dst = "$tmpdir/dst";
+my $tr = "$tmpdir/tr";
+my $cmd = [ $strace, "-o$tr", "-P$dst",
+		'-e', 'inject=writev,write:error=EIO',
+		$^X, qw(-w -MPublicInbox::IO=write_file -e),
+		q[write_file '>', $ARGV[0], 'hello world'], $dst ];
+xsys($cmd, $env, $opt);
+isnt($?, 0, 'write failed');
+like($err, qr/\bclose\b/, 'close error noted');
+is(-s $dst, 0, 'file created and empty after EIO');
+} # /SKIP
+
+done_testing;

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 10/14] spawn: support PerlIO layer in scalar redirects
  2023-11-02  9:35 [PATCH 00/14] IO/IPC-related cleanups Eric Wong
                   ` (8 preceding siblings ...)
  2023-11-02  9:35 ` [PATCH 09/14] io: introduce write_file helper sub Eric Wong
@ 2023-11-02  9:35 ` Eric Wong
  2023-11-02  9:35 ` [PATCH 11/14] treewide: check alternates writes with eof + autodie Eric Wong
                   ` (3 subsequent siblings)
  13 siblings, 0 replies; 17+ messages in thread
From: Eric Wong @ 2023-11-02  9:35 UTC (permalink / raw)
  To: meta

We have to deal with UTF-8 data for generating patches, so make
it easier to pass Perl utf8 data to git, diff, sdiff, etc. to
avoid "Wide character" warnings.
---
 lib/PublicInbox/MailDiff.pm  |  3 +--
 lib/PublicInbox/SearchIdx.pm |  2 +-
 lib/PublicInbox/Spawn.pm     | 30 ++++++++++++++++++++----------
 3 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/lib/PublicInbox/MailDiff.pm b/lib/PublicInbox/MailDiff.pm
index c7b991f1..b1c12d6d 100644
--- a/lib/PublicInbox/MailDiff.pm
+++ b/lib/PublicInbox/MailDiff.pm
@@ -63,7 +63,6 @@ sub next_smsg ($) {
 sub emit_msg_diff {
 	my ($bref, $self) = @_; # bref is `git diff' output
 	# will be escaped to `&#8226;' in HTML
-	utf8::decode($$bref);
 	$self->{ctx}->{ibx}->{obfuscate} and
 		obfuscate_addrs($self->{ctx}->{ibx}, $$bref, "\x{2022}");
 	print { $self->{ctx}->{zfh} } '</pre><hr><pre>' if $self->{nr} > 1;
@@ -77,7 +76,7 @@ sub do_diff {
 	my $dir = "$self->{tmp}/$n";
 	$self->dump_eml($dir, $eml);
 	my $cmd = [ qw(git diff --no-index --no-color -- a), $n ];
-	my $opt = { -C => "$self->{tmp}", quiet => 1 };
+	my $opt = { -C => "$self->{tmp}", quiet => 1, 1 => [':utf8', \my $o] };
 	my $qsp = PublicInbox::Qspawn->new($cmd, undef, $opt);
 	$qsp->psgi_qx($self->{ctx}->{env}, undef, \&emit_msg_diff, $self);
 }
diff --git a/lib/PublicInbox/SearchIdx.pm b/lib/PublicInbox/SearchIdx.pm
index 78519b22..9566b14d 100644
--- a/lib/PublicInbox/SearchIdx.pm
+++ b/lib/PublicInbox/SearchIdx.pm
@@ -353,7 +353,7 @@ sub index_diff ($$$) {
 sub patch_id {
 	my ($self, $sref) = @_;
 	my $git = ($self->{ibx} // $self->{eidx} // $self)->git;
-	my $opt = { 0 => $sref, 2 => \(my $err) };
+	my $opt = { 0 => [ ':utf8', $sref ], 2 => \(my $err) };
 	my $id = run_qx($git->cmd(qw(patch-id --stable)), undef, $opt);
 	warn $err if $err;
 	$id =~ /\A([a-f0-9]{40,})/ ? $1 : undef;
diff --git a/lib/PublicInbox/Spawn.pm b/lib/PublicInbox/Spawn.pm
index d3b7ef6f..b0edeb33 100644
--- a/lib/PublicInbox/Spawn.pm
+++ b/lib/PublicInbox/Spawn.pm
@@ -332,6 +332,18 @@ sub which ($) {
 	undef;
 }
 
+sub scalar_redirect {
+	my ($layer, $opt, $child_fd, $bref) = @_;
+	open my $fh, '+>'.$layer, undef;
+	$opt->{"fh.$child_fd"} = $fh;
+	if ($child_fd == 0) {
+		print $fh $$bref;
+		$fh->flush or die "flush: $!";
+		sysseek($fh, 0, SEEK_SET);
+	}
+	fileno($fh);
+}
+
 sub spawn ($;$$) {
 	my ($cmd, $env, $opt) = @_;
 	my $f = which($cmd->[0]) // die "$cmd->[0]: command not found\n";
@@ -342,15 +354,11 @@ sub spawn ($;$$) {
 	}
 	for my $child_fd (0..2) {
 		my $pfd = $opt->{$child_fd};
-		if ('SCALAR' eq ref($pfd)) {
-			open my $fh, '+>:utf8', undef;
-			$opt->{"fh.$child_fd"} = $fh;
-			if ($child_fd == 0) {
-				print $fh $$pfd;
-				$fh->flush or die "flush: $!";
-				sysseek($fh, 0, SEEK_SET);
-			}
-			$pfd = fileno($fh);
+		if ('ARRAY' eq ref($pfd)) {
+			my ($layer, $bref) = @$pfd;
+			$pfd = scalar_redirect($layer, $opt, $child_fd, $bref)
+		} elsif ('SCALAR' eq ref($pfd)) {
+			$pfd = scalar_redirect('', $opt, $child_fd, $pfd);
 		} elsif (defined($pfd) && $pfd !~ /\A[0-9]+\z/) {
 			my $fd = fileno($pfd) //
 					die "$pfd not an IO GLOB? $!";
@@ -394,7 +402,9 @@ sub read_out_err ($) {
 	for my $fd (1, 2) { # read stdout/stderr
 		my $fh = delete($opt->{"fh.$fd"}) // next;
 		seek($fh, 0, SEEK_SET);
-		${$opt->{$fd}} .= <$fh>;
+		my $dst = $opt->{$fd};
+		$dst = $opt->{$fd} = $dst->[1] if ref($dst) eq 'ARRAY';
+		$$dst .= <$fh>;
 		$fh->error and croak "E: read(FD=$fd): $!";
 	}
 }

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 11/14] treewide: check alternates writes with eof + autodie
  2023-11-02  9:35 [PATCH 00/14] IO/IPC-related cleanups Eric Wong
                   ` (9 preceding siblings ...)
  2023-11-02  9:35 ` [PATCH 10/14] spawn: support PerlIO layer in scalar redirects Eric Wong
@ 2023-11-02  9:35 ` Eric Wong
  2023-11-02  9:35 ` [PATCH 12/14] treewide: use eof and close to detect readline errors Eric Wong
                   ` (2 subsequent siblings)
  13 siblings, 0 replies; 17+ messages in thread
From: Eric Wong @ 2023-11-02  9:35 UTC (permalink / raw)
  To: meta

We must use eof() combined with close() to detect errors
in situations involving the readline()' op since `readline'
(and most buffered I/O libraries) have weak error detection
support.

This fixes error detection for files opened for read/write
access.  The next commit will fix error detection for read-only
files.
---
 lib/PublicInbox/LeiMirror.pm | 12 +++++-------
 lib/PublicInbox/LeiToMail.pm | 29 +++++++++++++++--------------
 2 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/lib/PublicInbox/LeiMirror.pm b/lib/PublicInbox/LeiMirror.pm
index 8542c587..fb6517bd 100644
--- a/lib/PublicInbox/LeiMirror.pm
+++ b/lib/PublicInbox/LeiMirror.pm
@@ -506,14 +506,12 @@ EOM
 	if (!$self->{dry_run}) {
 		my $alt = File::Spec->rel2abs("$dir/objects");
 		my $o = "$self->{cur_dst}/objects";
-		my $f = "$o/info/alternates";
 		my $l = File::Spec->abs2rel($alt, File::Spec->rel2abs($o));
-		open my $fh, '+>>', $f;
-		seek($fh, 0, SEEK_SET);
-		chomp(my @cur = <$fh>);
-		if (!grep(/\A\Q$l\E\z/, @cur)) {
-			say $fh $l;
-		}
+		open my $fh, '+>>', my $f = "$o/info/alternates";
+		seek($fh, 0, SEEK_SET); # Perl did SEEK_END when it saw '>>'
+		my $seen = grep(/\A\Q$l\E\n/, <$fh>); # error check with eof
+		eof($fh) or die "not at `$f' EOF ($!)"; # $! was set by readline
+		print $fh "$l\n" if !$seen;
 		close $fh;
 	}
 	bless {
diff --git a/lib/PublicInbox/LeiToMail.pm b/lib/PublicInbox/LeiToMail.pm
index b07c2c90..e80163e2 100644
--- a/lib/PublicInbox/LeiToMail.pm
+++ b/lib/PublicInbox/LeiToMail.pm
@@ -12,6 +12,7 @@ use PublicInbox::Spawn qw(spawn);
 use IO::Handle; # ->autoflush
 use Fcntl qw(SEEK_SET SEEK_END O_CREAT O_EXCL O_WRONLY);
 use PublicInbox::Syscall qw(rename_noreplace);
+use autodie qw(open seek close);
 
 my %kw2char = ( # Maildir characters
 	draft => 'D',
@@ -255,7 +256,7 @@ sub _buf2maildir ($$$$) {
 		$tmp = $dst.'tmp/'.$rand.$common;
 	} while (!($ok = sysopen($fh, $tmp, O_CREAT|O_EXCL|O_WRONLY)) &&
 		$!{EEXIST} && ($rand = _rand.','));
-	if ($ok && print $fh $$buf and close($fh)) {
+	if ($ok && print $fh $$buf and $fh->close) {
 		$dst .= $dir; # 'new/' or 'cur/'
 		$rand = '';
 		do {
@@ -458,7 +459,7 @@ sub _pre_augment_maildir {
 	require File::Path;
 	File::Path::make_path(map { $dst.$_ } qw(tmp new cur));
 	# for utime, so no opendir
-	open $self->{poke_dh}, '<', "${dst}cur" or die "open ${dst}cur: $!";
+	open $self->{poke_dh}, '<', "${dst}cur";
 }
 
 sub clobber_dst_prepare ($;$) {
@@ -538,11 +539,11 @@ sub _pre_augment_text {
 		$out = $lei->{$devfd};
 	} else { # normal-looking path
 		if (-p $dst) {
-			open $out, '>', $dst or die "open($dst): $!";
+			open $out, '>', $dst;
 		} elsif (-f _ || !-e _) {
 			# text allows augment, HTML/Atom won't
 			my $mode = $lei->{opt}->{augment} ? '>>' : '>';
-			open $out, $mode, $dst or die "open($mode, $dst): $!";
+			open $out, $mode, $dst;
 		} else {
 			die "$dst is not a file or FIFO\n";
 		}
@@ -561,7 +562,7 @@ sub _pre_augment_mbox {
 		$out = $lei->{$devfd};
 	} else { # normal-looking path
 		if (-p $dst) {
-			open $out, '>', $dst or die "open($dst): $!";
+			open $out, '>', $dst;
 		} elsif (-f _ || !-e _) {
 			require PublicInbox::MboxLock;
 			my $m = $lei->{opt}->{'lock'} //
@@ -574,7 +575,7 @@ sub _pre_augment_mbox {
 		$lei->{old_1} = $lei->{1}; # keep for spawning MUA
 	}
 	# Perl does SEEK_END even with O_APPEND :<
-	$self->{seekable} = seek($out, 0, SEEK_SET);
+	$self->{seekable} = $out->seek(0, SEEK_SET);
 	if (!$self->{seekable} && !$!{ESPIPE} && !defined($devfd)) {
 		die "seek($dst): $!\n";
 	}
@@ -616,7 +617,7 @@ sub _do_augment_mbox {
 	if (my $zsfx = $self->{zsfx}) {
 		$rd = PublicInbox::MboxReader::zsfxcat($out, $zsfx, $lei);
 	} else {
-		open($rd, '+>>&', $out) or die "dup: $!";
+		open($rd, '+>>&', $out);
 	}
 	my $dedupe;
 	if ($opt->{augment}) {
@@ -636,7 +637,7 @@ sub _do_augment_mbox {
 		PublicInbox::MboxReader->$fmt($rd, \&_augment, $lei);
 	}
 	# maybe some systems don't honor O_APPEND, Perl does this:
-	seek($out, 0, SEEK_END) or die "seek $dst: $!";
+	seek($out, 0, SEEK_END);
 	$dedupe->pause_dedupe if $dedupe;
 }
 
@@ -672,12 +673,12 @@ sub _pre_augment_v2 {
 	$lei->{v2w} = $v2w;
 	return if !$lei->{opt}->{shared};
 	my $d = "$lei->{ale}->{git}->{git_dir}/objects";
-	my $al = "$dir/git/0.git/objects/info/alternates";
-	open my $fh, '+>>', $al or die "open($al): $!";
-	seek($fh, 0, SEEK_SET) or die "seek($al): $!";
-	grep(/\A\Q$d\E\n/, <$fh>) and return;
-	print $fh "$d\n" or die "print($al): $!";
-	close $fh or die "close($al): $!";
+	open my $fh, '+>>', my $f = "$dir/git/0.git/objects/info/alternates";
+	seek($fh, 0, SEEK_SET); # Perl did SEEK_END when it saw '>>'
+	my $seen = grep(/\A\Q$d\E\n/, <$fh>);
+	eof($fh) or die "not at `$f' EOF ($!)"; # $! was set by readline
+	print $fh "$d\n" if !$seen;
+	close $fh;
 }
 
 sub pre_augment { # fast (1 disk seek), runs in same process as post_augment

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 12/14] treewide: use eof and close to detect readline errors
  2023-11-02  9:35 [PATCH 00/14] IO/IPC-related cleanups Eric Wong
                   ` (10 preceding siblings ...)
  2023-11-02  9:35 ` [PATCH 11/14] treewide: check alternates writes with eof + autodie Eric Wong
@ 2023-11-02  9:35 ` Eric Wong
  2023-11-02  9:35 ` [PATCH 13/14] move read_all, try_cat, and poll_in to PublicInbox::IO Eric Wong
  2023-11-02  9:35 ` [PATCH 14/14] t/cindex+extsearch: use write_file, autodie, etc Eric Wong
  13 siblings, 0 replies; 17+ messages in thread
From: Eric Wong @ 2023-11-02  9:35 UTC (permalink / raw)
  To: meta

readline (<FH>) isn't wrapped by autodie, and there's no
way to know if read(2) errors truncated the readline output.
IO::Handle->error isn't reliable on Perl < v5.34.

Thus, combining the `eof' and `close' (combined with autodie) is
the only way we can detect read(2) errors (injected via strace)
when called via `readline' (aka <$fh>).  Neither using `eof'
nor `close' alone is sufficient, they must be combined to detect
errors from buffered `readline'.
---
 lib/PublicInbox/CodeSearchIdx.pm | 4 ++--
 lib/PublicInbox/Gcf2.pm          | 2 ++
 lib/PublicInbox/LeiInput.pm      | 3 ++-
 lib/PublicInbox/LeiMirror.pm     | 4 ++--
 lib/PublicInbox/Spawn.pm         | 4 ++--
 5 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/lib/PublicInbox/CodeSearchIdx.pm b/lib/PublicInbox/CodeSearchIdx.pm
index ad915fa2..0b00c303 100644
--- a/lib/PublicInbox/CodeSearchIdx.pm
+++ b/lib/PublicInbox/CodeSearchIdx.pm
@@ -57,7 +57,7 @@ use PublicInbox::Git qw(%OFMT2HEXLEN);
 use PublicInbox::Compat qw(uniqstr);
 use PublicInbox::Aspawn qw(run_await);
 use Carp ();
-use autodie qw(pipe open sysread seek sysseek send);
+use autodie qw(close pipe open sysread seek sysseek send);
 our $DO_QUIT = 15; # signal number
 our (
 	$LIVE_JOBS, # integer
@@ -628,7 +628,7 @@ sub index_repo { # run_git cb
 	my $roots_fh = delete $repo->{roots_fh} // die 'BUG: no {roots_fh}';
 	seek($roots_fh, 0, SEEK_SET);
 	chomp(my @roots = <$roots_fh>);
-	close($roots_fh);
+	$roots_fh = eof($roots_fh) | close $roots_fh; # detect readline errors
 	if (!@roots) {
 		warn("E: $git->{git_dir} has no root commits\n");
 		return index_next($self);
diff --git a/lib/PublicInbox/Gcf2.pm b/lib/PublicInbox/Gcf2.pm
index 502bf33a..6ee0d7d9 100644
--- a/lib/PublicInbox/Gcf2.pm
+++ b/lib/PublicInbox/Gcf2.pm
@@ -11,6 +11,7 @@ use Time::HiRes qw(clock_gettime CLOCK_MONOTONIC);
 use IO::Handle; # autoflush
 use PublicInbox::Git;
 use PublicInbox::Lock;
+use autodie qw(close);
 
 BEGIN {
 	use autodie;
@@ -81,6 +82,7 @@ sub add_alt ($$) {
 	# See https://bugs.debian.org/975607
 	if (open(my $fh, '<', "$objdir/info/alternates")) {
 		chomp(my @abs_alt = grep(m!^/!, <$fh>));
+		$fh = eof($fh) | close $fh; # detect readline errors
 		$gcf2->add_alternate($_) for @abs_alt;
 	}
 	$gcf2->add_alternate($objdir);
diff --git a/lib/PublicInbox/LeiInput.pm b/lib/PublicInbox/LeiInput.pm
index f7c3f573..4cd18c09 100644
--- a/lib/PublicInbox/LeiInput.pm
+++ b/lib/PublicInbox/LeiInput.pm
@@ -80,7 +80,8 @@ sub input_net_cb { # imap_each, nntp_each cb
 sub input_fh {
 	my ($self, $ifmt, $fh, $name, @args) = @_;
 	if ($ifmt eq 'eml') {
-		my $buf = do { local $/; <$fh> } //
+		my $buf = do { local $/; <$fh> };
+		(defined($buf) && eof($fh) && close($fh)) or
 			return $self->{lei}->child_error(0, <<"");
 error reading $name: $!
 
diff --git a/lib/PublicInbox/LeiMirror.pm b/lib/PublicInbox/LeiMirror.pm
index fb6517bd..e4914f75 100644
--- a/lib/PublicInbox/LeiMirror.pm
+++ b/lib/PublicInbox/LeiMirror.pm
@@ -309,9 +309,9 @@ sub fgrp_update {
 	seek($srcfh, 0, SEEK_SET);
 	seek($dstfh, 0, SEEK_SET);
 	my %src = map { chomp; split(/\0/) } (<$srcfh>);
-	close $srcfh;
 	my %dst = map { chomp; split(/\0/) } (<$dstfh>);
-	close $dstfh;
+	$srcfh = eof($srcfh) | close $srcfh; # detects readline errors
+	$dstfh = eof($dstfh) | close $dstfh; # ditto
 	my $w = start_update_ref($fgrp) or return;
 	my $lei = $fgrp->{lei};
 	my $ndel;
diff --git a/lib/PublicInbox/Spawn.pm b/lib/PublicInbox/Spawn.pm
index b0edeb33..8c798b39 100644
--- a/lib/PublicInbox/Spawn.pm
+++ b/lib/PublicInbox/Spawn.pm
@@ -22,7 +22,7 @@ use Carp qw(croak);
 use PublicInbox::IO;
 our @EXPORT_OK = qw(which spawn popen_rd popen_wr run_die run_wait run_qx);
 our @RLIMITS = qw(RLIMIT_CPU RLIMIT_CORE RLIMIT_DATA);
-use autodie qw(open pipe seek sysseek truncate);
+use autodie qw(close open pipe seek sysseek truncate);
 
 BEGIN {
 	my $all_libc = <<'ALL_LIBC'; # all *nix systems we support
@@ -405,7 +405,7 @@ sub read_out_err ($) {
 		my $dst = $opt->{$fd};
 		$dst = $opt->{$fd} = $dst->[1] if ref($dst) eq 'ARRAY';
 		$$dst .= <$fh>;
-		$fh->error and croak "E: read(FD=$fd): $!";
+		$fh = eof($fh) | close $fh; # detects readline errors
 	}
 }
 

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 13/14] move read_all, try_cat, and poll_in to PublicInbox::IO
  2023-11-02  9:35 [PATCH 00/14] IO/IPC-related cleanups Eric Wong
                   ` (11 preceding siblings ...)
  2023-11-02  9:35 ` [PATCH 12/14] treewide: use eof and close to detect readline errors Eric Wong
@ 2023-11-02  9:35 ` Eric Wong
  2023-11-02 20:59   ` www: squash read_all usage fix Eric Wong
  2023-11-02  9:35 ` [PATCH 14/14] t/cindex+extsearch: use write_file, autodie, etc Eric Wong
  13 siblings, 1 reply; 17+ messages in thread
From: Eric Wong @ 2023-11-02  9:35 UTC (permalink / raw)
  To: meta

The IO package seems like a better home for I/O subs than the
Git package.  We lose the 60 second read timeout for `git
cat-file --batch-*' processes since it's probably not necessary
given how reliable the code has proven and things would fall
over hard in other ways if the storage device were completely
hosed.
---
 lib/PublicInbox/Gcf2.pm          |  2 +-
 lib/PublicInbox/Git.pm           | 24 ++----------------------
 lib/PublicInbox/IO.pm            | 26 ++++++++++++++++++++++----
 lib/PublicInbox/IdxStack.pm      |  2 +-
 lib/PublicInbox/Import.pm        |  2 +-
 lib/PublicInbox/Inbox.pm         |  2 +-
 lib/PublicInbox/InboxWritable.pm |  2 +-
 lib/PublicInbox/LeiALE.pm        |  6 +++---
 lib/PublicInbox/LeiBlob.pm       |  3 ++-
 lib/PublicInbox/LeiConfig.pm     |  2 +-
 lib/PublicInbox/LeiMailSync.pm   |  3 ++-
 lib/PublicInbox/LeiMirror.pm     | 11 +++++------
 lib/PublicInbox/LeiSucks.pm      |  2 +-
 lib/PublicInbox/MultiGit.pm      |  2 +-
 lib/PublicInbox/TestCommon.pm    |  4 ++--
 lib/PublicInbox/ViewVCS.pm       |  6 +++---
 lib/PublicInbox/WWW.pm           |  2 +-
 lib/PublicInbox/XapHelper.pm     |  7 +++----
 lib/PublicInbox/XapHelperCxx.pm  |  3 +--
 script/public-inbox-convert      |  2 +-
 script/public-inbox-edit         |  3 +--
 script/public-inbox-init         |  4 ++--
 t/cindex.t                       |  2 +-
 t/clone-coderepo.t               | 10 +++++-----
 t/init.t                         |  2 +-
 t/lei-mirror.t                   | 10 +++++-----
 26 files changed, 70 insertions(+), 74 deletions(-)

diff --git a/lib/PublicInbox/Gcf2.pm b/lib/PublicInbox/Gcf2.pm
index 6ee0d7d9..5490049d 100644
--- a/lib/PublicInbox/Gcf2.pm
+++ b/lib/PublicInbox/Gcf2.pm
@@ -37,7 +37,7 @@ BEGIN {
 		$vals->{$k} = $val;
 	}
 	my $f = "$dir/gcf2_libgit2.h";
-	$c_src = PublicInbox::Git::try_cat($f) or die "cat $f: $!";
+	$c_src = PublicInbox::IO::try_cat $f or die "cat $f: $!";
 	# append pkg-config results to the source to ensure Inline::C
 	# can rebuild if there's changes (it doesn't seem to detect
 	# $CFG{CCFLAGSEX} nor $CFG{CPPFLAGS} changes)
diff --git a/lib/PublicInbox/Git.pm b/lib/PublicInbox/Git.pm
index d00f576e..11712db2 100644
--- a/lib/PublicInbox/Git.pm
+++ b/lib/PublicInbox/Git.pm
@@ -18,16 +18,14 @@ use Errno qw(EINTR EAGAIN);
 use File::Glob qw(bsd_glob GLOB_NOSORT);
 use File::Spec ();
 use PublicInbox::Spawn qw(spawn popen_rd run_qx which);
-use PublicInbox::IO;
+use PublicInbox::IO qw(poll_in read_all try_cat);
 use PublicInbox::Tmpfile;
-use IO::Poll qw(POLLIN);
 use Carp qw(croak carp);
 use PublicInbox::SHA qw(sha_all);
 our %HEXLEN2SHA = (40 => 1, 64 => 256);
 our %OFMT2HEXLEN = (sha1 => 40, sha256 => 64);
-our @EXPORT_OK = qw(git_unquote git_quote %HEXLEN2SHA %OFMT2HEXLEN read_all);
+our @EXPORT_OK = qw(git_unquote git_quote %HEXLEN2SHA %OFMT2HEXLEN);
 our $in_cleanup;
-our $RDTIMEO = 60_000; # milliseconds
 our $async_warn; # true in read-only daemons
 
 # committerdate:unix is git 2.9.4+ (2017-05-05), so using raw instead
@@ -168,8 +166,6 @@ sub _sock_cmd {
 	$self->{sock} = PublicInbox::IO::attach_pid($s1, $pid);
 }
 
-sub poll_in ($) { IO::Poll::_poll($RDTIMEO, fileno($_[0]), my $ev = POLLIN) }
-
 sub my_read ($$$) {
 	my ($fh, $rbuf, $len) = @_;
 	my $left = $len - length($$rbuf);
@@ -555,22 +551,6 @@ sub modified ($;$) {
 	(split(/ /, <$fh> // time))[0] + 0; # integerize for JSON
 }
 
-# read_all/try_cat can probably be moved somewhere else...
-
-sub read_all ($;$$) {
-	my ($fh, $len, $bref) = @_;
-	$bref //= \(my $buf);
-	my $r = read($fh, $$bref, $len //= -s $fh);
-	croak("$fh read ($r != $len)") if $len != $r;
-	$$bref;
-}
-
-sub try_cat {
-	my ($path) = @_;
-	open(my $fh, '<', $path) or return '';
-	read_all($fh);
-}
-
 sub cat_desc ($) {
 	my $desc = try_cat($_[0]);
 	chomp $desc;
diff --git a/lib/PublicInbox/IO.pm b/lib/PublicInbox/IO.pm
index 4c92566d..0d303500 100644
--- a/lib/PublicInbox/IO.pm
+++ b/lib/PublicInbox/IO.pm
@@ -6,10 +6,9 @@ package PublicInbox::IO;
 use v5.12;
 use parent qw(IO::Handle Exporter);
 use PublicInbox::DS qw(awaitpid);
-our @EXPORT_OK = qw(write_file);
-
-# TODO: this can probably be the new home for read_all, try_cat
-# and maybe even buffered read/readline...
+our @EXPORT_OK = qw(poll_in read_all try_cat write_file);
+use Carp qw(croak);
+use IO::Poll qw(POLLIN);
 
 sub waitcb { # awaitpid callback
 	my ($pid, $errref, $cb, @args) = @_;
@@ -59,4 +58,23 @@ sub write_file ($$@) { # mode, filename, LIST (for print)
 	defined(wantarray) && !wantarray ? $fh : close $fh;
 }
 
+sub poll_in ($;$) {
+	IO::Poll::_poll($_[1] // -1, fileno($_[0]), my $ev = POLLIN);
+}
+
+sub read_all ($;$$) {
+	use autodie qw(read);
+	my ($io, $len, $bref) = @_;
+	$bref //= \(my $buf);
+	my $r = read($io, $$bref, $len //= -s $io);
+	croak("read($io) ($r != $len)") if $len != $r;
+	$$bref;
+}
+
+sub try_cat ($) {
+	my ($path) = @_;
+	open(my $fh, '<', $path) or return '';
+	read_all $fh;
+}
+
 1;
diff --git a/lib/PublicInbox/IdxStack.pm b/lib/PublicInbox/IdxStack.pm
index cc9e0125..7681ee6f 100644
--- a/lib/PublicInbox/IdxStack.pm
+++ b/lib/PublicInbox/IdxStack.pm
@@ -8,7 +8,7 @@ use v5.12;
 use Fcntl qw(:seek);
 use constant PACK_FMT => eval { pack('Q', 1) } ? 'A1QQH*H*' : 'A1IIH*H*';
 use autodie qw(open seek);
-use PublicInbox::Git qw(read_all);
+use PublicInbox::IO qw(read_all);
 
 # start off in write-only mode
 sub new {
diff --git a/lib/PublicInbox/Import.pm b/lib/PublicInbox/Import.pm
index 5b0201c6..2d60db55 100644
--- a/lib/PublicInbox/Import.pm
+++ b/lib/PublicInbox/Import.pm
@@ -21,7 +21,7 @@ use POSIX qw(strftime);
 use autodie qw(socketpair);
 use Carp qw(croak);
 use Socket qw(AF_UNIX SOCK_STREAM);
-use PublicInbox::Git qw(read_all);
+use PublicInbox::IO qw(read_all);
 
 sub default_branch () {
 	state $default_branch = do {
diff --git a/lib/PublicInbox/Inbox.pm b/lib/PublicInbox/Inbox.pm
index b31f3fff..e71ef6d2 100644
--- a/lib/PublicInbox/Inbox.pm
+++ b/lib/PublicInbox/Inbox.pm
@@ -189,7 +189,7 @@ sub cloneurl {
 	my ($self) = @_;
 	$self->{cloneurl} // do {
 		my @urls = split(/\s+/s,
-		  PublicInbox::Git::try_cat("$self->{inboxdir}/cloneurl"));
+			PublicInbox::IO::try_cat "$self->{inboxdir}/cloneurl");
 		scalar(@urls) ? ($self->{cloneurl} = \@urls) : undef;
 	} // [];
 }
diff --git a/lib/PublicInbox/InboxWritable.pm b/lib/PublicInbox/InboxWritable.pm
index 6af72e71..8e95cb28 100644
--- a/lib/PublicInbox/InboxWritable.pm
+++ b/lib/PublicInbox/InboxWritable.pm
@@ -7,7 +7,7 @@ use strict;
 use v5.10.1;
 use parent qw(PublicInbox::Inbox PublicInbox::Umask Exporter);
 use PublicInbox::Import;
-use PublicInbox::Git qw(read_all);
+use PublicInbox::IO qw(read_all);
 use PublicInbox::Filter::Base qw(REJECT);
 use Errno qw(ENOENT);
 our @EXPORT_OK = qw(eml_from_path);
diff --git a/lib/PublicInbox/LeiALE.pm b/lib/PublicInbox/LeiALE.pm
index 674d897e..528de22c 100644
--- a/lib/PublicInbox/LeiALE.pm
+++ b/lib/PublicInbox/LeiALE.pm
@@ -8,7 +8,7 @@
 package PublicInbox::LeiALE;
 use v5.12;
 use parent qw(PublicInbox::LeiSearch PublicInbox::Lock);
-use PublicInbox::Git qw(read_all);
+use PublicInbox::Git;
 use autodie qw(close open rename seek truncate);
 use PublicInbox::Import;
 use PublicInbox::LeiXSearch;
@@ -54,7 +54,7 @@ sub refresh_externals {
 	$self->git->cleanup;
 	my $lk = $self->lock_for_scope;
 	my $cur_lxs = ref($lxs)->new;
-	my $orig = read_all($self->{lockfh});
+	my $orig = PublicInbox::IO::read_all $self->{lockfh};
 	my $new = '';
 	my $old = '';
 	my $gone = 0;
@@ -86,7 +86,7 @@ sub refresh_externals {
 	}
 	$new = '';
 	my $f = $self->git->{git_dir}.'/objects/info/alternates';
-	$old = PublicInbox::Git::try_cat($f);
+	$old = PublicInbox::IO::try_cat $f;
 	for my $x (@ibxish) {
 		$new .= $lei->canonpath_harder($x->git->{git_dir})."/objects\n";
 	}
diff --git a/lib/PublicInbox/LeiBlob.pm b/lib/PublicInbox/LeiBlob.pm
index 648d35b6..127cc81e 100644
--- a/lib/PublicInbox/LeiBlob.pm
+++ b/lib/PublicInbox/LeiBlob.pm
@@ -10,7 +10,8 @@ use parent qw(PublicInbox::IPC);
 use PublicInbox::Spawn qw(run_wait run_qx which);
 use PublicInbox::DS;
 use PublicInbox::Eml;
-use PublicInbox::Git qw(read_all);
+use PublicInbox::Git;
+use PublicInbox::IO qw(read_all);
 
 sub get_git_dir ($$) {
 	my ($lei, $d) = @_;
diff --git a/lib/PublicInbox/LeiConfig.pm b/lib/PublicInbox/LeiConfig.pm
index b915d787..a50ff2b6 100644
--- a/lib/PublicInbox/LeiConfig.pm
+++ b/lib/PublicInbox/LeiConfig.pm
@@ -5,7 +5,7 @@ use v5.12;
 use PublicInbox::PktOp;
 use Fcntl qw(SEEK_SET);
 use autodie qw(open seek);
-use PublicInbox::Git qw(read_all);
+use PublicInbox::IO qw(read_all);
 
 sub cfg_do_edit ($;$) {
 	my ($self, $reason) = @_;
diff --git a/lib/PublicInbox/LeiMailSync.pm b/lib/PublicInbox/LeiMailSync.pm
index 74ef1362..17254a82 100644
--- a/lib/PublicInbox/LeiMailSync.pm
+++ b/lib/PublicInbox/LeiMailSync.pm
@@ -10,7 +10,8 @@ use PublicInbox::Compat qw(uniqstr);
 use DBI qw(:sql_types); # SQL_BLOB
 use PublicInbox::ContentHash qw(git_sha);
 use Carp ();
-use PublicInbox::Git qw(%HEXLEN2SHA read_all);
+use PublicInbox::Git qw(%HEXLEN2SHA);
+use PublicInbox::IO qw(read_all);
 
 sub dbh_new {
 	my ($self) = @_;
diff --git a/lib/PublicInbox/LeiMirror.pm b/lib/PublicInbox/LeiMirror.pm
index e4914f75..49febe9e 100644
--- a/lib/PublicInbox/LeiMirror.pm
+++ b/lib/PublicInbox/LeiMirror.pm
@@ -17,7 +17,6 @@ use Carp qw(croak);
 use URI;
 use PublicInbox::Config qw(glob2re);
 use PublicInbox::Inbox;
-use PublicInbox::Git qw(read_all);
 use PublicInbox::LeiCurl;
 use PublicInbox::OnDestroy;
 use PublicInbox::SHA qw(sha256_hex sha_all);
@@ -174,7 +173,7 @@ sub _get_txt_done { # returns true on error (non-fatal), undef on success
 	return warn("# @$cmd failed (non-fatal)\n") if $cerr;
 	seek($fh, 0, SEEK_SET);
 	$self->{"mtime.$endpoint"} = (stat($fh))[9];
-	$self->{"txt.$endpoint"} = read_all($fh, -s _);
+	$self->{"txt.$endpoint"} = PublicInbox::IO::read_all $fh, -s _;
 	undef; # success
 }
 
@@ -207,7 +206,7 @@ sub _write_inbox_config {
 sub set_description ($) {
 	my ($self) = @_;
 	my $dst = $self->{cur_dst} // $self->{dst};
-	chomp(my $orig = PublicInbox::Git::try_cat("$dst/description"));
+	chomp(my $orig = PublicInbox::IO::try_cat("$dst/description"));
 	my $d = $orig;
 	while (defined($d) && ($d =~ m!^\(\$INBOX_DIR/description missing\)! ||
 			$d =~ /^Unnamed repository/ || $d !~ /\S/)) {
@@ -806,7 +805,7 @@ sub update_ent {
 	}
 	if (defined(my $t = $self->{-ent}->{modified})) {
 		my ($dn, $bn) = ("$dst/info/web", 'last-modified');
-		my $orig = PublicInbox::Git::try_cat("$dn/$bn");
+		my $orig = PublicInbox::IO::try_cat("$dn/$bn");
 		$t = strftime('%F %T', gmtime($t))." +0000\n";
 		File::Path::mkpath($dn);
 		atomic_write($dn, $bn, $t) if $orig ne $t;
@@ -936,7 +935,7 @@ failed to extract epoch number from $src
 sub decode_manifest ($$$) {
 	my ($fh, $fn, $uri) = @_;
 	my $js;
-	my $gz = read_all($fh);
+	my $gz = PublicInbox::IO::read_all $fh;
 	gunzip(\$gz => \$js, MultiStream => 1) or
 		die "gunzip($uri): $GunzipError\n";
 	my $m = eval { PublicInbox::Config->json->decode($js) };
@@ -1083,7 +1082,7 @@ sub dump_manifest ($$) {
 sub dump_project_list ($$) {
 	my ($self, $m) = @_;
 	my $f = $self->{'-project-list'};
-	my $old = defined($f) ? PublicInbox::Git::try_cat($f) : '';
+	my $old = defined($f) ? PublicInbox::IO::try_cat($f) : '';
 	my %new;
 
 	open my $dh, '<', '.';
diff --git a/lib/PublicInbox/LeiSucks.pm b/lib/PublicInbox/LeiSucks.pm
index 82aea8d4..ddb3faf7 100644
--- a/lib/PublicInbox/LeiSucks.pm
+++ b/lib/PublicInbox/LeiSucks.pm
@@ -12,7 +12,7 @@ use Config;
 use POSIX ();
 use PublicInbox::Config;
 use PublicInbox::IPC;
-use PublicInbox::Git qw(read_all);
+use PublicInbox::IO qw(read_all);
 
 sub lei_sucks {
 	my ($lei, @argv) = @_;
diff --git a/lib/PublicInbox/MultiGit.pm b/lib/PublicInbox/MultiGit.pm
index 1e8eb47a..b7691806 100644
--- a/lib/PublicInbox/MultiGit.pm
+++ b/lib/PublicInbox/MultiGit.pm
@@ -9,7 +9,7 @@ use PublicInbox::Spawn qw(run_die run_qx);
 use PublicInbox::Import;
 use File::Temp 0.19;
 use List::Util qw(max);
-use PublicInbox::Git qw(read_all);
+use PublicInbox::IO qw(read_all);
 use autodie qw(chmod close rename);
 
 sub new {
diff --git a/lib/PublicInbox/TestCommon.pm b/lib/PublicInbox/TestCommon.pm
index 5ad12942..83e99b42 100644
--- a/lib/PublicInbox/TestCommon.pm
+++ b/lib/PublicInbox/TestCommon.pm
@@ -48,8 +48,8 @@ sub require_bsd (;$) {
 sub xbail (@) { BAIL_OUT join(' ', map { ref() ? (explain($_)) : ($_) } @_) }
 
 sub read_all ($;$$) {
-	require PublicInbox::Git;
-	PublicInbox::Git::read_all($_[0], $_[1], $_[2])
+	require PublicInbox::IO;
+	PublicInbox::IO::read_all($_[0], $_[1], $_[2])
 }
 
 sub eml_load ($) {
diff --git a/lib/PublicInbox/ViewVCS.pm b/lib/PublicInbox/ViewVCS.pm
index 6c588ddf..be062f36 100644
--- a/lib/PublicInbox/ViewVCS.pm
+++ b/lib/PublicInbox/ViewVCS.pm
@@ -17,7 +17,7 @@ use strict;
 use v5.10.1;
 use File::Temp 0.19 (); # newdir
 use PublicInbox::SolverGit;
-use PublicInbox::Git qw(read_all);
+use PublicInbox::Git;
 use PublicInbox::GitAsyncCat;
 use PublicInbox::WwwStream qw(html_oneshot);
 use PublicInbox::Linkify;
@@ -62,7 +62,7 @@ sub dbg_log ($) {
 		warn "seek(log): $!";
 		return '<pre>debug log seek error</pre>';
 	}
-	$log = eval { read_all($log) } // do {
+	$log = eval { PublicInbox::IO::read_all $log } // do {
 		warn "read(log): $@";
 		return '<pre>debug log read error</pre>';
 	};
@@ -248,7 +248,7 @@ EOM
 	if (-s $fh > $MAX_SIZE) {
 		print $zfh "---\n patch is too large to show\n";
 	} else { # prepare flush_diff:
-		read_all($fh, -s _, \$x);
+		PublicInbox::IO::read_all $fh, -s _, \$x;
 		utf8_maybe($x);
 		$ctx->{-apfx} = $ctx->{-spfx} = $upfx;
 		$x =~ s/\r?\n/\n/gs;
diff --git a/lib/PublicInbox/WWW.pm b/lib/PublicInbox/WWW.pm
index 183c5df7..502dd638 100644
--- a/lib/PublicInbox/WWW.pm
+++ b/lib/PublicInbox/WWW.pm
@@ -588,7 +588,7 @@ sub stylesheets_prepare ($$) {
 				next;
 			};
 			my $ctime = 0;
-			my $local = PublicInbox::Git::read_all($fh, -s $fh);
+			my $local = read_all $fh; # sets _ stat cache
 			if ($local =~ /\S/) {
 				$ctime = sprintf('%x',(stat(_))[10]);
 				$local = $mini->($local);
diff --git a/lib/PublicInbox/XapHelper.pm b/lib/PublicInbox/XapHelper.pm
index 41c66a12..1ee918e3 100644
--- a/lib/PublicInbox/XapHelper.pm
+++ b/lib/PublicInbox/XapHelper.pm
@@ -10,7 +10,7 @@ $GLP->configure(qw(require_order bundling no_ignore_case no_auto_abbrev));
 use PublicInbox::Search qw(xap_terms);
 use PublicInbox::CodeSearch;
 use PublicInbox::IPC;
-use PublicInbox::Git qw(read_all);
+use PublicInbox::IO qw(read_all);
 use Socket qw(SOL_SOCKET SO_TYPE SOCK_SEQPACKET AF_UNIX);
 use PublicInbox::DS qw(awaitpid);
 use autodie qw(open getsockopt);
@@ -125,9 +125,8 @@ sub cmd_dump_roots {
 	$req->{A} or return warn('dump_roots requires -A PREFIX');
 	open my $fh, '<', $root2id_file;
 	my $root2id; # record format: $OIDHEX "\0" uint32_t
-	my @x = split(/\0/, read_all($fh));
-	while (@x) {
-		my $oidhex = shift @x;
+	my @x = split(/\0/, read_all $fh);
+	while (defined(my $oidhex = shift @x)) {
 		$root2id->{$oidhex} = shift @x;
 	}
 	my $opt = { relevance => -1, limit => $req->{'m'},
diff --git a/lib/PublicInbox/XapHelperCxx.pm b/lib/PublicInbox/XapHelperCxx.pm
index 83503035..908a71f4 100644
--- a/lib/PublicInbox/XapHelperCxx.pm
+++ b/lib/PublicInbox/XapHelperCxx.pm
@@ -8,7 +8,6 @@
 package PublicInbox::XapHelperCxx;
 use v5.12;
 use PublicInbox::Spawn qw(run_qx which);
-use PublicInbox::Git qw(read_all);
 use PublicInbox::Search;
 use Fcntl qw(SEEK_SET);
 use Config;
@@ -67,7 +66,7 @@ sub build () {
 	for (@srcs) {
 		say $fh qq(# line 1 "$_");
 		open my $rfh, '<', $_;
-		print $fh read_all($rfh);
+		print $fh PublicInbox::IO::read_all $rfh;
 	}
 	print $fh PublicInbox::Search::generate_cxx();
 	print $fh PublicInbox::CodeSearch::generate_cxx();
diff --git a/script/public-inbox-convert b/script/public-inbox-convert
index d8186809..713c2881 100755
--- a/script/public-inbox-convert
+++ b/script/public-inbox-convert
@@ -131,7 +131,7 @@ while (<$rd>) {
 		$state = 'commit';
 	} elsif (/^data ([0-9]+)/) {
 		print $io $_ or $im->wfail;
-		print $io PublicInbox::Git::read_all($rd, $1) or $im->wfail;
+		print $io PublicInbox::IO::read_all($rd, $1) or $im->wfail;
 		next;
 	} elsif ($state eq 'commit') {
 		if (m{^M 100644 :([0-9]+) (${h}{2}/${h}{38})}o) {
diff --git a/script/public-inbox-edit b/script/public-inbox-edit
index 77028817..88115d7c 100755
--- a/script/public-inbox-edit
+++ b/script/public-inbox-edit
@@ -15,7 +15,6 @@ PublicInbox::Admin::check_require('-index');
 use PublicInbox::Eml;
 use PublicInbox::InboxWritable qw(eml_from_path);
 use PublicInbox::Import;
-use PublicInbox::Git qw(read_all);
 
 my $help = <<'EOF';
 usage: public-inbox-edit -m MESSAGE-ID [--all] [INBOX_DIRS]
@@ -185,7 +184,7 @@ retry_edit:
 	# rename/relink $edit_fn
 	open my $new_fh, '<', $edit_fn or
 		die "can't read edited file ($edit_fn): $!\n";
-	my $new_raw = read_all($new_fh);
+	my $new_raw = PublicInbox::IO::read_all $new_fh;
 
 	if (!$opt->{raw}) {
 		PublicInbox::Eml::strip_from($new_raw);
diff --git a/script/public-inbox-init b/script/public-inbox-init
index 6420db7e..8915cf31 100755
--- a/script/public-inbox-init
+++ b/script/public-inbox-init
@@ -126,12 +126,12 @@ my $auto_unlink = PublicInbox::OnDestroy->new($$, sub { unlink $lockfile });
 my $perm = 0644 & ~umask;
 my %seen;
 if (-e $pi_config) {
-	require PublicInbox::Git;
+	require PublicInbox::IO;
 	open(my $oh, '<', $pi_config);
 	my @st = stat($oh) or die "(f)stat failed on $pi_config: $!\n";
 	$perm = $st[2];
 	chmod($perm & 07777, $fh);
-	print $fh PublicInbox::Git::read_all($oh);
+	print $fh PublicInbox::IO::read_all($oh);
 	close $oh;
 
 	# yes, this conflict checking is racy if multiple instances of this
diff --git a/t/cindex.t b/t/cindex.t
index 09183518..2033945e 100644
--- a/t/cindex.t
+++ b/t/cindex.t
@@ -46,7 +46,7 @@ ok(run_script([qw(-cindex --dangerous -q), "$tmp/wt0"]), 'cindex internal');
 # (see c4201214cbf10636e2c1ab9131573f735b42c8d4 in linux.git)
 my $zp = create_coderepo 'NUL in patch', sub {
 	require PublicInbox::Git;
-	my $src = PublicInbox::Git::try_cat("$pwd/COPYING");
+	my $src = PublicInbox::IO::try_cat("$pwd/COPYING");
 	xsys_e([qw(git init -q)]);
 
 	# needs to be further than FIRST_FEW_BYTES (8000) in git.git
diff --git a/t/clone-coderepo.t b/t/clone-coderepo.t
index bce4ecd9..0e6b4ac7 100644
--- a/t/clone-coderepo.t
+++ b/t/clone-coderepo.t
@@ -95,7 +95,7 @@ is(xqx([qw(git config gitweb.owner)], { GIT_DIR => "$tmpdir/dst/a.git" }),
 	"\xc4\x80lice\n", 'a.git gitweb.owner set');
 is(xqx([qw(git config gitweb.owner)], { GIT_DIR => "$tmpdir/dst/b.git" }),
 	"Bob\n", 'b.git gitweb.owner set');
-my $desc = PublicInbox::Git::try_cat("$tmpdir/dst/a.git/description");
+my $desc = PublicInbox::IO::try_cat("$tmpdir/dst/a.git/description");
 is($desc, "\xc4\x80lice's repo\n", 'description set');
 
 my $dst_pl = "$tmpdir/dst/projects.list";
@@ -104,7 +104,7 @@ ok(!-d "$tmpdir/dst/objstore", 'no objstore created w/o forkgroups');
 my $r = $read_manifest->($dst_mf);
 is_deeply($r, $m, 'manifest matches');
 
-is(PublicInbox::Git::try_cat($dst_pl), "a.git\nb.git\n",
+is(PublicInbox::IO::try_cat($dst_pl), "a.git\nb.git\n",
 	'wrote projects.list');
 
 { # check symlinks
@@ -113,7 +113,7 @@ is(PublicInbox::Git::try_cat($dst_pl), "a.git\nb.git\n",
 	utime($t0, $t0, $dst_mf) or xbail "utime: $!";
 	ok(run_script($cmd), 'clone again +symlinks');
 	ok(-l "$tmpdir/dst/old/a.git", 'symlink created');
-	is(PublicInbox::Git::try_cat($dst_pl), "a.git\nb.git\n",
+	is(PublicInbox::IO::try_cat($dst_pl), "a.git\nb.git\n",
 		'projects.list does not include symlink by default');
 
 	$r = $read_manifest->($dst_mf);
@@ -127,7 +127,7 @@ is(PublicInbox::Git::try_cat($dst_pl), "a.git\nb.git\n",
 	utime($t0, $t0, $dst_mf) or xbail "utime: $!";
 	my $rdr = { 2 => \(my $err = '') };
 	ok(run_script($cmd, undef, $rdr), 'clone again for expired gone.git');
-	is(PublicInbox::Git::try_cat($dst_pl), "a.git\nb.git\n",
+	is(PublicInbox::IO::try_cat($dst_pl), "a.git\nb.git\n",
 		'project list cleaned');
 	like($err, qr/no longer exist.*\bgone\.git\b/s, 'gone.git noted');
 }
@@ -146,7 +146,7 @@ is(PublicInbox::Git::try_cat($dst_pl), "a.git\nb.git\n",
 	my $rdr = { 2 => \(my $err = '') };
 	my $xcmd = [ @$cmd, '--purge' ];
 	ok(run_script($xcmd, undef, $rdr), 'clone again for expired gone.git');
-	is(PublicInbox::Git::try_cat($dst_pl), "a.git\nb.git\n",
+	is(PublicInbox::IO::try_cat($dst_pl), "a.git\nb.git\n",
 		'project list cleaned');
 	like($err, qr!ignored/gone.*?\bgone-rdonly\.git\b!s,
 		'gone-rdonly.git noted');
diff --git a/t/init.t b/t/init.t
index 3d2c7873..275192cf 100644
--- a/t/init.t
+++ b/t/init.t
@@ -107,7 +107,7 @@ sub quiet_fail {
 	umask($umask) // xbail "umask: $!";
 	ok(-d "$tmpdir/a/b/c/d", 'directory created');
 	my $desc = "$tmpdir/a/b/c/d/description";
-	is(PublicInbox::Git::try_cat($desc),
+	is(PublicInbox::IO::try_cat($desc),
 		"public inbox for abcd\@example.com\n", 'description set');
 	my $mode = (stat($desc))[2];
 	is(sprintf('0%03o', $mode & 0777), '0644',
diff --git a/t/lei-mirror.t b/t/lei-mirror.t
index 08961491..37c9751b 100644
--- a/t/lei-mirror.t
+++ b/t/lei-mirror.t
@@ -23,7 +23,7 @@ test_lei({ tmpdir => $tmpdir }, sub {
 	lei_ok('add-external', $t1, '--mirror', "$http/t1/", \'--mirror v1');
 	my $mm_dup = "$t1/public-inbox/msgmap.sqlite3";
 	ok(-f $mm_dup, 't1-mirror indexed');
-	is(PublicInbox::Git::try_cat("$t1/description"),
+	is(PublicInbox::IO::try_cat("$t1/description"),
 		"mirror of $http/t1/\n", 'description set');
 	ok(-f "$t1/Makefile", 'convenience Makefile added (v1)');
 	SKIP: {
@@ -51,7 +51,7 @@ test_lei({ tmpdir => $tmpdir }, sub {
 	ok(-f $mm_dup, 't2-mirror indexed');
 	ok(-f "$t2/description", 't2 description');
 	ok(-f "$t2/Makefile", 'convenience Makefile added (v2)');
-	is(PublicInbox::Git::try_cat("$t2/description"),
+	is(PublicInbox::IO::try_cat("$t2/description"),
 		"mirror of $http/t2/\n", 'description set');
 	$tb = PublicInbox::Msgmap->new_file($mm_dup)->created_at;
 	is($tb, $created{v2}, 'created_at matched in v2 mirror');
@@ -207,14 +207,14 @@ $td->join;
 	my $exp = "mirror of https://example.com/src/\n";
 	my $f = "$tmpdir/description";
 	PublicInbox::LeiMirror::set_description($mrr);
-	is(PublicInbox::Git::try_cat($f), $exp, 'description set on ENOENT');
+	is(PublicInbox::IO::try_cat($f), $exp, 'description set on ENOENT');
 
 	my $fh;
 	(open($fh, '>', $f) and close($fh)) or xbail $!;
 	PublicInbox::LeiMirror::set_description($mrr);
-	is(PublicInbox::Git::try_cat($f), $exp, 'description set on empty');
+	is(PublicInbox::IO::try_cat($f), $exp, 'description set on empty');
 	(open($fh, '>', $f) and print $fh "x\n" and close($fh)) or xbail $!;
-	is(PublicInbox::Git::try_cat($f), "x\n",
+	is(PublicInbox::IO::try_cat($f), "x\n",
 		'description preserved if non-default');
 }
 

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* www: squash read_all usage fix
  2023-11-02  9:35 ` [PATCH 13/14] move read_all, try_cat, and poll_in to PublicInbox::IO Eric Wong
@ 2023-11-02 20:59   ` Eric Wong
  0 siblings, 0 replies; 17+ messages in thread
From: Eric Wong @ 2023-11-02 20:59 UTC (permalink / raw)
  To: meta

Eric Wong <e@80x24.org> wrote:
> --- a/lib/PublicInbox/WWW.pm
> +++ b/lib/PublicInbox/WWW.pm
> @@ -588,7 +588,7 @@ sub stylesheets_prepare ($$) {
>  				next;
>  			};
>  			my $ctime = 0;
> -			my $local = PublicInbox::Git::read_all($fh, -s $fh);
> +			my $local = read_all $fh; # sets _ stat cache
>  			if ($local =~ /\S/) {
>  				$ctime = sprintf('%x',(stat(_))[10]);
>  				$local = $mini->($local);

Oops :x  I should add tests there...

diff --git a/lib/PublicInbox/WWW.pm b/lib/PublicInbox/WWW.pm
index 502dd638..d2bd68ea 100644
--- a/lib/PublicInbox/WWW.pm
+++ b/lib/PublicInbox/WWW.pm
@@ -588,7 +588,7 @@ sub stylesheets_prepare ($$) {
 				next;
 			};
 			my $ctime = 0;
-			my $local = read_all $fh; # sets _ stat cache
+			my $local = PublicInbox::IO::read_all $fh; # sets _
 			if ($local =~ /\S/) {
 				$ctime = sprintf('%x',(stat(_))[10]);
 				$local = $mini->($local);

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 14/14] t/cindex+extsearch: use write_file, autodie, etc.
  2023-11-02  9:35 [PATCH 00/14] IO/IPC-related cleanups Eric Wong
                   ` (12 preceding siblings ...)
  2023-11-02  9:35 ` [PATCH 13/14] move read_all, try_cat, and poll_in to PublicInbox::IO Eric Wong
@ 2023-11-02  9:35 ` Eric Wong
  13 siblings, 0 replies; 17+ messages in thread
From: Eric Wong @ 2023-11-02  9:35 UTC (permalink / raw)
  To: meta

write_file is a new API which makes setting up config files more
pleasant, while autodie and scalarref redirects (in tests) have
been available for a while, now.  So do what we can to reduce
the code burden we have.
---
 t/cindex.t    | 15 ++++-----------
 t/extsearch.t | 48 ++++++++++++++----------------------------------
 2 files changed, 18 insertions(+), 45 deletions(-)

diff --git a/t/cindex.t b/t/cindex.t
index 2033945e..60711492 100644
--- a/t/cindex.t
+++ b/t/cindex.t
@@ -8,7 +8,7 @@ use List::Util qw(sum);
 use autodie qw(close open rename);
 require_mods(qw(json Xapian));
 use_ok 'PublicInbox::CodeSearchIdx';
-require PublicInbox::Import;
+use PublicInbox::Import;
 my ($tmp, $for_destroy) = tmpdir();
 my $pwd = getcwd();
 my @unused_keys = qw(last_commit has_threadid skip_docdata);
@@ -45,7 +45,6 @@ ok(run_script([qw(-cindex --dangerous -q), "$tmp/wt0"]), 'cindex internal');
 # it's possible for git to emit NUL characters in diffs
 # (see c4201214cbf10636e2c1ab9131573f735b42c8d4 in linux.git)
 my $zp = create_coderepo 'NUL in patch', sub {
-	require PublicInbox::Git;
 	my $src = PublicInbox::IO::try_cat("$pwd/COPYING");
 	xsys_e([qw(git init -q)]);
 
@@ -53,17 +52,13 @@ my $zp = create_coderepo 'NUL in patch', sub {
 	$src =~ s/\b(Limitation of Liability\.)\n\n/$1\n\0\n/s or
 		xbail "BUG: no `\\n\\n' in $pwd/COPYING";
 
-	open my $fh, '>', 'f';
-	print $fh $src or xbail "print: $!";
-	close $fh;
+	PublicInbox::IO::write_file '>', 'f', $src;
 	xsys_e([qw(/bin/sh -c), <<'EOM']);
 git add f &&
 git commit -q -m 'initial with NUL character'
 EOM
 	$src =~ s/\n\0\n/\n\n/ or xbail "BUG: no `\\n\\0\\n'";
-	open $fh, '>', 'f';
-	print $fh $src or xbail "print: $!";
-	close $fh;
+	PublicInbox::IO::write_file '>', 'f', $src;
 	xsys_e([qw(/bin/sh -c), <<'EOM']);
 git add f &&
 git commit -q -m 'remove NUL character' &&
@@ -207,13 +202,11 @@ my $basic = create_inbox 'basic', indexlevel => 'basic', sub {
 };
 {
 	my $env = { PI_CONFIG => "$tmp/pi_config" };
-	open my $fh, '>', $env->{PI_CONFIG};
-	print $fh <<EOM;
+	PublicInbox::IO::write_file '>', $env->{PI_CONFIG}, <<EOM;
 [publicinbox "basictest"]
 	inboxdir = $basic->{inboxdir}
 	address = basic\@example.com
 EOM
-	close $fh;
 	my $cmd = [ qw(-cindex -u --all --associate -d), "$tmp/ext",
 		'-I', $basic->{inboxdir} ];
 	$cidx_out = $cidx_err = '';
diff --git a/t/extsearch.t b/t/extsearch.t
index 2995cf95..1a1eb350 100644
--- a/t/extsearch.t
+++ b/t/extsearch.t
@@ -5,9 +5,9 @@ use v5.12;
 use PublicInbox::TestCommon;
 use PublicInbox::Config;
 use PublicInbox::InboxWritable;
-use Fcntl qw(:seek);
 require_git(2.6);
 require_mods(qw(json DBD::SQLite Xapian));
+use autodie qw(open rename truncate);
 require PublicInbox::Search;
 use_ok 'PublicInbox::ExtSearch';
 use_ok 'PublicInbox::ExtSearchIdx';
@@ -16,12 +16,10 @@ my ($home, $for_destroy) = tmpdir();
 local $ENV{HOME} = $home;
 mkdir "$home/.public-inbox" or BAIL_OUT $!;
 my $cfg_path = "$home/.public-inbox/config";
-open my $fh, '>', $cfg_path or BAIL_OUT $!;
-print $fh <<EOF or BAIL_OUT $!;
+PublicInbox::IO::write_file '>', $cfg_path, <<EOF;
 [publicinboxMda]
 	spamcheck = none
 EOF
-close $fh or BAIL_OUT $!;
 my $v2addr = 'v2test@example.com';
 my $v1addr = 'v1test@example.com';
 ok(run_script([qw(-init -Lbasic -V2 v2test --newsgroup v2.example),
@@ -30,24 +28,18 @@ my $env = { ORIGINAL_RECIPIENT => $v2addr };
 my $eml = eml_load('t/utf8.eml');
 
 $eml->header_set('List-Id', '<v2.example.com>');
-open($fh, '+>', undef) or BAIL_OUT $!;
-$fh->autoflush(1);
-print $fh $eml->as_string or BAIL_OUT $!;
-seek($fh, 0, SEEK_SET) or BAIL_OUT $!;
 
-run_script(['-mda', '--no-precheck'], $env, { 0 => $fh }) or BAIL_OUT '-mda';
+my $in = \($eml->as_string);
+run_script(['-mda', '--no-precheck'], $env, { 0 => $in }) or BAIL_OUT '-mda';
 
 ok(run_script([qw(-init -V1 v1test --newsgroup v1.example), "$home/v1test",
 	'http://example.com/v1test', $v1addr ]), 'v1test init');
 
 $eml->header_set('List-Id', '<v1.example.com>');
-seek($fh, 0, SEEK_SET) or BAIL_OUT $!;
-truncate($fh, 0) or BAIL_OUT $!;
-print $fh $eml->as_string or BAIL_OUT $!;
-seek($fh, 0, SEEK_SET) or BAIL_OUT $!;
+$in = \$eml->as_string;
 
 $env = { ORIGINAL_RECIPIENT => $v1addr };
-run_script(['-mda', '--no-precheck'], $env, { 0 => $fh }) or BAIL_OUT '-mda';
+run_script(['-mda', '--no-precheck'], $env, { 0 => $in }) or BAIL_OUT '-mda';
 
 run_script([qw(-index -Lbasic), "$home/v1test"]) or BAIL_OUT "index $?";
 
@@ -103,14 +95,11 @@ if ('with boost') {
 }
 
 { # TODO: -extindex should write this to config
-	open $fh, '>>', $cfg_path or BAIL_OUT $!;
-	print $fh <<EOF or BAIL_OUT $!;
+	PublicInbox::IO::write_file '>>', $cfg_path, <<EOF;
 ; for ->ALL
 [extindex "all"]
 	topdir = $home/extindex
 EOF
-	close $fh or BAIL_OUT $!;
-
 	my $pi_cfg = PublicInbox::Config->new;
 	$pi_cfg->fill_all;
 	ok($pi_cfg->ALL, '->ALL');
@@ -202,11 +191,7 @@ if ('inbox edited') {
 	is_deeply($res, $exp, 'isearch limited results');
 	$pi_cfg = $res = $exp = undef;
 
-	open my $rmfh, '+>', undef or BAIL_OUT $!;
-	$rmfh->autoflush(1);
-	print $rmfh $eml2->as_string or BAIL_OUT $!;
-	seek($rmfh, 0, SEEK_SET) or BAIL_OUT $!;
-	$opt->{0} = $rmfh;
+	$opt->{0} = \($eml2->as_string);
 	ok(run_script([qw(-learn rm --all)], undef, $opt), '-learn rm');
 
 	ok(run_script([qw(-extindex --all), "$home/extindex"], undef, undef),
@@ -245,13 +230,11 @@ if ('inject w/o indexing') {
 	isnt($tip, $cmt, '0.git v2 updated');
 
 	# inject a message w/o updating index
-	rename("$home/v1test/public-inbox", "$home/v1test/skip-index") or
-		BAIL_OUT $!;
-	open(my $eh, '<', 't/iso-2202-jp.eml') or BAIL_OUT $!;
+	rename("$home/v1test/public-inbox", "$home/v1test/skip-index");
+	open(my $eh, '<', 't/iso-2202-jp.eml');
 	run_script(['-mda', '--no-precheck'], $env, { 0 => $eh}) or
 		BAIL_OUT '-mda';
-	rename("$home/v1test/skip-index", "$home/v1test/public-inbox") or
-		BAIL_OUT $!;
+	rename("$home/v1test/skip-index", "$home/v1test/public-inbox");
 
 	my ($in, $out, $err);
 	$in = $out = $err = '';
@@ -500,10 +483,8 @@ SKIP: {
 		"$home/v2tmp", 'http://example.com/v2tmp', $tmp_addr ])
 		or xbail '-init';
 	$env = { ORIGINAL_RECIPIENT => $tmp_addr };
-	open $fh, '+>', undef or xbail "open $!";
-	$fh->autoflush(1);
 	my $mid = 'tmpmsg@example.com';
-	print $fh <<EOM or xbail "print $!";
+	my $in = \<<EOM;
 From: b\@z
 To: b\@r
 Message-Id: <$mid>
@@ -511,8 +492,7 @@ Subject: tmpmsg
 Date: Tue, 19 Jan 2038 03:14:07 +0000
 
 EOM
-	seek $fh, 0, SEEK_SET or xbail "seek $!";
-	run_script([qw(-mda --no-precheck)], $env, {0 => $fh}) or xbail '-mda';
+	run_script([qw(-mda --no-precheck)], $env, {0 => $in}) or xbail '-mda';
 	ok(run_script([qw(-extindex --all), "$home/extindex"]), 'update');
 	my $nr;
 	{
@@ -525,7 +505,7 @@ EOM
 		$mset = $es->search->mset('z:0..');
 		$nr = $mset->size;
 	}
-	truncate($cfg_path, $old_size) or xbail "truncate $!";
+	truncate($cfg_path, $old_size);
 	my $rdr = { 2 => \(my $err) };
 	ok(run_script([qw(-extindex --gc), "$home/extindex"], undef, $rdr),
 		'gc to get rid of removed inbox');

^ permalink raw reply related	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2023-11-02 21:35 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-02  9:35 [PATCH 00/14] IO/IPC-related cleanups Eric Wong
2023-11-02  9:35 ` [PATCH 01/14] xap_helper.pm: use do_fork to Reset and reseed Eric Wong
2023-11-02  9:35 ` [PATCH 02/14] ds: replace FD map hash table with array Eric Wong
2023-11-02  9:35 ` [PATCH 03/14] treewide: use ->close method rather than CORE::close Eric Wong
2023-11-02 21:35   ` [PATCH 15/14] ds: don't try ->close after ->accept_SSL failure Eric Wong
2023-11-02  9:35 ` [PATCH 04/14] cindex: drop redundant close on regular FH Eric Wong
2023-11-02  9:35 ` [PATCH 05/14] treewide: use ->close to call ProcessIO->CLOSE Eric Wong
2023-11-02  9:35 ` [PATCH 06/14] multi_git: use autodie Eric Wong
2023-11-02  9:35 ` [PATCH 07/14] git_credential: use autodie where appropriate Eric Wong
2023-11-02  9:35 ` [PATCH 08/14] replace ProcessIO with untied PublicInbox::IO Eric Wong
2023-11-02  9:35 ` [PATCH 09/14] io: introduce write_file helper sub Eric Wong
2023-11-02  9:35 ` [PATCH 10/14] spawn: support PerlIO layer in scalar redirects Eric Wong
2023-11-02  9:35 ` [PATCH 11/14] treewide: check alternates writes with eof + autodie Eric Wong
2023-11-02  9:35 ` [PATCH 12/14] treewide: use eof and close to detect readline errors Eric Wong
2023-11-02  9:35 ` [PATCH 13/14] move read_all, try_cat, and poll_in to PublicInbox::IO Eric Wong
2023-11-02 20:59   ` www: squash read_all usage fix Eric Wong
2023-11-02  9:35 ` [PATCH 14/14] t/cindex+extsearch: use write_file, autodie, etc Eric Wong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).