view: distinguish strict and loose thread matches

The "loose" (Subject:-based) thread matching yields too many
hits for some common subjects (e.g. "[GIT] Networking" on LKML)
and causes thread skeletons to not show the current messages.
Favor strict matches in the query and only add loose matches
if there's space.

While working on this, I noticed the backwards --reindex walk
breaks `tid' on v1 repositories, at least.  That bug was hidden
by the Subject: match logic and not discovered until now.  It
will be fixed separately.

Reported-by: Konstantin Ryabitsev <konstantin@linuxfoundation.org>

diff --git a/lib/PublicInbox/Over.pm b/lib/PublicInbox/Over.pm
index b2f68835a42738cce7613c6b544e6d93a9b0123d..dda1e6d008ca4eb08439681f94fc09ce77d65c8c 100644 (file)
--- a/lib/PublicInbox/Over.pm
+++ b/lib/PublicInbox/Over.pm
@@ -11,6 +11,7 @@ use DBI;
 use DBD::SQLite;
 use PublicInbox::SearchMsg;
 use Compress::Zlib qw(uncompress);
+use constant DEFAULT_LIMIT => 1000;
 
 sub dbh_new {
        my ($self) = @_;
@@ -53,7 +54,7 @@ sub load_from_row {
 sub do_get {
        my ($self, $sql, $opts, @args) = @_;
        my $dbh = $self->connect;
-       my $lim = (($opts->{limit} || 0) + 0) || 1000;
+       my $lim = (($opts->{limit} || 0) + 0) || DEFAULT_LIMIT;
        $sql .= "LIMIT $lim";
        my $msgs = $dbh->selectall_arrayref($sql, { Slice => {} }, @args);
        load_from_row($_) for @$msgs;
@@ -97,21 +98,47 @@ ORDER BY num ASC LIMIT 1
 SELECT tid,sid FROM over WHERE num = ? LIMIT 1
 
        defined $tid or return nothing; # $sid may be undef
+
+       my $cond_all = '(tid = ? OR sid = ?) AND num > ?';
        my $sort_col = 'ds';
        $num = 0;
-       if ($prev) {
+       if ($prev) { # mboxrd stream, only
                $num = $prev->{num} || 0;
                $sort_col = 'num';
        }
-       my $cond = '(tid = ? OR sid = ?) AND num > ?';
-       my $msgs = do_get($self, <<"", {}, $tid, $sid, $num);
-SELECT num,ts,ds,ddd FROM over WHERE $cond ORDER BY $sort_col ASC
 
-       return $msgs unless wantarray;
+       my $cols = 'num,ts,ds,ddd';
+       unless (wantarray) {
+               return do_get($self, <<"", {}, $tid, $sid, $num);
+SELECT $cols FROM over WHERE $cond_all
+ORDER BY $sort_col ASC
 
-       my $nr = $dbh->selectrow_array(<<"", undef, $tid, $sid, $num);
-SELECT COUNT(num) FROM over WHERE $cond
+       }
 
+       # HTML view always wants an array and never uses $prev,
+       # but the mbox stream never wants an array and always has $prev
+       die '$prev not supported with wantarray' if $prev;
+       my $nr = $dbh->selectrow_array(<<"", undef, $tid, $sid, $num);
+SELECT COUNT(num) FROM over WHERE $cond_all
+
+       # giant thread, prioritize strict (tid) matches and throw
+       # in the loose (sid) matches at the end
+       my $msgs = do_get($self, <<"", {}, $tid, $num);
+SELECT $cols FROM over WHERE tid = ? AND num > ?
+ORDER BY $sort_col ASC
+
+       # do we have room for loose matches? get the most recent ones, first:
+       my $lim = DEFAULT_LIMIT - scalar(@$msgs);
+       if ($lim > 0) {
+               my $opts = { limit => $lim };
+               my $loose = do_get($self, <<"", $opts, $tid, $sid, $num);
+SELECT $cols FROM over WHERE tid != ? AND sid = ? AND num > ?
+ORDER BY $sort_col DESC
+
+               # TODO separate strict and loose matches here once --reindex
+               # is fixed to preserve `tid' properly
+               push @$msgs, @$loose;
+       }
        ($nr, $msgs);
 }
 

diff --git a/lib/PublicInbox/View.pm b/lib/PublicInbox/View.pm
index 58851edc59f5226bc47a4c6ba3f7627a19ba3e00..eb002aebbb443d919502a6ac9b6e2ac0d0b77593 100644 (file)
--- a/lib/PublicInbox/View.pm
+++ b/lib/PublicInbox/View.pm
@@ -365,7 +365,7 @@ sub walk_thread {
        while (@q) {
                my ($level, $node, $i) = splice(@q, 0, 3);
                defined $node or next;
-               $cb->($ctx, $level, $node, $i);
+               $cb->($ctx, $level, $node, $i) or return;
                ++$level;
                $i = 0;
                unshift @q, map { ($level, $_, $i++) } @{$node->{children}};
@@ -818,10 +818,56 @@ sub indent_for {
        $level ? INDENT x ($level - 1) : '';
 }
 
+sub find_mid_root {
+       my ($ctx, $level, $node, $idx) = @_;
+       ++$ctx->{root_idx} if $level == 0;
+       if ($node->{id} eq $ctx->{mid}) {
+               $ctx->{found_mid_at} = $ctx->{root_idx};
+               return 0;
+       }
+       1;
+}
+
+sub strict_loose_note ($) {
+       my ($nr) = @_;
+       my $msg =
+"  -- strict thread matches above, loose matches on Subject: below --\n";
+
+       if ($nr > PublicInbox::Over::DEFAULT_LIMIT()) {
+               $msg .=
+"  -- use mbox.gz link to download all $nr messages --\n";
+       }
+       $msg;
+}
+
 sub thread_results {
        my ($ctx, $msgs) = @_;
        require PublicInbox::SearchThread;
-       PublicInbox::SearchThread::thread($msgs, *sort_ds, $ctx->{-inbox});
+       my $ibx = $ctx->{-inbox};
+       my $rootset = PublicInbox::SearchThread::thread($msgs, *sort_ds, $ibx);
+
+       # FIXME: `tid' is broken on --reindex, so that needs to be fixed
+       # and preserved in the future.  This bug is hidden by `sid' matches
+       # in get_thread, so we never noticed it until now.  And even when
+       # reindexing is fixed, we'll keep this code until a SCHEMA_VERSION
+       # bump since reindexing is expensive and users may not do it
+
+       # loose threading could've returned too many results,
+       # put the root the message we care about at the top:
+       my $mid = $ctx->{mid};
+       if (defined($mid) && scalar(@$rootset) > 1) {
+               $ctx->{root_idx} = -1;
+               my $nr = scalar @$msgs;
+               walk_thread($rootset, $ctx, *find_mid_root);
+               my $idx = $ctx->{found_mid_at};
+               if (defined($idx) && $idx != 0) {
+                       my $tip = splice(@$rootset, $idx, 1);
+                       @$rootset = reverse @$rootset;
+                       unshift @$rootset, $tip;
+                       $ctx->{sl_note} = strict_loose_note($nr);
+               }
+       }
+       $rootset
 }
 
 sub missing_thread {
@@ -864,6 +910,10 @@ sub skel_dump {
        my $cur = $ctx->{cur};
        my $mid = $smsg->{mid};
 
+       if ($level == 0 && $ctx->{skel_dump_roots}++) {
+               $$dst .= delete $ctx->{sl_note} || '';
+       }
+
        my $f = ascii_html($smsg->from_name);
        my $obfs_ibx = $ctx->{-obfs_ibx};
        obfuscate_addrs($obfs_ibx, $f) if $obfs_ibx;
@@ -882,7 +932,7 @@ sub skel_dump {
                        delete $ctx->{cur};
                        $$dst .= "<b>$d<a\nid=r\nhref=\"#t\">".
                                 "$attr [this message]</a></b>\n";
-                       return;
+                       return 1;
                } else {
                        $ctx->{prev_msg} = $mid;
                }
@@ -922,6 +972,7 @@ sub skel_dump {
                $m = $ctx->{-upfx}.mid_escape($mid).'/';
        }
        $$dst .=  $d . "<a\nhref=\"$m\"$id>" . $end;
+       1;
 }
 
 sub _skel_ghost {
@@ -947,6 +998,7 @@ sub _skel_ghost {
        }
        my $dst = $ctx->{dst};
        $$dst .= $d;
+       1;
 }
 
 sub sort_ds {
@@ -973,7 +1025,7 @@ sub acc_topic {
                        $topic = [ $ds, 1, { $subj => $mid }, $subj ];
                        $ctx->{-cur_topic} = $topic;
                        push @{$ctx->{order}}, $topic;
-                       return;
+                       return 1;
                }
 
                $topic = $ctx->{-cur_topic}; # should never be undef
@@ -987,11 +1039,12 @@ sub acc_topic {
                }
                $seen->{$subj} = $mid; # latest for subject
        } else { # ghost message
-               return if $level != 0; # ignore child ghosts
+               return 1 if $level != 0; # ignore child ghosts
                $topic = [ -666, 0, {} ];
                $ctx->{-cur_topic} = $topic;
                push @{$ctx->{order}}, $topic;
        }
+       1;
 }
 
 sub dump_topics {