[PATCH 0/4] v2 permissions

[PATCH 0/4] v2 permissions

[Eric Wong (Contractor, The Linux Foundation)]

Eric Wong (Contractor, The Linux Foundation) (4):
      convert: avoid redundant "done\n" statement for fast-import
      search: move permissions handling to InboxWritable
      t/v2writable: use simplify permissions reading
      v2: respect core.sharedRepository in git configs

 lib/PublicInbox/InboxWritable.pm |  68 ++++++++++++++++++++++++
 lib/PublicInbox/SearchIdx.pm     |  80 +++-------------------------
 lib/PublicInbox/V2Writable.pm    |  26 +++++----
 script/public-inbox-compact      | 110 ++++++++++++++++++++++-----------------
 script/public-inbox-convert      |  26 ++++++---
 t/convert-compact.t              |  43 +++++++++++++++
 t/search.t                       |  28 +++++-----
 t/v2writable.t                   |   8 +--
 8 files changed, 231 insertions(+), 158 deletions(-)

[PATCH 1/4] convert: avoid redundant "done\n" statement for fast-import

[Eric Wong (Contractor, The Linux Foundation)]

This bug was hidden due to timing problems with eatmydata or
running with tmpfs for TMPDIR.
---
 script/public-inbox-convert | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/script/public-inbox-convert b/script/public-inbox-convert
index 2b0a385..e6fb4f5 100755
--- a/script/public-inbox-convert
+++ b/script/public-inbox-convert
@@ -91,9 +91,8 @@ while (<$rd>) {
 			$from = $1;
 			# no next
 		}
-	} elsif ($_ eq "done\n") {
-		last;
 	}
+	last if $_ eq "done\n";
 	$w->print($_) or $im->wfail;
 }
 $w = $r = undef;
-- 
EW

[PATCH 2/4] search: move permissions handling to InboxWritable

[Eric Wong (Contractor, The Linux Foundation)]

We'll be making sure V2Writable uses this.
---
 lib/PublicInbox/InboxWritable.pm | 71 ++++++++++++++++++++++++++++
 lib/PublicInbox/SearchIdx.pm     | 80 ++++----------------------------
 t/search.t                       | 28 +++++------
 3 files changed, 93 insertions(+), 86 deletions(-)

diff --git a/lib/PublicInbox/InboxWritable.pm b/lib/PublicInbox/InboxWritable.pm
index 82834f0..92b9016 100644
--- a/lib/PublicInbox/InboxWritable.pm
+++ b/lib/PublicInbox/InboxWritable.pm
@@ -10,6 +10,14 @@ use PublicInbox::Import;
 use PublicInbox::Filter::Base;
 *REJECT = *PublicInbox::Filter::Base::REJECT;
 
+use constant {
+	PERM_UMASK => 0,
+	OLD_PERM_GROUP => 1,
+	OLD_PERM_EVERYBODY => 2,
+	PERM_GROUP => 0660,
+	PERM_EVERYBODY => 0664,
+};
+
 sub new {
 	my ($class, $ibx) = @_;
 	bless $ibx, $class;
@@ -157,4 +165,67 @@ sub import_mbox {
 	$im->done;
 }
 
+sub _read_git_config_perm {
+	my ($self) = @_;
+	my @cmd = qw(config);
+	if (($self->{version} ||= 1) == 2) {
+		push @cmd, "--file=$self->{mainrepo}/all.git/config";
+	}
+	chomp(my $perm = $self->git->qx(@cmd, 'core.sharedRepository'));
+	$perm;
+}
+
+sub _git_config_perm {
+	my $self = shift;
+	my $perm = scalar @_ ? $_[0] : _read_git_config_perm($self);
+	return PERM_GROUP 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) = @_;
+	my $old = umask $self->{umask};
+	my $rv = eval { $cb->() };
+	my $err = $@;
+	umask $old;
+	die $err if $err;
+	$rv;
+}
+
+sub umask_prepare {
+	my ($self) = @_;
+	my $perm = _git_config_perm($self);
+	my $umask = _umask_for($perm);
+	$self->{umask} = $umask;
+}
+
 1;
diff --git a/lib/PublicInbox/SearchIdx.pm b/lib/PublicInbox/SearchIdx.pm
index a234c8c..9638e0c 100644
--- a/lib/PublicInbox/SearchIdx.pm
+++ b/lib/PublicInbox/SearchIdx.pm
@@ -11,6 +11,7 @@ use strict;
 use warnings;
 use base qw(PublicInbox::Search PublicInbox::Lock);
 use PublicInbox::MIME;
+use PublicInbox::InboxWritable;
 use PublicInbox::MID qw/mid_clean id_compress mid_mime mids references/;
 use PublicInbox::MsgIter;
 use Carp qw(croak);
@@ -18,11 +19,6 @@ use POSIX qw(strftime);
 require PublicInbox::Git;
 
 use constant {
-	PERM_UMASK => 0,
-	OLD_PERM_GROUP => 1,
-	OLD_PERM_EVERYBODY => 2,
-	PERM_GROUP => 0660,
-	PERM_EVERYBODY => 0664,
 	BATCH_BYTES => 1_000_000,
 	DEBUG => !!$ENV{DEBUG},
 };
@@ -62,20 +58,19 @@ sub new {
 				PublicInbox::AltId->new($ibx, $_);
 			} @$altid ];
 		}
-		$git = $ibx->git;
-	} else {
-		$git = PublicInbox::Git->new($git_dir); # v1 only
+	} else { # v1
+		$ibx = { mainrepo => $git_dir, version => 1 };
 	}
+	$ibx = PublicInbox::InboxWritable->new($ibx);
 	require Search::Xapian::WritableDatabase;
 	my $self = bless {
 		mainrepo => $mainrepo,
-		git => $git,
+		-inbox => $ibx,
+		git => $ibx->git,
 		-altid => $altid,
 		version => $version,
 	}, $class;
-	my $perm = $self->_git_config_perm;
-	my $umask = _umask_for($perm);
-	$self->{umask} = $umask;
+	$ibx->umask_prepare;
 	if ($version == 1) {
 		$self->{lock_path} = "$mainrepo/ssoma.lock";
 	} elsif ($version == 2) {
@@ -648,7 +643,7 @@ sub do_cat_mail {
 
 sub index_sync {
 	my ($self, $opts) = @_;
-	with_umask($self, sub { $self->_index_sync($opts) });
+	$self->{-inbox}->with_umask(sub { $self->_index_sync($opts) })
 }
 
 sub batch_adjust ($$$$) {
@@ -846,65 +841,6 @@ sub merge_threads {
 	});
 }
 
-sub _read_git_config_perm {
-	my ($self) = @_;
-	my @cmd = qw(config);
-	if ($self->{version} == 2) {
-		push @cmd, "--file=$self->{mainrepo}/all.git/config";
-	}
-	my $fh = $self->{git}->popen(@cmd, 'core.sharedRepository');
-	local $/ = "\n";
-	my $perm = <$fh>;
-	chomp $perm if defined $perm;
-	$perm;
-}
-
-sub _git_config_perm {
-	my $self = shift;
-	my $perm = scalar @_ ? $_[0] : _read_git_config_perm($self);
-	return PERM_GROUP 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) = @_;
-	my $old = umask $self->{umask};
-	my $rv = eval { $cb->() };
-	my $err = $@;
-	umask $old;
-	die $err if $err;
-	$rv;
-}
-
 sub DESTROY {
 	# order matters for unlocking
 	$_[0]->{xdb} = undef;
diff --git a/t/search.t b/t/search.t
index ccf0f74..9ab15f7 100644
--- a/t/search.t
+++ b/t/search.t
@@ -18,6 +18,7 @@ ok($@, "exception raised on non-existent DB");
 my $rw = PublicInbox::SearchIdx->new($git_dir, 1);
 $rw->_xdb_acquire;
 $rw->_xdb_release;
+my $ibx = $rw->{-inbox};
 $rw = undef;
 my $ro = PublicInbox::Search->new($git_dir);
 my $rw_commit = sub {
@@ -28,26 +29,25 @@ my $rw_commit = sub {
 
 {
 	# git repository perms
-	is(PublicInbox::SearchIdx->_git_config_perm(undef),
-	   &PublicInbox::SearchIdx::PERM_GROUP,
+	is($ibx->_git_config_perm(), &PublicInbox::InboxWritable::PERM_GROUP,
 	   "undefined permission is group");
-	is(PublicInbox::SearchIdx::_umask_for(
-	     PublicInbox::SearchIdx->_git_config_perm('0644')),
+	is(PublicInbox::InboxWritable::_umask_for(
+	     PublicInbox::InboxWritable->_git_config_perm('0644')),
 	   0022, "644 => umask(0022)");
-	is(PublicInbox::SearchIdx::_umask_for(
-	     PublicInbox::SearchIdx->_git_config_perm('0600')),
+	is(PublicInbox::InboxWritable::_umask_for(
+	     PublicInbox::InboxWritable->_git_config_perm('0600')),
 	   0077, "600 => umask(0077)");
-	is(PublicInbox::SearchIdx::_umask_for(
-	     PublicInbox::SearchIdx->_git_config_perm('0640')),
+	is(PublicInbox::InboxWritable::_umask_for(
+	     PublicInbox::InboxWritable->_git_config_perm('0640')),
 	   0027, "640 => umask(0027)");
-	is(PublicInbox::SearchIdx::_umask_for(
-	     PublicInbox::SearchIdx->_git_config_perm('group')),
+	is(PublicInbox::InboxWritable::_umask_for(
+	     PublicInbox::InboxWritable->_git_config_perm('group')),
 	   0007, 'group => umask(0007)');
-	is(PublicInbox::SearchIdx::_umask_for(
-	     PublicInbox::SearchIdx->_git_config_perm('everybody')),
+	is(PublicInbox::InboxWritable::_umask_for(
+	     PublicInbox::InboxWritable->_git_config_perm('everybody')),
 	   0002, 'everybody => umask(0002)');
-	is(PublicInbox::SearchIdx::_umask_for(
-	     PublicInbox::SearchIdx->_git_config_perm('umask')),
+	is(PublicInbox::InboxWritable::_umask_for(
+	     PublicInbox::InboxWritable->_git_config_perm('umask')),
 	   umask, 'umask => existing umask');
 }
 
-- 
EW

[PATCH 3/4] t/v2writable: use simplify permissions reading

[Eric Wong (Contractor, The Linux Foundation)]

We have Git::qx nowadays.
---
 t/v2writable.t | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/t/v2writable.t b/t/v2writable.t
index 0eda432..7abb14f 100644
--- a/t/v2writable.t
+++ b/t/v2writable.t
@@ -43,13 +43,9 @@ if ('ensure git configs are correct') {
 		qw(core.sharedRepository 0644));
 	is(system(@cmd), 0, "set sharedRepository in all.git");
 	$git0 = PublicInbox::Git->new("$mainrepo/git/0.git");
-	my $fh = $git0->popen(qw(config core.sharedRepository));
-	my $v = eval { local $/; <$fh> };
-	chomp $v;
+	chomp(my $v = $git0->qx(qw(config core.sharedRepository)));
 	is($v, '0644', 'child repo inherited core.sharedRepository');
-	$fh = $git0->popen(qw(config --bool repack.writeBitmaps));
-	$v = eval { local $/; <$fh> };
-	chomp $v;
+	chomp($v = $git0->qx(qw(config --bool repack.writeBitmaps)));
 	is($v, 'true', 'child repo inherited repack.writeBitmaps');
 }
 
-- 
EW

[PATCH 4/4] v2: respect core.sharedRepository in git configs

[Eric Wong (Contractor, The Linux Foundation)]

Ensure -convert and -compact do not make repositories
unreadable on live servers.
---
 lib/PublicInbox/InboxWritable.pm |   5 +-
 lib/PublicInbox/V2Writable.pm    |  26 ++++----
 script/public-inbox-compact      | 110 +++++++++++++++++--------------
 script/public-inbox-convert      |  23 +++++--
 t/convert-compact.t              |  43 ++++++++++++
 5 files changed, 139 insertions(+), 68 deletions(-)

diff --git a/lib/PublicInbox/InboxWritable.pm b/lib/PublicInbox/InboxWritable.pm
index 92b9016..5c11a36 100644
--- a/lib/PublicInbox/InboxWritable.pm
+++ b/lib/PublicInbox/InboxWritable.pm
@@ -168,10 +168,7 @@ sub import_mbox {
 sub _read_git_config_perm {
 	my ($self) = @_;
 	my @cmd = qw(config);
-	if (($self->{version} ||= 1) == 2) {
-		push @cmd, "--file=$self->{mainrepo}/all.git/config";
-	}
-	chomp(my $perm = $self->git->qx(@cmd, 'core.sharedRepository'));
+	chomp(my $perm = $self->git->qx('config', 'core.sharedRepository'));
 	$perm;
 }
 
diff --git a/lib/PublicInbox/V2Writable.pm b/lib/PublicInbox/V2Writable.pm
index 34f13e2..c4368cc 100644
--- a/lib/PublicInbox/V2Writable.pm
+++ b/lib/PublicInbox/V2Writable.pm
@@ -50,6 +50,7 @@ sub new {
 	}
 	$nparts = nproc() if ($nparts == 0);
 
+	$v2ibx = PublicInbox::InboxWritable->new($v2ibx);
 	my $self = {
 		-inbox => $v2ibx,
 		im => undef, #  PublicInbox::Import
@@ -193,20 +194,23 @@ sub idx_init {
 	# frequently activated.
 	delete $ibx->{$_} foreach (qw(git mm search));
 
-	$self->lock_acquire;
+	$ibx->umask_prepare;
+	$ibx->with_umask(sub {
+		$self->lock_acquire;
 
-	# first time initialization, first we create the skeleton pipe:
-	my $skel = $self->{skel} = PublicInbox::SearchIdxSkeleton->new($self);
+		# first time initialization, first we create the skeleton pipe:
+		my $skel = PublicInbox::SearchIdxSkeleton->new($self);
+		$self->{skel} = $skel;
 
-	# need to create all parts before initializing msgmap FD
-	my $max = $self->{partitions} - 1;
-	my $idx = $self->{idx_parts} = [];
-	for my $i (0..$max) {
-		push @$idx, PublicInbox::SearchIdxPart->new($self, $i, $skel);
-	}
+		# need to create all parts before initializing msgmap FD
+		my $max = $self->{partitions} - 1;
+		@{$self->{idx_parts}} = map {
+			PublicInbox::SearchIdxPart->new($self, $_, $skel);
+		} (0..$max);
 
-	# Now that all subprocesses are up, we can open the FD for SQLite:
-	$skel->_msgmap_init->{dbh}->begin_work;
+		# Now that all subprocesses are up, we can open the FD for SQLite:
+		$skel->_msgmap_init->{dbh}->begin_work;
+	});
 }
 
 sub purge_oids {
diff --git a/script/public-inbox-compact b/script/public-inbox-compact
index 016873d..79cd039 100755
--- a/script/public-inbox-compact
+++ b/script/public-inbox-compact
@@ -15,6 +15,7 @@ my $usage = "Usage: public-inbox-compact REPO_DIR\n";
 my $dir = shift or die $usage;
 my $config = PublicInbox::Config->new;
 my $ibx;
+$dir = abs_path($dir);
 $config->each_inbox(sub {
 	$ibx = $_[0] if abs_path($_[0]->{mainrepo}) eq $dir
 });
@@ -29,66 +30,79 @@ unless ($ibx) {
 	$ibx = PublicInbox::Inbox->new($ibx);
 }
 my $v = ($ibx->{version} || 1);
+$ibx = PublicInbox::InboxWritable->new($ibx);
+$ibx->umask_prepare;
+
+sub commit_changes ($$$) {
+	my ($im, $old, $new) = @_;
+	my @st = stat($old) or die "failed to stat($old): $!\n";
+	rename($old, "$new/old") or die "rename $old => $new/old: $!\n";
+	chmod($st[2] & 07777, $new) or die "chmod $old: $!\n";
+	rename($new, $old) or die "rename $new => $old: $!\n";
+	$im->lock_release;
+	remove_tree("$old/old") or die "failed to remove $old/old: $!\n";
+}
+
 if ($v == 2) {
 	require PublicInbox::V2Writable;
 	my $v2w = PublicInbox::V2Writable->new($ibx);
 	my $xap_v = 'xap'.PublicInbox::Search::SCHEMA_VERSION;
-	my $xroot = "$ibx->{mainrepo}/$xap_v";
-	opendir my $dh, $xroot or die "Failed to opendir $xroot: $!\n";
-	$v2w->lock_acquire;
-	my $new = tempdir(CLEANUP => 1, DIR => $ibx->{mainrepo});
-	my @parts;
-	my $skel;
-	while (defined(my $dn = readdir($dh))) {
-		if ($dn =~ /\A\d+\z/) {
-			push @parts, "$xroot/$dn";
-		} elsif ($dn eq 'skel') {
-			$skel = "$xroot/$dn";
-		} elsif ($dn eq '.' || $dn eq '..') {
+	my $old = "$dir/$xap_v";
+	opendir my $dh, $old or die "Failed to opendir $old: $!\n";
+	my $new = tempdir('compact-XXXXXXXX', CLEANUP => 1, DIR => $dir);
+	$ibx->with_umask(sub {
+		$v2w->lock_acquire;
+		my @parts;
+		my $skel;
+		while (defined(my $dn = readdir($dh))) {
+			if ($dn =~ /\A\d+\z/) {
+				push @parts, "$old/$dn";
+			} elsif ($dn eq 'skel') {
+				$skel = "$old/$dn";
+			} elsif ($dn eq '.' || $dn eq '..') {
+			} else {
+				warn "W: skipping unknown Xapian DB: $old/$dn\n"
+			}
+		}
+		close $dh;
+		my %pids;
+
+		if (@parts) {
+			my $pid = spawn(['xapian-compact', @parts, "$new/0" ]);
+			defined $pid or die "compact failed: $?\n";
+			$pids{$pid} = 'xapian-compact (parts)';
 		} else {
-			warn "W: skipping unknown Xapian DB: $xroot/$dn\n";
+			warn "No parts found in $old\n";
 		}
-	}
-	close $dh;
-	my %pids;
-	if (@parts) {
-		my $pid = spawn([ qw(xapian-compact), @parts, "$new/0" ]);
-		defined $pid or die "compact failed: $?\n";
-		$pids{$pid} = 'xapian-compact (parts)';
-	} else {
-		warn "No parts found in $xroot\n";
-	}
-	if (defined $skel) {
-		my $pid = spawn([ qw(xapian-compact), $skel, "$new/skel" ]);
-		defined $pid or die "compact failed: $?\n";
-		$pids{$pid} = 'xapian-compact (skel)';
-	} else {
-		warn "$xroot/skel missing\n";
-	}
-	die "No xapian-compact processes running\n" unless scalar keys %pids;
-	while (scalar keys %pids) {
-		my $pid = waitpid(-1, 0);
-		my $desc = delete $pids{$pid};
-		die "$desc failed: $?\n" if $?;
-	}
-	rename($xroot, "$new/old") or die "rename $xroot => $new/old: $!\n";
-	rename($new, $xroot) or die "rename $new => $xroot: $!\n";
-	$v2w->lock_release;
-	remove_tree("$xroot/old") or die "failed to remove $xroot/old: $!\n";
+		if (defined $skel) {
+			my $pid = spawn(['xapian-compact', $skel, "$new/skel"]);
+			defined $pid or die "compact failed: $?\n";
+			$pids{$pid} = 'xapian-compact (skel)';
+		} else {
+			warn "$old/skel missing\n";
+		}
+		scalar keys %pids or
+			die "No xapian-compact processes running\n";
+		while (scalar keys %pids) {
+			my $pid = waitpid(-1, 0);
+			my $desc = delete $pids{$pid};
+			die "$desc failed: $?\n" if $?;
+		}
+		commit_changes($v2w, $old, $new);
+	});
 } elsif ($v == 1) {
 	require PublicInbox::Import;
 	my $im = PublicInbox::Import->new($ibx->git, undef, undef, $ibx);
 	my $xap_v = 'xapian'.PublicInbox::Search::SCHEMA_VERSION;
-	my $v1_root = "$ibx->{mainrepo}/public-inbox";
+	my $v1_root = "$dir/public-inbox";
 	my $old = "$v1_root/$xap_v";
 	-d $old or die "$old does not exist\n";
-	my $new = tempdir(CLEANUP => 1, DIR => $v1_root);
-	$im->lock_acquire;
-	PublicInbox::Import::run_die([ qw(xapian-compact), $old, $new ]);
-	rename($old, "$new/old") or die "rename $old => $new: $!\n";
-	rename($new, $old) or die "rename $new => $old: $!\n";
-	$im->lock_release;
-	remove_tree("$old/old") or die "failed to remove $old/old: $!\n";
+	my $new = tempdir('compact-XXXXXXXX', CLEANUP => 1, DIR => $v1_root);
+	$ibx->with_umask(sub {
+		$im->lock_acquire;
+		PublicInbox::Import::run_die(['xapian-compact', $old, $new]);
+		commit_changes($im, $old, $new);
+	});
 } else {
 	die "Unsupported inbox version: $v\n";
 }
diff --git a/script/public-inbox-convert b/script/public-inbox-convert
index e6fb4f5..f58bf27 100755
--- a/script/public-inbox-convert
+++ b/script/public-inbox-convert
@@ -5,9 +5,10 @@ use strict;
 use warnings;
 use Getopt::Long qw(:config gnu_getopt no_ignore_case auto_abbrev);
 use PublicInbox::MIME;
-use PublicInbox::Inbox;
+use PublicInbox::InboxWritable;
 use PublicInbox::Config;
 use PublicInbox::V2Writable;
+use PublicInbox::Import;
 use PublicInbox::Spawn qw(spawn);
 use Cwd 'abs_path';
 my $usage = "Usage: public-inbox-convert OLD NEW\n";
@@ -39,16 +40,28 @@ unless ($old) {
 	};
 	$old = PublicInbox::Inbox->new($old);
 }
+$old = PublicInbox::InboxWritable->new($old);
 if (($old->{version} || 1) >= 2) {
 	die "Only conversion from v1 inboxes is supported\n";
 }
 my $new = { %$old };
 delete $new->{altid}; # TODO: support altid for v2
-$new->{mainrepo} = $new_dir;
+$new->{mainrepo} = abs_path($new_dir);
 $new->{version} = 2;
-$new = PublicInbox::Inbox->new($new);
-my $v2w = PublicInbox::V2Writable->new($new, 1);
-$v2w->init_inbox($jobs);
+$new = PublicInbox::InboxWritable->new($new);
+my $v2w;
+$old->umask_prepare;
+$old->with_umask(sub {
+	local $ENV{GIT_CONFIG} = "$old->{mainrepo}/config";
+	$v2w = PublicInbox::V2Writable->new($new, 1);
+	$v2w->init_inbox($jobs);
+	chomp(my $sr = $old->git->qx('config', 'core.sharedRepository'));
+	if ($sr ne '') {
+		PublicInbox::Import::run_die(['git', 'config',
+			"--file=$new->{mainrepo}/all.git/config",
+			'core.sharedRepository', $sr]);
+	}
+});
 my $state = '';
 my ($prev, $from);
 my $head = $old->{ref_head} || 'HEAD';
diff --git a/t/convert-compact.t b/t/convert-compact.t
index 922ec9c..e51eadc 100644
--- a/t/convert-compact.t
+++ b/t/convert-compact.t
@@ -21,6 +21,9 @@ my $ibx = {
 
 ok(PublicInbox::Import::run_die([qw(git init --bare -q), $ibx->{mainrepo}]),
 	'initialized v1 repo');
+ok(umask(077), 'set restrictive umask');
+ok(PublicInbox::Import::run_die([qw(git) , "--git-dir=$ibx->{mainrepo}",
+	qw(config core.sharedRepository 0644)]), 'set sharedRepository');
 $ibx = PublicInbox::Inbox->new($ibx);
 my $im = PublicInbox::Import->new($ibx->git, undef, undef, $ibx);
 my $mime = PublicInbox::MIME->create(
@@ -36,6 +39,18 @@ my $mime = PublicInbox::MIME->create(
 ok($im->add($mime), 'added one message');
 $im->done;
 PublicInbox::SearchIdx->new($ibx, 1)->index_sync;
+
+is(((stat("$ibx->{mainrepo}/public-inbox"))[2]) & 07777, 0755,
+	'sharedRepository respected for v1');
+is(((stat("$ibx->{mainrepo}/public-inbox/msgmap.sqlite3"))[2]) & 07777, 0644,
+	'sharedRepository respected for v1 msgmap');
+my @xdir = glob("$ibx->{mainrepo}/public-inbox/xap*/*");
+foreach (@xdir) {
+	my @st = stat($_);
+	is($st[2] & 07777, -f _ ? 0644 : 0755,
+		'sharedRepository respected on file after convert');
+}
+
 local $ENV{PATH} = "blib/script:$ENV{PATH}";
 open my $err, '>>', "$tmpdir/err.log" or die "open: err.log $!\n";
 open my $out, '>>', "$tmpdir/out.log" or die "open: out.log $!\n";
@@ -44,8 +59,19 @@ my $rdr = { 1 => fileno($out), 2 => fileno($err) };
 my $cmd = [ 'public-inbox-compact', $ibx->{mainrepo} ];
 ok(PublicInbox::Import::run_die($cmd, undef, $rdr), 'v1 compact works');
 
+@xdir = glob("$ibx->{mainrepo}/public-inbox/xap*");
+is(scalar(@xdir), 1, 'got one xapian directory after compact');
+is(((stat($xdir[0]))[2]) & 07777, 0755,
+	'sharedRepository respected on v1 compact');
+
 $cmd = [ 'public-inbox-convert', $ibx->{mainrepo}, "$tmpdir/v2" ];
 ok(PublicInbox::Import::run_die($cmd, undef, $rdr), 'convert works');
+@xdir = glob("$tmpdir/v2/xap*/*");
+foreach (@xdir) {
+	my @st = stat($_);
+	is($st[2] & 07777, -f _ ? 0644 : 0755,
+		'sharedRepository respected after convert');
+}
 
 $cmd = [ 'public-inbox-compact', "$tmpdir/v2" ];
 my $env = { NPROC => 2 };
@@ -54,4 +80,21 @@ $ibx->{mainrepo} = "$tmpdir/v2";
 my $v2w = PublicInbox::V2Writable->new($ibx);
 is($v2w->{partitions}, 1, "only one partition in compacted repo");
 
+@xdir = glob("$tmpdir/v2/xap*/*");
+foreach (@xdir) {
+	my @st = stat($_);
+	is($st[2] & 07777, -f _ ? 0644 : 0755,
+		'sharedRepository respected after v2 compact');
+}
+is(((stat("$tmpdir/v2/msgmap.sqlite3"))[2]) & 07777, 0644,
+	'sharedRepository respected for v2 msgmap');
+
+@xdir = (glob("$tmpdir/v2/git/*.git/objects/*/*"),
+	 glob("$tmpdir/v2/git/*.git/objects/pack/*"));
+foreach (@xdir) {
+	my @st = stat($_);
+	is($st[2] & 07777, -f _ ? 0444 : 0755,
+		'sharedRepository respected after v2 compact');
+}
+
 done_testing();
-- 
EW