Re: [PATCH] cli: add options --offset and --limit to notmuch show

Subject: Re: [PATCH] cli: add options --offset and --limit to notmuch show

Date: Wed, 12 Oct 2022 22:02:08 +0200

To: Tomi Ollila, notmuch@notmuchmail.org

Cc: Tim Culverhouse

From: Robin Jarry

Hi Tomi,

Tomi Ollila, Oct 12, 2022 at 21:39:
> > diff --git a/notmuch-show.c b/notmuch-show.c
> > index ee9efa7448d7..ad31e0123268 100644
> > --- a/notmuch-show.c
> > +++ b/notmuch-show.c
> > @@ -1159,6 +1159,18 @@ do_show_threaded (void *ctx,
> >      notmuch_thread_t *thread;
> >      notmuch_messages_t *messages;
> >      notmuch_status_t status, res = NOTMUCH_STATUS_SUCCESS;
> > +    int i;
> > +
> > +    if (params->offset < 0) {
> > +	unsigned count;
> > +	notmuch_status_t s = notmuch_query_count_threads (query, &count);
> > +	if (print_status_query ("notmuch search", query, s))
> > +	    return 1;
> > +
> > +	params->offset += count;
> > +	if (params->offset < 0)
>
> this check and setting it to 0 is mystic to me, probably same code as in
> search (?) probably it is good (?) (will not comment the same below)

Yes, I copy/pasted that code from notmuch-search.c. I believe this to
handle the case where:

    --limit=N && --offset=-M && M > count

After adding count to offset, you may end up with a negative offset
which makes the loop exit condition invalid and the --limit value would
not be enforced as expected:

    i < params->offset + params->limit

> > diff --git a/test/T131-show-limiting.sh b/test/T131-show-limiting.sh
> > new file mode 100755
> > index 000000000000..a3da35944a3e
> > --- /dev/null
> > +++ b/test/T131-show-limiting.sh
> > @@ -0,0 +1,81 @@
> > +#!/usr/bin/env bash
> > +test_description='"notmuch show" --offset and --limit parameters'
> > +. $(dirname "$0")/test-lib.sh || exit 1
> > +
> > +add_email_corpus
> > +
> > +function show() {
>
> 'function' not used in other function defitions in other files, so this is
> inconsistent (otherwise the content looks "better" than what I see usual ;D)

I concur that this is a bash-only construct. I could remove the function
keyword and we would have the same result.

> > +    test_begin_subtest "${outp}: offset = 0"
>
> inconsistent ${outp} (where $outp used elsewhere) ...  

Indeed. I had removed all the ${} except this one. For consistency with
the other test scripts, I could add them back everywhere. I don't mind
either way.

I'll hold off for other remarks before sending a v2.

Cheers,
_______________________________________________
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-leave@notmuchmail.org

Thread: