[PATCH 5/6] lei: fix `-c NAME=VALUE' config support

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

We can pass `-c NAME=VALUE' args directly to git-config without
needing a temporary directory nor file.  Furthermore, this opens
the door to us being able to correctly handle `-c NAME=VALUE'
after `delete $lei->{cfg}' if we need to reload the config
during a command.

This tightens up error-checking for `lei config' and ensures we
can make config settings changes while using `-c NAME=VALUE'
instead of editing the temporary file.

The non-obvious part was avoiding the use of the -f/--file arg for
`git config' for read-only operations and include relying on
`-c include.path=$ABS_PATH'.  This is done by parsing the
switches to be passed to `git config' to determine if it's a
read-only operation or not.
---
 lib/PublicInbox/Config.pm    | 52 +++++++++++++++-----
 lib/PublicInbox/LEI.pm       | 95 +++++++++++++++++++-----------------
 lib/PublicInbox/LeiConfig.pm | 23 ++++++---
 lib/PublicInbox/LeiMirror.pm |  5 +-
 t/lei.t                      | 17 ++++++-
 5 files changed, 123 insertions(+), 69 deletions(-)

diff --git a/lib/PublicInbox/Config.pm b/lib/PublicInbox/Config.pm
index f6236d84..533f4a52 100644
--- a/lib/PublicInbox/Config.pm
+++ b/lib/PublicInbox/Config.pm
@@ -22,7 +22,7 @@ sub _array ($) { ref($_[0]) eq 'ARRAY' ? $_[0] : [ $_[0] ] }
 # returns key-value pairs of config directives in a hash
 # if keys may be multi-value, the value is an array ref containing all values
 sub new {
-	my ($class, $file, $errfh) = @_;
+	my ($class, $file, $lei) = @_;
 	$file //= default_file();
 	my $self;
 	my $set_dedupe;
@@ -36,7 +36,7 @@ sub new {
 			$self = $DEDUPE->{$file} and return $self;
 			$set_dedupe = 1;
 		}
-		$self = git_config_dump($class, $file, $errfh);
+		$self = git_config_dump($class, $file, $lei);
 		$self->{'-f'} = $file;
 	}
 	# caches
@@ -174,13 +174,34 @@ sub config_fh_parse ($$$) {
 	\%rv;
 }
 
+sub tmp_cmd_opt ($$) {
+	my ($env, $opt) = @_;
+	# quiet global and system gitconfig if supported by installed git,
+	# but normally harmless if too noisy (NOGLOBAL no longer exists)
+	$env->{GIT_CONFIG_NOSYSTEM} = 1;
+	$env->{GIT_CONFIG_GLOBAL} = '/dev/null'; # git v2.32+
+	$opt->{-C} = '/'; # avoid $worktree/.git/config on MOST systems :P
+}
+
 sub git_config_dump {
-	my ($class, $file, $errfh) = @_;
-	return bless {}, $class unless -e $file;
-	my $cmd = [ qw(git config -z -l --includes), "--file=$file" ];
-	my $fh = popen_rd($cmd, undef, { 2 => $errfh // 2 });
+	my ($class, $file, $lei) = @_;
+	my @opt_c = map { ('-c', $_) } @{$lei->{opt}->{c} // []};
+	$file = undef if !-e $file;
+	# XXX should we set {-f} if !-e $file?
+	return bless {}, $class if (!@opt_c && !defined($file));
+	my %env;
+	my $opt = { 2 => $lei->{2} // 2 };
+	if (@opt_c) {
+		unshift(@opt_c, '-c', "include.path=$file") if defined($file);
+		tmp_cmd_opt(\%env, $opt);
+	}
+	my @cmd = ('git', @opt_c, qw(config -z -l --includes));
+	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";
+	close $fh or die "@cmd failed: \$?=$?\n";
+	$rv->{-opt_c} = \@opt_c if @opt_c; # for ->urlmatch
+	$rv->{-f} = $file;
 	bless $rv, $class;
 }
 
@@ -544,14 +565,23 @@ sub _fill_ei ($$) {
 	$es;
 }
 
+sub config_cmd {
+	my ($self, $env, $opt) = @_;
+	my $f = $self->{-f} // default_file();
+	my @opt_c = @{$self->{-opt_c} // []};
+	my @cmd = ('git', @opt_c, 'config');
+	@opt_c ? tmp_cmd_opt($env, $opt) : push(@cmd, '-f', $f);
+	\@cmd;
+}
+
 sub urlmatch {
 	my ($self, $key, $url, $try_git) = @_;
 	state $urlmatch_broken; # requires git 1.8.5
 	return if $urlmatch_broken;
-	my $file = $self->{'-f'} // default_file();
-	my $cmd = [qw/git config -z --includes --get-urlmatch/,
-		"--file=$file", $key, $url ];
-	my $fh = popen_rd($cmd);
+	my (%env, %opt);
+	my $cmd = $self->config_cmd(\%env, \%opt);
+	push @$cmd, qw(-z --includes --get-urlmatch), $key, $url;
+	my $fh = popen_rd($cmd, \%env, \%opt);
 	local $/ = "\0";
 	my $val = <$fh>;
 	if (!close($fh)) {
diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm
index 488006e0..8b62def2 100644
--- a/lib/PublicInbox/LEI.pm
+++ b/lib/PublicInbox/LEI.pm
@@ -281,7 +281,8 @@ our %CMD = ( # sorted in order of importance/use:
 	"use a patch to generate a query for `lei q --stdin'",
 	qw(stdin| in-format|F=s want|w=s@ uri debug), @net_opt, @c_opt ],
 'config' => [ '[...]', sub {
-		'git-config(1) wrapper for '._config_path($_[0]);
+		'git-config(1) wrapper for '._config_path($_[0]). "\n" .
+	'-l/--list and other common git-config uses are supported'
 	}, qw(config-file|system|global|file|f=s), # for conflict detection
 	 qw(edit|e c=s@ C=s@), pass_through('git config') ],
 'inspect' => [ 'ITEMS...|--stdin', 'inspect lei/store and/or local external',
@@ -456,6 +457,7 @@ my %OPTDESC = (
 'z|0' => 'use NUL \\0 instead of newline (CR) to delimit lines',
 
 'signal|s=s' => [ 'SIG', 'signal to send lei-daemon (default: TERM)' ],
+'edit|e	config' => 'open an editor to modify the lei config file',
 ); # %OPTDESC
 
 my %CONFIG_KEYS = (
@@ -760,36 +762,6 @@ sub optparse ($$$) {
 	$err ? fail($self, "usage: lei $cmd $proto\nE: $err") : 1;
 }
 
-sub _tmp_cfg { # for lei -c <name>=<value> ...
-	my ($self) = @_;
-	my $cfg = _lei_cfg($self, 1);
-	require File::Temp;
-	my $ft = File::Temp->new(TEMPLATE => 'lei_cfg-XXXX', TMPDIR => 1);
-	my $tmp = { '-f' => $ft->filename, -tmp => $ft };
-	$ft->autoflush(1);
-	print $ft <<EOM or return fail($self, "$tmp->{-f}: $!");
-[include]
-	path = $cfg->{-f}
-EOM
-	$tmp = $self->{cfg} = bless { %$cfg, %$tmp }, ref($cfg);
-	for (@{$self->{opt}->{c}}) {
-		/\A([^=\.]+\.[^=]+)(?:=(.*))?\z/ or return fail($self, <<EOM);
-`-c $_' is not of the form -c <name>=<value>'
-EOM
-		my ($name, $value) = ($1, $2 // 1);
-		_config($self, '--add', $name, $value) or return;
-		if (defined(my $v = $tmp->{$name})) {
-			if (ref($v) eq 'ARRAY') {
-				push @$v, $value;
-			} else {
-				$tmp->{$name} = [ $v, $value ];
-			}
-		} else {
-			$tmp->{$name} = $value;
-		}
-	}
-}
-
 sub lazy_cb ($$$) { # $pfx is _complete_ or lei_
 	my ($self, $cmd, $pfx) = @_;
 	my $ucmd = $cmd;
@@ -819,7 +791,6 @@ sub dispatch {
 	}
 	if (my $cb = lazy_cb(__PACKAGE__, $cmd, 'lei_')) {
 		optparse($self, $cmd, \@argv) or return;
-		$self->{opt}->{c} and (_tmp_cfg($self) // return);
 		if (my $chdir = $self->{opt}->{C}) {
 			for my $d (@$chdir) {
 				next if $d eq ''; # same as git(1)
@@ -844,17 +815,20 @@ sub _lei_cfg ($;$) {
 	my $f = _config_path($self);
 	my @st = stat($f);
 	my $cur_st = @st ? pack('dd', $st[10], $st[7]) : ''; # 10:ctime, 7:size
-	my ($sto, $sto_dir, $watches, $lne);
-	if (my $cfg = $PATH2CFG{$f}) { # reuse existing object in common case
-		return ($self->{cfg} = $cfg) if $cur_st eq $cfg->{-st};
+	my ($sto, $sto_dir, $watches, $lne, $cfg);
+	if ($cfg = $PATH2CFG{$f}) { # reuse existing object in common case
+		($cur_st eq $cfg->{-st} && !$self->{opt}->{c}) and
+			return ($self->{cfg} = $cfg);
+		# reuse some fields below if they match:
 		($sto, $sto_dir, $watches, $lne) =
 				@$cfg{qw(-lei_store leistore.dir -watches
 					-lei_note_event)};
 	}
 	if (!@st) {
-		unless ($creat) {
-			delete $self->{cfg};
-			return bless {}, 'PublicInbox::Config';
+		unless ($creat) { # any commands which write to cfg must creat
+			$cfg = PublicInbox::Config->git_config_dump(
+							'/dev/null', $self);
+			return ($self->{cfg} = $cfg);
 		}
 		my ($cfg_dir) = ($f =~ m!(.*?/)[^/]+\z!);
 		File::Path::mkpath($cfg_dir);
@@ -863,9 +837,8 @@ sub _lei_cfg ($;$) {
 		$cur_st = pack('dd', $st[10], $st[7]);
 		qerr($self, "# $f created") if $self->{cmd} ne 'config';
 	}
-	my $cfg = PublicInbox::Config->git_config_dump($f, $self->{2});
+	$cfg = PublicInbox::Config->git_config_dump($f, $self);
 	$cfg->{-st} = $cur_st;
-	$cfg->{'-f'} = $f;
 	if ($sto && canonpath_harder($sto_dir // store_path($self))
 			eq canonpath_harder($cfg->{'leistore.dir'} //
 						store_path($self))) {
@@ -877,7 +850,7 @@ sub _lei_cfg ($;$) {
 		# FIXME: use inotify/EVFILT_VNODE to detect unlinked configs
 		delete(@PATH2CFG{grep(!-f, keys %PATH2CFG)});
 	}
-	$self->{cfg} = $PATH2CFG{$f} = $cfg;
+	$self->{cfg} = $self->{opt}->{c} ? $cfg : ($PATH2CFG{$f} = $cfg);
 	refresh_watches($self);
 	$cfg;
 }
@@ -898,11 +871,41 @@ sub _lei_store ($;$) {
 sub _config {
 	my ($self, @argv) = @_;
 	my $err_ok = ($argv[0] // '') eq '+e' ? shift(@argv) : undef;
-	my %env = (%{$self->{env}}, GIT_CONFIG => undef);
+	my %env;
+	my %opt = map { $_ => $self->{$_} } (0..2);
 	my $cfg = _lei_cfg($self, 1);
-	my $cmd = [ qw(git config -f), $cfg->{'-f'}, @argv ];
-	my %rdr = map { $_ => $self->{$_} } (0..2);
-	waitpid(spawn($cmd, \%env, \%rdr), 0);
+	my $opt_c = delete local $cfg->{-opt_c};
+	my @file_arg;
+	if ($opt_c) {
+		my ($set, $get, $nondash);
+		for (@argv) { # order matters for git-config
+			if (!$nondash) {
+				if (/\A--(?:add|rename-section|remove-section|
+						replace-all|
+						unset-all|unset)\z/x) {
+					++$set;
+				} elsif ($_ eq '-l' || $_ eq '--list' ||
+						/\A--get/) {
+					++$get;
+				} elsif (/\A-/) { # -z and such
+				} else {
+					++$nondash;
+				}
+			} else {
+				++$nondash;
+			}
+		}
+		if ($set || ($nondash//0) > 1 && !$get) {
+			@file_arg = ('-f', $cfg->{-f});
+			$env{GIT_CONFIG} = $file_arg[1];
+		} else { # OK, we can use `-c n=v' for read-only
+			$cfg->{-opt_c} = $opt_c;
+			$env{GIT_CONFIG} = undef;
+		}
+	}
+	my $cmd = $cfg->config_cmd(\%env, \%opt);
+	push @$cmd, @file_arg, @argv;
+	waitpid(spawn($cmd, \%env, \%opt), 0);
 	$? == 0 ? 1 : ($err_ok ? undef : fail($self, $?));
 }
 
@@ -1545,7 +1548,7 @@ sub sto_done_request {
 
 sub cfg_dump ($$) {
 	my ($lei, $f) = @_;
-	my $ret = eval { PublicInbox::Config->git_config_dump($f, $lei->{2}) };
+	my $ret = eval { PublicInbox::Config->git_config_dump($f, $lei) };
 	return $ret if !$@;
 	warn($@);
 	undef;
diff --git a/lib/PublicInbox/LeiConfig.pm b/lib/PublicInbox/LeiConfig.pm
index fd4b0eca..76fc43e7 100644
--- a/lib/PublicInbox/LeiConfig.pm
+++ b/lib/PublicInbox/LeiConfig.pm
@@ -1,8 +1,7 @@
-# Copyright (C) 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>
-package PublicInbox::LeiConfig;
-use strict;
-use v5.10.1;
+package PublicInbox::LeiConfig; # subclassed by LeiEditSearch
+use v5.12;
 use PublicInbox::PktOp;
 use Fcntl qw(SEEK_SET);
 use autodie qw(open seek);
@@ -41,10 +40,18 @@ sub lei_config {
 	my ($lei, @argv) = @_;
 	$lei->{opt}->{'config-file'} and return $lei->fail(
 		"config file switches not supported by `lei config'");
-	return $lei->_config(@argv) unless $lei->{opt}->{edit};
-	my $f = $lei->_lei_cfg(1)->{-f};
-	my $self = bless { lei => $lei, -f => $f }, __PACKAGE__;
-	cfg_do_edit($self);
+	if ($lei->{opt}->{edit}) {
+		@argv and return $lei->fail(
+'--edit must be used without other arguments');
+		$lei->{opt}->{c} and return $lei->fail(
+"`-c $lei->{opt}->{c}->[0]' not allowed with --edit");
+		my $f = $lei->_lei_cfg(1)->{-f};
+		cfg_do_edit(bless { lei => $lei, -f => $f }, __PACKAGE__);
+	} elsif (@argv) { # let git-config do error-checking
+		$lei->_config(@argv);
+	} else {
+		$lei->_help('no options given');
+	}
 }
 
 1;
diff --git a/lib/PublicInbox/LeiMirror.pm b/lib/PublicInbox/LeiMirror.pm
index bed034f1..fed6b668 100644
--- a/lib/PublicInbox/LeiMirror.pm
+++ b/lib/PublicInbox/LeiMirror.pm
@@ -193,7 +193,8 @@ sub _write_inbox_config {
 	} elsif (!$!{EEXIST}) {
 		die "open($f): $!";
 	}
-	my $cfg = PublicInbox::Config->git_config_dump($f, $self->{lei}->{2});
+	my $cfg = PublicInbox::Config->git_config_dump($f,
+						{ 2 => $self->{lei}->{2} });
 	my $ibx = $self->{ibx} = {}; # for indexing
 	for my $sec (grep(/\Apublicinbox\./, @{$cfg->{-section_order}})) {
 		for (qw(address newsgroup nntpmirror)) {
@@ -238,7 +239,7 @@ sub index_cloned_inbox {
 		}
 		# force synchronous awaitpid for v2:
 		local $PublicInbox::DS::in_loop = 0;
-		my $cfg = PublicInbox::Config->new(undef, $lei->{2});
+		my $cfg = PublicInbox::Config->new(undef, { 2 => $lei->{2} });
 		my $env = PublicInbox::Admin::index_prepare($opt, $cfg);
 		local %ENV = (%ENV, %$env) if $env;
 		PublicInbox::Admin::progress_prepare($opt, $lei->{2});
diff --git a/t/lei.t b/t/lei.t
index 1199ca75..3ac804a8 100644
--- a/t/lei.t
+++ b/t/lei.t
@@ -40,10 +40,21 @@ my $test_help = sub {
 	lei_ok(qw(config -h));
 	like($lei_out, qr! \Q$home\E/\.config/lei/config\b!,
 		'actual path shown in config -h');
+	my $exp_help = qr/\Q$lei_out\E/s;
+	ok(!lei('config'), 'config w/o args fails');
+	like($lei_err, $exp_help, 'config w/o args shows our help in stderr');
 	lei_ok(qw(config -h), { XDG_CONFIG_HOME => '/XDC' },
 		\'config with XDG_CONFIG_HOME');
 	like($lei_out, qr! /XDC/lei/config\b!, 'XDG_CONFIG_HOME in config -h');
 	is($lei_err, '', 'no errors from config -h');
+
+	lei_ok(qw(-c foo.bar config dash.c works));
+	lei_ok(qw(config dash.c));
+	is($lei_out, "works\n", 'config set w/ -c');
+
+	lei_ok(qw(-c foo.bar config --add dash.c add-works));
+	lei_ok(qw(config --get-all dash.c));
+	is($lei_out, "works\nadd-works\n", 'config --add w/ -c');
 };
 
 my $ok_err_info = sub {
@@ -101,9 +112,11 @@ my $test_config = sub {
 	is($lei_out, "tr00\n", "-c string value passed as-is");
 	lei_ok(qw(-c imap.debug=a -c imap.debug=b config --get-all imap.debug));
 	is($lei_out, "a\nb\n", '-c and --get-all work together');
-
-	lei_ok([qw(config -e)], { VISUAL => 'cat', EDITOR => 'cat' });
+	my $env = { VISUAL => 'cat', EDITOR => 'cat' };
+	lei_ok([qw(config -e)], $env);
 	is($lei_out, "[a]\n\tb = c\n", '--edit works');
+	ok(!lei([qw(-c a.b=c config -e)], $env), '-c conflicts with -e');
+	like($lei_err, qr/not allowed/, 'error message shown');
 };
 
 my $test_completion = sub {