Re: [PATCH 4/4] stash: convert pop to builtin
- From
Thomas Gummerer <t.gummerer@gmail.com>
- Date
- Mar 25, 2018, 17:36 UTC
- Message-ID
- <20180325173635.GD10909@hank>
- In-Reply-To
- <20180324173707.17699-5-joel@teichroeb.net>
On 03/24, Joel Teichroeb wrote:
Show 56 quoted lines
> ---
> 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.
Show 33 quoted lines
> +
> + 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
>