[PATCH 3/6] umask: hoist out of InboxWritable

[Eric Wong <e@80x24.org>]

Since CodeSearchIdx doesn't deal with inboxes, it makes sense
to split it out from inbox-specific code and start moving
towards using OnDestroy to restore the umask at the end of
scope and reducing extra functions.
---
 MANIFEST                         |  1 +
 lib/PublicInbox/CodeSearchIdx.pm | 28 ++++---------
 lib/PublicInbox/ExtSearchIdx.pm  |  3 +-
 lib/PublicInbox/InboxWritable.pm | 70 +-------------------------------
 lib/PublicInbox/SearchIdx.pm     |  6 ++-
 lib/PublicInbox/Umask.pm         | 70 ++++++++++++++++++++++++++++++++
 t/search.t                       |  7 ++--
 7 files changed, 91 insertions(+), 94 deletions(-)
 create mode 100644 lib/PublicInbox/Umask.pm

diff --git a/MANIFEST b/MANIFEST
index a0e64c6a..c338f0f4 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -340,6 +340,7 @@ lib/PublicInbox/TestCommon.pm
 lib/PublicInbox/Tmpfile.pm
 lib/PublicInbox/URIimap.pm
 lib/PublicInbox/URInntps.pm
+lib/PublicInbox/Umask.pm
 lib/PublicInbox/Unsubscribe.pm
 lib/PublicInbox/UserContent.pm
 lib/PublicInbox/V2Writable.pm
diff --git a/lib/PublicInbox/CodeSearchIdx.pm b/lib/PublicInbox/CodeSearchIdx.pm
index 3a3fc03e..78032c00 100644
--- a/lib/PublicInbox/CodeSearchIdx.pm
+++ b/lib/PublicInbox/CodeSearchIdx.pm
@@ -770,28 +770,23 @@ sub init_tmp_git_dir ($) {
 
 sub prep_umask ($) {
 	my ($self) = @_;
-	my $um;
-	my $cur = umask;
 	if ($self->{-cidx_internal}) { # respect core.sharedRepository
 		@{$self->{git_dirs}} == 1 or die 'BUG: only for GIT_DIR';
-		# yuck, FIXME move umask handling out of inbox-specific stuff
-		require PublicInbox::InboxWritable;
-		my $git = PublicInbox::Git->new($self->{git_dirs}->[0]);
-		chomp($um = $git->qx('config', 'core.sharedRepository') // '');
-		$um = PublicInbox::InboxWritable::_git_config_perm(undef, $um);
-		$um = PublicInbox::InboxWritable::_umask_for($um);
-		umask == $um or progress($self, 'umask from git: ',
-						sprintf('0%03o', $um));
+		local $self->{git} =
+			PublicInbox::Git->new($self->{git_dirs}->[0]);
+		$self->with_umask;
 	} elsif (-d $self->{cidx_dir}) { # respect existing perms
 		my @st = stat(_);
-		$um = (~$st[2] & 0777);
+		my $um = (~$st[2] & 0777);
+		$self->{umask} = $um; # for SearchIdx->with_umask
 		umask == $um or progress($self, 'using umask from ',
 						$self->{cidx_dir}, ': ',
 						sprintf('0%03o', $um));
-	}
-	defined($um) ?
-		PublicInbox::OnDestroy->new(\&CORE::umask, umask($um)) :
+		PublicInbox::OnDestroy->new($$, \&CORE::umask, umask($um));
+	} else {
+		$self->{umask} = umask; # for SearchIdx->with_umask
 		undef;
+	}
 }
 
 sub start_prune ($) {
@@ -896,9 +891,4 @@ sub shard_done_wait { # awaitpid cb via ipc_worker_reap
 	PublicInbox::DS::enqueue_reap() if !shards_active(); # once more for PLC
 }
 
-sub with_umask { # TODO get rid of this treewide and rely on OnDestroy
-	my ($self, $cb, @arg) = @_;
-	$cb->(@arg);
-}
-
 1;
diff --git a/lib/PublicInbox/ExtSearchIdx.pm b/lib/PublicInbox/ExtSearchIdx.pm
index 5445b156..6f3711ba 100644
--- a/lib/PublicInbox/ExtSearchIdx.pm
+++ b/lib/PublicInbox/ExtSearchIdx.pm
@@ -16,7 +16,7 @@
 package PublicInbox::ExtSearchIdx;
 use strict;
 use v5.10.1;
-use parent qw(PublicInbox::ExtSearch PublicInbox::Lock);
+use parent qw(PublicInbox::ExtSearch PublicInbox::Umask PublicInbox::Lock);
 use Carp qw(croak carp);
 use Scalar::Util qw(blessed);
 use Sys::Hostname qw(hostname);
@@ -1397,7 +1397,6 @@ sub eidx_watch { # public-inbox-extindex --watch main loop
 
 no warnings 'once';
 *done = \&PublicInbox::V2Writable::done;
-*with_umask = \&PublicInbox::InboxWritable::with_umask;
 *parallel_init = \&PublicInbox::V2Writable::parallel_init;
 *nproc_shards = \&PublicInbox::V2Writable::nproc_shards;
 *sync_prepare = \&PublicInbox::V2Writable::sync_prepare;
diff --git a/lib/PublicInbox/InboxWritable.pm b/lib/PublicInbox/InboxWritable.pm
index 022e2a69..65952aa2 100644
--- a/lib/PublicInbox/InboxWritable.pm
+++ b/lib/PublicInbox/InboxWritable.pm
@@ -1,25 +1,17 @@
-# Copyright (C) 2018-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>
 
 # Extends read-only Inbox for writing
 package PublicInbox::InboxWritable;
 use strict;
 use v5.10.1;
-use parent qw(PublicInbox::Inbox Exporter);
+use parent qw(PublicInbox::Inbox PublicInbox::Umask Exporter);
 use PublicInbox::Import;
 use PublicInbox::Filter::Base qw(REJECT);
 use Errno qw(ENOENT);
 our @EXPORT_OK = qw(eml_from_path);
 use Fcntl qw(O_RDONLY O_NONBLOCK);
 
-use constant {
-	PERM_UMASK => 0,
-	OLD_PERM_GROUP => 1,
-	OLD_PERM_EVERYBODY => 2,
-	PERM_GROUP => 0660,
-	PERM_EVERYBODY => 0664,
-};
-
 sub new {
 	my ($class, $ibx, $creat_opt) = @_;
 	return $ibx if ref($ibx) eq $class;
@@ -176,64 +168,6 @@ sub import_mbox {
 	$im->done;
 }
 
-sub _read_git_config_perm {
-	my ($self) = @_;
-	chomp(my $perm = $self->git->qx('config', 'core.sharedRepository'));
-	$perm;
-}
-
-sub _git_config_perm {
-	my $self = shift;
-	my $perm = scalar @_ ? $_[0] : _read_git_config_perm($self);
-	return PERM_UMASK if (!defined($perm) || $perm eq '');
-	return PERM_UMASK if ($perm eq 'umask');
-	return PERM_GROUP if ($perm eq 'group');
-	if ($perm =~ /\A(?:all|world|everybody)\z/) {
-		return PERM_EVERYBODY;
-	}
-	return PERM_GROUP if ($perm =~ /\A(?:true|yes|on|1)\z/);
-	return PERM_UMASK if ($perm =~ /\A(?:false|no|off|0)\z/);
-
-	my $i = oct($perm);
-	return PERM_UMASK if ($i == PERM_UMASK);
-	return PERM_GROUP if ($i == OLD_PERM_GROUP);
-	return PERM_EVERYBODY if ($i == OLD_PERM_EVERYBODY);
-
-	if (($i & 0600) != 0600) {
-		die "core.sharedRepository mode invalid: ".
-		    sprintf('%.3o', $i) . "\nOwner must have permissions\n";
-	}
-	($i & 0666);
-}
-
-sub _umask_for {
-	my ($perm) = @_; # _git_config_perm return value
-	my $rv = $perm;
-	return umask if $rv == 0;
-
-	# set +x bit if +r or +w were set
-	$rv |= 0100 if ($rv & 0600);
-	$rv |= 0010 if ($rv & 0060);
-	$rv |= 0001 if ($rv & 0006);
-	(~$rv & 0777);
-}
-
-sub with_umask {
-	my ($self, $cb, @arg) = @_;
-	my $old = umask($self->{umask} //= umask_prepare($self));
-	my $rv = eval { $cb->(@arg) };
-	my $err = $@;
-	umask $old;
-	die $err if $err;
-	$rv;
-}
-
-sub umask_prepare {
-	my ($self) = @_;
-	my $perm = _git_config_perm($self);
-	_umask_for($perm);
-}
-
 sub cleanup ($) {
 	delete @{$_[0]}{qw(over mm git search)};
 }
diff --git a/lib/PublicInbox/SearchIdx.pm b/lib/PublicInbox/SearchIdx.pm
index 699af432..69ab30e6 100644
--- a/lib/PublicInbox/SearchIdx.pm
+++ b/lib/PublicInbox/SearchIdx.pm
@@ -9,7 +9,8 @@
 package PublicInbox::SearchIdx;
 use strict;
 use v5.10.1;
-use parent qw(PublicInbox::Search PublicInbox::Lock Exporter);
+use parent qw(PublicInbox::Search PublicInbox::Lock PublicInbox::Umask
+	Exporter);
 use PublicInbox::Eml;
 use PublicInbox::Search qw(xap_terms);
 use PublicInbox::InboxWritable;
@@ -821,7 +822,8 @@ sub unindex_both { # git->cat_async callback
 
 sub with_umask {
 	my $self = shift;
-	($self->{ibx} // $self->{eidx})->with_umask(@_);
+	my $owner = $self->{ibx} // $self->{eidx};
+	$owner ? $owner->with_umask(@_) : $self->SUPER::with_umask(@_)
 }
 
 # called by public-inbox-index
diff --git a/lib/PublicInbox/Umask.pm b/lib/PublicInbox/Umask.pm
new file mode 100644
index 00000000..00772ce5
--- /dev/null
+++ b/lib/PublicInbox/Umask.pm
@@ -0,0 +1,70 @@
+# Copyright (C) all contributors <meta@public-inbox.org>
+# License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
+
+# base class to ensures Xapian||SQLite files respect core.sharedRepository
+# of git repos
+package PublicInbox::Umask;
+use v5.12;
+use PublicInbox::OnDestroy;
+
+use constant {
+	PERM_UMASK => 0,
+	OLD_PERM_GROUP => 1,
+	OLD_PERM_EVERYBODY => 2,
+	PERM_GROUP => 0660,
+	PERM_EVERYBODY => 0664,
+};
+
+sub _read_git_config_perm {
+	my ($self) = @_;
+	chomp(my $perm = $self->git->qx('config', 'core.sharedRepository'));
+	$perm;
+}
+
+sub _git_config_perm {
+	my $self = shift;
+	my $perm = scalar @_ ? $_[0] : _read_git_config_perm($self);
+	$perm //= '';
+	return PERM_UMASK if $perm eq '' || $perm eq 'umask';
+	return PERM_GROUP if $perm eq 'group';
+	return PERM_EVERYBODY if $perm =~ /\A(?:all|world|everybody)\z/;
+	return PERM_GROUP if ($perm =~ /\A(?:true|yes|on|1)\z/);
+	return PERM_UMASK if ($perm =~ /\A(?:false|no|off|0)\z/);
+
+	my $i = oct($perm);
+	return PERM_UMASK if $i == PERM_UMASK;
+	return PERM_GROUP if $i == OLD_PERM_GROUP;
+	return PERM_EVERYBODY if $i == OLD_PERM_EVERYBODY;
+
+	if (($i & 0600) != 0600) {
+		die "core.sharedRepository mode invalid: ".
+		    sprintf('%.3o', $i) . "\nOwner must have permissions\n";
+	}
+	($i & 0666);
+}
+
+sub _umask_for {
+	my ($perm) = @_; # _git_config_perm return value
+	my $rv = $perm;
+	return umask if $rv == 0;
+
+	# set +x bit if +r or +w were set
+	$rv |= 0100 if ($rv & 0600);
+	$rv |= 0010 if ($rv & 0060);
+	$rv |= 0001 if ($rv & 0006);
+	(~$rv & 0777);
+}
+
+sub with_umask {
+	my ($self, $cb, @arg) = @_;
+	my $old = umask($self->{umask} //= umask_prepare($self));
+	my $restore = PublicInbox::OnDestroy->new($$, \&CORE::umask, $old);
+	$cb ? $cb->(@arg) : $restore;
+}
+
+sub umask_prepare {
+	my ($self) = @_;
+	_umask_for(_git_config_perm($self));
+}
+
+1;
diff --git a/t/search.t b/t/search.t
index cf639a6d..ea5a2a5a 100644
--- a/t/search.t
+++ b/t/search.t
@@ -41,8 +41,9 @@ sub oct_is ($$$) {
 
 {
 	# git repository perms
+	use_ok 'PublicInbox::Umask';
 	oct_is($ibx->_git_config_perm(),
-		&PublicInbox::InboxWritable::PERM_GROUP,
+		PublicInbox::Umask::PERM_GROUP(),
 		'undefined permission is group');
 	my @t = (
 		[ '0644', 0022, '644 => umask(0022)' ],
@@ -54,8 +55,8 @@ sub oct_is ($$$) {
 	);
 	for (@t) {
 		my ($perm, $exp, $msg) = @$_;
-		my $got = PublicInbox::InboxWritable::_umask_for(
-			PublicInbox::InboxWritable->_git_config_perm($perm));
+		my $got = PublicInbox::Umask::_umask_for(
+			PublicInbox::Umask->_git_config_perm($perm));
 		oct_is($got, $exp, $msg);
 	}
 }