From: Austin Clements <amdragon@MIT.EDU>
To: Jani Nikula <jani@nikula.org>, notmuch@notmuchmail.org
Subject: Re: [PATCH v4 2/6] cli: add options --first and --maxitems to notmuch
	search
In-Reply-To:
 <45e0f0f47250b20b4c3372910fea4d2fd5144e20.1320615322.git.jani@nikula.org>
References: <cover.1320615322.git.jani@nikula.org>
	<45e0f0f47250b20b4c3372910fea4d2fd5144e20.1320615322.git.jani@nikula.org>
User-Agent: Notmuch/0.9+29~g81d35f0 (http://notmuchmail.org) Emacs/23.3.1
	(i486-pc-linux-gnu)
Date: Sun, 13 Nov 2011 11:52:19 -0500
Message-ID: <87ehxcufr0.fsf@awakening.csail.mit.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

Code LGTM.  Some documentation nits below.

On Sun,  6 Nov 2011 23:47:11 +0200, Jani Nikula <jani@nikula.org> wrote:
> Add options --first=[-]N and --maxitems=M to notmuch search to determine
> the first result and maximum number of results to display.
> 
> Option --maxitems=M limits the maximum number of results to display to M.
> 
> Option --first=[-]N skips the first N results; with the leading '-' skip
> until the Nth result from the end (showing a total of N results if within
> bounds of the total number of results and not limited with --maxitems).
> 
> Note that --first with a negative N for thread or summary output requires
> counting the number of matching threads in advance.
> 
> Signed-off-by: Jani Nikula <jani@nikula.org>
> ---
>  NEWS             |    5 ++++
>  notmuch-search.c |   70 ++++++++++++++++++++++++++++++++++++++++++++---------
>  notmuch.1        |   21 ++++++++++++++-
>  3 files changed, 82 insertions(+), 14 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index b44b11e..bfdba7b 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -23,6 +23,11 @@ Add search terms to  "notmuch dump"
>    search/show/tag. The output file argument of dump is deprecated in
>    favour of using stdout.
>  
> +Add "notmuch search" --first and --maxitems options
> +
> +  The search command now takes options --first=[-]N and --maxitems=N to limit
> +  the number of results shown.
> +
>  Notmuch 0.9 (2011-10-01)
>  ========================
>  
> diff --git a/notmuch-search.c b/notmuch-search.c
> index 6f04d9a..c62a594 100644
> --- a/notmuch-search.c
> +++ b/notmuch-search.c
> @@ -194,13 +194,22 @@ static int
>  do_search_threads (const search_format_t *format,
>  		   notmuch_query_t *query,
>  		   notmuch_sort_t sort,
> -		   output_t output)
> +		   output_t output,
> +		   int first,
> +		   int maxitems)
>  {
>      notmuch_thread_t *thread;
>      notmuch_threads_t *threads;
>      notmuch_tags_t *tags;
>      time_t date;
>      int first_thread = 1;
> +    int i;
> +
> +    if (first < 0) {
> +	first += notmuch_query_count_threads (query);
> +	if (first < 0)
> +	    first = 0;
> +    }
>  
>      threads = notmuch_query_search_threads (query);
>      if (threads == NULL)
> @@ -208,17 +217,23 @@ do_search_threads (const search_format_t *format,
>  
>      fputs (format->results_start, stdout);
>  
> -    for (;
> -	 notmuch_threads_valid (threads);
> -	 notmuch_threads_move_to_next (threads))
> +    for (i = 0;
> +	 notmuch_threads_valid (threads) &&
> +	     (maxitems < 0 || i < first + maxitems);
> +	 notmuch_threads_move_to_next (threads), i++)
>      {
>  	int first_tag = 1;
>  
> +	thread = notmuch_threads_get (threads);
> +
> +	if (i < first) {
> +	    notmuch_thread_destroy (thread);
> +	    continue;
> +	}
> +
>  	if (! first_thread)
>  	    fputs (format->item_sep, stdout);
>  
> -	thread = notmuch_threads_get (threads);
> -
>  	if (output == OUTPUT_THREADS) {
>  	    format->item_id (thread, "thread:",
>  			     notmuch_thread_get_thread_id (thread));
> @@ -271,12 +286,21 @@ do_search_threads (const search_format_t *format,
>  static int
>  do_search_messages (const search_format_t *format,
>  		    notmuch_query_t *query,
> -		    output_t output)
> +		    output_t output,
> +		    int first,
> +		    int maxitems)
>  {
>      notmuch_message_t *message;
>      notmuch_messages_t *messages;
>      notmuch_filenames_t *filenames;
>      int first_message = 1;
> +    int i;
> +
> +    if (first < 0) {
> +	first += notmuch_query_count_messages (query);
> +	if (first < 0)
> +	    first = 0;
> +    }
>  
>      messages = notmuch_query_search_messages (query);
>      if (messages == NULL)
> @@ -284,10 +308,14 @@ do_search_messages (const search_format_t *format,
>  
>      fputs (format->results_start, stdout);
>  
> -    for (;
> -	 notmuch_messages_valid (messages);
> -	 notmuch_messages_move_to_next (messages))
> +    for (i = 0;
> +	 notmuch_messages_valid (messages) &&
> +	     (maxitems < 0 || i < first + maxitems);
> +	 notmuch_messages_move_to_next (messages), i++)
>      {
> +	if (i < first)
> +	    continue;
> +
>  	message = notmuch_messages_get (messages);
>  
>  	if (output == OUTPUT_FILES) {
> @@ -394,6 +422,8 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>      const search_format_t *format = &format_text;
>      int i, ret;
>      output_t output = OUTPUT_SUMMARY;
> +    int maxitems = -1; /* unlimited */
> +    int first = 0;
>  
>      argc--; argv++; /* skip subcommand argument */
>  
> @@ -412,6 +442,22 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>  		fprintf (stderr, "Invalid value for --sort: %s\n", opt);
>  		return 1;
>  	    }
> +	} else if (STRNCMP_LITERAL (argv[i], "--first=") == 0) {
> +	    char *p;
> +	    opt = argv[i] + sizeof ("--first=") - 1;
> +	    first = strtol (opt, &p, 10);
> +	    if (*opt == '\0' || p == opt || *p != '\0') {
> +		fprintf (stderr, "Invalid value for --first: %s\n", opt);
> +		return 1;
> +	    }
> +	} else if (STRNCMP_LITERAL (argv[i], "--maxitems=") == 0) {
> +	    char *p;
> +	    opt = argv[i] + sizeof ("--maxitems=") - 1;
> +	    maxitems = strtoul (opt, &p, 10);
> +	    if (*opt == '\0' || p == opt || *p != '\0') {
> +		fprintf (stderr, "Invalid value for --maxitems: %s\n", opt);
> +		return 1;
> +	    }
>  	} else if (STRNCMP_LITERAL (argv[i], "--format=") == 0) {
>  	    opt = argv[i] + sizeof ("--format=") - 1;
>  	    if (strcmp (opt, "text") == 0) {
> @@ -478,11 +524,11 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
>      default:
>      case OUTPUT_SUMMARY:
>      case OUTPUT_THREADS:
> -	ret = do_search_threads (format, query, sort, output);
> +	ret = do_search_threads (format, query, sort, output, first, maxitems);
>  	break;
>      case OUTPUT_MESSAGES:
>      case OUTPUT_FILES:
> -	ret = do_search_messages (format, query, output);
> +	ret = do_search_messages (format, query, output, first, maxitems);
>  	break;
>      case OUTPUT_TAGS:
>  	ret = do_search_tags (notmuch, format, query);
> diff --git a/notmuch.1 b/notmuch.1
> index bba479e..c97334c 100644
> --- a/notmuch.1
> +++ b/notmuch.1
> @@ -214,11 +214,28 @@ when sorting by
>  .B newest\-first
>  the threads will be sorted by the newest message in each thread.
>  
> -.RE
> -.RS 4
>  By default, results will be displayed in reverse chronological order,
>  (that is, the newest results will be displayed first).
> +.RE
> +
> +.RS 4
> +.TP 4
> +.BR \-\-first=[\-]N
> +
> +This option can be used to skip the display of first N results. With the

To be consistent with the other documentation style, this should be
"Skip the display of the first N results." (or, better, "Skip displaying
the first N results.")

> +leading '\-' skip until the Nth result from the end, showing a total of N
> +results if not otherwise bounded by total number of matching results or
> +\-\-maxitems.

".. if not otherwise bounded by total number of matching results .."  I
see what you're saying here, but I read this over several times before
it clicked that you were talking about being bounded by the *beginning*
of the search results, not the *end* of the search results.  I don't
think it's necessary to talk about the bounding here at all.  Obviously
it can't give you more search results than there are, and maxitems is
discussed separately.  Perhaps just "With the leading '\-', start at the
Nth result from the end."?

> +.RE
> +
> +.RS 4
> +.TP 4
> +.BR \-\-maxitems=N
>  
> +This option can be used to limit the number of results to display to N.

Likewise, "Limit the number results to display to N." (or, better,
"Limit the number of displayed results to N.")

> +.RE
> +
> +.RS 4
>  See the
>  .B "SEARCH SYNTAX"
>  section below for details of the supported syntax for <search-terms>.
> -- 
> 1.7.5.4
>