[Patch v2 10/17] cli: add support for batch tagging operations to "notmuch tag"
Jani Nikula
jani at nikula.org
Sat Dec 1 15:55:08 PST 2012
Hi David, overall looks good. The tag-util abstractions are paying off
nicely here.
First two high level bikesheds: 1) This could be split in two patches,
first one adding the tag-util usage to normal operation and second one
adding the batch tagging, and 2) I think it would be neater if the batch
tagging was in a separate function instead of inline in the top level
function. I'm not insisting though; we should probably just go with
this, and the latter part can be refactored later if needed.
Some minor nits inline.
BR,
Jani.
On Sat, 24 Nov 2012, david at tethera.net wrote:
> From: Jani Nikula <jani at nikula.org>
>
> Add support for batch tagging operations through stdin to "notmuch
> tag". This can be enabled with the new --stdin command line option to
--batch
> "notmuch tag". The input 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.
>
> Signed-off-by: Jani Nikula <jani at nikula.org>
>
> Hacked-like-crazy-by: David Bremner <david at tethera.net>
> ---
> notmuch-tag.c | 194 +++++++++++++++++++++++++++++++++++----------------------
> 1 file changed, 118 insertions(+), 76 deletions(-)
>
> diff --git a/notmuch-tag.c b/notmuch-tag.c
> index 88d559b..8a8af0b 100644
> --- a/notmuch-tag.c
> +++ b/notmuch-tag.c
> @@ -19,6 +19,7 @@
> */
>
> #include "notmuch-client.h"
> +#include "tag-util.h"
>
> static volatile sig_atomic_t interrupted;
>
> @@ -54,14 +55,9 @@ _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,
> - const tag_operation_t *tag_ops)
> + const tag_op_list_t *list)
> {
> /* This is subtler than it looks. Xapian ignores the '-' operator
> * at the beginning both queries and parenthesized groups and,
> @@ -73,19 +69,20 @@ _optimize_tag_query (void *ctx, const char *orig_query_string,
>
> char *escaped, *query_string;
> const char *join = "";
> - int i;
> + size_t i;
> unsigned int max_tag_len = 0;
>
> /* Don't optimize if there are no tag changes. */
> - if (tag_ops[0].tag == NULL)
> + if (tag_op_list_size (list) == 0)
> return talloc_strdup (ctx, orig_query_string);
>
> /* 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; tag_ops[i].tag; i++)
> - if (strlen (tag_ops[i].tag) > max_tag_len)
> - max_tag_len = strlen (tag_ops[i].tag);
> + for (i = 0; i < tag_op_list_size (list); i++)
> + if (strlen (tag_op_list_tag (list, i)) > max_tag_len)
> + max_tag_len = strlen (tag_op_list_tag (list, i));
> +
> escaped = talloc_array (ctx, char, max_tag_len * 2 + 3);
> if (! escaped)
> return NULL;
> @@ -96,11 +93,11 @@ _optimize_tag_query (void *ctx, const char *orig_query_string,
> else
> query_string = talloc_asprintf (ctx, "( %s ) and (", orig_query_string);
>
> - for (i = 0; tag_ops[i].tag && query_string; i++) {
> + for (i = 0; i < tag_op_list_size (list) && query_string; i++) {
> query_string = talloc_asprintf_append_buffer (
> query_string, "%s%stag:%s", join,
> - tag_ops[i].remove ? "" : "not ",
> - _escape_tag (escaped, tag_ops[i].tag));
> + tag_op_list_isremove (list, i) ? "" : "not ",
> + _escape_tag (escaped, tag_op_list_tag (list, i)));
> join = " or ";
> }
>
> @@ -116,12 +113,11 @@ _optimize_tag_query (void *ctx, const char *orig_query_string,
> * element. */
> static int
> tag_query (void *ctx, notmuch_database_t *notmuch, const char *query_string,
> - tag_operation_t *tag_ops, notmuch_bool_t synchronize_flags)
> + tag_op_list_t *tag_ops, tag_op_flag_t 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. */
> @@ -144,21 +140,7 @@ tag_query (void *ctx, notmuch_database_t *notmuch, const char *query_string,
> 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);
> -
> + tag_op_list_apply (message, tag_ops, flags);
> notmuch_message_destroy (message);
> }
>
> @@ -170,15 +152,17 @@ tag_query (void *ctx, notmuch_database_t *notmuch, const char *query_string,
> int
> notmuch_tag_command (void *ctx, int argc, char *argv[])
> {
> - tag_operation_t *tag_ops;
> - int tag_ops_count = 0;
> - char *query_string;
> + tag_op_list_t *tag_ops = NULL;
> + char *query_string = NULL;
> notmuch_config_t *config;
> notmuch_database_t *notmuch;
> struct sigaction action;
> - notmuch_bool_t synchronize_flags;
> - int i;
> - int ret;
> + tag_op_flag_t synchronize_flags = TAG_FLAG_NONE;
Maybe make that tag_flags or something so we don't need to rename it if
we ever use the other flags here?
> + notmuch_bool_t batch = FALSE;
> + FILE *input = stdin;
> + char *input_file_name = NULL;
> + int i, opt_index;
> + int ret = 0;
>
> /* Setup our handler for SIGINT */
> memset (&action, 0, sizeof (struct sigaction));
> @@ -187,53 +171,76 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
> action.sa_flags = SA_RESTART;
> sigaction (SIGINT, &action, NULL);
>
> - argc--; argv++; /* skip subcommand argument */
> + notmuch_opt_desc_t options[] = {
> + { NOTMUCH_OPT_BOOLEAN, &batch, "batch", 0, 0 },
> + { NOTMUCH_OPT_STRING, &input_file_name, "input", 'i', 0 },
> + { 0, 0, 0, 0, 0 }
> + };
>
> - /* 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");
> + opt_index = parse_arguments (argc, argv, options, 1);
> + if (opt_index < 0)
> return 1;
> +
> + if (input_file_name) {
> + batch = TRUE;
> + input = fopen (input_file_name, "r");
> + if (input == NULL) {
> + fprintf (stderr, "Error opening %s for reading: %s\n",
> + input_file_name, strerror (errno));
> + return 1;
> + }
> }
>
> - for (i = 0; i < argc; i++) {
> - if (strcmp (argv[i], "--") == 0) {
> - i++;
> - break;
> + if (batch) {
> + if (opt_index != argc) {
> + fprintf (stderr, "Can't specify both cmdline and stdin!\n");
> + return 1;
> + }
> + } else {
> + /* Array of tagging operations (add or remove), terminated with an
> + * empty element. */
> + tag_ops = tag_op_list_create (ctx);
> + if (tag_ops == NULL) {
> + fprintf (stderr, "Out of memory.\n");
> + return 1;
> }
> - if (argv[i][0] == '+' || argv[i][0] == '-') {
> - if (argv[i][0] == '+' && argv[i][1] == '\0') {
> - fprintf (stderr, "Error: tag names cannot be empty.\n");
> - return 1;
> +
> + for (i = opt_index; i < argc; i++) {
> + if (strcmp (argv[i], "--") == 0) {
> + i++;
> + break;
> }
> - if (argv[i][0] == '+' && argv[i][1] == '-') {
> - /* This disallows adding the non-removable tag "-" and
> - * enables notmuch tag to take long options in the
> - * future. */
> - fprintf (stderr, "Error: tag names must not start with '-'.\n");
> - return 1;
> + if (argv[i][0] == '+' || argv[i][0] == '-') {
> + /* FIXME: checks should be consistent with those in batch tagging */
> + if (argv[i][0] == '+' && argv[i][1] == '\0') {
> + fprintf (stderr, "Error: tag names cannot be empty.\n");
> + return 1;
> + }
> + if (argv[i][0] == '+' && argv[i][1] == '-') {
> + /* This disallows adding the non-removable tag "-" and
> + * enables notmuch tag to take long options in the
> + * future. */
> + fprintf (stderr, "Error: tag names must not start with '-'.\n");
> + return 1;
> + }
> + tag_op_list_append (ctx, tag_ops,
> + argv[i] + 1, (argv[i][0] == '-'));
> + } else {
> + break;
> }
> - tag_ops[tag_ops_count].tag = argv[i] + 1;
> - tag_ops[tag_ops_count].remove = (argv[i][0] == '-');
> - tag_ops_count++;
> - } else {
> - break;
> }
> - }
>
> - 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;
> - }
> + if (tag_op_list_size (tag_ops) == 0) {
> + fprintf (stderr, "Error: 'notmuch tag' requires at least one tag to add or remove.\n");
> + return 1;
> + }
>
> - query_string = query_string_from_args (ctx, argc - i, &argv[i]);
> + query_string = query_string_from_args (ctx, argc - i, &argv[i]);
>
> - if (*query_string == '\0') {
> - fprintf (stderr, "Error: notmuch tag requires at least one search term.\n");
> - return 1;
> + if (*query_string == '\0') {
> + fprintf (stderr, "Error: notmuch tag requires at least one search term.\n");
> + return 1;
> + }
> }
>
> config = notmuch_config_open (ctx, NULL, NULL);
> @@ -244,11 +251,46 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
> NOTMUCH_DATABASE_MODE_READ_WRITE, ¬much))
> return 1;
>
> - synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);
> + if (notmuch_config_get_maildir_synchronize_flags (config))
> + synchronize_flags = TAG_FLAG_MAILDIR_SYNC;
Use |= to be future compatible?
> +
> + if (batch) {
> +
> + char *line = NULL;
> + size_t line_size = 0;
> + ssize_t line_len;
> + int ret = 0;
> + tag_op_list_t *tag_ops;
The two above shadow the function level variables.
> +
> + tag_ops = tag_op_list_create (ctx);
> + if (tag_ops == NULL) {
> + fprintf (stderr, "Out of memory.\n");
> + return 1;
> + }
>
> - ret = tag_query (ctx, notmuch, query_string, tag_ops, synchronize_flags);
> + while ((line_len = getline (&line, &line_size, input)) != -1 &&
> + ! interrupted) {
> +
> + char *query_string;
> +
> + ret = parse_tag_line (ctx, line, TAG_FLAG_NONE,
> + &query_string, tag_ops);
Extra space after =.
> +
> + if (ret > 0)
> + continue;
> +
> + if (ret < 0 || tag_query (ctx, notmuch, query_string,
> + tag_ops, synchronize_flags))
> + break;
> + }
> +
> + if (line)
> + free (line);
> + } else
> + ret = tag_query (ctx, notmuch, query_string, tag_ops, synchronize_flags);
I'd like braces around the else block too.
>
> notmuch_database_destroy (notmuch);
>
> - return ret;
> + return ret || interrupted;
> +
> }
> --
> 1.7.10.4
More information about the notmuch
mailing list