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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 11, 2025, 16:29 UTC
Message-ID: <xmqqcy7xosu8.fsf@gitster.g>
In-Reply-To: <aMJm57VmWb3h167z@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 14 quoted lines

>> +	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.