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

[Phillip Wood <phillip.wood123@gmail.com>]

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(...).

> 
>> +static enum create_result stash_staged(struct stash_info *info,
>> +				  struct strbuf *out_patch, int quiet)
>>   {
>> -	int ret = 0;
>> +	enum create_result ret = 0;
> 
> Not CREATE_SUCCESS?  This can be returned if various checks in the
> function assigns different values to this variable, right?

Yes, I forgot to convert the initializer when I changed the type, will fix.

>>   	if (patch_mode) {
>>   		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

>>   	} else if (only_staged) {
>>   		ret = stash_staged(info, patch, quiet);
>> -		if (ret < 0) {
>> +		if (ret == CREATE_ERROR) {
>>   			if (!quiet)
>>   				fprintf_ln(stderr, _("Cannot save the current "
>>   						     "staged state"));
>>   			goto done;
>> -		} else if (ret > 0) {
>> +		} else if (ret != CREATE_SUCCESS) {
>>   			goto done;
>>   		}
>>   	} else {
> 
> Ditto.

and here

> 
>> @@ -1660,12 +1667,13 @@ static int create_stash(int argc, const char **argv, const char *prefix UNUSED,
>>   
>>   	ret = do_create_stash(&ps, &stash_msg_buf, 0, 0, NULL, 0, &info,
>>   			      NULL, 0);
>> -	if (!ret)
>> +	if (ret == CREATE_SUCCESS)
>>   		printf_ln("%s", oid_to_hex(&info.w_commit));
>>   
>>   	free_stash_info(&info);
>>   	strbuf_release(&stash_msg_buf);
>> -	return ret;
>> +	/* Do not return an error if there are no changes */
>> +	return ret == CREATE_ERROR ? -1 : 0;
>>   }
> 
> Ditto.

but here we're checking for a single value, so using a switch seems like 
overkill.

I'll try and push out a new version tomorrow

Thanks

Phillip