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:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Change the return type of do_create_stash() to an enum in preparation
> for adding a separate value for "there are no changes to stash" in a
> future commit. There is a small change to behavior here which fixes
> an anomaly with the exit status of create_stash(). That function calls
> check_changes() without refreshing the index and exits 0 if there are
> no changes. However, because it does not refresh the index, that check
> is unreliable which means do_create_stash() can return 1 when there
> aren't any changes, and then create_stash() exits 1 rather than 0.
> This commit fixes that anomaly.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/stash.c | 83 ++++++++++++++++++++++++++++---------------------
>  1 file changed, 47 insertions(+), 36 deletions(-)

The patch makes it look more scary larger than it actually is.

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

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

> +static enum create_result stash_patch(struct stash_info *info,
> +		const struct pathspec *ps, struct strbuf *out_patch, int quiet,
> +		struct interactive_options *interactive_opts)
>  {
> -	int ret = 0;
> +	enum create_result ret = 0;

CREATE_SUCCESS?

> +static enum create_result do_create_stash(const struct pathspec *ps,
> +		struct strbuf *stash_msg_buf, int include_untracked,
> +		int patch_mode, struct interactive_options *interactive_opts,
> +		int only_staged, struct stash_info *info, struct strbuf *patch,
> +		int quiet)
>  {
> -	int ret = 0;
> +	enum create_result ret = 0;

CREATE_SUCCESS?

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

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

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