unofficial mirror of meta@public-inbox.org
 help / color / mirror / Atom feed
* [PATCH 0/6] path usability and some tiny cleanups
@ 2020-12-20  6:30 Eric Wong
  2020-12-20  6:30 ` [PATCH 1/6] script/public-inbox-*: favor caller-provided pathnames Eric Wong
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: Eric Wong @ 2020-12-20  6:30 UTC (permalink / raw)
  To: meta

1/6 makes some (IMHO) important usability improvements to
existing commands.  The rest are some yak-shaving while
I attempt to speed up config handling with thousands of
inboxes...

Eric Wong (6):
  script/public-inbox-*: favor caller-provided pathnames
  inboxidle: remove needless check for {inboxdir}
  daemon: lazy load Cwd only for --daemonize users
  daemon: unconditionally call IO::Handle::blocking(0)
  daemon: kill_workers: eliminate unnecessary loop
  config: eliminate unnecessary join call up front

 lib/PublicInbox/Admin.pm     | 102 +++++++++++++++++++++++------------
 lib/PublicInbox/Config.pm    |  14 +++--
 lib/PublicInbox/Daemon.pm    |  17 +++---
 lib/PublicInbox/InboxIdle.pm |   7 +--
 script/public-inbox-convert  |  27 +++-------
 script/public-inbox-init     |  10 +---
 t/admin.t                    |  16 +++---
 7 files changed, 100 insertions(+), 93 deletions(-)

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

* [PATCH 1/6] script/public-inbox-*: favor caller-provided pathnames
  2020-12-20  6:30 [PATCH 0/6] path usability and some tiny cleanups Eric Wong
@ 2020-12-20  6:30 ` Eric Wong
  2020-12-20  6:30 ` [PATCH 2/6] inboxidle: remove needless check for {inboxdir} Eric Wong
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Eric Wong @ 2020-12-20  6:30 UTC (permalink / raw)
  To: meta

We'll try to avoid calling Cwd::abs_path and use
File::Spec->rel2abs instead, since abs_path will resolve
symlinks the user specified on the command-line.

Unfortunately, ->rel2abs still leaves "/.." and "/../"
uncollapsed, so we still need to fall back to Cwd::abs_path in
those cases.

While we are at it, we'll also resolve inboxdir from deep inside
v2 directories instead of misdetecting them as v1 bare git
repos.

In any case, stop matching directories by name and instead rely
on the unique combination of st_dev + st_ino on stat() as we
started doing in the extindex code.
---
 lib/PublicInbox/Admin.pm    | 102 ++++++++++++++++++++++++------------
 script/public-inbox-convert |  27 +++-------
 script/public-inbox-init    |  10 +---
 t/admin.t                   |  16 +++---
 4 files changed, 87 insertions(+), 68 deletions(-)

diff --git a/lib/PublicInbox/Admin.pm b/lib/PublicInbox/Admin.pm
index 3977d812..ea82133a 100644
--- a/lib/PublicInbox/Admin.pm
+++ b/lib/PublicInbox/Admin.pm
@@ -6,15 +6,15 @@
 package PublicInbox::Admin;
 use strict;
 use parent qw(Exporter);
-use Cwd qw(abs_path);
-use POSIX ();
 our @EXPORT_OK = qw(setup_signals);
 use PublicInbox::Config;
 use PublicInbox::Inbox;
 use PublicInbox::Spawn qw(popen_rd);
+use File::Spec ();
 
 sub setup_signals {
 	my ($cb, $arg) = @_; # optional
+	require POSIX;
 
 	# we call exit() here instead of _exit() so DESTROY methods
 	# get called (e.g. File::Temp::Dir and PublicInbox::Msgmap)
@@ -27,21 +27,43 @@ sub setup_signals {
 	};
 }
 
+# abs_path resolves symlinks, so we want to avoid it if rel2abs
+# is sufficient and doesn't leave "/.." or "/../"
+sub rel2abs_collapsed ($) {
+	my $p = File::Spec->rel2abs($_[0]);
+	return $p if substr($p, -3, 3) ne '/..' && index($p, '/../') < 0; # likely
+	require Cwd;
+	Cwd::abs_path($p);
+}
+
 sub resolve_inboxdir {
 	my ($cd, $ver) = @_;
-	my $prefix = defined $cd ? $cd : './';
-	if (-d $prefix && -f "$prefix/inbox.lock") { # v2
-		$$ver = 2 if $ver;
-		return abs_path($prefix);
+	my $try = $cd // '.';
+	my $root_dev_ino;
+	while (1) { # favor v2, first
+		if (-f "$try/inbox.lock") {
+			$$ver = 2 if $ver;
+			return rel2abs_collapsed($try);
+		} elsif (-d $try) {
+			my @try = stat _;
+			$root_dev_ino //= do {
+				my @root = stat('/') or die "stat /: $!\n";
+				"$root[0]\0$root[1]";
+			};
+			last if "$try[0]\0$try[1]" eq $root_dev_ino;
+			$try .= '/..'; # continue, cd up
+		} else {
+			die "`$try' is not a directory\n";
+		}
 	}
+	# try v1 bare git dirs
 	my $cmd = [ qw(git rev-parse --git-dir) ];
 	my $fh = popen_rd($cmd, undef, {-C => $cd});
 	my $dir = do { local $/; <$fh> };
-	close $fh or die "error in ".join(' ', @$cmd)." (cwd:$cd): $!\n";
+	close $fh or die "error in @$cmd (cwd:${\($cd // '.')}): $!\n";
 	chomp $dir;
 	$$ver = 1 if $ver;
-	return abs_path($cd) if ($dir eq '.' && defined $cd);
-	abs_path($dir);
+	rel2abs_collapsed($dir eq '.' ? ($cd // $dir) : $dir);
 }
 
 # for unconfigured inboxes
@@ -78,8 +100,8 @@ sub unconfigured_ibx ($$) {
 		name => $name,
 		address => [ "$name\@example.com" ],
 		inboxdir => $dir,
-		# TODO: consumers may want to warn on this:
-		#-unconfigured => 1,
+		# consumers (-convert) warn on this:
+		-unconfigured => 1,
 	});
 }
 
@@ -95,41 +117,53 @@ sub resolve_inboxes ($;$$) {
 	}
 
 	my $min_ver = $opt->{-min_inbox_version} || 0;
+	# lookup inboxes by st_dev + st_ino instead of {inboxdir} pathnames,
+	# pathnames are not unique due to symlinks and bind mounts
 	my (@old, @ibxs);
-	my %dir2ibx;
-	my $all = $opt->{all} ? [] : undef;
-	if ($cfg) {
+	if ($opt->{all}) {
 		$cfg->each_inbox(sub {
 			my ($ibx) = @_;
-			my $path = abs_path($ibx->{inboxdir});
-			if (defined($path)) {
-				$dir2ibx{$path} = $ibx;
-				push @$all, $ibx if $all;
+			if (-e $ibx->{inboxdir}) {
+				push(@ibxs, $ibx) if $ibx->version >= $min_ver;
 			} else {
-				warn <<EOF;
-W: $ibx->{name} $ibx->{inboxdir}: $!
-EOF
+				warn "W: $ibx->{name} $ibx->{inboxdir}: $!\n";
 			}
 		});
-	}
-	if ($all) {
-		@$all = grep { $_->version >= $min_ver } @$all;
-		@ibxs = @$all;
 	} else { # directories specified on the command-line
-		my $i = 0;
 		my @dirs = @$argv;
 		push @dirs, '.' if !@dirs && $opt->{-use_cwd};
-		foreach (@dirs) {
-			my $v;
-			my $dir = resolve_inboxdir($_, \$v);
-			if ($v < $min_ver) {
+		my %s2i; # "st_dev\0st_ino" => array index
+		for (my $i = 0; $i <= $#dirs; $i++) {
+			my $dir = $dirs[$i];
+			my @st = stat($dir) or die "stat($dir): $!\n";
+			$dir = resolve_inboxdir($dir, \(my $ver));
+			if ($ver >= $min_ver) {
+				$s2i{"$st[0]\0$st[1]"} //= $i;
+			} else {
 				push @old, $dir;
-				next;
 			}
-			my $ibx = $dir2ibx{$dir} ||= unconfigured_ibx($dir, $i);
-			$i++;
-			push @ibxs, $ibx;
 		}
+		my $done = \'done';
+		eval {
+			$cfg->each_inbox(sub {
+				my ($ibx) = @_;
+				return if $ibx->version < $min_ver;
+				my $dir = $ibx->{inboxdir};
+				if (my @s = stat $dir) {
+					my $i = delete($s2i{"$s[0]\0$s[1]"})
+						// return;
+					$ibxs[$i] = $ibx;
+					die $done if !keys(%s2i);
+				} else {
+					warn "W: $ibx->{name} $dir: $!\n";
+				}
+			});
+		};
+		die $@ if $@ && $@ ne $done;
+		for my $i (sort { $a <=> $b } values %s2i) {
+			$ibxs[$i] = unconfigured_ibx($dirs[$i], $i);
+		}
+		@ibxs = grep { defined } @ibxs; # duplicates are undef
 	}
 	if (@old) {
 		die "-V$min_ver inboxes not supported by $0\n\t",
diff --git a/script/public-inbox-convert b/script/public-inbox-convert
index b61c743f..fbd527a6 100755
--- a/script/public-inbox-convert
+++ b/script/public-inbox-convert
@@ -47,34 +47,21 @@ die $help if (scalar(@ARGV) || $new_dir eq '' || $old_dir eq '');
 die "$new_dir exists\n" if -d $new_dir;
 die "$old_dir not a directory\n" unless -d $old_dir;
 
-require Cwd;
-Cwd->import('abs_path');
+require PublicInbox::Admin;
 require PublicInbox::Config;
 require PublicInbox::InboxWritable;
 
-my $abs = abs_path($old_dir);
-die "failed to resolve $old_dir: $!\n" if (!defined($abs));
-
 my $cfg = PublicInbox::Config->new;
-my $old;
-$cfg->each_inbox(sub {
-	$old = $_[0] if abs_path($_[0]->{inboxdir}) eq $old_dir;
-});
-if ($old) {
-	$old = PublicInbox::InboxWritable->new($old);
-} else {
+my @old = PublicInbox::Admin::resolve_inboxes([$old_dir], undef, $cfg);
+@old > 1 and die "BUG: resolved several inboxes from $old_dir:\n",
+		map { "\t$_->{inboxdir}\n" } @old;
+my $old = PublicInbox::InboxWritable->new($old[0]);
+if (delete $old->{-unconfigured}) {
 	warn "W: $old_dir not configured in " .
 		PublicInbox::Config::default_file() . "\n";
-	$old = PublicInbox::InboxWritable->new({
-		inboxdir => $old_dir,
-		name => 'ignored',
-		-primary_address => 'old@example.com',
-		address => [ 'old@example.com' ],
-	});
 }
 die "Only conversion from v1 inboxes is supported\n" if $old->version >= 2;
 
-require File::Spec;
 require PublicInbox::Admin;
 my $detected = PublicInbox::Admin::detect_indexlevel($old);
 $old->{indexlevel} //= $detected;
@@ -88,7 +75,7 @@ if ($opt->{'index'}) {
 }
 local %ENV = (%$env, %ENV) if $env;
 my $new = { %$old };
-$new->{inboxdir} = File::Spec->canonpath($new_dir);
+$new->{inboxdir} = PublicInbox::Admin::rel2abs_collapsed($new_dir);
 $new->{version} = 2;
 $new = PublicInbox::InboxWritable->new($new, { nproc => $opt->{jobs} });
 $new->{-no_fsync} = 1 if !$opt->{fsync};
diff --git a/script/public-inbox-init b/script/public-inbox-init
index c775eb31..eb605a51 100755
--- a/script/public-inbox-init
+++ b/script/public-inbox-init
@@ -138,10 +138,9 @@ close($fh) or die "failed to close $pi_config_tmp: $!\n";
 my $pfx = "publicinbox.$name";
 my @x = (qw/git config/, "--file=$pi_config_tmp");
 
-require File::Spec;
-$inboxdir = File::Spec->canonpath($inboxdir);
+PublicInbox::Admin::rel2abs_collapsed($inboxdir);
+die "`\\n' not allowed in `$inboxdir'\n" if index($inboxdir, "\n") >= 0;
 
-die "`\\n' not allowed in `$inboxdir'\n" if $inboxdir =~ /\n/s;
 if (-f "$inboxdir/inbox.lock") {
 	if (!defined $version) {
 		$version = 2;
@@ -186,11 +185,6 @@ if ($skip_docdata) {
 	$ibx->{-skip_docdata} = $skip_docdata;
 }
 $ibx->init_inbox(0, $skip_epoch, $skip_artnum);
-require Cwd;
-my $tmp = Cwd::abs_path($inboxdir);
-defined($tmp) or die "failed to resolve $inboxdir: $!\n";
-$inboxdir = $tmp;
-die "`\\n' not allowed in `$inboxdir'\n" if $inboxdir =~ /\n/s;
 
 # needed for git prior to v2.1.0
 umask(0077) if defined $perm;
diff --git a/t/admin.t b/t/admin.t
index af132577..60c6037d 100644
--- a/t/admin.t
+++ b/t/admin.t
@@ -12,10 +12,7 @@ my $v2_dir = "$tmpdir/v2";
 my ($res, $err, $v);
 
 PublicInbox::Import::init_bare($git_dir);
-*resolve_inboxdir = do {
-	no warnings 'once';
-	*PublicInbox::Admin::resolve_inboxdir;
-};
+*resolve_inboxdir = \&PublicInbox::Admin::resolve_inboxdir;
 
 # v1
 is(resolve_inboxdir($git_dir), $git_dir, 'top-level GIT_DIR resolved');
@@ -72,16 +69,23 @@ SKIP: {
 	ok(-e "$v2_dir/inbox.lock", 'exists');
 	is(resolve_inboxdir($v2_dir), $v2_dir,
 		'resolve_inboxdir works on v2_dir');
-	ok(chdir($v2_dir), 'chdir v2_dir OK');
+	chdir($v2_dir) or BAIL_OUT "chdir v2_dir: $!";
 	is(resolve_inboxdir(), $v2_dir, 'resolve_inboxdir works inside v2_dir');
 	$res = resolve_inboxdir(undef, \$v);
 	is($v, 2, 'version 2 detected');
 	is($res, $v2_dir, 'detects directory along with version');
 
 	# TODO: should work from inside Xapian dirs, and git dirs, here...
+	PublicInbox::Import::init_bare("$v2_dir/git/0.git");
+	my $objdir = "$v2_dir/git/0.git/objects";
+	is($v2_dir, resolve_inboxdir($objdir, \$v), 'at $objdir');
+	is($v, 2, 'version 2 detected at $objdir');
+	chdir($objdir) or BAIL_OUT "chdir objdir: $!";
+	is(resolve_inboxdir(undef, \$v), $v2_dir, 'inside $objdir');
+	is($v, 2, 'version 2 detected inside $objdir');
 }
 
-chdir '/';
+chdir '/' or BAIL_OUT "chdir: $!";
 
 my @pairs = (
 	'1g' => 1024 ** 3,

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

* [PATCH 2/6] inboxidle: remove needless check for {inboxdir}
  2020-12-20  6:30 [PATCH 0/6] path usability and some tiny cleanups Eric Wong
  2020-12-20  6:30 ` [PATCH 1/6] script/public-inbox-*: favor caller-provided pathnames Eric Wong
@ 2020-12-20  6:30 ` Eric Wong
  2020-12-20  6:30 ` [PATCH 3/6] daemon: lazy load Cwd only for --daemonize users Eric Wong
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Eric Wong @ 2020-12-20  6:30 UTC (permalink / raw)
  To: meta

->each_inbox will never attempt to iterate an object
without {inboxdir}, and simplify + short-circuit the
corresponding code
---
 lib/PublicInbox/Config.pm    | 7 +++----
 lib/PublicInbox/InboxIdle.pm | 7 +------
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/lib/PublicInbox/Config.pm b/lib/PublicInbox/Config.pm
index cafd9c3b..199ce019 100644
--- a/lib/PublicInbox/Config.pm
+++ b/lib/PublicInbox/Config.pm
@@ -391,9 +391,9 @@ EOF
 		}
 	}
 
-	# backwards compatibility:
-	$ibx->{inboxdir} //= $self->{"$pfx.mainrepo"};
-	if (($ibx->{inboxdir} // '') =~ /\n/s) {
+	# "mainrepo" is backwards compatibility:
+	$ibx->{inboxdir} //= $self->{"$pfx.mainrepo"} // return;
+	if ($ibx->{inboxdir} =~ /\n/s) {
 		warn "E: `$ibx->{inboxdir}' must not contain `\\n'\n";
 		return;
 	}
@@ -415,7 +415,6 @@ EOF
 		}
 	}
 
-	return unless defined($ibx->{inboxdir});
 	my $name = $pfx;
 	$name =~ s/\Apublicinbox\.//;
 
diff --git a/lib/PublicInbox/InboxIdle.pm b/lib/PublicInbox/InboxIdle.pm
index 2737bbbd..f1cbc012 100644
--- a/lib/PublicInbox/InboxIdle.pm
+++ b/lib/PublicInbox/InboxIdle.pm
@@ -7,7 +7,6 @@
 package PublicInbox::InboxIdle;
 use strict;
 use parent qw(PublicInbox::DS);
-use Cwd qw(abs_path);
 use PublicInbox::Syscall qw(EPOLLIN EPOLLET);
 my $IN_MODIFY = 0x02; # match Linux inotify
 my $ino_cls;
@@ -22,11 +21,7 @@ require PublicInbox::In2Tie if $ino_cls;
 
 sub in2_arm ($$) { # PublicInbox::Config::each_inbox callback
 	my ($ibx, $self) = @_;
-	my $dir = abs_path($ibx->{inboxdir});
-	if (!defined($dir)) {
-		warn "W: $ibx->{inboxdir} not watched: $!\n";
-		return;
-	}
+	my $dir = $ibx->{inboxdir};
 	my $inot = $self->{inot};
 	my $cur = $self->{pathmap}->{$dir} //= [];
 

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

* [PATCH 3/6] daemon: lazy load Cwd only for --daemonize users
  2020-12-20  6:30 [PATCH 0/6] path usability and some tiny cleanups Eric Wong
  2020-12-20  6:30 ` [PATCH 1/6] script/public-inbox-*: favor caller-provided pathnames Eric Wong
  2020-12-20  6:30 ` [PATCH 2/6] inboxidle: remove needless check for {inboxdir} Eric Wong
@ 2020-12-20  6:30 ` Eric Wong
  2020-12-20  6:30 ` [PATCH 4/6] daemon: unconditionally call IO::Handle::blocking(0) Eric Wong
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Eric Wong @ 2020-12-20  6:30 UTC (permalink / raw)
  To: meta

systemd users won't need it polluting the namespace; though
other things are still likely to load it.
---
 lib/PublicInbox/Daemon.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/Daemon.pm b/lib/PublicInbox/Daemon.pm
index a2171535..d1a42fc3 100644
--- a/lib/PublicInbox/Daemon.pm
+++ b/lib/PublicInbox/Daemon.pm
@@ -11,7 +11,6 @@ use IO::Socket;
 use POSIX qw(WNOHANG :signal_h);
 use Socket qw(IPPROTO_TCP SOL_SOCKET);
 sub SO_ACCEPTFILTER () { 0x1000 }
-use Cwd qw/abs_path/;
 STDOUT->autoflush(1);
 STDERR->autoflush(1);
 use PublicInbox::DS qw(now);
@@ -202,10 +201,11 @@ sub check_absolute ($$) {
 
 sub daemonize () {
 	if ($daemonize) {
+		require Cwd;
 		foreach my $i (0..$#ARGV) {
 			my $arg = $ARGV[$i];
 			next unless -e $arg;
-			$ARGV[$i] = abs_path($arg);
+			$ARGV[$i] = Cwd::abs_path($arg);
 		}
 		check_absolute('stdout', $stdout);
 		check_absolute('stderr', $stderr);

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

* [PATCH 4/6] daemon: unconditionally call IO::Handle::blocking(0)
  2020-12-20  6:30 [PATCH 0/6] path usability and some tiny cleanups Eric Wong
                   ` (2 preceding siblings ...)
  2020-12-20  6:30 ` [PATCH 3/6] daemon: lazy load Cwd only for --daemonize users Eric Wong
@ 2020-12-20  6:30 ` Eric Wong
  2020-12-20  6:30 ` [PATCH 5/6] daemon: kill_workers: eliminate unnecessary loop Eric Wong
  2020-12-20  6:30 ` [PATCH 6/6] config: eliminate unnecessary join call up front Eric Wong
  5 siblings, 0 replies; 7+ messages in thread
From: Eric Wong @ 2020-12-20  6:30 UTC (permalink / raw)
  To: meta

IO::Handle::blocking will always return the initial value from
the F_GETFL op and it won't issue F_SETFL if a socket is already
non-blocking.
---
 lib/PublicInbox/Daemon.pm | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/PublicInbox/Daemon.pm b/lib/PublicInbox/Daemon.pm
index d1a42fc3..eeac3bd2 100644
--- a/lib/PublicInbox/Daemon.pm
+++ b/lib/PublicInbox/Daemon.pm
@@ -367,14 +367,12 @@ sub inherit ($) {
 	foreach my $fd (3..$end) {
 		my $s = IO::Handle->new_from_fd($fd, 'r');
 		if (my $k = sockname($s)) {
-			if ($s->blocking) {
-				$s->blocking(0);
-				warn <<"";
+			my $prev_was_blocking = $s->blocking(0);
+			warn <<"" if $prev_was_blocking;
 Inherited socket (fd=$fd) is blocking, making it non-blocking.
 Set 'NonBlocking = true' in the systemd.service unit to avoid stalled
 processes when multiple service instances start.
 
-			}
 			$listener_names->{$k} = $s;
 			push @rv, $s;
 		} else {

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

* [PATCH 5/6] daemon: kill_workers: eliminate unnecessary loop
  2020-12-20  6:30 [PATCH 0/6] path usability and some tiny cleanups Eric Wong
                   ` (3 preceding siblings ...)
  2020-12-20  6:30 ` [PATCH 4/6] daemon: unconditionally call IO::Handle::blocking(0) Eric Wong
@ 2020-12-20  6:30 ` Eric Wong
  2020-12-20  6:30 ` [PATCH 6/6] config: eliminate unnecessary join call up front Eric Wong
  5 siblings, 0 replies; 7+ messages in thread
From: Eric Wong @ 2020-12-20  6:30 UTC (permalink / raw)
  To: meta

The `kill' perl op takes multiple PIDs, so there's no need to
iterate through the %pids hash.
---
 lib/PublicInbox/Daemon.pm | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/lib/PublicInbox/Daemon.pm b/lib/PublicInbox/Daemon.pm
index eeac3bd2..1762be0b 100644
--- a/lib/PublicInbox/Daemon.pm
+++ b/lib/PublicInbox/Daemon.pm
@@ -419,11 +419,8 @@ sub upgrade { # $_[0] = signal name or number (unused)
 }
 
 sub kill_workers ($) {
-	my ($s) = @_;
-
-	while (my ($pid, $id) = each %pids) {
-		kill $s, $pid;
-	}
+	my ($sig) = @_;
+	kill $sig, keys(%pids);
 }
 
 sub upgrade_aborted ($) {

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

* [PATCH 6/6] config: eliminate unnecessary join call up front
  2020-12-20  6:30 [PATCH 0/6] path usability and some tiny cleanups Eric Wong
                   ` (4 preceding siblings ...)
  2020-12-20  6:30 ` [PATCH 5/6] daemon: kill_workers: eliminate unnecessary loop Eric Wong
@ 2020-12-20  6:30 ` Eric Wong
  5 siblings, 0 replies; 7+ messages in thread
From: Eric Wong @ 2020-12-20  6:30 UTC (permalink / raw)
  To: meta

We can rely on implicit join in string interpolation on die()
iff needed.

And just creating the arrayref up front to avoid an extra
backslash seems nicer at the moment.
---
 lib/PublicInbox/Config.pm | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/lib/PublicInbox/Config.pm b/lib/PublicInbox/Config.pm
index 199ce019..2f5c83cd 100644
--- a/lib/PublicInbox/Config.pm
+++ b/lib/PublicInbox/Config.pm
@@ -163,11 +163,10 @@ sub config_fh_parse ($$$) {
 sub git_config_dump {
 	my ($file) = @_;
 	return {} unless -e $file;
-	my @cmd = (qw/git config -z -l --includes/, "--file=$file");
-	my $cmd = join(' ', @cmd);
-	my $fh = popen_rd(\@cmd);
+	my $cmd = [ qw(git config -z -l --includes), "--file=$file" ];
+	my $fh = popen_rd($cmd);
 	my $rv = config_fh_parse($fh, "\0", "\n");
-	close $fh or die "failed to close ($cmd) pipe: $?";
+	close $fh or die "failed to close (@$cmd) pipe: $?";
 	$rv;
 }
 

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

end of thread, other threads:[~2020-12-20  6:30 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-20  6:30 [PATCH 0/6] path usability and some tiny cleanups Eric Wong
2020-12-20  6:30 ` [PATCH 1/6] script/public-inbox-*: favor caller-provided pathnames Eric Wong
2020-12-20  6:30 ` [PATCH 2/6] inboxidle: remove needless check for {inboxdir} Eric Wong
2020-12-20  6:30 ` [PATCH 3/6] daemon: lazy load Cwd only for --daemonize users Eric Wong
2020-12-20  6:30 ` [PATCH 4/6] daemon: unconditionally call IO::Handle::blocking(0) Eric Wong
2020-12-20  6:30 ` [PATCH 5/6] daemon: kill_workers: eliminate unnecessary loop Eric Wong
2020-12-20  6:30 ` [PATCH 6/6] config: eliminate unnecessary join call up front 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).