Re: [PATCH v3 1/7] stash: use an enum for create status

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

Phillip Wood <phillip.wood123@gmail.com> writes:

> On 21/10/2026 07:07, Junio C Hamano wrote:
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>> 
>>> +enum create_result {
>>> +	CREATE_ERROR = -1,
>>> +	CREATE_SUCCESS,
>>> +	/* No changes were stashed */
>>> +	CREATE_EMPTY,
>>> +};
>> 
>> SUCCESS immediatly following ERROR that is explicitly defined to be
>> (-1) may qualify as explicitly defined to be 0, but personally I
>> probably prefer to be extra explicit here, like
>> 
>> 	CREATE_ERROR = -1,
>> 	CREATE_SUCCESS = 0,
>> 
>> even though this is not worth rerolling the series for.
>
> I can do that, though we really shouldn't be relying on the value for 
> any of the member other than CREATE_ERROR which we want to match the 
> return value of error(...).

The suggestion was merely because I thought that we also expect
SUCCESS to be 0 to write something like this:

	if (!attempt_to_create(...))
		happy;

If we truly care about ERROR == -1 and nothing else, then please
ignore the above.

>>>   		ret = stash_patch(info, ps, patch, quiet, interactive_opts);
>>> -		if (ret < 0) {
>>> +		if (ret == CREATE_ERROR) {
>>>   			if (!quiet)
>>>   				fprintf_ln(stderr, _("Cannot save the current "
>>>   						     "worktree state"));
>>>   			goto done;
>>> -		} else if (ret > 0) {
>>> +		} else if (ret != CREATE_SUCCESS) {
>>>   			goto done;
>>>   		}
>> 
>> I wonder if switch/case is easier to read for these users of these
>> enum return values.  Some check explicitly against CREATE_ERROR
>> (like the one above), some treat anything other than CREATE_SUCCESS
>> as undesirable, etc.
>
> I've had a play with this and I think the enum is better here

Puzzled.  I wasn't contrasting enum vs switch.  I was contrasting
if/else if cascade against switch, assuming that both use enum.