On Thu, May 21, 2026 at 06:20:04PM +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.
>
> 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)
> > > +{
> > > + 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.
I'd prefer the compact version, but that is just me. I just wanted to
speak up to say that such discussions are not helpful, IMHO.
