From cd51321d28bce36e40fc077ac229e1fc319562e8 Mon Sep 17 00:00:00 2001 From: Jani Nikula Date: Mon, 26 Mar 2012 00:10:47 +0300 Subject: [PATCH] Re: [PATCH v2 2/3] cli: refactor "notmuch tag" query tagging into a separate function --- 4d/394d712a29747f6b9e21e492bb6794f6a7b582 | 268 ++++++++++++++++++++++ 1 file changed, 268 insertions(+) create mode 100644 4d/394d712a29747f6b9e21e492bb6794f6a7b582 diff --git a/4d/394d712a29747f6b9e21e492bb6794f6a7b582 b/4d/394d712a29747f6b9e21e492bb6794f6a7b582 new file mode 100644 index 000000000..3b5a4c50f --- /dev/null +++ b/4d/394d712a29747f6b9e21e492bb6794f6a7b582 @@ -0,0 +1,268 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +Received: from localhost (localhost [127.0.0.1]) + by olra.theworths.org (Postfix) with ESMTP id 50C57431FAF + for ; Sun, 25 Mar 2012 14:10:55 -0700 (PDT) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: -0.7 +X-Spam-Level: +X-Spam-Status: No, score=-0.7 tagged_above=-999 required=5 + tests=[RCVD_IN_DNSWL_LOW=-0.7] 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 ZO8g2gEUOwlL for ; + Sun, 25 Mar 2012 14:10:54 -0700 (PDT) +Received: from mail-lpp01m010-f53.google.com (mail-lpp01m010-f53.google.com + [209.85.215.53]) (using TLSv1 with cipher RC4-SHA (128/128 bits)) + (No client certificate requested) + by olra.theworths.org (Postfix) with ESMTPS id F4018431FAE + for ; Sun, 25 Mar 2012 14:10:53 -0700 (PDT) +Received: by lahc1 with SMTP id c1so3793358lah.26 + for ; Sun, 25 Mar 2012 14:10:52 -0700 (PDT) +X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; + d=google.com; s=20120113; + h=from:to:subject:in-reply-to:references:user-agent:date:message-id + :mime-version:content-type:x-gm-message-state; + bh=9hHmtG0snsLviFKubRbN/T7CeOjwgWpQm9ycyJDVLEI=; + b=mQ6P3qpT9zQrYUFolZcvLVd985gVGNkz0WCT7DsnIfZR938Izd6GIRK0xnrug0qJip + 8yH33JkEdkCZXwC1+H+3t4KhpWjmmltoLmdVmIZ2i4PQoa5fQ/ERpxu/xkWFMW44VofL + K8GjmC+WoD+PBiZ2revfi+11wK1BaS/7XU7PZihIt9GQ3i0fFLr1E0Y0geWmQFFHGNJi + yRnTU6zd55GGt3vxqaDmWVp5/Msy0Vz/gJA8W9sHIbjN9G5I4GHA88auXJ/hyexKbpDf + 8VkwrA24oHQxNdhXC4WyxjkWn8Q41wtvnTnMilpVUecYC26gf6ajFGFLD/sLtYV+ZtRe + F1/Q== +Received: by 10.112.39.169 with SMTP id q9mr7260399lbk.47.1332709852140; + Sun, 25 Mar 2012 14:10:52 -0700 (PDT) +Received: from localhost (dsl-hkibrasgw4-fe50f800-253.dhcp.inet.fi. + [84.248.80.253]) + by mx.google.com with ESMTPS id gn6sm19338860lbb.5.2012.03.25.14.10.49 + (version=SSLv3 cipher=OTHER); Sun, 25 Mar 2012 14:10:50 -0700 (PDT) +From: Jani Nikula +To: Tomi Ollila , notmuch@notmuchmail.org +Subject: Re: [PATCH v2 2/3] cli: refactor "notmuch tag" query tagging into a + separate function +In-Reply-To: +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) +User-Agent: Notmuch/0.11.1+295~g780f284 (http://notmuchmail.org) Emacs/23.3.1 + (i686-pc-linux-gnu) +Date: Mon, 26 Mar 2012 00:10:47 +0300 +Message-ID: <87limo2xpk.fsf@nikula.org> +MIME-Version: 1.0 +Content-Type: text/plain; charset=us-ascii +X-Gm-Message-State: + ALoCoQnMPoT6Xdvpr05QMUWU6EHaJpvE4fPv/jSjO4rjOxghfJqXI8hQsCkbclE1eCgLxz8qVqdl +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 21:10:55 -0000 + +On Sun, 25 Mar 2012 23:45:39 +0300, Tomi Ollila wrote: +> 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). + +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 -- 2.26.2