Jani Nikula <jani@nikula.org> writes: > 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? Hmmm, yes... -- tag...query... ok, I can live with that if no-one else get same impression I got first... > >> 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. The thing is that notmuch_tag_command () check that there is at least one tagging operation to be done, but tag_query () doesn't do such checking... >> 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. :) ... therefore the requirement that there needs to tagging information in tag_ops is in place. > >> 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. Assigment is much lower in precedence than equality comparison -- but as this is so seldom-used construct, humans read (with understanding) the code faster with this added clarity. >> Everything else LGTM. > > Thanks for the review, > Jani. > >> >> Tomi 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 > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch