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

[Tomi Ollila <tomi.ollila@iki.fi>]

On Wed, Oct 12 2022, Robin Jarry wrote:

> 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.

Bash constructs are allowed (even perhaps encouraged in many cases), I just
care about consistency (it is easier to browse through large set of files
if there is not too much differenct constructs used)

In case of 'function' it also works in zsh (and probably ksh). I have
started to use in all (except one, I just noticed) function definitions
in my zshrc. I recall I had a problem which using function fixed
-- probably related to something with

   ff () { ...; }
   alias ff='noglob ff'

which did not exactly worked like it should have. function ff () { ...; }
fixed that case (cannot remember exact details, changed 2021-03-08 w/
somewhat vague git commit message (in private dotdir repo :)))


>
>> > +    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 personally prefer w/o {} in trivial cases, shorter and clearer to read
(and use $FOO''bar instead of ${FOO}bar when such need arrives ;/). that
is such a small difference (IMO) that either goes...

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

Tomi