[PATCH 0/3] cli: notmuch tag/restore refactoring

[PATCH 0/3] cli: notmuch tag/restore refactoring

[Jani Nikula]

Hi, here's some refactoring of notmuch tag/restore with mostly
non-functional changes, in preparation of later, more interesting
changes to dump/restore/tag.

BR,
Jani.


Jani Nikula (3):
  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-client.h  |    5 +
 notmuch-restore.c |   73 +---------------
 notmuch-tag.c     |  242 +++++++++++++++++++++++++++++++++++------------------
 3 files changed, 169 insertions(+), 151 deletions(-)

-- 
1.7.5.4

[PATCH 1/3] 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.

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

diff --git a/notmuch-tag.c b/notmuch-tag.c
index 36b9b09..98b2126 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,
@@ -74,12 +78,9 @@ _optimize_tag_query (void *ctx, const char *orig_query_string, char *argv[],
     /* 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 +91,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++) {
-	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++) {
+    for (i = 0; tag_ops[i].tag && 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 +109,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 +128,34 @@ 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_operation_t) {
+		.tag = argv[i] + 1,
+		.remove = argv[i][0] == '-',
+	    };
 	} 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 +169,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 +203,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);
 
-- 
1.7.5.4

[PATCH 2/3] 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 |  101 ++++++++++++++++++++++++++++++++-------------------------
 1 files changed, 57 insertions(+), 44 deletions(-)

diff --git a/notmuch-tag.c b/notmuch-tag.c
index 98b2126..c4e3f9d 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));
@@ -167,14 +219,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;
@@ -186,40 +230,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 3/3] 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-client.h  |    5 +++
 notmuch-restore.c |   73 ++-------------------------------------------------
 notmuch-tag.c     |   75 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 83 insertions(+), 70 deletions(-)

diff --git a/notmuch-client.h b/notmuch-client.h
index fa04fa2..7e3deee 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -203,6 +203,11 @@ json_quote_chararray (const void *ctx, const char *str, const size_t len);
 char *
 json_quote_str (const void *ctx, const char *str);
 
+int
+tag_message (void *ctx, notmuch_database_t *notmuch, const char *message_id,
+	     char *file_tags, notmuch_bool_t remove_all,
+	     notmuch_bool_t synchronize_flags);
+
 /* notmuch-config.c */
 
 typedef struct _notmuch_config notmuch_config_t;
diff --git a/notmuch-restore.c b/notmuch-restore.c
index 87d9772..bd6b884 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -88,11 +88,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 +105,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 (ctx, notmuch, message_id, file_tags, !accumulate,
+		     synchronize_flags);
 
-      NEXT_LINE:
-	if (message)
-	    notmuch_message_destroy (message);
-	message = NULL;
 	free (message_id);
 	free (file_tags);
     }
diff --git a/notmuch-tag.c b/notmuch-tag.c
index c4e3f9d..3aeb878 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -161,6 +161,81 @@ tag_query (void *ctx, notmuch_database_t *notmuch, const char *query_string,
 }
 
 int
+tag_message (void *ctx, 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_tag_command (void *ctx, int argc, char *argv[])
 {
     tag_operation_t *tag_ops;
-- 
1.7.5.4

Re: [PATCH 1/3] cli: refactor "notmuch tag" data structures for tagging operations

[David Bremner]

On Sat, 24 Mar 2012 18:14:35 +0200, Jani Nikula <jani@nikula.org> wrote:
> +	if (argv[i][0] == '+' || argv[i][0] == '-') {
> +	    tag_ops[tag_ops_count++] = (tag_operation_t) {
> +		.tag = argv[i] + 1,
> +		.remove = argv[i][0] == '-',
> +	    };

I'm not sure if this is a worthwhile use of a C99. Wouldn't it be
simpler to just use two assignments? and maybe increment the index
after? Still 3 lines of code.

Other than that, this patch looked ok to me.  I think it probably
deserves a NEWS patch that the ordering behaviour changed. I do think
the new order is more sensible, and the old one was never documented.


d

Re: [PATCH 1/3] cli: refactor "notmuch tag" data structures for tagging operations

[David Bremner]

On Sat, 24 Mar 2012 18:14:35 +0200, Jani Nikula <jani@nikula.org> wrote:
> 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.

Like I said on IRC, my only minor reservation with this patch is that
the use of C99 struct assignment is maybe not a good tradeoff here,
since it makes the code a bit more mysterious (for the great unwashed
masses that don't know C99 as well as C89). Still, if you think extra
initialization features are worth it, I can live with it. 

d

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

[David Bremner]

On Sat, 24 Mar 2012 18:14:37 +0200, Jani Nikula <jani@nikula.org> wrote:
> Refactor to make tagging code easier to reuse in the future. No
> functional changes.

The second two look OK as well.  It isn't obvious to me that in the long
term we want an API using "file_tags" to pass in a string. It reminds me
of the term "stringly typed". Of course, I see why it is this way in an
intial refactor, but maybe something to think about before we start
propagating such an API.

d