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, [email protected] wrote: > From: Jani Nikula <[email protected]> > > 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 <[email protected]> > > Hacked-like-crazy-by: David Bremner <[email protected]> > --- > 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 _______________________________________________ notmuch mailing list [email protected] http://notmuchmail.org/mailman/listinfo/notmuch
