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

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

[Jani Nikula]

v2 of id:"cover.1332604895.git.jani@nikula.org" with the following
non-functional changes, addressing David's concerns in mail and IRC:

 - do not use C99 style struct assignment in patch 1
 - for now, keep tag_message() local to notmuch-restore.c in patch 3

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-restore.c |  148 +++++++++++++++++++++++++----------------------
 notmuch-tag.c     |  166 +++++++++++++++++++++++++++--------------------------
 2 files changed, 163 insertions(+), 151 deletions(-)

-- 
1.7.5.4

[PATCH v2 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 |   79 +++++++++++++++++++++++++-------------------------------
 1 files changed, 35 insertions(+), 44 deletions(-)

diff --git a/notmuch-tag.c b/notmuch-tag.c
index 36b9b09..c39b235 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,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 +168,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 +202,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

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

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 701 bytes --]

On Sun, 25 Mar 2012 22:18:43 +0300, 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.
> 
> 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.

If this changes the behavior of notmuch then I think there should be a
test for it.  We should probably see a test for the current behavior,
and a modification of it to reflect whatever behavior is changing.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

[PATCH v2 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 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

Re: [PATCH v2 2/3] cli: refactor "notmuch tag" query tagging into a separate function

[Tomi Ollila]

Jani Nikula <jani@nikula.org> writes:

> Refactor to make tagging code easier to reuse in the future. No
> functional changes.
>
> Signed-off-by: Jani Nikula <jani@nikula.org>
> ---

tag_query() is pretty generic name for a function which (also) does
tagging operations (adds and removes tags based on tag_ops).

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.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.

---

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] == '-');



Everything else LGTM.

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

Re: [PATCH v2 2/3] cli: refactor "notmuch tag" query tagging into a separate function

[Jani Nikula]

On Sun, 25 Mar 2012 23:45:39 +0300, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> Jani Nikula <jani@nikula.org> writes:
> 
> > Refactor to make tagging code easier to reuse in the future. No
> > functional changes.
> >
> > Signed-off-by: Jani Nikula <jani@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?

> 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

Re: [PATCH v2 2/3] cli: refactor "notmuch tag" query tagging into a separate function

[Tomi Ollila]

Jani Nikula <jani@nikula.org> writes:

> On Sun, 25 Mar 2012 23:45:39 +0300, Tomi Ollila <tomi.ollila@iki.fi> wrote:
>> Jani Nikula <jani@nikula.org> writes:
>> 
>> > Refactor to make tagging code easier to reuse in the future. No
>> > functional changes.
>> >
>> > Signed-off-by: Jani Nikula <jani@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.

>
>> 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.

>> 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@notmuchmail.org
>> > http://notmuchmail.org/mailman/listinfo/notmuch
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v2 2/3] cli: refactor "notmuch tag" query tagging into a separate function

[Jani Nikula]

On Mon, 26 Mar 2012 00:26:50 +0300, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> Jani Nikula <jani@nikula.org> writes:
> 
> > On Sun, 25 Mar 2012 23:45:39 +0300, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> >> Jani Nikula <jani@nikula.org> writes:
> >> 
> >> > Refactor to make tagging code easier to reuse in the future. No
> >> > functional changes.
> >> >
> >> > Signed-off-by: Jani Nikula <jani@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@notmuchmail.org
> >> > http://notmuchmail.org/mailman/listinfo/notmuch
> > _______________________________________________
> > notmuch mailing list
> > notmuch@notmuchmail.org
> > http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH v2 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-restore.c |  148 ++++++++++++++++++++++++++++-------------------------
 1 files changed, 78 insertions(+), 70 deletions(-)

diff --git a/notmuch-restore.c b/notmuch-restore.c
index 87d9772..2e965af 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -21,6 +21,81 @@
 #include "notmuch-client.h"
 
 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_restore_command (unused (void *ctx), int argc, char *argv[])
 {
     notmuch_config_t *config;
@@ -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 (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);
     }
-- 
1.7.5.4