[PATCH v3 0/4] cli: notmuch tag/restore refactoring

[PATCH v3 0/4] cli: notmuch tag/restore refactoring

[Jani Nikula]

v3 of id:"cover.1332702915.git.jani@nikula.org" with the following
mostly non-functional changes:

 - add test for the current tagging behaviour in patch 1, and change the
   test in patch 2 when the behaviour is changed
 - handle the no tag changes case in _optimize_tag_query() in patch 2
 - add braces around the "tag_ops[tag_ops_count].remove = (argv[i][0] == '-');"
   assignment
 - document the tag_query() function a bit in patch 3
 - make tag_message() static in patch 4

BR,
Jani.


Jani Nikula (4):
  test: add test for both adding and removing a tag at the same time
  cli: refactor "notmuch tag" data structures for tagging operations
  cli: refactor "notmuch tag" query tagging into a separate function
  cli: refactor "notmuch restore" message tagging into a separate
    function

 notmuch-restore.c |  148 ++++++++++++++++++++++++---------------------
 notmuch-tag.c     |  173 ++++++++++++++++++++++++++++-------------------------
 test/tagging      |    8 +++
 3 files changed, 178 insertions(+), 151 deletions(-)

-- 
1.7.5.4

[PATCH v3 1/4] test: add test for both adding and removing a tag at the same time

[Jani Nikula]

The current behaviour is that regardless of the order in which the
addition and removal of a tag are specified, the tag is added.

Signed-off-by: Jani Nikula <jani@nikula.org>
---
 test/tagging |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/test/tagging b/test/tagging
index 77202bf..3acf1bc 100755
--- a/test/tagging
+++ b/test/tagging
@@ -38,4 +38,12 @@ test_expect_equal "$output" "\
 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 unread)"
 
+test_begin_subtest "Tagging order"
+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; Two (inbox tag1 tag4 unread)"
+
 test_done
-- 
1.7.5.4

[PATCH v3 2/4] cli: refactor "notmuch tag" data structures for tagging operations

[Jani Nikula]

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

[PATCH v3 3/4] cli: refactor "notmuch tag" query tagging into a separate function

[Jani Nikula]

Refactor to make tagging code easier to reuse in the future. No
functional changes.

Signed-off-by: Jani Nikula <jani@nikula.org>
---
 notmuch-tag.c |  104 +++++++++++++++++++++++++++++++++------------------------
 1 files changed, 60 insertions(+), 44 deletions(-)

diff --git a/notmuch-tag.c b/notmuch-tag.c
index 0a6b140..05feed3 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -110,6 +110,63 @@ _optimize_tag_query (void *ctx, const char *orig_query_string,
     return query_string;
 }
 
+/* Tag messages matching 'query_string' according to 'tag_ops', which
+ * must be an array of tagging operations terminated with an empty
+ * element. */
+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[])
 {
@@ -118,12 +175,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));
@@ -170,14 +225,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;
@@ -189,40 +236,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

[PATCH v3 4/4] cli: refactor "notmuch restore" message tagging into a separate function

[Jani Nikula]

Refactor to make tagging code easier to reuse in the future. No
functional changes.

Signed-off-by: Jani Nikula <jani@nikula.org>
---
 notmuch-restore.c |  148 ++++++++++++++++++++++++++++-------------------------
 1 files changed, 78 insertions(+), 70 deletions(-)

diff --git a/notmuch-restore.c b/notmuch-restore.c
index 87d9772..d3b9246 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -20,6 +20,81 @@
 
 #include "notmuch-client.h"
 
+static int
+tag_message (notmuch_database_t *notmuch, const char *message_id,
+	     char *file_tags, notmuch_bool_t remove_all,
+	     notmuch_bool_t synchronize_flags)
+{
+    notmuch_status_t status;
+    notmuch_tags_t *db_tags;
+    char *db_tags_str;
+    notmuch_message_t *message = NULL;
+    const char *tag;
+    char *next;
+    int ret = 0;
+
+    status = notmuch_database_find_message (notmuch, message_id, &message);
+    if (status || message == NULL) {
+	fprintf (stderr, "Warning: Cannot apply tags to %smessage: %s\n",
+		 message ? "" : "missing ", message_id);
+	if (status)
+	    fprintf (stderr, "%s\n", notmuch_status_to_string(status));
+	return 1;
+    }
+
+    /* In order to detect missing messages, this check/optimization is
+     * intentionally done *after* first finding the message. */
+    if (!remove_all && (file_tags == NULL || *file_tags == '\0'))
+	goto DONE;
+
+    db_tags_str = NULL;
+    for (db_tags = notmuch_message_get_tags (message);
+	 notmuch_tags_valid (db_tags);
+	 notmuch_tags_move_to_next (db_tags)) {
+	tag = notmuch_tags_get (db_tags);
+
+	if (db_tags_str)
+	    db_tags_str = talloc_asprintf_append (db_tags_str, " %s", tag);
+	else
+	    db_tags_str = talloc_strdup (message, tag);
+    }
+
+    if (((file_tags == NULL || *file_tags == '\0') &&
+	 (db_tags_str == NULL || *db_tags_str == '\0')) ||
+	(file_tags && db_tags_str && strcmp (file_tags, db_tags_str) == 0))
+	goto DONE;
+
+    notmuch_message_freeze (message);
+
+    if (remove_all)
+	notmuch_message_remove_all_tags (message);
+
+    next = file_tags;
+    while (next) {
+	tag = strsep (&next, " ");
+	if (*tag == '\0')
+	    continue;
+	status = notmuch_message_add_tag (message, tag);
+	if (status) {
+	    fprintf (stderr, "Error applying tag %s to message %s:\n",
+		     tag, message_id);
+	    fprintf (stderr, "%s\n", notmuch_status_to_string (status));
+	    ret = 1;
+	}
+    }
+
+    notmuch_message_thaw (message);
+
+    if (synchronize_flags)
+	notmuch_message_tags_to_maildir_flags (message);
+
+DONE:
+    if (message)
+	notmuch_message_destroy (message);
+
+    return ret;
+}
+
 int
 notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
 {
@@ -88,11 +163,7 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
 
     while ((line_len = getline (&line, &line_size, input)) != -1) {
 	regmatch_t match[3];
-	char *message_id, *file_tags, *tag, *next;
-	notmuch_message_t *message = NULL;
-	notmuch_status_t status;
-	notmuch_tags_t *db_tags;
-	char *db_tags_str;
+	char *message_id, *file_tags;
 
 	chomp_newline (line);
 
@@ -109,72 +180,9 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
 	file_tags = xstrndup (line + match[2].rm_so,
 			      match[2].rm_eo - match[2].rm_so);
 
-	status = notmuch_database_find_message (notmuch, message_id, &message);
-	if (status || message == NULL) {
-	    fprintf (stderr, "Warning: Cannot apply tags to %smessage: %s\n",
-		     message ? "" : "missing ", message_id);
-	    if (status)
-		fprintf (stderr, "%s\n",
-			 notmuch_status_to_string(status));
-	    goto NEXT_LINE;
-	}
-
-	/* In order to detect missing messages, this check/optimization is
-	 * intentionally done *after* first finding the message.  */
-	if (accumulate && (file_tags == NULL || *file_tags == '\0'))
-	{
-	    goto NEXT_LINE;
-	}
-
-	db_tags_str = NULL;
-	for (db_tags = notmuch_message_get_tags (message);
-	     notmuch_tags_valid (db_tags);
-	     notmuch_tags_move_to_next (db_tags))
-	{
-	    const char *tag = notmuch_tags_get (db_tags);
-
-	    if (db_tags_str)
-		db_tags_str = talloc_asprintf_append (db_tags_str, " %s", tag);
-	    else
-		db_tags_str = talloc_strdup (message, tag);
-	}
-
-	if (((file_tags == NULL || *file_tags == '\0') &&
-	     (db_tags_str == NULL || *db_tags_str == '\0')) ||
-	    (file_tags && db_tags_str && strcmp (file_tags, db_tags_str) == 0))
-	{
-	    goto NEXT_LINE;
-	}
-
-	notmuch_message_freeze (message);
-
-	if (!accumulate)
-	    notmuch_message_remove_all_tags (message);
-
-	next = file_tags;
-	while (next) {
-	    tag = strsep (&next, " ");
-	    if (*tag == '\0')
-		continue;
-	    status = notmuch_message_add_tag (message, tag);
-	    if (status) {
-		fprintf (stderr,
-			 "Error applying tag %s to message %s:\n",
-			 tag, message_id);
-		fprintf (stderr, "%s\n",
-			 notmuch_status_to_string (status));
-	    }
-	}
-
-	notmuch_message_thaw (message);
-
-	if (synchronize_flags)
-	    notmuch_message_tags_to_maildir_flags (message);
+	tag_message (notmuch, message_id, file_tags, !accumulate,
+		     synchronize_flags);
 
-      NEXT_LINE:
-	if (message)
-	    notmuch_message_destroy (message);
-	message = NULL;
 	free (message_id);
 	free (file_tags);
     }
-- 
1.7.5.4

Re: [PATCH v3 0/4] cli: notmuch tag/restore refactoring

[Tomi Ollila]

On Tue, Mar 27 2012, Jani Nikula <jani@nikula.org> wrote:

> v3 of id:"cover.1332702915.git.jani@nikula.org" with the following
> mostly non-functional changes:
>
>  - add test for the current tagging behaviour in patch 1, and change the
>    test in patch 2 when the behaviour is changed
>  - handle the no tag changes case in _optimize_tag_query() in patch 2
>  - add braces around the "tag_ops[tag_ops_count].remove = (argv[i][0] == '-');"
>    assignment
>  - document the tag_query() function a bit in patch 3
>  - make tag_message() static in patch 4

LGTM.

>
> BR,
> Jani.

Tomi

>
>
> Jani Nikula (4):
>   test: add test for both adding and removing a tag at the same time
>   cli: refactor "notmuch tag" data structures for tagging operations
>   cli: refactor "notmuch tag" query tagging into a separate function
>   cli: refactor "notmuch restore" message tagging into a separate
>     function
>
>  notmuch-restore.c |  148 ++++++++++++++++++++++++---------------------
>  notmuch-tag.c     |  173 ++++++++++++++++++++++++++++-------------------------
>  test/tagging      |    8 +++
>  3 files changed, 178 insertions(+), 151 deletions(-)
>
> -- 
> 1.7.5.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3 0/4] cli: notmuch tag/restore refactoring

[Austin Clements]

Quoth Jani Nikula on Mar 27 at 12:04 am:
> v3 of id:"cover.1332702915.git.jani@nikula.org" with the following
> mostly non-functional changes:
> 
>  - add test for the current tagging behaviour in patch 1, and change the
>    test in patch 2 when the behaviour is changed
>  - handle the no tag changes case in _optimize_tag_query() in patch 2
>  - add braces around the "tag_ops[tag_ops_count].remove = (argv[i][0] == '-');"
>    assignment
>  - document the tag_query() function a bit in patch 3
>  - make tag_message() static in patch 4
> 
> BR,
> Jani.

LGTM.

Re: [PATCH v3 0/4] cli: notmuch tag/restore refactoring

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> v3 of id:"cover.1332702915.git.jani@nikula.org" with the following
> mostly non-functional changes:

pushed.

d