From: Jani Nikula <jani@nikula.org>
To: david@tethera.net, notmuch@notmuchmail.org
Subject: Re: [Patch v6 1/6] tag-util: factor out rules for illegal tags,
	use in parse_tag_line
In-Reply-To: <1355096008-4544-2-git-send-email-david@tethera.net>
References: <1355096008-4544-1-git-send-email-david@tethera.net>
	<1355096008-4544-2-git-send-email-david@tethera.net>
User-Agent: Notmuch/0.14+138~g7041c56 (http://notmuchmail.org) Emacs/23.4.1
	(i686-pc-linux-gnu)
Date: Mon, 10 Dec 2012 22:55:38 +0200
Message-ID: <87mwxlaaph.fsf@nikula.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: David Bremner <bremner@debian.org>
Precedence: list

On Mon, 10 Dec 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> This will allow us to be consistent between batch tagging and command
> line tagging as far as what is an illegal tag.

LGTM, with some nitpick and observations below.

Jani.

> ---
>  tag-util.c |   35 ++++++++++++++++++++++++++++++-----
>  1 file changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/tag-util.c b/tag-util.c
> index eab482f..c071ea8 100644
> --- a/tag-util.c
> +++ b/tag-util.c
> @@ -31,6 +31,29 @@ line_error (tag_parse_status_t status,
>      return status;
>  }
>  
> +/*
> + * Test tags for some forbidden cases.
> + *
> + * return: NULL if OK,
> + *	   explanatory message otherwise.
> + */
> +
> +static const char *
> +illegal_tag (const char *tag, notmuch_bool_t remove) {

Add \n before opening brace.

> +
> +    if (*tag == '\0' && !remove)
> +	return "adding empty tag";
> +
> +    /* This disallows adding the non-removable tag "-" and
> +     * enables notmuch tag to take long options more easily.
> +     */

Maybe make that: "This disallows adding tags starting with "-",
including the non-removable tag "-", and enables ...", or similar?

> +
> +    if (*tag == '-' && !remove)
> +	return  "adding tag starting with -";
> +
> +    return NULL;
> +}
> +
>  tag_parse_status_t
>  parse_tag_line (void *ctx, char *line,
>  		tag_op_flag_t flags,
> @@ -95,11 +118,13 @@ parse_tag_line (void *ctx, char *line,
>  	remove = (*tok == '-');
>  	tag = tok + 1;
>  
> -	/* Maybe refuse empty tags. */
> -	if (! (flags & TAG_FLAG_BE_GENEROUS) && *tag == '\0') {
> -	    ret = line_error (TAG_PARSE_INVALID, line_for_error,
> -			      "empty tag");
> -	    goto DONE;
> +	/* Maybe refuse illegal tags. */
> +	if (! (flags & TAG_FLAG_BE_GENEROUS)) {
> +	    const char *msg = illegal_tag (tag, remove);
> +	    if (msg) {
> +		ret = line_error (TAG_PARSE_INVALID, line_for_error, msg);
> +		goto DONE;
> +	    }
>  	}

I guess I failed to notice during the dump/restore review that this
function was (and still is until later in the series) always called with
TAG_FLAG_BE_GENEROUS. I guess that's what we want with restore;
otherwise we should warn about such tags during dump.

>  
>  	/* Decode tag. */
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch