From: Mark Walters <markwalters1009@gmail.com>
To: Peter Wang <novalazy@gmail.com>, notmuch@notmuchmail.org
Subject: Re: [PATCH] cli: make the command line parser's errors more
	informative.
In-Reply-To: <20120605184037.GB14297@hili.localdomain>
References: <1338723972-13063-1-git-send-email-markwalters1009@gmail.com>
	<1338724128-13158-1-git-send-email-markwalters1009@gmail.com>
	<20120605184037.GB14297@hili.localdomain>
User-Agent: Notmuch/0.13+55~g992aa73 (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Tue, 05 Jun 2012 15:34:19 +0100
Message-ID: <87oboxakus.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


On Tue, 05 Jun 2012, Peter Wang <novalazy@gmail.com> wrote:
> On Sun,  3 Jun 2012 12:48:48 +0100, Mark Walters <markwalters1009@gmail.com> wrote:
>>  
>> +static notmuch_bool_t
>> +_process_int_arg (const notmuch_opt_desc_t *arg_desc, char next, const char *arg_str) {
>> +
>> +    char *endptr;
>> +    if (next == 0 || arg_str[0] == 0) {
>> +	fprintf (stderr, "Option \"%s\" needs an integer argument.\n", arg_desc->name);
>> +	return FALSE;
>> +    }
>> +
>> +    *((int *)arg_desc->output_var) = strtol (arg_str, &endptr, 10);
>> +    if (*endptr == 0)
>> +	return TRUE;
>
> It's usually clearer to write '\0' for the null character.

Yes I agree: fixed. I also changed the other instances.

>> @@ -99,20 +133,13 @@ parse_option (const char *arg,
>>  	    char next = arg[strlen (try->name)];
>>  	    const char *value= arg+strlen(try->name)+1;
>>  
>> -	    char *endptr;
>> -
>> -	    /* Everything but boolean arguments (switches) needs a
>> -	     * delimiter, and a non-zero length value. Boolean
>> -	     * arguments may take an optional =true or =false value.
>> -	     */
>> -	    if (next != '=' && next != ':' && next != 0) return FALSE;
>> -	    if (next == 0) {
>> -		if (try->opt_type != NOTMUCH_OPT_BOOLEAN &&
>> -		    try->opt_type != NOTMUCH_OPT_KEYWORD)
>> -		    return FALSE;
>> -	    } else {
>> -		if (value[0] == 0) return FALSE;
>> -	    }
>> +	    /* If this is not the end of the argument (i.e. the next
>> +	     * character is not a space or a delimiter) we stop
>> +	     * parsing for this option but allow the parsing to
>> +	     * continue to for other options. This should allow
>> +	     * options to be initial segments of other options. */
>
> It took me a little while to figure out what the last sentence was
> about.  Perhaps:
>
>     If we have not reached the end of the argument (i.e. the next
>     character is not a space or delimiter) then the argument could
>     still match a longer option name later in the option table.

This is much clearer, thanks!

> (otherwise, "continue to for other")
>
>> +	    if (next != '=' && next != ':' && next != 0)
>> +		goto DONE_THIS_OPTION;
>
> The `goto' could be expressed as a `continue' in a `for' loop, AFAICS.

This is also much nicer. Updated patch follows

Thanks for the review!

Best wishes

Mark