Re: [PATCH] Fix pager search behavior with REG_NEWLINE.

Alejandro Colomar via Mutt-dev <[email protected]> Thu, 21 May 2026 12:57:39 +0200
Newsgroups	gmane.mail.mutt.devel
Message-ID	<ag7kLaBgb51u3-2z@devuan>
Hi Kevin,

On 2026-05-21T18:20:04+0800, Kevin J. McCarthy wrote:
> On Thu, May 21, 2026 at 11:24:38AM +0200, Alejandro Colomar via Mutt-dev wrote:
> > > 
> > >  pager.c | 69 ++++++++++++++++++++++++++++++++++++---------------------
> > >  1 file changed, 44 insertions(+), 25 deletions(-)
> 
> Hi Alex,
> 
> Thanks for the review.

You're welcome!

> I should have mentioned that this patch is basically stitching together two
> parts of the existing code.  Both of them are old, and so are... to say it
> politely... "compact".

:)

> The search chunk was relocated into a function resolve_search().
> 
> The newline removal/re-add was copied from a different function in the
> pager code, match_body_patterns().
> 
> > > diff --git a/pager.c b/pager.c
> > > index 92fa152a..75221643 100644
> > > --- a/pager.c
> > > +++ b/pager.c
> > > @@ -1268,6 +1268,47 @@ bail:
> > >    return rc;
> > >  }
> > > 
> > > +static void resolve_search(struct line_t *lineInfo, int n, char *fmt,
> > > +                           regex_t *SearchRE)

Here, fmt is declared as char*, regardless of where it comes from
(see below).

> > > +{
> > > +  size_t fmtlen;
> > > +  int has_nl = 0, offset = 0;
> > > +  regmatch_t pmatch[1];
> > > +
> > > +  /* don't consider line endings part of the buffer
> > > +   * for regex matching */
> > > +  if ((fmtlen = mutt_strlen(fmt)) > 0 && fmt[fmtlen - 1] == '\n')
> > 
> > I would very much rewrite the above in two lines.  I don't see a good
> > reason to embed the assignment in the conditional (and that often leads
> > to bugs due to accidents).  I suggest:
> > 
> > 	fmtlen = mutt_strlen(fmt);
> > 	if (fmtlen > 0 && fmt[fmtlen - 1] == '\n')
> 
> Sure I'll make this change.  Again, in my defense, this is verbatim
> pulled from match_body_patterns().  So although it's compact, I knew it
> was working.

Makes sense.

> > > +  {
> > > +    has_nl = 1;
> > > +    fmt[fmtlen - 1] = 0;
> > > +  }
> > > +
> > > +  offset = 0;
> > > +  lineInfo[n].search_cnt = 0;
> > > +  while (regexec(SearchRE, (char *) fmt + offset, 1, pmatch, (offset ? REG_NOTBOL : 0)) == 0)
> > 
> > What's the point of the cast?  fmt is already char*, right?
> 
> Actually, no.  buf and fmt are unsigned char *.  In parts of the pager
> code, the chars are upcast to int, so I'm assuming this was done
> purposely to avoid high-bit set signed char conversion issues in those
> places.

But we've done the conversion already when calling this function (you
had to do the cast there already).  Inside the function, it's already
declared as a char* (see the parameter declaration above), so this is
redundant, AFAICS.

> 
> [snipping out your comments after realizing they were for relocated
> code, since I'm not up for rewriting pager code right now. :-)]

(I can send a patch, if you want it.)

> 
> > Have a lovely day!
> > Alex
> 
> Thanks, you too.

:-)

Cheers,
Alex

-- 
<https://www.alejandro-colomar.es>
signature.asc (application/pgp-signature, 833 B)