From: Tomi Ollila <tomi.ollila@iki.fi>
To: Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org
Subject: Re: [PATCH v2 00/11] Fix search tagging races
In-Reply-To: <1382627951-25252-1-git-send-email-amdragon@mit.edu>
References: <1382627951-25252-1-git-send-email-amdragon@mit.edu>
User-Agent: Notmuch/0.16+119~g219c55f (http://notmuchmail.org) Emacs/24.3.1
	(x86_64-unknown-linux-gnu)
Date: Thu, 07 Nov 2013 19:26:32 +0200
Message-ID: <m2k3gki0mv.fsf@guru.guru-group.fi>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list

On Thu, Oct 24 2013, Austin Clements <amdragon@MIT.EDU> wrote:

> This is v2 of id:1381185201-25197-1-git-send-email-amdragon@mit.edu.
> It fixes several comments from Mark and Jani.  This has been rebased
> on top of the tag completion changes, so doing * from a large search
> buffer will no longer crash.  Hence, this series depends on the
> (currently pending) series in
> id:1382487721-31776-1-git-send-email-amdragon@mit.edu.
>
> This version does not address what happens when you * on a search
> buffer that's still filling.  With this series, it will apply to all
> messages that have appeared when the user finishes entering tag
> changes.  This isn't ideal, but this seems pretty obscure and I'm not
> sure what the right answer is, so I'm punting it to the future.
>
> Another thing that may be worth changing in a follow-up is what
> messages * applies to.  Currently, * applies *only* to matched
> messages, not all threads in the search, which I think was an accident
> of implementation.  This series retains that behavior, but opens up
> the possibility of applying to all threads instead.  I think that
> would be much more consistent and less surprising behavior.

LGTM. applies cleanly and tests pass

Tomi


>
> An approximate diff from v1 is below.  This diff is prior to rebasing,
> since the post-rebase diff is not useful.
>
> diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
> index a4eec14..36937fb 100644
> --- a/emacs/notmuch-tag.el
> +++ b/emacs/notmuch-tag.el
> @@ -242,7 +242,11 @@ from TAGS if present."
>  	   (error "Changed tag must be of the form `+this_tag' or `-that_tag'")))))
>      (sort result-tags 'string<)))
>  
> -(defconst notmuch-tag-argument-limit 1000)
> +(defconst notmuch-tag-argument-limit 1000
> +  "Use batch tagging if the tagging query is longer than this.
> +
> +This limits the length of arguments passed to the notmuch CLI to
> +avoid system argument length limits and performance problems.")
>  
>  (defun notmuch-tag (query &optional tag-changes)
>    "Add/remove tags in TAG-CHANGES to messages matching QUERY.
> @@ -276,7 +280,6 @@ notmuch-after-tag-hook will be run."
>        ;; Use batch tag mode to avoid argument length limitations
>        (let ((batch-op (concat (mapconcat #'notmuch-hex-encode tag-changes " ")
>  			      " -- " query)))
> -	(message "Batch tagging with %s" batch-op)
>  	(notmuch-call-notmuch-process :stdin-string batch-op "tag" "--batch")))
>      (run-hooks 'notmuch-after-tag-hook))
>    ;; in all cases we return tag-changes as a list
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 1b14910..8bc1a2a 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -144,7 +144,8 @@ chomp_newline (char *str)
>  #define NOTMUCH_FORMAT_MIN 1
>  /* The minimum non-deprecated structured output format version.
>   * Requests for format versions below this will print a stern warning.
> - * Must be >= NOTMUCH_FORMAT_MIN and < NOTMUCH_FORMAT_CUR.
> + * Must be between NOTMUCH_FORMAT_MIN and NOTMUCH_FORMAT_CUR,
> + * inclusive.
>   */
>  #define NOTMUCH_FORMAT_MIN_ACTIVE 1
>  
> diff --git a/notmuch-search.c b/notmuch-search.c
> index 1d14651..7c973b3 100644
> --- a/notmuch-search.c
> +++ b/notmuch-search.c
> @@ -53,13 +53,13 @@ sanitize_string (const void *ctx, const char *str)
>   * NULL. */
>  static int
>  get_thread_query (notmuch_thread_t *thread,
> -		  char **matched_out, char **unmached_out)
> +		  char **matched_out, char **unmatched_out)
>  {
>      notmuch_messages_t *messages;
>      char *escaped = NULL;
>      size_t escaped_len = 0;
>  
> -    *matched_out = *unmached_out = NULL;
> +    *matched_out = *unmatched_out = NULL;
>  
>      for (messages = notmuch_thread_get_messages (thread);
>  	 notmuch_messages_valid (messages);
> @@ -69,17 +69,16 @@ get_thread_query (notmuch_thread_t *thread,
>  	const char *mid = notmuch_message_get_message_id (message);
>  	/* Determine which query buffer to extend */
>  	char **buf = notmuch_message_get_flag (
> -	    message, NOTMUCH_MESSAGE_FLAG_MATCH) ? matched_out : unmached_out;
> -	/* Allocate the query buffer is this is the first message */
> -	if (!*buf && (*buf = talloc_strdup (thread, "")) == NULL)
> -	    return -1;
> +	    message, NOTMUCH_MESSAGE_FLAG_MATCH) ? matched_out : unmatched_out;
>  	/* Add this message's id: query.  Since "id" is an exclusive
>  	 * prefix, it is implicitly 'or'd together, so we only need to
>  	 * join queries with a space. */
>  	if (make_boolean_term (thread, "id", mid, &escaped, &escaped_len) < 0)
>  	    return -1;
> -	*buf = talloc_asprintf_append_buffer (
> -	    *buf, "%s%s", **buf ? " " : "", escaped);
> +	if (*buf)
> +	    *buf = talloc_asprintf_append_buffer (*buf, " %s", escaped);
> +	else
> +	    *buf = talloc_strdup (thread, escaped);
>  	if (!*buf)
>  	    return -1;
>      }
>
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch