From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: X-Spam-Status: No, score=-4.0 required=3.0 tests=ALL_TRUSTED,BAYES_00 shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id 95FBC1F8C8; Sat, 11 Sep 2021 00:19:17 +0000 (UTC) From: Eric Wong To: meta@public-inbox.org Cc: Konstantin Ryabitsev Subject: [PATCH 1/3] lei: fix handling of broken lei.saved-search config files Date: Sat, 11 Sep 2021 00:19:15 +0000 Message-Id: <20210911001917.1310-2-e@80x24.org> In-Reply-To: <20210911001917.1310-1-e@80x24.org> References: <20210911001917.1310-1-e@80x24.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit List-Id: lei shouldn't become unusable if a config file is invalid. Instead, show the "git config" stderr and attempt to continue gracefully. Reported-by: Konstantin Ryabitsev Link: https://public-inbox.org/meta/20210910141157.6u5adehpx7wftkor@meerkat.local/ --- lib/PublicInbox/Config.pm | 6 +-- lib/PublicInbox/LEI.pm | 21 ++++++-- lib/PublicInbox/LeiEditSearch.pm | 79 +++++++++++++++++++++++++++++-- lib/PublicInbox/LeiSavedSearch.pm | 77 ++++++------------------------ script/lei | 2 + t/lei-q-save.t | 17 +++++++ 6 files changed, 128 insertions(+), 74 deletions(-) diff --git a/lib/PublicInbox/Config.pm b/lib/PublicInbox/Config.pm index b3e00ae0..74a1a6f5 100644 --- a/lib/PublicInbox/Config.pm +++ b/lib/PublicInbox/Config.pm @@ -164,12 +164,12 @@ sub config_fh_parse ($$$) { } sub git_config_dump { - my ($class, $file) = @_; + 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); + my $fh = popen_rd($cmd, undef, { 2 => $errfh // 2 }); my $rv = config_fh_parse($fh, "\0", "\n"); - close $fh or die "failed to close (@$cmd) pipe: $?"; + close $fh or die "@$cmd failed: \$?=$?\n"; bless $rv, $class; } diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm index bbb6ab7e..0b4c99dc 100644 --- a/lib/PublicInbox/LEI.pm +++ b/lib/PublicInbox/LEI.pm @@ -1029,13 +1029,15 @@ sub path_to_fd { # caller needs to "-t $self->{1}" to check if tty sub start_pager { - my ($self) = @_; + my ($self, $new_env) = @_; my $fh = popen_rd([qw(git var GIT_PAGER)]); chomp(my $pager = <$fh> // ''); close($fh) or warn "`git var PAGER' error: \$?=$?"; return if $pager eq 'cat' || $pager eq ''; - my $new_env = { LESS => 'FRX', LV => '-c' }; - $new_env->{MORE} = 'FRX' if $^O eq 'freebsd'; + $new_env //= {}; + $new_env->{LESS} //= 'FRX'; + $new_env->{LV} //= '-c'; + $new_env->{MORE} = $new_env->{LESS} if $^O eq 'freebsd'; pipe(my ($r, $wpager)) or return warn "pipe: $!"; my $rdr = { 0 => $r, 1 => $self->{1}, 2 => $self->{2} }; my $pgr = [ undef, @$rdr{1, 2} ]; @@ -1052,6 +1054,19 @@ sub start_pager { $self->{pgr} = $pgr; } +# display a message for user before spawning full-screen $VISUAL +sub pgr_err { + my ($self, @msg) = @_; + return $self->err(@msg) unless $self->{sock} && -t $self->{2}; + start_pager($self, { LESS => 'RX' }); # no 'F' so we prompt + print { $self->{2} } @msg; + $self->{2}->autoflush(1); + my $pgr = delete($self->{pgr}) or return; + $self->{2} = $pgr->[2]; + $self->{1} = $pgr->[1]; + send($self->{sock}, 'wait', MSG_EOR); # wait for user to quit pager +} + sub stop_pager { my ($self) = @_; my $pgr = delete($self->{pgr}) or return; diff --git a/lib/PublicInbox/LeiEditSearch.pm b/lib/PublicInbox/LeiEditSearch.pm index 82dfbf63..47166ce7 100644 --- a/lib/PublicInbox/LeiEditSearch.pm +++ b/lib/PublicInbox/LeiEditSearch.pm @@ -8,20 +8,89 @@ use v5.10.1; use PublicInbox::LeiSavedSearch; use PublicInbox::LeiUp; -sub lei_edit_search { - my ($lei, $out) = @_; - my $lss = PublicInbox::LeiSavedSearch->up($lei, $out) or return; +sub edit_begin { + my ($lss, $lei) = @_; + if (ref($lss->{-cfg}->{'lei.q.output'})) { + delete $lss->{-cfg}->{'lei.q.output'}; # invalid + $lei->pgr_err(<{-f} has multiple values of lei.q.output +please remove redundant ones +EOM + } + $lei->{-lss_for_edit} = $lss; +} + +sub do_edit ($$;$) { + my ($lss, $lei, $reason) = @_; + $lei->pgr_err($reason) if defined $reason; my @cmd = (qw(git config --edit -f), $lss->{'-f'}); $lei->qerr("# spawning @cmd"); - $lss->edit_begin($lei); + edit_begin($lss, $lei); # run in script/lei foreground require PublicInbox::PktOp; my ($op_c, $op_p) = PublicInbox::PktOp->pair; # $op_p will EOF when $EDITOR is done - $op_c->{ops} = { '' => [$lss->can('edit_done'), $lss, $lei] }; + $op_c->{ops} = { '' => [\&op_edit_done, $lss, $lei] }; $lei->send_exec_cmd([ @$lei{qw(0 1 2)}, $op_p->{op_p} ], \@cmd, {}); } +sub _edit_done { + my ($lss, $lei) = @_; + my $cfg = $lss->can('cfg_dump')->($lei, $lss->{'-f'}) // + return do_edit($lss, $lei, <{-f} is unparseable +EOM + my $new_out = $cfg->{'lei.q.output'} // ''; + return do_edit($lss, $lei, <{-f} has multiple values of lei.q.output +EOM + return do_edit($lss, $lei, <{-f} needs lei.q.output +EOM + my $old_out = $lss->{-cfg}->{'lei.q.output'} // return; + return if $old_out eq $new_out; + my $old_path = $old_out; + my $new_path = $new_out; + s!$PublicInbox::LeiSavedSearch::LOCAL_PFX!! for ($old_path, $new_path); + my $dir_old = $lss->can('lss_dir_for')->($lei, \$old_path, 1); + my $dir_new = $lss->can('lss_dir_for')->($lei, \$new_path); + return if $dir_new eq $dir_old; + + ($old_out =~ m!\Av2:!i || $new_out =~ m!\Av2:!) and + return do_edit($lss, $lei, <puts("lei.q.output changed from $old_sq to $new_sq"); + $lei->qerr("# lei convert $old_sq -o $new_sq"); + my $v = !$lei->{opt}->{quiet}; + $lei->{opt} = { output => $new_out, verbose => $v }; + require PublicInbox::LeiConvert; + PublicInbox::LeiConvert::lei_convert($lei, $old_out); + + $lei->fail(<fail($@) if $@; +} + +sub lei_edit_search { + my ($lei, $out) = @_; + my $lss = PublicInbox::LeiSavedSearch->up($lei, $out) or return; + do_edit($lss, $lei); +} + *_complete_edit_search = \&PublicInbox::LeiUp::_complete_up; 1; diff --git a/lib/PublicInbox/LeiSavedSearch.pm b/lib/PublicInbox/LeiSavedSearch.pm index fd51fe38..8ceaaf01 100644 --- a/lib/PublicInbox/LeiSavedSearch.pm +++ b/lib/PublicInbox/LeiSavedSearch.pm @@ -14,7 +14,7 @@ use PublicInbox::Spawn qw(run_die); use PublicInbox::ContentHash qw(git_sha); use PublicInbox::MID qw(mids_for_index); use Digest::SHA qw(sha256_hex); -my $LOCAL_PFX = qr!\A(?:maildir|mh|mbox.+|mmdf|v2):!i; # TODO: put in LeiToMail? +our $LOCAL_PFX = qr!\A(?:maildir|mh|mbox.+|mmdf|v2):!i; # TODO: put in LeiToMail? # move this to PublicInbox::Config if other things use it: my %cquote = ("\n" => '\\n', "\t" => '\\t', "\b" => '\\b'); @@ -29,6 +29,14 @@ sub BOOL_FIELDS () { qw(external local remote import-remote import-before threads) } +sub cfg_dump ($$) { + my ($lei, $f) = @_; + my $ret = eval { PublicInbox::Config->git_config_dump($f, $lei->{2}) }; + return $ret if !$@; + $lei->err($@); + undef; +} + sub lss_dir_for ($$;$) { my ($lei, $dstref, $on_fs) = @_; my @n; @@ -56,7 +64,7 @@ sub lss_dir_for ($$;$) { for my $g ("$n[0]-*", '*') { my @maybe = glob("$lss_dir$g/lei.saved-search"); for my $f (@maybe) { - $c = PublicInbox::Config->git_config_dump($f); + $c = cfg_dump($lei, $f) // next; $o = $c->{'lei.q.output'} // next; $o =~ s!$LOCAL_PFX!! or next; @st = stat($o) or next; @@ -80,9 +88,9 @@ sub list { print $fh "\tpath = ", cquote_val($p), "\n"; } close $fh or die "close $f: $!"; - my $cfg = PublicInbox::Config->git_config_dump($f); + my $cfg = cfg_dump($lei, $f); unlink($f); - my $out = $cfg->get_all('lei.q.output') or return (); + my $out = $cfg ? $cfg->get_all('lei.q.output') : []; map {; s!$LOCAL_PFX!!; $_; @@ -105,7 +113,7 @@ sub up { # updating existing saved search via "lei up" output2lssdir($self, $lei, \$dir, \$f) or return $lei->fail("--save was not used with $dst cwd=". $lei->rel2abs('.')); - $self->{-cfg} = PublicInbox::Config->git_config_dump($f); + $self->{-cfg} = cfg_dump($lei, $f) // return $lei->fail; $self->{-ovf} = "$dir/over.sqlite3"; $self->{'-f'} = $f; $self->{lock_path} = "$self->{-f}.flock"; @@ -267,7 +275,7 @@ sub output2lssdir { my $dir = lss_dir_for($lei, \$dst, 1); my $f = "$dir/lei.saved-search"; if (-f $f && -r _) { - $self->{-cfg} = PublicInbox::Config->git_config_dump($f); + $self->{-cfg} = cfg_dump($lei, $f) // return; $$dir_ref = $dir; $$fn_ref = $f; return 1; @@ -275,63 +283,6 @@ sub output2lssdir { undef; } -sub edit_begin { - my ($self, $lei) = @_; - if (ref($self->{-cfg}->{'lei.q.output'})) { - delete $self->{-cfg}->{'lei.q.output'}; # invalid - $lei->err(<{-f} has multiple values of lei.q.output -please remove redundant ones -EOM - } - $lei->{-lss_for_edit} = $self; -} - -sub edit_done { - my ($self, $lei) = @_; - my $cfg = PublicInbox::Config->git_config_dump($self->{'-f'}); - my $new_out = $cfg->{'lei.q.output'} // ''; - return $lei->fail(<{-f} has multiple values of lei.q.output -please edit again -EOM - return $lei->fail(<{-f} needs lei.q.output -please edit again -EOM - my $old_out = $self->{-cfg}->{'lei.q.output'} // ''; - return if $old_out eq $new_out; - my $old_path = $old_out; - my $new_path = $new_out; - s!$LOCAL_PFX!! for ($old_path, $new_path); - my $dir_old = lss_dir_for($lei, \$old_path, 1); - my $dir_new = lss_dir_for($lei, \$new_path); - return if $dir_new eq $dir_old; # no change, likely - - ($old_out =~ m!\Av2:!i || $new_out =~ m!\Av2:!) and - return $lei->fail(<fail(<puts("lei.q.output changed from $old_sq to $new_sq"); - $lei->qerr("# lei convert $old_sq -o $new_sq"); - my $v = !$lei->{opt}->{quiet}; - $lei->{opt} = { output => $new_out, verbose => $v }; - require PublicInbox::LeiConvert; - PublicInbox::LeiConvert::lei_convert($lei, $old_out); - - $lei->fail(<has_entries sub has_entries { my $oidx = $_[0]->{oidx} // die 'BUG: no {oidx}'; diff --git a/script/lei b/script/lei index 99d94b4e..2d84487a 100755 --- a/script/lei +++ b/script/lei @@ -127,6 +127,8 @@ while (1) { last; } elsif ($buf =~ /\Achild_error ([0-9]+)\z/) { $x_it_code ||= $1 + 0; + } elsif ($buf eq 'wait') { + $sigchld->(); } else { $sigchld->(); die $buf; diff --git a/t/lei-q-save.t b/t/lei-q-save.t index 743a7b70..9c17a011 100644 --- a/t/lei-q-save.t +++ b/t/lei-q-save.t @@ -215,5 +215,22 @@ test_lei(sub { 'absolute path appears in ls-search'; lei_ok qw(up ../s -C), "$home/v2s", \'relative lei up'; lei_ok qw(up), "$home/s", \'absolute lei up'; + + # mess up a config file + my @lss = glob("$home/" . + '.local/share/lei/saved-searches/*/lei.saved-search'); + my $out = xqx([qw(git config -f), $lss[0], 'lei.q.output']); + xsys($^X, qw(-i -p -e), "s/\\[/\\0/", $lss[0]) + and xbail "-ipe $lss[0]: $?"; + lei_ok qw(ls-search); + like($lei_err, qr/bad config line.*?\Q$lss[0]\E/, + 'git config parse error shown w/ lei ls-search'); + lei_ok qw(up --all), \'up works with bad config'; + like($lei_err, qr/bad config line.*?\Q$lss[0]\E/, + 'git config parse error shown w/ lei up'); + xsys($^X, qw(-i -p -e), "s/\\0/\\[/", $lss[0]) + and xbail "-ipe $lss[0]: $?"; + lei_ok qw(ls-search); + is($lei_err, '', 'no errors w/ fixed config'); }); done_testing;