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

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

[Robin Jarry]

notmuch search does not output header values. However, when browsing
through a large email corpus, it can be time saving to be able to
paginate without running notmuch show for each message/thread.

Add --offset and --limit options to notmuch show. This is inspired from
commit 796b629c3b82 ("cli: add options --offset and --limit to notmuch
search").

Update man page, shell completion and add a test case to ensure it works
as expected.

Cc: Tim Culverhouse <tim@timculverhouse.com>
Signed-off-by: Robin Jarry <robin@jarry.cc>
---
 completion/notmuch-completion.bash |  2 +-
 completion/zsh/_notmuch            |  2 +
 doc/man1/notmuch-show.rst          |  9 ++++
 notmuch-client.h                   |  2 +
 notmuch-show.c                     | 49 +++++++++++++++---
 test/T131-show-limiting.sh         | 81 ++++++++++++++++++++++++++++++
 6 files changed, 138 insertions(+), 7 deletions(-)
 create mode 100755 test/T131-show-limiting.sh

diff --git a/completion/notmuch-completion.bash b/completion/notmuch-completion.bash
index 0022b54bff5d..3748846edf83 100644
--- a/completion/notmuch-completion.bash
+++ b/completion/notmuch-completion.bash
@@ -530,7 +530,7 @@ _notmuch_show()
     ! $split &&
     case "${cur}" in
 	-*)
-	    local options="--entire-thread= --format= --exclude= --body= --format-version= --part= --verify --decrypt= --include-html ${_notmuch_shared_options}"
+	    local options="--entire-thread= --format= --exclude= --body= --format-version= --part= --verify --decrypt= --include-html --limit= --offset= ${_notmuch_shared_options}"
 	    compopt -o nospace
 	    COMPREPLY=( $(compgen -W "$options" -- ${cur}) )
 	    ;;
diff --git a/completion/zsh/_notmuch b/completion/zsh/_notmuch
index e207d90b7202..0bdd7f772a7a 100644
--- a/completion/zsh/_notmuch
+++ b/completion/zsh/_notmuch
@@ -245,6 +245,8 @@ _notmuch_show() {
     '--exclude=[respect excluded tags setting]:exclude tags:(true false)' \
     '--body=[output body]:output body content:(true false)' \
     '--include-html[include text/html parts in the output]' \
+    '--limit=[limit the number of displayed results]:limit: ' \
+    '--offset=[skip displaying the first N results]:offset: ' \
     '*::search term:_notmuch_search_term'
 }
 
diff --git a/doc/man1/notmuch-show.rst b/doc/man1/notmuch-show.rst
index 2c0a0de6ad16..c13d94de0244 100644
--- a/doc/man1/notmuch-show.rst
+++ b/doc/man1/notmuch-show.rst
@@ -130,6 +130,15 @@ Supported options for **show** include
    By default, results will be displayed in reverse chronological
    order, (that is, the newest results will be displayed first).
 
+.. option:: --offset=[-]N
+
+   Skip displaying the first N results. With the leading '-', start
+   at the Nth result from the end.
+
+.. option:: --limit=N
+
+   Limit the number of displayed results to N.
+
 .. option:: --verify
 
    Compute and report the validity of any MIME cryptographic
diff --git a/notmuch-client.h b/notmuch-client.h
index 21b49908ae24..1a87240d3c21 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -77,6 +77,8 @@ typedef struct notmuch_show_params {
     bool output_body;
     int duplicate;
     int part;
+    int offset;
+    int limit;
     _notmuch_crypto_t crypto;
     bool include_html;
     GMimeStream *out_stream;
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)
+	    params->offset = 0;
+    }
 
     status = notmuch_query_search_threads (query, &threads);
     if (print_status_query ("notmuch show", query, status))
@@ -1166,11 +1178,16 @@ do_show_threaded (void *ctx,
 
     sp->begin_list (sp);
 
-    for (;
-	 notmuch_threads_valid (threads);
-	 notmuch_threads_move_to_next (threads)) {
+    for (i = 0;
+	 notmuch_threads_valid (threads) && (params->limit < 0 || i < params->offset + params->limit);
+	 notmuch_threads_move_to_next (threads), i++) {
 	thread = notmuch_threads_get (threads);
 
+	if (i < params->offset) {
+	    notmuch_thread_destroy (thread);
+	    continue;
+	}
+
 	messages = notmuch_thread_get_toplevel_messages (thread);
 
 	if (messages == NULL)
@@ -1201,6 +1218,18 @@ do_show_unthreaded (void *ctx,
     notmuch_message_t *message;
     notmuch_status_t status, res = NOTMUCH_STATUS_SUCCESS;
     notmuch_bool_t excluded;
+    int i;
+
+    if (params->offset < 0) {
+	unsigned count;
+	notmuch_status_t s = notmuch_query_count_messages (query, &count);
+	if (print_status_query ("notmuch search", query, s))
+	    return 1;
+
+	params->offset += count;
+	if (params->offset < 0)
+	    params->offset = 0;
+    }
 
     status = notmuch_query_search_messages (query, &messages);
     if (print_status_query ("notmuch show", query, status))
@@ -1208,9 +1237,13 @@ do_show_unthreaded (void *ctx,
 
     sp->begin_list (sp);
 
-    for (;
-	 notmuch_messages_valid (messages);
-	 notmuch_messages_move_to_next (messages)) {
+    for (i = 0;
+	 notmuch_messages_valid (messages) && (params->limit < 0 || i < params->offset + params->limit);
+	 notmuch_messages_move_to_next (messages), i++) {
+	if (i < params->offset) {
+	    continue;
+	}
+
 	sp->begin_list (sp);
 	sp->begin_list (sp);
 
@@ -1287,6 +1320,8 @@ notmuch_show_command (notmuch_database_t *notmuch, int argc, char *argv[])
     notmuch_show_params_t params = {
 	.part = -1,
 	.duplicate = 0,
+	.offset = 0,
+	.limit = -1, /* unlimited */
 	.omit_excluded = true,
 	.output_body = true,
 	.crypto = { .decrypt = NOTMUCH_DECRYPT_AUTO },
@@ -1328,6 +1363,8 @@ notmuch_show_command (notmuch_database_t *notmuch, int argc, char *argv[])
 	{ .opt_bool = &params.output_body, .name = "body" },
 	{ .opt_bool = &params.include_html, .name = "include-html" },
 	{ .opt_int = &params.duplicate, .name = "duplicate" },
+	{ .opt_int = &params.limit, .name = "limit" },
+	{ .opt_int = &params.offset, .name = "offset" },
 	{ .opt_inherit = notmuch_shared_options },
 	{ }
     };
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() {
+    local kind="$1"
+    shift
+    if [ "$kind" = messages ]; then
+        set -- --unthreaded "$@"
+    fi
+    notmuch show --body=false --format=text --entire-thread=false "$@" "*" |
+        sed -nre 's/^.message\{.*\<depth:0\>.*/&/p'
+}
+
+for outp in messages threads; do
+    test_begin_subtest "$outp: limit does the right thing"
+    show $outp | head -n 20 >expected
+    show $outp --limit=20 >output
+    test_expect_equal_file expected output
+
+    test_begin_subtest "$outp: concatenation of limited shows"
+    show $outp | head -n 20 >expected
+    show $outp --limit=10 >output
+    show $outp --limit=10 --offset=10 >>output
+    test_expect_equal_file expected output
+
+    test_begin_subtest "$outp: limit larger than result set"
+    N=$(notmuch count --output=$outp "*")
+    show $outp >expected
+    show $outp --limit=$((1 + N)) >output
+    test_expect_equal_file expected output
+
+    test_begin_subtest "$outp: limit = 0"
+    test_expect_equal "$(show $outp --limit=0)" ""
+
+    test_begin_subtest "$outp: offset does the right thing"
+    # note: tail -n +N is 1-based
+    show $outp | tail -n +21 >expected
+    show $outp --offset=20 >output
+    test_expect_equal_file expected output
+
+    test_begin_subtest "${outp}: offset = 0"
+    show $outp >expected
+    show $outp --offset=0 >output
+    test_expect_equal_file expected output
+
+    test_begin_subtest "${outp}: negative offset"
+    show $outp | tail -n 20 >expected
+    show $outp --offset=-20 >output
+    test_expect_equal_file expected output
+
+    test_begin_subtest "${outp}: negative offset"
+    show $outp | tail -n 1 >expected
+    show $outp --offset=-1 >output
+    test_expect_equal_file expected output
+
+    test_begin_subtest "${outp}: negative offset combined with limit"
+    show $outp | tail -n 20 | head -n 10 >expected
+    show $outp --offset=-20 --limit=10 >output
+    test_expect_equal_file expected output
+
+    test_begin_subtest "${outp}: negative offset combined with equal limit"
+    show $outp | tail -n 20 >expected
+    show $outp --offset=-20 --limit=20 >output
+    test_expect_equal_file expected output
+
+    test_begin_subtest "${outp}: negative offset combined with large limit"
+    show $outp | tail -n 20 >expected
+    show $outp --offset=-20 --limit=50 >output
+    test_expect_equal_file expected output
+
+    test_begin_subtest "${outp}: negative offset larger than results"
+    N=$(notmuch count --output=$outp "*")
+    show $outp >expected
+    show $outp --offset=-$((1 + N)) >output
+    test_expect_equal_file expected output
+done
+
+test_done
-- 
2.37.3

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

[Robin Jarry]

Robin Jarry, Oct 12, 2022 at 00:19:
> +	if (print_status_query ("notmuch search", query, s))

I just realized that I copy pasted code from notmuch-search.c and did
not updated everything on the way...

I'll hold before sending a v2 if there are other remarks or changes
required.

Cheers all.

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

[Tomi Ollila]

On Wed, Oct 12 2022, Robin Jarry wrote:

> notmuch search does not output header values. However, when browsing
> through a large email corpus, it can be time saving to be able to
> paginate without running notmuch show for each message/thread.
>
> Add --offset and --limit options to notmuch show. This is inspired from
> commit 796b629c3b82 ("cli: add options --offset and --limit to notmuch
> search").
>
> Update man page, shell completion and add a test case to ensure it works
> as expected.
>
> Cc: Tim Culverhouse <tim@timculverhouse.com>
> Signed-off-by: Robin Jarry <robin@jarry.cc>
> ---
>  completion/notmuch-completion.bash |  2 +-
>  completion/zsh/_notmuch            |  2 +
>  doc/man1/notmuch-show.rst          |  9 ++++
>  notmuch-client.h                   |  2 +
>  notmuch-show.c                     | 49 +++++++++++++++---
>  test/T131-show-limiting.sh         | 81 ++++++++++++++++++++++++++++++
>  6 files changed, 138 insertions(+), 7 deletions(-)
>  create mode 100755 test/T131-show-limiting.sh
>
> diff --git a/completion/notmuch-completion.bash b/completion/notmuch-completion.bash
> index 0022b54bff5d..3748846edf83 100644
> --- a/completion/notmuch-completion.bash
> +++ b/completion/notmuch-completion.bash
> @@ -530,7 +530,7 @@ _notmuch_show()
>      ! $split &&
>      case "${cur}" in
>  	-*)
> -	    local options="--entire-thread= --format= --exclude= --body= --format-version= --part= --verify --decrypt= --include-html ${_notmuch_shared_options}"
> +	    local options="--entire-thread= --format= --exclude= --body= --format-version= --part= --verify --decrypt= --include-html --limit= --offset= ${_notmuch_shared_options}"
>  	    compopt -o nospace
>  	    COMPREPLY=( $(compgen -W "$options" -- ${cur}) )
>  	    ;;
> diff --git a/completion/zsh/_notmuch b/completion/zsh/_notmuch
> index e207d90b7202..0bdd7f772a7a 100644
> --- a/completion/zsh/_notmuch
> +++ b/completion/zsh/_notmuch
> @@ -245,6 +245,8 @@ _notmuch_show() {
>      '--exclude=[respect excluded tags setting]:exclude tags:(true false)' \
>      '--body=[output body]:output body content:(true false)' \
>      '--include-html[include text/html parts in the output]' \
> +    '--limit=[limit the number of displayed results]:limit: ' \
> +    '--offset=[skip displaying the first N results]:offset: ' \
>      '*::search term:_notmuch_search_term'
>  }
>  
> diff --git a/doc/man1/notmuch-show.rst b/doc/man1/notmuch-show.rst
> index 2c0a0de6ad16..c13d94de0244 100644
> --- a/doc/man1/notmuch-show.rst
> +++ b/doc/man1/notmuch-show.rst
> @@ -130,6 +130,15 @@ Supported options for **show** include
>     By default, results will be displayed in reverse chronological
>     order, (that is, the newest results will be displayed first).
>  
> +.. option:: --offset=[-]N
> +
> +   Skip displaying the first N results. With the leading '-', start
> +   at the Nth result from the end.
> +
> +.. option:: --limit=N
> +
> +   Limit the number of displayed results to N.
> +
>  .. option:: --verify
>  
>     Compute and report the validity of any MIME cryptographic
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 21b49908ae24..1a87240d3c21 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -77,6 +77,8 @@ typedef struct notmuch_show_params {
>      bool output_body;
>      int duplicate;
>      int part;
> +    int offset;
> +    int limit;
>      _notmuch_crypto_t crypto;
>      bool include_html;
>      GMimeStream *out_stream;
> 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)

> +	    params->offset = 0;
> +    }
>  
>      status = notmuch_query_search_threads (query, &threads);
>      if (print_status_query ("notmuch show", query, status))
> @@ -1166,11 +1178,16 @@ do_show_threaded (void *ctx,
>  
>      sp->begin_list (sp);
>  
> -    for (;
> -	 notmuch_threads_valid (threads);
> -	 notmuch_threads_move_to_next (threads)) {
> +    for (i = 0;
> +	 notmuch_threads_valid (threads) && (params->limit < 0 || i < params->offset + params->limit);
> +	 notmuch_threads_move_to_next (threads), i++) {
>  	thread = notmuch_threads_get (threads);
>  
> +	if (i < params->offset) {
> +	    notmuch_thread_destroy (thread);
> +	    continue;
> +	}
> +
>  	messages = notmuch_thread_get_toplevel_messages (thread);
>  
>  	if (messages == NULL)
> @@ -1201,6 +1218,18 @@ do_show_unthreaded (void *ctx,
>      notmuch_message_t *message;
>      notmuch_status_t status, res = NOTMUCH_STATUS_SUCCESS;
>      notmuch_bool_t excluded;
> +    int i;
> +
> +    if (params->offset < 0) {
> +	unsigned count;
> +	notmuch_status_t s = notmuch_query_count_messages (query, &count);
> +	if (print_status_query ("notmuch search", query, s))
> +	    return 1;
> +
> +	params->offset += count;
> +	if (params->offset < 0)
> +	    params->offset = 0;
> +    }
>  
>      status = notmuch_query_search_messages (query, &messages);
>      if (print_status_query ("notmuch show", query, status))
> @@ -1208,9 +1237,13 @@ do_show_unthreaded (void *ctx,
>  
>      sp->begin_list (sp);
>  
> -    for (;
> -	 notmuch_messages_valid (messages);
> -	 notmuch_messages_move_to_next (messages)) {
> +    for (i = 0;
> +	 notmuch_messages_valid (messages) && (params->limit < 0 || i < params->offset + params->limit);
> +	 notmuch_messages_move_to_next (messages), i++) {
> +	if (i < params->offset) {
> +	    continue;
> +	}
> +
>  	sp->begin_list (sp);
>  	sp->begin_list (sp);
>  
> @@ -1287,6 +1320,8 @@ notmuch_show_command (notmuch_database_t *notmuch, int argc, char *argv[])
>      notmuch_show_params_t params = {
>  	.part = -1,
>  	.duplicate = 0,
> +	.offset = 0,
> +	.limit = -1, /* unlimited */
>  	.omit_excluded = true,
>  	.output_body = true,
>  	.crypto = { .decrypt = NOTMUCH_DECRYPT_AUTO },
> @@ -1328,6 +1363,8 @@ notmuch_show_command (notmuch_database_t *notmuch, int argc, char *argv[])
>  	{ .opt_bool = &params.output_body, .name = "body" },
>  	{ .opt_bool = &params.include_html, .name = "include-html" },
>  	{ .opt_int = &params.duplicate, .name = "duplicate" },
> +	{ .opt_int = &params.limit, .name = "limit" },
> +	{ .opt_int = &params.offset, .name = "offset" },
>  	{ .opt_inherit = notmuch_shared_options },
>  	{ }
>      };
> 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)

> +    local kind="$1"
> +    shift
> +    if [ "$kind" = messages ]; then
> +        set -- --unthreaded "$@"
> +    fi
> +    notmuch show --body=false --format=text --entire-thread=false "$@" "*" |
> +        sed -nre 's/^.message\{.*\<depth:0\>.*/&/p'
> +}
> +
> +for outp in messages threads; do
> +    test_begin_subtest "$outp: limit does the right thing"
> +    show $outp | head -n 20 >expected
> +    show $outp --limit=20 >output
> +    test_expect_equal_file expected output
> +
> +    test_begin_subtest "$outp: concatenation of limited shows"
> +    show $outp | head -n 20 >expected
> +    show $outp --limit=10 >output
> +    show $outp --limit=10 --offset=10 >>output
> +    test_expect_equal_file expected output
> +
> +    test_begin_subtest "$outp: limit larger than result set"
> +    N=$(notmuch count --output=$outp "*")
> +    show $outp >expected
> +    show $outp --limit=$((1 + N)) >output
> +    test_expect_equal_file expected output
> +
> +    test_begin_subtest "$outp: limit = 0"
> +    test_expect_equal "$(show $outp --limit=0)" ""
> +
> +    test_begin_subtest "$outp: offset does the right thing"
> +    # note: tail -n +N is 1-based
> +    show $outp | tail -n +21 >expected
> +    show $outp --offset=20 >output
> +    test_expect_equal_file expected output
> +
> +    test_begin_subtest "${outp}: offset = 0"

inconsistent ${outp} (where $outp used elsewhere) ...  

> +    show $outp >expected
> +    show $outp --offset=0 >output
> +    test_expect_equal_file expected output
> +
> +    test_begin_subtest "${outp}: negative offset"
> +    show $outp | tail -n 20 >expected
> +    show $outp --offset=-20 >output
> +    test_expect_equal_file expected output
> +
> +    test_begin_subtest "${outp}: negative offset"
> +    show $outp | tail -n 1 >expected
> +    show $outp --offset=-1 >output
> +    test_expect_equal_file expected output
> +
> +    test_begin_subtest "${outp}: negative offset combined with limit"
> +    show $outp | tail -n 20 | head -n 10 >expected
> +    show $outp --offset=-20 --limit=10 >output
> +    test_expect_equal_file expected output
> +
> +    test_begin_subtest "${outp}: negative offset combined with equal limit"
> +    show $outp | tail -n 20 >expected
> +    show $outp --offset=-20 --limit=20 >output
> +    test_expect_equal_file expected output
> +
> +    test_begin_subtest "${outp}: negative offset combined with large limit"
> +    show $outp | tail -n 20 >expected
> +    show $outp --offset=-20 --limit=50 >output
> +    test_expect_equal_file expected output
> +
> +    test_begin_subtest "${outp}: negative offset larger than results"
> +    N=$(notmuch count --output=$outp "*")
> +    show $outp >expected
> +    show $outp --offset=-$((1 + N)) >output
> +    test_expect_equal_file expected output
> +done
> +
> +test_done
> -- 
> 2.37.3

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

[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,

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

[Tomi Ollila]

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