Re: [PATCH 2/4] stash: convert branch to builtin

[Thomas Gummerer]

On 03/24, Joel Teichroeb wrote:
> ---
>  builtin/stash--helper.c | 44 
>  git-stash.sh|  3 ++-
>  2 files changed, 46 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> index e9a9574f40..18c4aba665 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 apply [--index] [-q|--quiet] []"),
> + N_("git stash--helper branch  []"),
>   NULL
>  };
>  
> @@ -20,6 +21,11 @@ static const char * const git_stash_helper_apply_usage[] = 
> {
>   NULL
>  };
>  
> +static const char * const git_stash_helper_branch_usage[] = {
> + N_("git stash--helper branch  []"),
> + NULL
> +};
> +
>  static const char *ref_stash = "refs/stash";
>  static int quiet;
>  static char stash_index_path[PATH_MAX];
> @@ -307,6 +313,42 @@ static int apply_stash(int argc, const char **argv, 
> const char *prefix)
>   return do_apply_stash(prefix, , index);
>  }
>  
> +static int branch_stash(int argc, const char **argv, const char *prefix)
> +{
> + const char *commit = NULL, *branch = NULL;
> + int ret;
> + struct argv_array args = ARGV_ARRAY_INIT;
> + struct stash_info info;
> + struct option options[] = {
> + OPT_END()
> + };
> +
> + argc = parse_options(argc, argv, prefix, options,
> + git_stash_helper_branch_usage, 0);
> +
> + if (argc != 0) {
> + branch = argv[0];
> + if (argc == 2)
> + commit = argv[1];
> + }
> +
> + if (get_stash_info(, commit))
> + return -1;

I see this is supposed to do something similar to what
'assert_stash_like' was doing.  However we never end up die'ing with
"... is not a a stash-like commit" here from what I can see.  I think
I can see where this is coming from, and I missed it when reading over
1/4 here.  I'll go back and comment there, where I think we're going
slightly wrong.

Either way while I tripped over the 'get_stash_info' call here, I
think it's the right thing to do.

> + argv_array_pushl(, "checkout", "-b", NULL);
> + argv_array_push(, branch);
> + argv_array_push(, sha1_to_hex(info.b_commit.hash));
> + ret = cmd_checkout(args.argc, args.argv, prefix);
> + if (ret)
> + return -1;
> +
> + ret = do_apply_stash(prefix, , 1);
> + if (!ret && info.is_stash_ref)
> + ret = do_drop_stash(prefix, );

'do_drop_stash' is only defined in the next patch.  Maybe maybe 2/4
and 3/4 need to swap places?

All patches should compile individually, and all tests should pass for
each patch, so we maintain bisectability of the codebase.

> +
> + return ret;
> +}
> +
>  int cmd_stash__helper(int argc, const char **argv, const char *prefix)
>  {
>   int result = 0;
> @@ -329,6 +371,8 @@ int cmd_stash__helper(int argc, const char **argv, const 
> char *prefix)
>   usage_with_options(git_stash_helper_usage, options);
>   else if (!strcmp(argv[0], "apply"))
>   result = apply_stash(argc, argv, prefix);
> + else if (!strcmp(argv[0], "branch"))
> + result = branch_stash(argc, argv, prefix);
>   else {
>   error(_("unknown subcommand: %s"), argv[0]);
>   usage_with_options(git_stash_helper_usage, options);
> diff --git a/git-stash.sh b/git-stash.sh
> index 92c084eb17..360643ad4e 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -736,7 +736,8 @@ pop)
>   ;;
>  branch)
>   shift
> - apply_to_branch "$@"
> + cd "$START_DIR"
> + git stash--helper branch "$@"
>   ;;
>  *)
>   case $# in
> -- 
> 2.16.2
> 

Re: [PATCH 2/4] stash: convert branch to builtin

[Christian Couder]

On Sat, Mar 24, 2018 at 6:37 PM, Joel Teichroeb  wrote:
> diff --git a/git-stash.sh b/git-stash.sh
> index 92c084eb17..360643ad4e 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -736,7 +736,8 @@ pop)
> ;;
>  branch)
> shift
> -   apply_to_branch "$@"
> +   cd "$START_DIR"
> +   git stash--helper branch "$@"
> ;;
>  *)
> case $# in

Can the apply_to_branch() shell function be removed from git-stash.sh?

Re: [PATCH 2/4] stash: convert branch 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
> @@ -307,6 +313,42 @@ static int apply_stash(int argc, const char **argv, 
> const char *prefix)
> +static int branch_stash(int argc, const char **argv, const char *prefix)
> +{
> +   const char *commit = NULL, *branch = NULL;
> +
> +   argc = parse_options(argc, argv, prefix, options,
> +   git_stash_helper_branch_usage, 0);
> +
> +   if (argc != 0) {
> +   branch = argv[0];
> +   if (argc == 2)
> +   commit = argv[1];
> +   }

This seems fragile. What happens if there are three args?

> +   if (get_stash_info(, commit))
> +   return -1;
> +
> +   argv_array_pushl(, "checkout", "-b", NULL);
> +   argv_array_push(, branch);
> +   argv_array_push(, sha1_to_hex(info.b_commit.hash));
> +   ret = cmd_checkout(args.argc, args.argv, prefix);
> +   if (ret)
> +   return -1;
> +
> +   ret = do_apply_stash(prefix, , 1);
> +   if (!ret && info.is_stash_ref)
> +   ret = do_drop_stash(prefix, );
> +
> +   return ret;
> +}

[PATCH 2/4] stash: convert branch to builtin

[Joel Teichroeb]

---
 builtin/stash--helper.c | 44 
 git-stash.sh|  3 ++-
 2 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index e9a9574f40..18c4aba665 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 apply [--index] [-q|--quiet] []"),
+   N_("git stash--helper branch  []"),
NULL
 };
 
@@ -20,6 +21,11 @@ static const char * const git_stash_helper_apply_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_branch_usage[] = {
+   N_("git stash--helper branch  []"),
+   NULL
+};
+
 static const char *ref_stash = "refs/stash";
 static int quiet;
 static char stash_index_path[PATH_MAX];
@@ -307,6 +313,42 @@ static int apply_stash(int argc, const char **argv, const 
char *prefix)
return do_apply_stash(prefix, , index);
 }
 
+static int branch_stash(int argc, const char **argv, const char *prefix)
+{
+   const char *commit = NULL, *branch = NULL;
+   int ret;
+   struct argv_array args = ARGV_ARRAY_INIT;
+   struct stash_info info;
+   struct option options[] = {
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+   git_stash_helper_branch_usage, 0);
+
+   if (argc != 0) {
+   branch = argv[0];
+   if (argc == 2)
+   commit = argv[1];
+   }
+
+   if (get_stash_info(, commit))
+   return -1;
+
+   argv_array_pushl(, "checkout", "-b", NULL);
+   argv_array_push(, branch);
+   argv_array_push(, sha1_to_hex(info.b_commit.hash));
+   ret = cmd_checkout(args.argc, args.argv, prefix);
+   if (ret)
+   return -1;
+
+   ret = do_apply_stash(prefix, , 1);
+   if (!ret && info.is_stash_ref)
+   ret = do_drop_stash(prefix, );
+
+   return ret;
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
int result = 0;
@@ -329,6 +371,8 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
usage_with_options(git_stash_helper_usage, options);
else if (!strcmp(argv[0], "apply"))
result = apply_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "branch"))
+   result = branch_stash(argc, argv, prefix);
else {
error(_("unknown subcommand: %s"), argv[0]);
usage_with_options(git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index 92c084eb17..360643ad4e 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -736,7 +736,8 @@ pop)
;;
 branch)
shift
-   apply_to_branch "$@"
+   cd "$START_DIR"
+   git stash--helper branch "$@"
;;
 *)
case $# in
-- 
2.16.2