From: Jani Nikula <jani@nikula.org>
To: notmuch@notmuchmail.org
Subject: [PATCH v3 2/4] cli: refactor "notmuch tag" data structures for
	tagging operations
Date: Tue, 27 Mar 2012 00:04:09 +0300
Message-Id:
 <dc6a35c80e3d56fabebf3f577470f0f015377204.1332795415.git.jani@nikula.org>
In-Reply-To: <cover.1332795415.git.jani@nikula.org>
References: <cover.1332795415.git.jani@nikula.org>
In-Reply-To: <cover.1332795415.git.jani@nikula.org>
References: <cover.1332795415.git.jani@nikula.org>
Cc: tomi.ollila@iki.fi
Precedence: list

To simplify code, keep all tagging operations in a single array
instead of separate add and remove arrays. Apply tag changes in the
order specified on the command line, instead of first removing and
then adding the tags.

This results in a minor functional change: If a tag is both added and
removed, the last specified operation is now used. Previously the tag
was always added. Change the relevant test to reflect the new
behaviour.

Signed-off-by: Jani Nikula <jani@nikula.org>
---
 notmuch-tag.c |   83 ++++++++++++++++++++++++++------------------------------
 test/tagging  |    2 +-
 2 files changed, 40 insertions(+), 45 deletions(-)

diff --git a/notmuch-tag.c b/notmuch-tag.c
index 36b9b09..0a6b140 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -53,10 +53,14 @@ _escape_tag (char *buf, const char *tag)
     return buf;
 }
 
+typedef struct {
+    const char *tag;
+    notmuch_bool_t remove;
+} tag_operation_t;
+
 static char *
-_optimize_tag_query (void *ctx, const char *orig_query_string, char *argv[],
-		     int *add_tags, int add_tags_count,
-		     int *remove_tags, int remove_tags_count)
+_optimize_tag_query (void *ctx, const char *orig_query_string,
+		     const tag_operation_t *tag_ops)
 {
     /* This is subtler than it looks.  Xapian ignores the '-' operator
      * at the beginning both queries and parenthesized groups and,
@@ -71,15 +75,16 @@ _optimize_tag_query (void *ctx, const char *orig_query_string, char *argv[],
     int i;
     unsigned int max_tag_len = 0;
 
+    /* Don't optimize if there are no tag changes. */
+    if (tag_ops[0].tag == NULL)
+	return talloc_strdup (ctx, orig_query_string);
+
     /* Allocate a buffer for escaping tags.  This is large enough to
      * hold a fully escaped tag with every character doubled plus
      * enclosing quotes and a NUL. */
-    for (i = 0; i < add_tags_count; i++)
-	if (strlen (argv[add_tags[i]] + 1) > max_tag_len)
-	    max_tag_len = strlen (argv[add_tags[i]] + 1);
-    for (i = 0; i < remove_tags_count; i++)
-	if (strlen (argv[remove_tags[i]] + 1) > max_tag_len)
-	    max_tag_len = strlen (argv[remove_tags[i]] + 1);
+    for (i = 0; tag_ops[i].tag; i++)
+	if (strlen (tag_ops[i].tag) > max_tag_len)
+	    max_tag_len = strlen (tag_ops[i].tag);
     escaped = talloc_array(ctx, char, max_tag_len * 2 + 3);
     if (!escaped)
 	return NULL;
@@ -90,16 +95,11 @@ _optimize_tag_query (void *ctx, const char *orig_query_string, char *argv[],
     else
 	query_string = talloc_asprintf (ctx, "( %s ) and (", orig_query_string);
 
-    for (i = 0; i < add_tags_count && query_string; i++) {
+    for (i = 0; tag_ops[i].tag && query_string; i++) {
 	query_string = talloc_asprintf_append_buffer (
-	    query_string, "%snot tag:%s", join,
-	    _escape_tag (escaped, argv[add_tags[i]] + 1));
-	join = " or ";
-    }
-    for (i = 0; i < remove_tags_count && query_string; i++) {
-	query_string = talloc_asprintf_append_buffer (
-	    query_string, "%stag:%s", join,
-	    _escape_tag (escaped, argv[remove_tags[i]] + 1));
+	    query_string, "%s%stag:%s", join,
+	    tag_ops[i].remove ? "" : "not ",
+	    _escape_tag (escaped, tag_ops[i].tag));
 	join = " or ";
     }
 
@@ -113,9 +113,8 @@ _optimize_tag_query (void *ctx, const char *orig_query_string, char *argv[],
 int
 notmuch_tag_command (void *ctx, int argc, char *argv[])
 {
-    int *add_tags, *remove_tags;
-    int add_tags_count = 0;
-    int remove_tags_count = 0;
+    tag_operation_t *tag_ops;
+    int tag_ops_count = 0;
     char *query_string;
     notmuch_config_t *config;
     notmuch_database_t *notmuch;
@@ -133,35 +132,33 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
     action.sa_flags = SA_RESTART;
     sigaction (SIGINT, &action, NULL);
 
-    add_tags = talloc_size (ctx, argc * sizeof (int));
-    if (add_tags == NULL) {
-	fprintf (stderr, "Out of memory.\n");
-	return 1;
-    }
+    argc--; argv++; /* skip subcommand argument */
 
-    remove_tags = talloc_size (ctx, argc * sizeof (int));
-    if (remove_tags == NULL) {
+    /* Array of tagging operations (add or remove), terminated with an
+     * empty element. */
+    tag_ops = talloc_array (ctx, tag_operation_t, argc + 1);
+    if (tag_ops == NULL) {
 	fprintf (stderr, "Out of memory.\n");
 	return 1;
     }
 
-    argc--; argv++; /* skip subcommand argument */
-
     for (i = 0; i < argc; i++) {
 	if (strcmp (argv[i], "--") == 0) {
 	    i++;
 	    break;
 	}
-	if (argv[i][0] == '+') {
-	    add_tags[add_tags_count++] = i;
-	} else if (argv[i][0] == '-') {
-	    remove_tags[remove_tags_count++] = i;
+	if (argv[i][0] == '+' || argv[i][0] == '-') {
+	    tag_ops[tag_ops_count].tag = argv[i] + 1;
+	    tag_ops[tag_ops_count].remove = (argv[i][0] == '-');
+	    tag_ops_count++;
 	} else {
 	    break;
 	}
     }
 
-    if (add_tags_count == 0 && remove_tags_count == 0) {
+    tag_ops[tag_ops_count].tag = NULL;
+
+    if (tag_ops_count == 0) {
 	fprintf (stderr, "Error: 'notmuch tag' requires at least one tag to add or remove.\n");
 	return 1;
     }
@@ -175,9 +172,7 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
 
     /* Optimize the query so it excludes messages that already have
      * the specified set of tags. */
-    query_string = _optimize_tag_query (ctx, query_string, argv,
-					add_tags, add_tags_count,
-					remove_tags, remove_tags_count);
+    query_string = _optimize_tag_query (ctx, query_string, tag_ops);
     if (query_string == NULL) {
 	fprintf (stderr, "Out of memory.\n");
 	return 1;
@@ -211,12 +206,12 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
 
 	notmuch_message_freeze (message);
 
-	for (i = 0; i < remove_tags_count; i++)
-	    notmuch_message_remove_tag (message,
-					argv[remove_tags[i]] + 1);
-
-	for (i = 0; i < add_tags_count; i++)
-	    notmuch_message_add_tag (message, argv[add_tags[i]] + 1);
+	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);
 
diff --git a/test/tagging b/test/tagging
index 3acf1bc..e4782ed 100755
--- a/test/tagging
+++ b/test/tagging
@@ -43,7 +43,7 @@ notmuch tag +tag4 -tag4 One
 notmuch tag -tag4 +tag4 Two
 output=$(notmuch search \* | notmuch_search_sanitize)
 test_expect_equal "$output" "\
-thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; One (:\"  inbox tag1 tag4 unread)
+thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; One (:\"  inbox tag1 unread)
 thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Two (inbox tag1 tag4 unread)"
 
 test_done
-- 
1.7.5.4