[PATCH 0/2] Perl XapHelper fix + cleanup

[PATCH 0/2] Perl XapHelper fix + cleanup

[Eric Wong]

The XH.pm fix actually eluded me across several days which
inspired me to do the last round of cleanups to eliminate
possible sources of bugs.

Eric Wong (2):
  xap_helper: drop qp_extra_done flag and conditions
  XapHelper.pm: fix QP_FLAGS initialization

 lib/PublicInbox/XapHelper.pm | 17 ++++++------
 lib/PublicInbox/xap_helper.h | 54 +++++++++++++++++-------------------
 2 files changed, 33 insertions(+), 38 deletions(-)

[PATCH 1/2] xap_helper: drop qp_extra_done flag and conditions

[Eric Wong]

As with -d (directories), the -Q (extra query prefixes) flag is
already part of the cache key so there's no need to initialize
it lazily.

While we're at it, drop a `map' op from the cache key generation
for the Perl implementation.
---
 lib/PublicInbox/XapHelper.pm | 12 ++++----
 lib/PublicInbox/xap_helper.h | 54 +++++++++++++++++-------------------
 2 files changed, 30 insertions(+), 36 deletions(-)

diff --git a/lib/PublicInbox/XapHelper.pm b/lib/PublicInbox/XapHelper.pm
index e98553c8..8f3e7e84 100644
--- a/lib/PublicInbox/XapHelper.pm
+++ b/lib/PublicInbox/XapHelper.pm
@@ -186,16 +186,15 @@ sub cmd_mset { # to be used by WWW + IMAP
 	}
 }
 
-sub srch_init_extra ($) {
-	my ($req) = @_;
-	my $qp = $req->{srch}->{qp};
+sub srch_init_extra ($$) {
+	my ($srch, $req) = @_;
+	my $qp = $srch->{qp};
 	for (@{$req->{Q}}) {
 		my ($upfx, $m, $xpfx) = split /([:=])/;
 		$xpfx // die "E: bad -Q $_";
 		$m = $m eq '=' ? 'add_boolean_prefix' : 'add_prefix';
 		$qp->$m($upfx, $xpfx);
 	}
-	$req->{srch}->{qp_extra_done} = 1;
 }
 
 sub dispatch {
@@ -205,7 +204,7 @@ sub dispatch {
 		or return;
 	my $dirs = delete $req->{d} or die 'no -d args';
 	my $key = "-d\0".join("\0-d\0", @$dirs);
-	$key .= "\0".join("\0", map { ('-Q', $_) } @{$req->{Q}}) if $req->{Q};
+	$key .= "\0-Q\0".join("\0-Q\0", @{$req->{Q}}) if $req->{Q};
 	my $new;
 	$req->{srch} = $SRCH{$key} // do {
 		$new = { qp_flags => $QP_FLAGS };
@@ -240,11 +239,10 @@ sub dispatch {
 		bless $new, $req->{c} ? 'PublicInbox::CodeSearch' :
 					'PublicInbox::Search';
 		$new->qparse_new;
+		srch_init_extra $new, $req;
 		$SRCH{$key} = $new;
 	};
 	$req->{srch}->{xdb}->reopen unless $new;
-	$req->{Q} && !$req->{srch}->{qp_extra_done} and
-		srch_init_extra $req;
 	my $timeo = $req->{K};
 	alarm($timeo) if $timeo;
 	$fn->($req, @argv);
diff --git a/lib/PublicInbox/xap_helper.h b/lib/PublicInbox/xap_helper.h
index f6da52fb..fd52a70d 100644
--- a/lib/PublicInbox/xap_helper.h
+++ b/lib/PublicInbox/xap_helper.h
@@ -120,7 +120,6 @@ static void *xreallocarray(void *ptr, size_t nmemb, size_t size)
 struct srch {
 	int ckey_len; // int for comparisons
 	unsigned qp_flags;
-	bool qp_extra_done;
 	Xapian::Database *db;
 	Xapian::QueryParser *qp;
 	unsigned char ckey[]; // $shard_path0\0$shard_path1\0...
@@ -594,6 +593,30 @@ static void srch_cache_renew(struct srch *keep)
 #include "xh_date.h" // GitDateRangeProcessor + GitDateFieldProcessor
 #include "xh_thread_fp.h" // ThreadFieldProcessor
 
+// setup query parser for altid and arbitrary headers
+static void srch_init_extra(struct srch *srch, struct req *req)
+{
+	const char *XPFX;
+	for (int i = 0; i < req->qpfxc; i++) {
+		size_t len = strlen(req->qpfxv[i]);
+		char *c = (char *)memchr(req->qpfxv[i], '=', len);
+
+		if (c) { // it's boolean "gmane=XGMANE"
+			XPFX = c + 1;
+			*c = 0;
+			srch->qp->add_boolean_prefix(req->qpfxv[i], XPFX);
+			continue;
+		}
+		// maybe it's a non-boolean prefix "blob:XBLOBID"
+		c = (char *)memchr(req->qpfxv[i], ':', len);
+		if (!c)
+			errx(EXIT_FAILURE, "bad -Q %s", req->qpfxv[i]);
+		XPFX = c + 1;
+		*c = 0;
+		srch->qp->add_prefix(req->qpfxv[i], XPFX);
+	}
+}
+
 static void srch_init(struct req *req)
 {
 	int i;
@@ -659,31 +682,7 @@ static void srch_init(struct req *req)
 			srch->qp->add_boolean_prefix("L", "L");
 		}
 	}
-}
-
-// setup query parser for altid and arbitrary headers
-static void srch_init_extra(struct req *req)
-{
-	const char *XPFX;
-	for (int i = 0; i < req->qpfxc; i++) {
-		size_t len = strlen(req->qpfxv[i]);
-		char *c = (char *)memchr(req->qpfxv[i], '=', len);
-
-		if (c) { // it's boolean "gmane=XGMANE"
-			XPFX = c + 1;
-			*c = 0;
-			req->srch->qp->add_boolean_prefix(req->qpfxv[i], XPFX);
-			continue;
-		}
-		// maybe it's a non-boolean prefix "blob:XBLOBID"
-		c = (char *)memchr(req->qpfxv[i], ':', len);
-		if (!c)
-			errx(EXIT_FAILURE, "bad -Q %s", req->qpfxv[i]);
-		XPFX = c + 1;
-		*c = 0;
-		req->srch->qp->add_prefix(req->qpfxv[i], XPFX);
-	}
-	req->srch->qp_extra_done = true;
+	srch_init_extra(req->srch, req);
 }
 
 #define OPT_U(ch, var, fn, max) do { \
@@ -764,7 +763,6 @@ static void dispatch(struct req *req)
 	ERR_CLOSE(kfp, EXIT_FAILURE); // may ENOMEM, sets kbuf.srch
 	kbuf.srch->db = NULL;
 	kbuf.srch->qp = NULL;
-	kbuf.srch->qp_extra_done = false;
 	kbuf.srch->ckey_len = size - offsetof(struct srch, ckey);
 	if (kbuf.srch->ckey_len <= 0 || !req->dirc)
 		ABORT("no -d args (or too many)");
@@ -780,8 +778,6 @@ static void dispatch(struct req *req)
 		srch_free(kbuf.srch);
 		req->srch->db->reopen();
 	}
-	if (req->qpfxc && !req->srch->qp_extra_done)
-		srch_init_extra(req);
 	if (req->timeout_sec)
 		alarm(req->timeout_sec > UINT_MAX ?
 			UINT_MAX : (unsigned)req->timeout_sec);

[PATCH 2/2] XapHelper.pm: fix QP_FLAGS initialization

[Eric Wong]

We can't use $PublicInbox::Search::QP_FLAGS until after
load_xapian is called.  Failure to set the correct query parser
flags was causing failures in
`PI_NO_CXX=1 TEST_DAEMON_XH=-X0 prove -bw t/imapd.t'
since phrase parsing was broken with the Perl bindings
XapHelper implementation.

Now, the `check-xh0' and `check-xh1' targets both pass with
PI_NO_CXX=1 set to disable the C++ xap_helper.

Fixes: fa6a7919 (xap_helper: enable FLAG_PURE_NOT in external process, 2025-02-23)
---
 lib/PublicInbox/XapHelper.pm | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/XapHelper.pm b/lib/PublicInbox/XapHelper.pm
index 8f3e7e84..54e2c174 100644
--- a/lib/PublicInbox/XapHelper.pm
+++ b/lib/PublicInbox/XapHelper.pm
@@ -20,7 +20,7 @@ use Carp qw(croak);
 my $X = \%PublicInbox::Search::X;
 our (%SRCH, %WORKERS, $nworker, $workerset, $in, $SHARD_NFD, $MY_FD_MAX);
 our $stderr = \*STDERR;
-my $QP_FLAGS = $PublicInbox::Search::QP_FLAGS;
+my $QP_FLAGS;
 
 sub cmd_test_inspect {
 	my ($req) = @_;
@@ -338,6 +338,8 @@ sub start (@) {
 
 	local (%SRCH, %WORKERS, $SHARD_NFD, $MY_FD_MAX);
 	PublicInbox::Search::load_xapian();
+	$QP_FLAGS = $PublicInbox::Search::QP_FLAGS |
+		PublicInbox::Search::FLAG_PURE_NOT();
 	$GLP->getoptionsfromarray(\@argv, my $opt = { j => 1 }, 'j=i') or
 		die 'bad args';
 	local $workerset = POSIX::SigSet->new;
@@ -349,7 +351,6 @@ sub start (@) {
 		die "E: unable to get RLIMIT_NOFILE: $!";
 	warn "W: RLIMIT_NOFILE=$MY_FD_MAX too low\n" if $MY_FD_MAX < 72;
 	$MY_FD_MAX -= 64;
-	$QP_FLAGS |= PublicInbox::Search::FLAG_PURE_NOT();
 
 	local $nworker = $opt->{j};
 	return recv_loop() if $nworker == 0;