From: Tomi Ollila <tomi.ollila@iki.fi>
To: Jani Nikula <jani@nikula.org>, notmuch@notmuchmail.org
Subject: Re: [PATCH v2 2/3] cli: refactor "notmuch tag" query tagging into a
	separate function
In-Reply-To:
 <584067da00dd9f04a7f3982b76fd4b4918e4fe61.1332702915.git.jani@nikula.org>
References: <cover.1332702915.git.jani@nikula.org>
	<584067da00dd9f04a7f3982b76fd4b4918e4fe61.1332702915.git.jani@nikula.org>User-Agent:	Notmuch/0.12+71~gdacf76b (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-unknown-linux-gnu)
Date: Sun, 25 Mar 2012 23:45:39 +0300
Message-ID: <m262dse7f0.fsf@guru.guru-group.fi>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

Jani Nikula <jani@nikula.org> writes:

> Refactor to make tagging code easier to reuse in the future. No
> functional changes.
>
> Signed-off-by: Jani Nikula <jani@nikula.org>
> ---

tag_query() is pretty generic name for a function which (also) does
tagging operations (adds and removes tags based on tag_ops).

If, however, tag_opts != NULL (as is needs to be) but tag_opts[0] == NULL
(now tag_query() would not do any tagging operations, but
optimize_tag_query would mangle query string ( '*' to '()' and 
'any_other' to '( any_other ) and ()' )

I.e. IMO the function name should be more spesific & the fact that this
function needs tag changing data in tag_ops array should be documented.

---

Minor thing in patch #1:

 +	    tag_ops[tag_ops_count].remove = argv[i][0] == '-';

would be more clearer as:

 +	    tag_ops[tag_ops_count].remove = (argv[i][0] == '-');



Everything else LGTM.

Tomi


>  notmuch-tag.c |  101 ++++++++++++++++++++++++++++++++-------------------------
>  1 files changed, 57 insertions(+), 44 deletions(-)
>
> diff --git a/notmuch-tag.c b/notmuch-tag.c
> index c39b235..edbfdec 100644
> --- a/notmuch-tag.c
> +++ b/notmuch-tag.c
> @@ -106,6 +106,60 @@ _optimize_tag_query (void *ctx, const char *orig_query_string,
>      return query_string;
>  }
>  
> +static int
> +tag_query (void *ctx, notmuch_database_t *notmuch, const char *query_string,
> +	   tag_operation_t *tag_ops, notmuch_bool_t synchronize_flags)
> +{
> +    notmuch_query_t *query;
> +    notmuch_messages_t *messages;
> +    notmuch_message_t *message;
> +    int i;
> +
> +    /* Optimize the query so it excludes messages that already have
> +     * the specified set of tags. */
> +    query_string = _optimize_tag_query (ctx, query_string, tag_ops);
> +    if (query_string == NULL) {
> +	fprintf (stderr, "Out of memory.\n");
> +	return 1;
> +    }
> +
> +    query = notmuch_query_create (notmuch, query_string);
> +    if (query == NULL) {
> +	fprintf (stderr, "Out of memory.\n");
> +	return 1;
> +    }
> +
> +    /* tagging is not interested in any special sort order */
> +    notmuch_query_set_sort (query, NOTMUCH_SORT_UNSORTED);
> +
> +    for (messages = notmuch_query_search_messages (query);
> +	 notmuch_messages_valid (messages) && !interrupted;
> +	 notmuch_messages_move_to_next (messages))
> +    {
> +	message = notmuch_messages_get (messages);
> +
> +	notmuch_message_freeze (message);
> +
> +	for (i = 0; tag_ops[i].tag; i++) {
> +	    if (tag_ops[i].remove)
> +		notmuch_message_remove_tag (message, tag_ops[i].tag);
> +	    else
> +		notmuch_message_add_tag (message, tag_ops[i].tag);
> +	}
> +
> +	notmuch_message_thaw (message);
> +
> +	if (synchronize_flags)
> +	    notmuch_message_tags_to_maildir_flags (message);
> +
> +	notmuch_message_destroy (message);
> +    }
> +
> +    notmuch_query_destroy (query);
> +
> +    return interrupted;
> +}
> +
>  int
>  notmuch_tag_command (void *ctx, int argc, char *argv[])
>  {
> @@ -114,12 +168,10 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
>      char *query_string;
>      notmuch_config_t *config;
>      notmuch_database_t *notmuch;
> -    notmuch_query_t *query;
> -    notmuch_messages_t *messages;
> -    notmuch_message_t *message;
>      struct sigaction action;
>      notmuch_bool_t synchronize_flags;
>      int i;
> +    int ret;
>  
>      /* Setup our handler for SIGINT */
>      memset (&action, 0, sizeof (struct sigaction));
> @@ -166,14 +218,6 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
>  	return 1;
>      }
>  
> -    /* Optimize the query so it excludes messages that already have
> -     * the specified set of tags. */
> -    query_string = _optimize_tag_query (ctx, query_string, tag_ops);
> -    if (query_string == NULL) {
> -	fprintf (stderr, "Out of memory.\n");
> -	return 1;
> -    }
> -
>      config = notmuch_config_open (ctx, NULL, NULL);
>      if (config == NULL)
>  	return 1;
> @@ -185,40 +229,9 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
>  
>      synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);
>  
> -    query = notmuch_query_create (notmuch, query_string);
> -    if (query == NULL) {
> -	fprintf (stderr, "Out of memory.\n");
> -	return 1;
> -    }
> -
> -    /* tagging is not interested in any special sort order */
> -    notmuch_query_set_sort (query, NOTMUCH_SORT_UNSORTED);
> +    ret = tag_query (ctx, notmuch, query_string, tag_ops, synchronize_flags);
>  
> -    for (messages = notmuch_query_search_messages (query);
> -	 notmuch_messages_valid (messages) && !interrupted;
> -	 notmuch_messages_move_to_next (messages))
> -    {
> -	message = notmuch_messages_get (messages);
> -
> -	notmuch_message_freeze (message);
> -
> -	for (i = 0; tag_ops[i].tag; i++) {
> -	    if (tag_ops[i].remove)
> -		notmuch_message_remove_tag (message, tag_ops[i].tag);
> -	    else
> -		notmuch_message_add_tag (message, tag_ops[i].tag);
> -	}
> -
> -	notmuch_message_thaw (message);
> -
> -	if (synchronize_flags)
> -	    notmuch_message_tags_to_maildir_flags (message);
> -
> -	notmuch_message_destroy (message);
> -    }
> -
> -    notmuch_query_destroy (query);
>      notmuch_database_close (notmuch);
>  
> -    return interrupted;
> +    return ret;
>  }
> -- 
> 1.7.5.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch