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
Description: PGP signature
