From: Mark Walters <markwalters1009@gmail.com>
To: david@tethera.net, notmuch@notmuchmail.org
Subject: Re: [Patch v3b 5/9] notmuch-restore: add support for input format
	'batch-tag'
In-Reply-To: <1354843607-17980-6-git-send-email-david@tethera.net>
References: <1354843607-17980-1-git-send-email-david@tethera.net>
	<1354843607-17980-6-git-send-email-david@tethera.net>
User-Agent: Notmuch/0.14+81~g9730584 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Sat, 08 Dec 2012 11:43:10 +0000
Message-ID: <87wqwsg46p.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: David Bremner <bremner@debian.org>
Precedence: list


Hi

Basically LGTM: just one comment below.

On Fri, 07 Dec 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> This can be enabled with the new --format=batch-tag command line
> option to "notmuch restore". The input must consist of lines of the
> format:
>
>     +<tag>|-<tag> [...] [--] id:<msg-id>
>
> Each line is interpreted similarly to "notmuch tag" command line
> arguments. The delimiter is one or more spaces ' '. Any characters in
> <tag> and <search-terms> MAY be hex encoded with %NN where NN is the
> hexadecimal value of the character. Any ' ' and '%' characters in
> <tag> and <msg-id> MUST be hex encoded (using %20 and %25,
> respectively). Any characters that are not part of <tag> or
> <search-terms> MUST NOT be hex encoded.
>
> Leading and trailing space ' ' is ignored. Empty lines and lines
> beginning with '#' are ignored.
>
> Commit message mainly stolen from Jani's batch tagging commit, to
> follow.
> ---
>  notmuch-restore.c |  206 ++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 131 insertions(+), 75 deletions(-)
>
> diff --git a/notmuch-restore.c b/notmuch-restore.c
> index f03dcac..ceec2d3 100644
> --- a/notmuch-restore.c
> +++ b/notmuch-restore.c
> @@ -19,18 +19,22 @@
>   */
>  
>  #include "notmuch-client.h"
> +#include "dump-restore-private.h"
> +#include "tag-util.h"
> +#include "string-util.h"
> +
> +static volatile sig_atomic_t interrupted;
> +static regex_t regex;
>  
>  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)
> +tag_message (unused (void *ctx),
> +	     notmuch_database_t *notmuch,
> +	     const char *message_id,
> +	     tag_op_list_t *tag_ops,
> +	     tag_op_flag_t 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);
> @@ -44,55 +48,62 @@ tag_message (notmuch_database_t *notmuch, const char *message_id,
>  
>      /* 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 ((flags & TAG_FLAG_REMOVE_ALL) || tag_op_list_size (tag_ops))
> +	tag_op_list_apply (message, tag_ops, flags);
>  
> -    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_destroy (message);
>  
> -    notmuch_message_freeze (message);
> +    return ret;
> +}
>  
> -    if (remove_all)
> -	notmuch_message_remove_all_tags (message);
> +static int
> +parse_sup_line (void *ctx, char *line,
> +		char **query_str, tag_op_list_t *tag_ops)
> +{
>  
> -    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;
> -	}
> +    regmatch_t match[3];
> +    char *file_tags;
> +    int rerr;
> +
> +    tag_op_list_reset (tag_ops);
> +
> +    chomp_newline (line);
> +
> +    /* Silently ignore blank lines */
> +    if (line[0] == '\0') {
> +	return 1;
> +    }
> +
> +    rerr = xregexec (&regex, line, 3, match, 0);
> +    if (rerr == REG_NOMATCH) {
> +	fprintf (stderr, "Warning: Ignoring invalid sup format line: %s\n",
> +		 line);
> +	return 1;
>      }
>  
> -    notmuch_message_thaw (message);
> +    *query_str = talloc_strndup (ctx, line + match[1].rm_so,
> +				 match[1].rm_eo - match[1].rm_so);
> +    file_tags = talloc_strndup (ctx, line + match[2].rm_so,
> +				match[2].rm_eo - match[2].rm_so);
>  
> -    if (synchronize_flags)
> -	notmuch_message_tags_to_maildir_flags (message);
> +    char *tok = file_tags;
> +    size_t tok_len = 0;
>  
> -  DONE:
> -    if (message)
> -	notmuch_message_destroy (message);
> +    tag_op_list_reset (tag_ops);
> +
> +    while ((tok = strtok_len (tok + tok_len, " ", &tok_len)) != NULL) {
> +
> +	if (*(tok + tok_len) != '\0') {
> +	    *(tok + tok_len) = '\0';
> +	    tok_len++;
> +	}
> +
> +	if (tag_op_list_append (ctx, tag_ops, tok, FALSE))
> +	    return -1;
> +    }
> +
> +    return 0;
>  
> -    return ret;
>  }
>  
>  int
> @@ -100,16 +111,19 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
>  {
>      notmuch_config_t *config;
>      notmuch_database_t *notmuch;
> -    notmuch_bool_t synchronize_flags;
>      notmuch_bool_t accumulate = FALSE;
> +    tag_op_flag_t flags = 0;
> +    tag_op_list_t *tag_ops;
> +
>      char *input_file_name = NULL;
>      FILE *input = stdin;
>      char *line = NULL;
>      size_t line_size;
>      ssize_t line_len;
> -    regex_t regex;
> -    int rerr;
> +
> +    int ret = 0;
>      int opt_index;
> +    int input_format = DUMP_FORMAT_AUTO;
>  
>      config = notmuch_config_open (ctx, NULL, NULL);
>      if (config == NULL)
> @@ -119,9 +133,15 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
>  			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
>  	return 1;
>  
> -    synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);
> +    if (notmuch_config_get_maildir_synchronize_flags (config))
> +	flags |= TAG_FLAG_MAILDIR_SYNC;
>  
>      notmuch_opt_desc_t options[] = {
> +	{ NOTMUCH_OPT_KEYWORD, &input_format, "format", 'f',
> +	  (notmuch_keyword_t []){ { "auto", DUMP_FORMAT_AUTO },
> +				  { "batch-tag", DUMP_FORMAT_BATCH_TAG },
> +				  { "sup", DUMP_FORMAT_SUP },
> +				  { 0, 0 } } },
>  	{ NOTMUCH_OPT_STRING, &input_file_name, "input", 'i', 0 },
>  	{ NOTMUCH_OPT_BOOLEAN,  &accumulate, "accumulate", 'a', 0 },
>  	{ 0, 0, 0, 0, 0 }
> @@ -134,6 +154,9 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
>  	return 1;
>      }
>  
> +    if (! accumulate)
> +	flags |= TAG_FLAG_REMOVE_ALL;
> +
>      if (input_file_name) {
>  	input = fopen (input_file_name, "r");
>  	if (input == NULL) {
> @@ -154,44 +177,77 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
>       * non-space characters for the message-id, then one or more
>       * spaces, then a list of space-separated tags as a sequence of
>       * characters within literal '(' and ')'. */

^^^ shouldn't this comment move to parse_sup_line?

Best wishes

Mark


> -    if ( xregcomp (&regex,
> -		   "^([^ ]+) \\(([^)]*)\\)$",
> -		   REG_EXTENDED) )
> -	INTERNAL_ERROR ("compile time constant regex failed.");
> +    char *p;
>  
> -    while ((line_len = getline (&line, &line_size, input)) != -1) {
> -	regmatch_t match[3];
> -	char *message_id, *file_tags;
> +    line_len = getline (&line, &line_size, input);
> +    if (line_len == 0)
> +	return 0;
>  
> -	chomp_newline (line);
> +    tag_ops = tag_op_list_create (ctx);
> +    if (tag_ops == NULL) {
> +	fprintf (stderr, "Out of memory.\n");
> +	return 1;
> +    }
>  
> -	rerr = xregexec (&regex, line, 3, match, 0);
> -	if (rerr == REG_NOMATCH) {
> -	    fprintf (stderr, "Warning: Ignoring invalid input line: %s\n",
> -		     line);
> -	    continue;
> +    for (p = line; (input_format == DUMP_FORMAT_AUTO) && *p; p++) {
> +	if (*p == '(')
> +	    input_format = DUMP_FORMAT_SUP;
> +    }
> +
> +    if (input_format == DUMP_FORMAT_AUTO)
> +	input_format = DUMP_FORMAT_BATCH_TAG;
> +
> +    if (input_format == DUMP_FORMAT_SUP)
> +	if ( xregcomp (&regex,
> +		       "^([^ ]+) \\(([^)]*)\\)$",
> +		       REG_EXTENDED) )
> +	    INTERNAL_ERROR ("compile time constant regex failed.");
> +
> +    do {
> +	char *query_string;
> +
> +	if (input_format == DUMP_FORMAT_SUP) {
> +	    ret = parse_sup_line (ctx, line, &query_string, tag_ops);
> +	} else {
> +	    ret = parse_tag_line (ctx, line, TAG_FLAG_BE_GENEROUS,
> +				  &query_string, tag_ops);
> +
> +	    if (ret == 0) {
> +		if (strncmp ("id:", query_string, 3) != 0) {
> +		    fprintf (stderr, "Unsupported query: %s\n", query_string);
> +		    continue;
> +		}
> +		/* delete id: from front of string; tag_message
> +		 * expects a raw message-id.
> +		 *
> +		 * XXX: Note that query string id:foo and bar will be
> +		 * interpreted as a message id "foo and bar". This
> +		 * should eventually be fixed to give a better error
> +		 * message.
> +		 */
> +		query_string = query_string + 3;
> +	    }
>  	}
>  
> -	message_id = xstrndup (line + match[1].rm_so,
> -			       match[1].rm_eo - match[1].rm_so);
> -	file_tags = xstrndup (line + match[2].rm_so,
> -			      match[2].rm_eo - match[2].rm_so);
> +	if (ret > 0)
> +	    continue;
>  
> -	tag_message (notmuch, message_id, file_tags, ! accumulate,
> -		     synchronize_flags);
> +	if (ret < 0 || tag_message (ctx, notmuch, query_string,
> +				    tag_ops, flags))
> +	    break;
>  
> -	free (message_id);
> -	free (file_tags);
> -    }
> +    }  while ((line_len = getline (&line, &line_size, input)) != -1);
>  
> -    regfree (&regex);
> +    if (input_format == DUMP_FORMAT_SUP)
> +	regfree (&regex);
>  
>      if (line)
>  	free (line);
>  
>      notmuch_database_destroy (notmuch);
> +
>      if (input != stdin)
>  	fclose (input);
>  
> -    return 0;
> +    return ret;
>  }
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch