From: Mark Walters <markwalters1009@gmail.com>
To: Austin Clements <amdragon@MIT.EDU>
Subject: Re: [PATCH 1/7] cli: add --do-not-exclude option to count and search.
In-Reply-To: <20120131041732.GY17991@mit.edu>
References: <8762fu4aqt.fsf@qmul.ac.uk>
	<1327862394-14334-1-git-send-email-markwalters1009@gmail.com>
	<20120131041732.GY17991@mit.edu>
User-Agent: Notmuch/0.11+137~g98adc3d (http://notmuchmail.org) Emacs/23.2.1
	(i486-pc-linux-gnu)
Date: Tue, 31 Jan 2012 11:40:00 +0000
Message-ID: <871uqgdrsf.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: notmuch@notmuchmail.org
Precedence: list


Thanks for the review. Almost all of it (for all all the patches) I
agree with and will just fix but I do have a couple of queries.

On Mon, 30 Jan 2012 23:17:32 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> Quoth Mark Walters on Jan 29 at  6:39 pm:
> > This option turns off the exclusion so all matching messages are
> > returned. We do not need to add this to show as notmuch-show does not
> > (yet) exclude.
> > ---
> >  notmuch-count.c  |   12 ++++++++----
> >  notmuch-search.c |   12 ++++++++----
> >  2 files changed, 16 insertions(+), 8 deletions(-)
> > 
> > diff --git a/notmuch-count.c b/notmuch-count.c
> > index 63459fb..c88975e 100644
> > --- a/notmuch-count.c
> > +++ b/notmuch-count.c
> > @@ -37,6 +37,7 @@ notmuch_count_command (void *ctx, int argc, char *argv[])
> >      int output = OUTPUT_MESSAGES;
> >      const char **search_exclude_tags;
> >      size_t search_exclude_tags_length;
> > +    notmuch_bool_t do_not_exclude = FALSE;
> >      unsigned int i;
> >  
> >      notmuch_opt_desc_t options[] = {
> > @@ -44,6 +45,7 @@ notmuch_count_command (void *ctx, int argc, char *argv[])
> >  	  (notmuch_keyword_t []){ { "threads", OUTPUT_THREADS },
> >  				  { "messages", OUTPUT_MESSAGES },
> >  				  { 0, 0 } } },
> > +	{ NOTMUCH_OPT_BOOLEAN,  &do_not_exclude, "do-not-exclude", 'd', 0 },
> 
> Maybe just "no-exclude"?  "do-not-exclude" seems needlessly verbose.

The reason I went for verbose do-not-exclude was to try and avoid the
double negative ambiguity: does no-exclude mean do-not-exclude or
do-note-return-excluded-messages. Possibly I am worrying needlessly, and
obviously I am quite happy to change.

> Also, you have an extra space after the first comma.

Will fix.

> 
> >  	{ 0, 0, 0, 0, 0 }
> >      };
> >  
> > @@ -78,10 +80,12 @@ notmuch_count_command (void *ctx, int argc, char *argv[])
> >  	return 1;
> >      }
> >  
> > -    search_exclude_tags = notmuch_config_get_search_exclude_tags
> > -	(config, &search_exclude_tags_length);
> > -    for (i = 0; i < search_exclude_tags_length; i++)
> > -	notmuch_query_add_tag_exclude (query, search_exclude_tags[i]);
> > +    if (!do_not_exclude) {
> 
> You could move search_exclude_tags and search_exclude_tags_length in
> here now that it's a block (but you don't have to).

Will fix

> > +	search_exclude_tags = notmuch_config_get_search_exclude_tags
> > +	    (config, &search_exclude_tags_length);
> > +	for (i = 0; i < search_exclude_tags_length; i++)
> > +	    notmuch_query_add_tag_exclude (query, search_exclude_tags[i]);
> > +    }
> >  
> >      switch (output) {
> >      case OUTPUT_MESSAGES:
> > diff --git a/notmuch-search.c b/notmuch-search.c
> > index d504051..084dd05 100644
> > --- a/notmuch-search.c
> > +++ b/notmuch-search.c
> > @@ -425,6 +425,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
> >      int limit = -1; /* unlimited */
> >      const char **search_exclude_tags;
> >      size_t search_exclude_tags_length;
> > +    notmuch_bool_t do_not_exclude = FALSE;
> >      unsigned int i;
> >  
> >      enum { NOTMUCH_FORMAT_JSON, NOTMUCH_FORMAT_TEXT }
> > @@ -446,6 +447,7 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
> >  				  { "files", OUTPUT_FILES },
> >  				  { "tags", OUTPUT_TAGS },
> >  				  { 0, 0 } } },
> > +        { NOTMUCH_OPT_BOOLEAN,  &do_not_exclude, "do-not-exclude", 'd', 0 },
> 
> Same.

Will fix

> 
> >  	{ NOTMUCH_OPT_INT, &offset, "offset", 'O', 0 },
> >  	{ NOTMUCH_OPT_INT, &limit, "limit", 'L', 0  },
> >  	{ 0, 0, 0, 0, 0 }
> > @@ -493,10 +495,12 @@ notmuch_search_command (void *ctx, int argc, char *argv[])
> >  
> >      notmuch_query_set_sort (query, sort);
> >  
> > -    search_exclude_tags = notmuch_config_get_search_exclude_tags
> > -	(config, &search_exclude_tags_length);
> > -    for (i = 0; i < search_exclude_tags_length; i++)
> > -	notmuch_query_add_tag_exclude (query, search_exclude_tags[i]);
> > +    if (!do_not_exclude) {
> > +	search_exclude_tags = notmuch_config_get_search_exclude_tags
> > +	    (config, &search_exclude_tags_length);
> > +	for (i = 0; i < search_exclude_tags_length; i++)
> > +	    notmuch_query_add_tag_exclude (query, search_exclude_tags[i]);
> > +    }
> >  
> >      switch (output) {
> >      default:

Thanks

Mark