On Sun, 25 Mar 2012 23:45:39 +0300, Tomi Ollila <tomi.ollila@iki.fi> wrote: > 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). Mmmh, if you think of "tag" as a verb, it fairly accurately describes what the function does: tag (messages matching this) query (according given arguments). I don't mind changing, but can't come up with anything better right now either. notmuch_{search,query}_change_tags()? Meh? > 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'm not sure I follow you. AFAICT the behaviour does not change from what it was before, apart from the tag addition and removal being mixed together. > 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. Documentation good. :) > 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] == '-'); I usually don't add braces where they aren't needed, but can do here. > Everything else LGTM. Thanks for the review, Jani. > > 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