[Patch v3b 9/9] tag-util: optimization of tag application
Mark Walters
markwalters1009 at gmail.com
Sat Dec 8 03:22:53 PST 2012
On Fri, 07 Dec 2012, david at tethera.net wrote:
> From: David Bremner <bremner at debian.org>
>
> The idea is not to bother with restore operations if they don't change
> the set of tags. This is actually a relatively common case.
>
> In order to avoid fancy datastructures, this method is quadratic in
> the number of tags; at least on my mail database this doesn't seem to
> be a big problem.
> ---
> tag-util.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 66 insertions(+)
>
> diff --git a/tag-util.c b/tag-util.c
> index 932ee7f..3d54e9e 100644
> --- a/tag-util.c
> +++ b/tag-util.c
> @@ -124,6 +124,69 @@ message_error (notmuch_message_t *message,
> fprintf (stderr, "Status: %s\n", notmuch_status_to_string (status));
> }
>
> +static int
> +makes_changes (notmuch_message_t *message,
> + tag_op_list_t *list,
> + tag_op_flag_t flags)
> +{
> +
> + size_t i;
> +
> + notmuch_tags_t *tags;
> + notmuch_bool_t changes = FALSE;
> +
> + /* First, do we delete an existing tag? */
> + changes = FALSE;
> + for (tags = notmuch_message_get_tags (message);
> + ! changes && notmuch_tags_valid (tags);
> + notmuch_tags_move_to_next (tags)) {
> + const char *cur_tag = notmuch_tags_get (tags);
> + int last_op = (flags & TAG_FLAG_REMOVE_ALL) ? -1 : 0;
> +
> + /* slight contortions to count down with an unsigned index */
> + for (i = list->count; i-- > 0; /*nothing*/) {
> + if (strcmp (cur_tag, list->ops[i].tag) == 0) {
> + last_op = list->ops[i].remove ? -1 : 1;
> + break;
> + }
> + }
I agree that this is a little ugly but ok I think. Is it worth adding a
comment as to why you are counting backwards? eg " we count backwards to
check whether the last change for the tag foo is removal"
But basically LGTM
Mark
> +
> + changes = (last_op == -1);
> + }
> + notmuch_tags_destroy (tags);
> +
> + if (changes)
> + return TRUE;
> +
> + /* Now check for adding new tags */
> + for (i = 0; i < list->count; i++) {
> + notmuch_bool_t exists = FALSE;
> +
> + if (list->ops[i].remove)
> + continue;
> +
> + for (tags = notmuch_message_get_tags (message);
> + notmuch_tags_valid (tags);
> + notmuch_tags_move_to_next (tags)) {
> + const char *cur_tag = notmuch_tags_get (tags);
> + if (strcmp (cur_tag, list->ops[i].tag) == 0) {
> + exists = TRUE;
> + break;
> + }
> + }
> + notmuch_tags_destroy (tags);
> +
> + /* the following test is conservative,
> + * in the sense it ignores cases like +foo ... -foo
> + * but this is OK from a correctness point of view
> + */
> + if (! exists)
> + return TRUE;
> + }
> + return FALSE;
> +
> +}
> +
> notmuch_status_t
> tag_op_list_apply (notmuch_message_t *message,
> tag_op_list_t *list,
> @@ -133,6 +196,9 @@ tag_op_list_apply (notmuch_message_t *message,
> notmuch_status_t status = 0;
> tag_operation_t *tag_ops = list->ops;
>
> + if (! (flags & TAG_FLAG_PRE_OPTIMIZED) && ! makes_changes (message, list, flags))
> + return NOTMUCH_STATUS_SUCCESS;
> +
> status = notmuch_message_freeze (message);
> if (status) {
> message_error (message, status, "freezing message");
> --
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
More information about the notmuch
mailing list