Re: [PATCH 4/4] stash: convert pop to builtin

[Thomas Gummerer <t.gummerer@gmail.com>]

On 03/24, Joel Teichroeb wrote:
> ---
>  builtin/stash--helper.c | 38 ++++++++++++++++++++++++++++++++++++++
>  git-stash.sh            |  3 ++-
>  2 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> index 1598b82ac2..b912f84c97 100644
> --- a/builtin/stash--helper.c
> +++ b/builtin/stash--helper.c
> @@ -12,6 +12,7 @@
>  
>  static const char * const git_stash_helper_usage[] = {
>  	N_("git stash--helper drop [-q|--quiet] [<stash>]"),
> +	N_("git stash--helper pop [--index] [-q|--quiet] [<stash>]"),
>  	N_("git stash--helper apply [--index] [-q|--quiet] [<stash>]"),
>  	N_("git stash--helper branch <branchname> [<stash>]"),
>  	N_("git stash--helper clear"),
> @@ -23,6 +24,11 @@ static const char * const git_stash_helper_drop_usage[] = {
>  	NULL
>  };
>  
> +static const char * const git_stash_helper_pop_usage[] = {
> +	N_("git stash--helper pop [--index] [-q|--quiet] [<stash>]"),
> +	NULL
> +};
> +
>  static const char * const git_stash_helper_apply_usage[] = {
>  	N_("git stash--helper apply [--index] [-q|--quiet] [<stash>]"),
>  	NULL
> @@ -402,6 +408,36 @@ static int drop_stash(int argc, const char **argv, const char *prefix)
>  	return do_drop_stash(prefix, &info);
>  }
>  
> +static int pop_stash(int argc, const char **argv, const char *prefix)
> +{
> +	int index = 0;
> +	const char *commit = NULL;
> +	struct stash_info info;
> +	struct option options[] = {
> +		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
> +		OPT_BOOL(0, "index", &index,
> +			N_("attempt to ininstate the index")),
> +		OPT_END()
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options,
> +			git_stash_helper_pop_usage, 0);
> +
> +	if (argc == 1)
> +		commit = argv[0];
> +
> +	if (get_stash_info(&info, commit))
> +		return -1;
> +
> +	if (!info.is_stash_ref)
> +		return error(_("'%s' is not a stash reference"), commit);

The pattern above appears twice now, is it worth factoring it out into
a separate function, similar to 'assert_stash_ref'?

> +
> +	if (do_apply_stash(prefix, &info, index))
> +		return -1;

If we fail, currently we print "The stash entry is kept in case you
need it again", which we are loosing here.  I think that's useful
output in case the 'apply' command fails, especially in the case of a
merge conflict, where I think the 'apply' will fail as well, and the
user may be confused whether/why the stash is not dropped.

> +
> +	return do_drop_stash(prefix, &info);
> +}
> +
>  static int branch_stash(int argc, const char **argv, const char *prefix)
>  {
>  	const char *commit = NULL, *branch = NULL;
> @@ -464,6 +500,8 @@ int cmd_stash__helper(int argc, const char **argv, const char *prefix)
>  		result = clear_stash(argc, argv, prefix);
>  	else if (!strcmp(argv[0], "drop"))
>  		result = drop_stash(argc, argv, prefix);
> +	else if (!strcmp(argv[0], "pop"))
> +		result = pop_stash(argc, argv, prefix);
>  	else if (!strcmp(argv[0], "branch"))
>  		result = branch_stash(argc, argv, prefix);
>  	else {
> diff --git a/git-stash.sh b/git-stash.sh
> index 54d0a6c21f..d595bbaf64 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -732,7 +732,8 @@ drop)
>  	;;
>  pop)
>  	shift
> -	pop_stash "$@"
> +	cd "$START_DIR"
> +	git stash--helper pop "$@"
>  	;;
>  branch)
>  	shift
> -- 
> 2.16.2
>