From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by olra.theworths.org (Postfix) with ESMTP id 3CA5F431FAF for ; Sun, 25 Mar 2012 13:45:42 -0700 (PDT) X-Virus-Scanned: Debian amavisd-new at olra.theworths.org X-Spam-Flag: NO X-Spam-Score: 0 X-Spam-Level: X-Spam-Status: No, score=0 tagged_above=-999 required=5 tests=[none] autolearn=disabled Received: from olra.theworths.org ([127.0.0.1]) by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id e-2IhMkHqoPm for ; Sun, 25 Mar 2012 13:45:41 -0700 (PDT) Received: from guru.guru-group.fi (guru-group.fi [87.108.86.66]) by olra.theworths.org (Postfix) with ESMTP id 317EB431FAE for ; Sun, 25 Mar 2012 13:45:41 -0700 (PDT) Received: by guru.guru-group.fi (Postfix, from userid 501) id CD4E668055; Sun, 25 Mar 2012 23:45:39 +0300 (EEST) From: Tomi Ollila To: Jani Nikula , 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: <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) X-Face: HhBM'cA~ MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: "Use and development of the notmuch mail system." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 25 Mar 2012 20:45:42 -0000 Jani Nikula writes: > Refactor to make tagging code easier to reuse in the future. No > functional changes. > > Signed-off-by: Jani Nikula > --- 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