[PATCH v2 2/3] cli: refactor "notmuch tag" query tagging into a separate function
Jani Nikula
jani at nikula.org
Mon Mar 26 00:38:47 PDT 2012
On Mon, 26 Mar 2012 00:26:50 +0300, Tomi Ollila <tomi.ollila at iki.fi> wrote:
> Jani Nikula <jani at nikula.org> writes:
>
> > On Sun, 25 Mar 2012 23:45:39 +0300, Tomi Ollila <tomi.ollila at iki.fi> wrote:
> >> Jani Nikula <jani at nikula.org> writes:
> >>
> >> > Refactor to make tagging code easier to reuse in the future. No
> >> > functional changes.
> >> >
> >> > Signed-off-by: Jani Nikula <jani at 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.
Actually, no. In the future (the follow-up patches are in my local
tree...) tag_query() should work also when there are no tagging
operations. I'll either need to fix tag_query() or _optimize_tag_query()
to handle this gracefully. Thanks for spelling this out for me! :)
> >> 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.
Sounds reasonable.
BR,
Jani.
>
> >> 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 at notmuchmail.org
> >> > http://notmuchmail.org/mailman/listinfo/notmuch
> > _______________________________________________
> > notmuch mailing list
> > notmuch at notmuchmail.org
> > http://notmuchmail.org/mailman/listinfo/notmuch
More information about the notmuch
mailing list