Re: [PATCH 1/2] gpg-interface: refactor 'enum sign_mode' parsing

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

>> +	if (!parse_sign_mode(arg, val))
>> +		return 0;
>> +
>> +	return error("Unknown %s mode: %s", opt->long_name, arg);
>
> Would it make sense to maybe reverse the error handling and say
> something like:
>
>     if (parse_sign_mode(arg, val) < 0)
>         return error("Unknown %s mode: %s", opt->long_name, arg);
>
>     return 0;
>
> That reads a bit more natural to me at least.

I also thought that the original had a "Huh?" factor, but my
reaction was more like "wouldn't it be easier to see if these are
both sides of if/else?", i.e.

	if (parse_sign_mode(...))
		return error(...);
	else
		return 0;

which made me think that the differences were mostly subjective and
refrained from commenting.

But after I see your version, I tend to agree that it is easier to
see the flow if the most-straightforward success case ran through to
the end, while exceptional cases (i.e. an error return, or the
initial "unset" case) did early returns.  I have to add, however,
that I feel that this is mostly subjective and falls into "once it
is in the tree, it's not really worth the patch noise to go and fix
it up" category.  If we will have v2 of this series, I do prefer to
see it written more in your way, though.

Thanks.