From: David Bremner <david@tethera.net>
To: Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org
Subject: Re: [PATCH 2/5] util: Function to parse boolean term queries
In-Reply-To: <1356415076-5692-3-git-send-email-amdragon@mit.edu>
References: <1356415076-5692-1-git-send-email-amdragon@mit.edu>
	<1356415076-5692-3-git-send-email-amdragon@mit.edu>
User-Agent: Notmuch/0.14+213~g4af1ac6 (http://notmuchmail.org) Emacs/24.2.1
	(x86_64-pc-linux-gnu)
Date: Tue, 25 Dec 2012 10:18:42 -0400
Message-ID: <87obhidxkt.fsf@zancas.localnet>
MIME-Version: 1.0
Content-Type: text/plain
X-Spam_bar: -
X-BeenThere: notmuch@notmuchmail.org
X-Mailman-Version: 2.1.13
Precedence: list
List-Id: "Use and development of the notmuch mail system."
	<notmuch.notmuchmail.org>
List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,
	<mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>
List-Archive: <http://notmuchmail.org/pipermail/notmuch>
List-Post: <mailto:notmuch@notmuchmail.org>
List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>
List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,
	<mailto:notmuch-request@notmuchmail.org?subject=subscribe>
X-List-Received-Date: Tue, 25 Dec 2012 14:18:55 -0000

Austin Clements <amdragon@MIT.EDU> writes:

> +    if (consume_double_quote (&pos)) {
> +	char *out = talloc_strdup (ctx, pos);
> +	pos = *term_out = out;
> +	while (1) {

Overall the control flow here is a bit tricky to follow. I'm not sure if
a real loop condition would help or make it worse.

> +	    if (! *pos) {
> +		/* Premature end of string */
> +		goto FAIL;
> +	    } else if (*pos == '"') {
> +		if (*++pos != '"')
> +		    break;
> +	    } else if (consume_double_quote (&pos)) {
> +		break;
> +	    }

I'm confused by the asymmetry here. Quoted strings can start with
unicode quotes, but internally can only have ascii '"'? Is this
documented somewhere?

> +    } else {
> +	while (*pos > ' ' && *pos != ')')
> +	    ++pos;
> +	if (*pos)
> +	    goto FAIL;
> +    }

So if there is no quote, we skip the part after the ':'? I guess I
probably missed something because that doesn't sound like the intended
behaviour.