From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: X-Spam-Status: No, score=-4.2 required=3.0 tests=ALL_TRUSTED,BAYES_00, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF shortcircuit=no autolearn=ham autolearn_force=no version=3.4.6 Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id 219EA1F568 for ; Sun, 24 Sep 2023 05:42:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=80x24.org; s=selector1; t=1695534135; bh=yP1q8vPBUlfbQFhHYfNV2IqJa3AL8m4TyKk89wJ9ZYQ=; h=From:To:Subject:Date:In-Reply-To:References:From; b=nStjlzkvclKEk5xawDVmw9bUvqN78Ua7h9/v4IsXahyl5fuTBtmDA3RykN9O1zNq7 fW+ckm4H28p8G2271fLDfZTH83/Tv1vM//CIyl3eFIcK+cfliEO37cgHqIb4z7rnAb Fvywxi6PWKg4ndMW2eXZ6vLa6ssraNxIidKPogvw= From: Eric Wong To: meta@public-inbox.org Subject: [PATCH 5/6] lei: fix `-c NAME=VALUE' config support Date: Sun, 24 Sep 2023 05:42:13 +0000 Message-ID: <20230924054214.3539091-6-e@80x24.org> In-Reply-To: <20230924054214.3539091-1-e@80x24.org> References: <20230924054214.3539091-1-e@80x24.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit List-Id: 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 = ... - 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 <{-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 - 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 +# Copyright (C) all contributors # License: AGPL-3.0+ -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 {