Re: [PATCH v4 1/3] cli: some keyword options can be supplied with no argument

[Daniel Kahn Gillmor <dkg@fifthhorseman.net>]

On Sat 2017-12-23 10:29:30 -0400, David Bremner wrote:
> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>
>>  
>> +	bool incremented = false;
>>  	if (next == '\0' && next_arg != NULL && ! try->opt_bool) {
>>  	    next = ' ';
>>  	    value = next_arg;
>> +	    incremented = true;
>>  	    opt_index ++;
>>  	}
>
> Is incremented == true exactly when next == ' ' ? It might be nice to
> make that more explicit by setting one based on the other. You could
> also use (next == ' ') as your test condition, but I understand that
> might not be that obvious to read.

yes, i was aiming for readability.  I'm assuming that the compiler can
optimize this as needed.

> The thing I'm most nervous about here is the interaction between this
> new code and the relatively recent code that permits ' ' as a
> seperator. Would you mind adding one or more tests for that case? For
> example, that I checked that
>
>          ./notmuch show --format=json --decrypt true $id
>
> continues to work, and that's great, but it seems like something to
> check on the argument parsing level, i.e
>
> --keyword␣non-default-value
>
> (pardon my unicode)

I'm fine with that, and with your proposed revision of this patch that
includes the amended test.  thanks for pushing it forward.

         --dkg