bug#49534: [External] : Re: bug#49534: 26.3; Isearch should support using filter predicates with empty search hits

[Drew Adams <drew.adams@oracle.com>]

> I've tried to remove (= (match-beginning 0) (match-end 0))
> and then tried the test case provided by Drew,
> but it goes into an infinite loop.
> 
> So it requires advancing by 1 char - the same trick as it's used
> in query-replace, etc.

Yes, my code does that.

> Also I noticed that Drew's test case matches at 'eob'
> that is wrong.  So I moved checks for bobp/eobp outside.

What do you mean by Drew's test case?

Do you mean the updated isearch+.el definition of `isearch-search', with the bug fix?  That is, do you mean this code?

 (when (or (not isearch-success)
           (isearchp-reached-limit-p) ; <==============
           (funcall isearch-filter-predicate
                    (match-beginning 0)
                    (match-end 0)))
   (setq retry  nil))

where

(defun isearchp-reached-limit-p ()
  "Return non-nil if at search-boundary limit in current search direction."
  (if isearch-forward
      (or (eobp) ; <==================
          (and isearchp-reg-end
               (> (point) isearchp-reg-end)))
      (or (bobp) ; <==================
          (and isearchp-reg-beg
               (< (point) isearchp-reg-beg)))))

Is that the use of `eobp' you're talking about?
(If not, what is?)  That `isearchp-reached-limit-p'
just replaces the original isearch.el code, which
just tested (or (bobp) (eobp)):

(or (not isearch-success)
    (bobp) (eobp) ; <=================
    (= (match-beginning 0) (match-end 0))
    (funcall isearch-filter-predicate
             (match-beginning 0) (match-end 0))) 

My code just uses the (original) region limits
instead of the buffer limits, when searching the
active region.

Your code (in master) removes that boundary test
altogether (no `bobp' or `eobp' test).  Why is
that the right thing?  Is it because the match
should be allowed to match up to `bobp' or `eobp'?
If so, why was that test in isearch.el in the
first place?

Actually, you do still test for reaching the
boundary, but only for an empty match and after
filter failure.  Why is that?

And why do you not need to back up a char after
the loop, if the match was empty the last time
around and the next time it fails?  It'll have
advanced a char; should it stay there instead
of backing up?  (Dunno, but I supposed not.)

Our code in those spots is slightly different.
I'd like to know why you did just as you did.

> The third problem I noticed thanks to Drew's test case
> is that lazy-highlighting incorrectly highlights empty matches.
> Fixed as well.

Yes, I left that code alone for the moment, but
your change is no doubt the right thing there.