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

[Thomas Gummerer]

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] []"),
> + N_("git stash--helper pop [--index] [-q|--quiet] []"),
>   N_("git stash--helper apply [--index] [-q|--quiet] []"),
>   N_("git stash--helper branch  []"),
>   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] []"),
> + NULL
> +};
> +
>  static const char * const git_stash_helper_apply_usage[] = {
>   N_("git stash--helper apply [--index] [-q|--quiet] []"),
>   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
> 

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

[Eric Sunshine]

On Sat, Mar 24, 2018 at 1:37 PM, Joel Teichroeb  wrote:
> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> @@ -402,6 +408,36 @@ static int drop_stash(int argc, const char **argv, const 
> char *prefix)
> +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")),

"ininstate"??

> +   OPT_END()
> +   };
> +
> +   argc = parse_options(argc, argv, prefix, options,
> +   git_stash_helper_pop_usage, 0);
> +
> +   if (argc == 1)
> +   commit = argv[0];

Seems fragile. What if there are two arguments?

> +   if (get_stash_info(&info, commit))
> +   return -1;
> +
> +   if (!info.is_stash_ref)
> +   return error(_("'%s' is not a stash reference"), commit);
> +
> +   if (do_apply_stash(prefix, &info, index))
> +   return -1;
> +
> +   return do_drop_stash(prefix, &info);
> +}

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

[Joel Teichroeb]

---
 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] []"),
+   N_("git stash--helper pop [--index] [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
N_("git stash--helper branch  []"),
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] []"),
+   NULL
+};
+
 static const char * const git_stash_helper_apply_usage[] = {
N_("git stash--helper apply [--index] [-q|--quiet] []"),
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);
+
+   if (do_apply_stash(prefix, &info, index))
+   return -1;
+
+   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