From: Mark Walters <markwalters1009@gmail.com>
To: david@tethera.net, notmuch@notmuchmail.org
Subject: Re: [PATCH 08/16] tag-util.[ch]: New files for common tagging
 routines
In-Reply-To: <1353265498-3839-9-git-send-email-david@tethera.net>
References: <1353265498-3839-1-git-send-email-david@tethera.net>
	<1353265498-3839-9-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: Thu, 22 Nov 2012 12:22:50 +0000
Message-ID: <87k3tdltd1.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: David Bremner <bremner@debian.org>
Precedence: list


Hi

I am slowly working my way through this series. I have some queries
comments and things that confused me in this patch.

One longer term concern (but I have not got far enough to see if it
could be a problem): could someone use dump and then not be able to
restore because some of the tags are now banned (eg starting with -
etc)? Obviously anything which fails currently is not a concern, only if
some cases stop some currently working cases.


One particular comment is that most of the functions do not have a
comment defining them in tag-util.h. So for example (at this point in
reading the series) I am not sure of why we need both parse_tag_line and
tag_op_list_from_string.


On Sun, 18 Nov 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> These are meant to be shared between notmuch-tag and notmuch-restore.
>
> The bulk of the routines implement a "tag operation list" abstract
> data type act as a structured representation of a set of tag
> operations (typically coming from a single tag command or line of
> input).
> ---
>  Makefile.local |    1 +
>  tag-util.c     |  273 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tag-util.h     |   71 +++++++++++++++
>  3 files changed, 345 insertions(+)
>  create mode 100644 tag-util.c
>  create mode 100644 tag-util.h
>
> diff --git a/Makefile.local b/Makefile.local
> index 2b91946..854867d 100644
> --- a/Makefile.local
> +++ b/Makefile.local
> @@ -274,6 +274,7 @@ notmuch_client_srcs =		\
>  	query-string.c		\
>  	mime-node.c		\
>  	crypto.c		\
> +	tag-util.c
>  
>  notmuch_client_modules = $(notmuch_client_srcs:.c=.o)
>  
> diff --git a/tag-util.c b/tag-util.c
> new file mode 100644
> index 0000000..5329b1f
> --- /dev/null
> +++ b/tag-util.c
> @@ -0,0 +1,273 @@
> +#include "tag-util.h"
> +#include "hex-escape.h"
> +
> +static char *
> +strtok_len (char *s, const char *delim, size_t *len)
> +{
> +    /* skip initial delims */
> +    s += strspn (s, delim);
> +
> +    /* length of token */
> +    *len = strcspn (s, delim);
> +
> +    return *len ? s : NULL;
> +}
> +
> +/* Tag messages according to 'input', which must consist of lines of
> + * the format:
> + *
> + * +<tag>|-<tag> [...] [--] <search-terms>
> + *
> + * 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 <search-terms> 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.
> + */

The comment says lines starting with '#' are ignored but I don't see
anything actually ignoring them.

> +int
> +parse_tag_line (void *ctx, char *line,
> +		char **query_string,
> +		tag_op_list_t *tag_ops)
> +{
> +    char *tok = line;
> +    size_t tok_len = 0;
> +
> +    chomp_newline (line);
> +    tag_op_list_reset (tag_ops);
> +
> +    /* Parse tags. */
> +    while ((tok = strtok_len (tok + tok_len, " ", &tok_len)) != NULL) {
> +	notmuch_bool_t remove;
> +	char *tag;
> +
> +	/* Optional explicit end of tags marker. */
> +	if (strncmp (tok, "--", tok_len) == 0) {
> +	    tok = strtok_len (tok + tok_len, " ", &tok_len);
> +	    break;
> +	}
> +
> +	/* Implicit end of tags. */
> +	if (*tok != '-' && *tok != '+')
> +	    break;
> +
> +	/* If tag is terminated by NUL, there's no query string. */
> +	if (*(tok + tok_len) == '\0') {
> +	    tok = NULL;
> +	    break;
> +	}
> +
> +	/* Terminate, and start next token after terminator. */
> +	*(tok + tok_len++) = '\0';
> +
> +	remove = (*tok == '-');
> +	tag = tok + 1;
> +
> +	/* Refuse empty tags. */
> +	if (*tag == '\0') {
> +	    tok = NULL;
> +	    break;
> +	}
> +
> +	/* Decode tag. */
> +	if (hex_decode_inplace (tag) != HEX_SUCCESS) {
> +	    tok = NULL;
> +	    break;
> +	}
> +
> +	if (tag_op_list_append (ctx, tag_ops, tag, remove))
> +	    return -1;
> +    }
> +
> +    if (tok == NULL || tag_ops->count == 0) {
> +	/* FIXME: line has been modified! */
> +	fprintf (stderr, "Warning: Ignoring invalid input line: %s\n",
> +		 line);
> +	return 1;
> +    }
> +
> +    /* tok now points to the query string */
> +    if (hex_decode_inplace (tok) != HEX_SUCCESS) {
> +	/* FIXME: line has been modified! */
> +	fprintf (stderr, "Warning: Ignoring invalid input line: %s\n",
> +		 line);
> +	return 1;
> +    }
> +
> +    *query_string = tok;
> +
> +    return 0;
> +}
> +
> +static inline void
> +message_error (notmuch_message_t *message,
> +	       notmuch_status_t status,
> +	       const char *format, ...)
> +{
> +    va_list va_args;
> +
> +    va_start (va_args, format);
> +
> +    vfprintf (stderr, format, va_args);
> +    fprintf (stderr, "Message-ID: %s\n", notmuch_message_get_message_id (message));
> +    fprintf (stderr, "Status: %s\n", notmuch_status_to_string (status));
> +}
> +
> +notmuch_status_t
> +tag_op_list_apply (notmuch_message_t *message,
> +		   tag_op_list_t *list,
> +		   tag_op_flag_t flags)
> +{
> +    int i;
> +
> +    notmuch_status_t status = 0;
> +    tag_operation_t *tag_ops = list->ops;
> +
> +    status = notmuch_message_freeze (message);
> +    if (status) {
> +	message_error (message, status, "freezing message");
> +	return status;
> +    }
> +
> +    if (flags & TAG_FLAG_REMOVE_ALL) {
> +	status = notmuch_message_remove_all_tags (message);
> +	if (status) {
> +	    message_error (message, status, "removing all tags" );
> +	    return status;
> +	}
> +    }
> +
> +    for (i = 0; i < list->count; i++) {
> +	if (tag_ops[i].remove) {
> +	    status = notmuch_message_remove_tag (message, tag_ops[i].tag);
> +	    if (status) {
> +		message_error (message, status, "removing tag %s", tag_ops[i].tag);
> +		return status;
> +	    }
> +	} else {
> +	    status = notmuch_message_add_tag (message, tag_ops[i].tag);
> +	    if (status) {
> +		message_error (message, status, "adding tag %s", tag_ops[i].tag);
> +		return status;
> +	    }
> +
> +	}
> +    }
> +
> +    status = notmuch_message_thaw (message);
> +    if (status) {
> +	message_error (message, status, "thawing message");
> +	return status;
> +    }
> +
> +
> +    if (flags & TAG_FLAG_MAILDIR_SYNC) {
> +	status = notmuch_message_tags_to_maildir_flags (message);
> +	if (status) {
> +	    message_error (message, status, "synching tags to maildir");
> +	    return status;
> +	}
> +    }
> +
> +    return NOTMUCH_STATUS_SUCCESS;
> +
> +}
> +
> +
> +/* Array of tagging operations (add or remove), terminated with an
> + * empty element. Size will be increased as necessary. */

Is the list termination used (above you used list->count) or is it just
that you can always safely write to the last element?

> +
> +tag_op_list_t *
> +tag_op_list_create (void *ctx)
> +{
> +    tag_op_list_t *list;
> +
> +    list = talloc (ctx, tag_op_list_t);
> +    if (list == NULL)
> +	return NULL;
> +
> +    list->size = TAG_OP_LIST_INITIAL_SIZE;
> +    list->count = 0;
> +
> +    list->ops = talloc_array (ctx, tag_operation_t, list->size);
> +    if (list->ops == NULL)
> +	return NULL;
> +
> +    return list;
> +}
> +
> +
> +int
> +tag_op_list_append (void *ctx,
> +		    tag_op_list_t *list,
> +		    const char *tag,
> +		    notmuch_bool_t remove)
> +{
> +    list->ops[list->count].tag = tag;
> +    list->ops[list->count].remove = remove;
> +    list->count++;
> +
> +    /* Make room for terminating empty element and potential
> +     * new tags, if necessary. This should be a fairly rare
> +     * case, considering the initial array size. */
> +    if (list->count == list->size) {
> +	list->size *= 2;
> +	list->ops = talloc_realloc (ctx, list->ops, tag_operation_t,
> +				    list->size);
> +	if (list->ops == NULL) {
> +	    fprintf (stderr, "Out of memory.\n");
> +	    return 1;
> +	}
> +    }
> +
> +    list->ops[list->count].tag = NULL;
> +
> +    return 0;
> +}
> +
> +
> +/* WARNING: modifies it's string argument */
> +
> +int
> +tag_op_list_from_string (void *ctx,
> +			 char *tag_string,
> +			 notmuch_bool_t remove,
> +			 tag_op_list_t *tag_ops)

As mentioned above I wasn't sure why both this and parse_tag_line are
needed. Also should one use the other? Otherwise we could get different
allowed tags in different places?

> +{
> +    char *tok = tag_string;
> +    size_t tok_len = 0;
> +
> +    tag_op_list_reset (tag_ops);
> +
> +/* Parse tags. */
> +    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, remove))
> +	    return -1;
> +    }
> +
> +    return 0;
> +
> +}
> +
> +notmuch_bool_t
> +tag_op_list_empty (tag_op_list_t *list)
> +{
> +    return (list->count == 0);
> +}
> +
> +
> +void
> +tag_op_list_reset (tag_op_list_t *list)
> +{
> +    list->count = 0;
> +}
> diff --git a/tag-util.h b/tag-util.h
> new file mode 100644
> index 0000000..b381b8e
> --- /dev/null
> +++ b/tag-util.h
> @@ -0,0 +1,71 @@
> +#ifndef _TAG_UTIL_H
> +#define _TAG_UTIL_H
> +
> +#include "notmuch-client.h"
> +
> +typedef struct {
> +    const char *tag;
> +    notmuch_bool_t remove;
> +} tag_operation_t;
> +
> +#define TAG_OP_LIST_INITIAL_SIZE 10
> +
> +typedef struct {
> +    tag_operation_t *ops;
> +    int count;
> +    int size;
> +} tag_op_list_t;

I thought it would be helpful to say something about *ops: e.g., "ops
points to the first element of an array of count tag_operation_t's. This
array should be empty terminated (if true)."

One trivial comment for the next patch: the commit message says "notmuch
new" rather than "notmuch tag".

Best wishes

Mark
 
> +
> +/* Use powers of 2 */
> +typedef enum  { TAG_FLAG_NONE = 0,
> +		TAG_FLAG_MAILDIR_SYNC = 1,
> +		TAG_FLAG_REMOVE_ALL = 2 } tag_op_flag_t;
> +
> +
> +typedef int (*tag_callback_t)(void *ctx,
> +			      notmuch_database_t *notmuch,
> +			      const char *query_string,
> +			      tag_op_list_t *tag_ops,
> +			      tag_op_flag_t flags);
> +
> +int
> +parse_tag_stream (void *ctx,
> +		  notmuch_database_t *notmuch,
> +		  FILE *input,
> +		  tag_callback_t callback,
> +		  tag_op_flag_t flags,
> +		  volatile sig_atomic_t *interrupted);
> +
> +/*
> + * Returns: 0 for OK, 1 for skipped line, -1 for fatal(ish) error.
> + */
> +int
> +parse_tag_line (void *ctx, char *line, char **query_str, tag_op_list_t *ops);
> +
> +tag_op_list_t
> +*tag_op_list_create (void *ctx);
> +
> +int
> +tag_op_list_append (void *ctx,
> +		    tag_op_list_t *list,
> +		    const char *tag,
> +		    notmuch_bool_t remove);
> +
> +notmuch_status_t
> +tag_op_list_apply (notmuch_message_t *message,
> +		   tag_op_list_t *tag_ops,
> +		   tag_op_flag_t flags);
> +
> +int
> +tag_op_list_from_string (void *ctx,
> +			 char *tag_string,
> +			 notmuch_bool_t remove,
> +			 tag_op_list_t *tag_ops);
> +
> +notmuch_bool_t
> +tag_op_list_empty (tag_op_list_t *list);
> +
> +void
> +tag_op_list_reset (tag_op_list_t *list);
> +
> +#endif
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch