]> Sergey Matveev's repositories - public-inbox.git/commitdiff
lei: fix handling of broken lei.saved-search config files
authorEric Wong <e@80x24.org>
Sat, 11 Sep 2021 00:19:15 +0000 (00:19 +0000)
committerEric Wong <e@80x24.org>
Sat, 11 Sep 2021 00:24:14 +0000 (00:24 +0000)
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 <konstantin@linuxfoundation.org>
Link: https://public-inbox.org/meta/20210910141157.6u5adehpx7wftkor@meerkat.local/
lib/PublicInbox/Config.pm
lib/PublicInbox/LEI.pm
lib/PublicInbox/LeiEditSearch.pm
lib/PublicInbox/LeiSavedSearch.pm
script/lei
t/lei-q-save.t

index b3e00ae07a06f6e65ea8a4ca9d4d4efdd7875618..74a1a6f5a776fd388bed0b9a16b53263f914e03e 100644 (file)
@@ -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;
 }
 
index bbb6ab7e439c0f6332fba41d1115b7af53f70833..0b4c99dc45dc2cd159e5bdd22847a53a2fad8ff1 100644 (file)
@@ -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;
index 82dfbf63c411356138f27b611986747b0e006fe3..47166ce724837013d3b13286326fae75a2c863e2 100644 (file)
@@ -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(<<EOM);
+$lss->{-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, <<EOM);
+$lss->{-f} is unparseable
+EOM
+       my $new_out = $cfg->{'lei.q.output'} // '';
+       return do_edit($lss, $lei, <<EOM) if ref $new_out;
+$lss->{-f} has multiple values of lei.q.output
+EOM
+       return do_edit($lss, $lei, <<EOM) if $new_out eq '';
+$lss->{-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, <<EOM);
+conversions from/to v2 inboxes not supported at this time
+EOM
+       return do_edit($lss, $lei, <<EOM) if -e $dir_new;
+lei.q.output changed from `$old_out' to `$new_out'
+However, $dir_new exists
+EOM
+       # start the conversion asynchronously
+       my $old_sq = PublicInbox::Config::squote_maybe($old_out);
+       my $new_sq = PublicInbox::Config::squote_maybe($new_out);
+       $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(<<EOM) if -e $dir_old && !rename($dir_old, $dir_new);
+E: rename($dir_old, $dir_new) error: $!
+EOM
+}
+
+sub op_edit_done { # PktOp
+       my ($lss, $lei) = @_;
+       eval { _edit_done($lss, $lei) };
+       $lei->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;
index fd51fe38681612c6f9f07a7aec888a3700e1d160..8ceaaf018ec628a6cba2132304304797ce733f4f 100644 (file)
@@ -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(<<EOM);
-$self->{-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(<<EOM) if ref $new_out;
-$self->{-f} has multiple values of lei.q.output
-please edit again
-EOM
-       return $lei->fail(<<EOM) if $new_out eq '';
-$self->{-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(<<EOM);
-conversions from/to v2 inboxes not supported at this time
-EOM
-
-       return $lei->fail(<<EOM) if -e $dir_new;
-lei.q.output changed from `$old_out' to `$new_out'
-However, $dir_new exists
-EOM
-       # start the conversion asynchronously
-       my $old_sq = PublicInbox::Config::squote_maybe($old_out);
-       my $new_sq = PublicInbox::Config::squote_maybe($new_out);
-       $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(<<EOM) if -e $dir_old && !rename($dir_old, $dir_new);
-E: rename($dir_old, $dir_new) error: $!
-EOM
-}
-
 # cf. LeiDedupe->has_entries
 sub has_entries {
        my $oidx = $_[0]->{oidx} // die 'BUG: no {oidx}';
index 99d94b4e6618f18a84d57826676d6b6afd1caba9..2d84487a89dc5d27acd2771339b4525463198b30 100755 (executable)
@@ -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;
index 743a7b70e5185a0ea0d88fe83d125bb2bd9a4138..9c17a0110f456198756a1830dae9630bd1db9b36 100644 (file)
@@ -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;