Re: [PATCH v4 1/2] stash: reserve exit status 1 for conflicts

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

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> "git stash apply", "pop" and "branch" exit with status 1 both when
> applying the stash entry resulted in conflicts and when they fail for
> other reasons, so callers cannot tell the two apart.
>
> Follow the convention of "git merge-tree" and the merge strategies,
> which exit with status 1 to indicate conflicts and with a different
> non-zero status for errors: those subcommands now exit with status 1
> only when applying the stash entry resulted in conflicts, in which
> case the stash entry is left in place, and exit with status 128, the
> status die() uses, when they fail for other reasons.  Document the
> exit statuses.
>
> cmd_stash() used to collapse the return values of the subcommand
> implementations to a boolean.  It now maps negative values, which
> signal a failure, to 128 and passes everything else through as-is.
> The only implementations that return a positive value are "apply",
> "pop" and "branch", which return the value of do_apply_stash():
> "apply" returns it directly, and "pop" and "branch" drop the stash
> entry, via do_drop_stash(), which always returns 0, only when the
> application succeeded.  The positive value is always 1, as
> do_apply_stash() only returns a positive value when the three-way
> merge was unclean.
>
> Make the convention explicit by introducing enum stash_apply_result
> with the values STASH_APPLY_CLEAN, STASH_APPLY_CONFLICT and
> STASH_APPLY_ERROR, and use it for the in-process autostash helpers,
> too.  They spawn "git stash apply" and can now tell conflicts apart
> from other failures, e.g. a crash or death by signal of the child,
> which map to exit statuses above 1.  Since we know the stash entry
> was saved, tell users so in the error message instead of leaving them
> wondering what happened to their stashed changes.
>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---

The above is on the overly verbose side.  The first two paragraphs
give enough discussion and the remainder mostly repeats with small
details sprinkled in, which can probably be shortened to 1/4 of the
amount of text, but it is OK.

> diff --git a/Documentation/git-stash.adoc b/Documentation/git-stash.adoc
> index 50bb89f483..fc6a9a008c 100644
> --- a/Documentation/git-stash.adoc
> +++ b/Documentation/git-stash.adoc
> @@ -426,6 +426,15 @@ include::includes/cmd-config-section-all.adoc[]
>  :git-stash: 1
>  include::config/stash.adoc[]
>  
> +EXIT STATUS
> +-----------
> +
> +The `git stash` subcommands exit with status 0 on success.  The
> +subcommands that apply a stash entry, i.e. `apply`, `pop` and `branch`,
> +exit with status 1 when applying the stash entry resulted in conflicts,
> +in which case the stash entry is left in place, and with a non-zero
> +status other than 1 when they fail for other reasons.
> +

Great.

> +static enum stash_apply_result do_apply_stash(const char *prefix,
> +					      struct stash_info *info,
> +					      int index, int quiet,
> +					      const char *label_ours,
> +					      const char *label_theirs,
> +					      const char *label_base)
>  {
>  	int clean, ret;
>  	int has_index = index;
> @@ -717,8 +720,8 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
>  
>  	/*
>  	 * If 'clean' >= 0, reverse the value for 'ret' so 'ret' is 0 when the
> -	 * merge was clean, and nonzero if the merge was unclean or encountered
> -	 * an error.
> +	 * merge was clean, and 1 if the merge was unclean or a negative value
> +	 * if it encountered an error.
>  	 */
>  	ret = clean >= 0 ? !clean : clean;

OK.

> +	if (fn) {
> +		ret = fn(argc, argv, prefix, repo);
> +
> +		/*
> +		 * The subcommand implementations return 0 on success, a
> +		 * negative value on failure, and STASH_APPLY_CONFLICT
> +		 * when applying a stash entry resulted in conflicts.
> +		 * Map failures to 128, the status die() uses, so that
> +		 * exit status 1 unambiguously indicates conflicts.
> +		 */
> +		if (ret < 0)
> +			return 128;
> +		return ret;
> +	} else if (!argc)
>  		return !!push_stash_unassumed(0, NULL, prefix, repo);

Style.  Once one of "if", "else if" and "else" cascade gains
{braches}, others should do so as well.

> +static enum stash_apply_result apply_save_autostash_oid(const char *stash_oid,
> +							int attempt_apply,
> +							const char *label_ours,
> +							const char *label_theirs,
> +							const char *label_base,
> +							const char *stash_msg)
>  {
>  	struct child_process child = CHILD_PROCESS_INIT;
> -	int ret = 0;
> +	enum stash_apply_result ret = STASH_APPLY_CLEAN;
>  
>  	if (attempt_apply) {
>  		child.git_cmd = 1;
> @@ -4816,9 +4819,11 @@ static int apply_save_autostash_oid(const char *stash_oid, int attempt_apply,
>  			strvec_pushf(&child.args, "--label-base=%s", label_base);
>  		strvec_push(&child.args, stash_oid);
>  		ret = run_command(&child);
> +		if (ret > 1)
> +			ret = STASH_APPLY_ERROR;

This kind of code that assigns any random "int" that is returned by
run_command() to "enum ret" that has much narrower valid value range
and then makes corrections annoys me a bit.

One way to do this cleanly might be to make a small helper function
do_stash_apply(), and use it like so:

	if (attempt_apply)
		ret = do_stash_apply(stash_oid, label_ours, label_theirs,
				     label_base);

The implementation of do_stash_apply() would be like what you have
in "if (attempt_apply) {...}" block, perhaps like:

	static enum stash_apply_result do_stash_apply(const char *stash_oid,
						      const char *label_ours,
						      const char *label_theirs,
						      const char *label_base)
	{
		struct child_process child = CHILD_PROCESS_INIT;

		child.git_cmd = 1;
		...
                strvec_push(&child.args, stash_oid);
                switch (run_command(&child)) {
		case 0: return STASH_APPLY_CLEAN;
		case 1: return STASH_APPLY_CONFLICT;
		default: return STASH_APPLY_ERROR;
		}
	}

> -	if (attempt_apply && !ret)
> +	if (attempt_apply && ret == STASH_APPLY_CLEAN)
>  		fprintf(stderr, _("Applied autostash.\n"));
>  	else {
>  		struct child_process store = CHILD_PROCESS_INIT;

Good, and the rest of this function is good.