From: Jani Nikula <jani@nikula.org>
To: david@tethera.net, notmuch@notmuchmail.org
Subject: Re: [Patch v2 09/17] tag-util.[ch]: New files for common tagging
	routines
In-Reply-To: <1353792017-31459-10-git-send-email-david@tethera.net>
References: <1353792017-31459-1-git-send-email-david@tethera.net>
	<1353792017-31459-10-git-send-email-david@tethera.net>
User-Agent: Notmuch/0.14+124~g3b17402 (http://notmuchmail.org) Emacs/23.4.1
	(i686-pc-linux-gnu)
Date: Sun, 02 Dec 2012 01:28:29 +0200
Message-ID: <87wqx1qrmq.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: David Bremner <bremner@debian.org>
Precedence: list

On Sat, 24 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     |  264 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tag-util.h     |  120 ++++++++++++++++++++++++++
>  3 files changed, 385 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..287cc67
> --- /dev/null
> +++ b/tag-util.c
> @@ -0,0 +1,264 @@
> +#include "string-util.h"
> +#include "tag-util.h"
> +#include "hex-escape.h"
> +
> +struct _tag_operation_t {
> +    const char *tag;
> +    notmuch_bool_t remove;
> +};
> +
> +struct _tag_op_list_t {
> +    tag_operation_t *ops;
> +    int count;
> +    int size;
> +};
> +
> +int
> +parse_tag_line (void *ctx, char *line,
> +		tag_op_flag_t flags,
> +		char **query_string,
> +		tag_op_list_t *tag_ops)
> +{
> +    char *tok = line;
> +    size_t tok_len = 0;
> +
> +    chomp_newline (line);
> +
> +    /* remove leading space */
> +    while (*tok == ' ' || *tok == '\t')
> +	tok++;
> +
> +    /* Skip empty and comment lines. */
> +    if (*tok == '\0' || *tok == '#')
> +	    return 1;
> +
> +    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) {

I think this should be

	if (tok_len == 2 && strncmp (tok, "--", tok_len) == 0) {

to not match "--foo" as "--". At this level, "--foo" should probably be
interpreted as removal of tag "-foo", and any policy about tag contents
should be done by the caller on the returned tag ops list.

(I'm tempted to blame the above on you, but yeah it's my mistake... ;)

> +	    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;
> +
> +	/* Maybe refuse empty tags. */
> +	if (!(flags & TAG_FLAG_BE_GENEROUS) && *tag == '\0') {

I'm divided whether this deserves to be here, or whether it should be
checked by the caller. Maybe it's all right, since my first instinct
would be to just fail on lone + or - unconditionally, so this is a
special case.

> +	    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! */

I think we should either ditch the %s as it's bound to be completely
useless like this, or make a copy of the input line.

> +	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! */

Ditto.

> +	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" );

Extra space before ).

> +	    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. */

"terminated with an empty element" is obsolete as you've added .count.

> +
> +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)
> +{
> +    /* Make room if current array is full.  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;
> +	}
> +    }
> +
> +    /* add the new operation */
> +
> +    list->ops[list->count].tag = tag;
> +    list->ops[list->count].remove = remove;
> +    list->count++;
> +    return 0;
> +}
> +
> +/*
> + *   Is the i'th tag operation a remove?
> + */
> +
> +notmuch_bool_t
> +tag_op_list_isremove (const tag_op_list_t *list, size_t i)
> +{

	assert (i < list->count); ?

> +    return list->ops[i].remove;
> +}
> +
> +/*
> + * Reset a list to contain no operations
> + */
> +
> +void
> +tag_op_list_reset (tag_op_list_t *list)
> +{
> +    list->count = 0;
> +}
> +
> +/*
> + * Return the number of operations in a list
> + */
> +
> +size_t
> +tag_op_list_size (const tag_op_list_t *list)
> +{
> +    return list->count;
> +}
> +
> +/*
> + *   return the i'th tag in the list
> + */
> +
> +const char *
> +tag_op_list_tag (const tag_op_list_t *list, size_t i)
> +{

	assert (i < list->count); ?

> +    return list->ops[i].tag;
> +}
> diff --git a/tag-util.h b/tag-util.h
> new file mode 100644
> index 0000000..508806f
> --- /dev/null
> +++ b/tag-util.h
> @@ -0,0 +1,120 @@
> +#ifndef _TAG_UTIL_H
> +#define _TAG_UTIL_H
> +
> +#include "notmuch-client.h"
> +
> +typedef struct _tag_operation_t tag_operation_t;
> +typedef struct _tag_op_list_t tag_op_list_t;
> +
> +#define TAG_OP_LIST_INITIAL_SIZE 10

This is implementation details, could be in the .c file.

> +
> +/* Use powers of 2 */
> +typedef enum  { TAG_FLAG_NONE = 0,
> +		/* Operations are synced to maildir, if possible */
> +
> +		TAG_FLAG_MAILDIR_SYNC = 1,
> +
> +		/* Remove all tags from message before applying
> +		 * list */
> +
> +		TAG_FLAG_REMOVE_ALL = 2,
> +
> +		/* Don't try to avoid database operations.  Useful
> +		 * when we know that message passed needs these
> +		 *  operations. */
> +
> +		TAG_FLAG_PRE_OPTIMIZED = 4,
> +
> +		/* Accept strange tags that might be user error;
> +		   intended for use by notmuch-restore.
> +		*/
> +
> +		TAG_FLAG_BE_GENEROUS = 8} tag_op_flag_t;
> +
> +/* Parse a string of the following 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.
> + *
> + * Returns: 0 for OK, 1 for skipped line, -1 for fatal(ish) error.
> + *
> + * Output Parameters:
> + *	ops	contains a list of tag operations
> + *	query_str the search terms.
> + */
> +int
> +parse_tag_line (void *ctx, char *line,
> +		tag_op_flag_t flags,
> +		char **query_str, tag_op_list_t *ops);
> +
> +/*
> + * Create an empty list of tag operations
> + *
> + * ctx is passed to talloc
> + */
> +
> +tag_op_list_t
> +*tag_op_list_create (void *ctx);

The * belongs at the end of previous line.

BR,
Jani.

> +
> +/*
> + * Add a tag operation (delete iff remote == TRUE) to a list.
> + * The list is expanded as necessary.
> + */
> +
> +int
> +tag_op_list_append (void *ctx,
> +		    tag_op_list_t *list,
> +		    const char *tag,
> +		    notmuch_bool_t remove);
> +
> +/*
> + * Apply a list of tag operations, in order to a message.
> + *
> + * Flags can be bitwise ORed; see enum above for possibilies.
> + */
> +
> +notmuch_status_t
> +tag_op_list_apply (notmuch_message_t *message,
> +		   tag_op_list_t *tag_ops,
> +		   tag_op_flag_t flags);
> +
> +/*
> + * Return the number of operations in a list
> + */
> +
> +size_t
> +tag_op_list_size (const tag_op_list_t *list);
> +
> +/*
> + * Reset a list to contain no operations
> + */
> +
> +void
> +tag_op_list_reset (tag_op_list_t *list);
> +
> +
> + /*
> +  *   return the i'th tag in the list
> +  */
> +
> +const char *
> +tag_op_list_tag (const tag_op_list_t *list, size_t i);
> +
> +/*
> + *   Is the i'th tag operation a remove?
> + */
> +
> +notmuch_bool_t
> +tag_op_list_isremove (const tag_op_list_t *list, size_t i);
> +
> +#endif
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch