From: Mark Walters <markwalters1009@gmail.com>
To: Jani Nikula <jani@nikula.org>, notmuch@notmuchmail.org
Subject: Re: [PATCH v2 1/7] cli: add --duplicate=N option to notmuch search
In-Reply-To:
 <61ed86f221d65b4dba438cbc2b4c5b77a484a534.1370775663.git.jani@nikula.org>
References: <cover.1370775663.git.jani@nikula.org>
	<61ed86f221d65b4dba438cbc2b4c5b77a484a534.1370775663.git.jani@nikula.org>
User-Agent: Notmuch/0.15.2+171~ge2f30a2 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Sun, 09 Jun 2013 21:22:42 +0100
Message-ID: <871u8b11bh.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


Overall I like this series and am happy to give it a +1 as is but have a
few comments which might be worth considering.

Is the order of filenames clear? eg is it the order that notmuch new met
them? In particular is duplicate=1 the oldest and duplicate=N the
newest? If so that might be worth mentioning in the manpage.

On Sun, 09 Jun 2013, Jani Nikula <jani@nikula.org> wrote:
> Effective with --output=files, output the Nth filename associated with
> each message matching the query (N is 0-based). If N is equal to or
> greater than the number of files associated with the message, don't
> print anything.
> ---
>  notmuch-search.c |   18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/notmuch-search.c b/notmuch-search.c
> index 4323201..196934b 100644
> --- a/notmuch-search.c
> +++ b/notmuch-search.c
> @@ -177,7 +177,8 @@ do_search_messages (sprinter_t *format,
>  		    notmuch_query_t *query,
>  		    output_t output,
>  		    int offset,
> -		    int limit)
> +		    int limit,
> +		    int dupe)
>  {
>      notmuch_message_t *message;
>      notmuch_messages_t *messages;
> @@ -206,14 +207,17 @@ do_search_messages (sprinter_t *format,
>  	message = notmuch_messages_get (messages);
>  
>  	if (output == OUTPUT_FILES) {
> +	    int j;
>  	    filenames = notmuch_message_get_filenames (message);
>  
> -	    for (;
> +	    for (j = 1;
>  		 notmuch_filenames_valid (filenames);
> -		 notmuch_filenames_move_to_next (filenames))
> +		 notmuch_filenames_move_to_next (filenames), j++)
>  	    {
> -		format->string (format, notmuch_filenames_get (filenames));
> -		format->separator (format);
> +		if (dupe < 0 || dupe == j) {
> +		    format->string (format, notmuch_filenames_get (filenames));
> +		    format->separator (format);

Is it deliberate that dupe == 0 is not covered? If my newest oldest
thing above is correct then maybe dupe == 0 could be the all option +N
the Nth oldest and -N the Nth newest. This may be not-trivial enough
it's not worth doing.

> +		}
>  	    }
>  	    
>  	    notmuch_filenames_destroy( filenames );
> @@ -303,6 +307,7 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[])
>      int offset = 0;
>      int limit = -1; /* unlimited */
>      int exclude = EXCLUDE_TRUE;
> +    int dupe = -1;
>      unsigned int i;
>  
>      enum {
> @@ -339,6 +344,7 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[])
>                                    { 0, 0 } } },
>  	{ NOTMUCH_OPT_INT, &offset, "offset", 'O', 0 },
>  	{ NOTMUCH_OPT_INT, &limit, "limit", 'L', 0  },
> +	{ NOTMUCH_OPT_INT, &dupe, "duplicate", 'D', 0  },
>  	{ 0, 0, 0, 0, 0 }
>      };
>  
> @@ -424,7 +430,7 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[])
>  	break;
>      case OUTPUT_MESSAGES:
>      case OUTPUT_FILES:
> -	ret = do_search_messages (format, query, output, offset, limit);
> +	ret = do_search_messages (format, query, output, offset, limit, dupe);

Should there be an error message if duplicate=x is chosen with
output!=files?

Best wishes

Mark


>  	break;
>      case OUTPUT_TAGS:
>  	ret = do_search_tags (notmuch, format, query);
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch