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

[Johannes Schindelin]

Hi Joel,

On Wed, 4 Apr 2018, Joel Teichroeb wrote:

> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> index 520cd746c4..486796bb6a 100644
> --- a/builtin/stash--helper.c
> +++ b/builtin/stash--helper.c
> @@ -502,6 +508,49 @@ static int drop_stash(int argc, const char **argv, const 
> char *prefix)
>   return ret;
>  }
>  
> +static int branch_stash(int argc, const char **argv, const char *prefix)
> +{
> + const char *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)
> + return error(_("No branch name specified"));

We should probably also test for argc > 2 and error out in that case...

> +
> + branch = argv[0];
> +
> + if (get_stash_info(, argc - 1, argv + 1))
> + return -1;
> +
> + /* Checkout does not currently provide a function for checking out a 
> branch
> +  * as cmd_checkout does a large amount of sanity checks first that we
> +  * require here.
> +  */

While this is true, this code comment is *prone* to become stale. Maybe
move this remark into the commit message?

> + argv_array_pushl(, "checkout", "-b", NULL);
> + argv_array_push(, branch);
> + argv_array_push(, oid_to_hex(_commit));
> + ret = cmd_checkout(args.argc, args.argv, prefix);
> + if (ret) {
> + free_stash_info();
> + return -1;
> + }
> +
> + ret = do_apply_stash(prefix, , 1);
> + if (!ret && info.is_stash_ref)
> + ret = do_drop_stash(prefix, );

An alternative way to translate that &&-chain would be to do this:

ret = cmd_checkout(args.argc, args.argv, prefix);
if (!ret)
ret = do_apply_stash(prefix, , 1);
if (!ret && info.is_stash_ref)
ret = do_drop_stash(prefix, );

... which might be a bit easier to read and to maintain?

> +
> + free_stash_info();
> +
> + return ret;
> +}
> +
>  int cmd_stash__helper(int argc, const char **argv, const char *prefix)
>  {
>   int result = 0;
> @@ -528,6 +577,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], "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 0b8f07b38a..c5fd4c6c44 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -598,20 +598,6 @@ drop_stash () {
>   clear_stash
>  }
>  
> -apply_to_branch () {
> - test -n "$1" || die "$(gettext "No branch name specified")"
> - branch=$1
> - shift 1
> -
> - set -- --index "$@"
> - assert_stash_like "$@"
> -
> - git checkout -b $branch $REV^ &&
> - apply_stash "$@" && {
> - test -z "$IS_STASH_REF" || drop_stash "$@"
> - }
> -}
> -
>  test "$1" = "-p" && set "push" "$@"
>  
>  PARSE_CACHE='--not-parsed'
> @@ -672,7 +658,8 @@ pop)
>   ;;
>  branch)
>   shift
> - apply_to_branch "$@"
> + cd "$START_DIR"
> + git stash--helper branch "$@"
>   ;;
>  *)
>   case $# in

The rest looks obviously good to me (I am not all that sure about the `cd
"$START_DIR"` but it definitely does not hurt).

Ciao,
Dscho

[PATCH v5 4/5] stash: convert branch to builtin

[Joel Teichroeb]

Add stash branch to the helper and delete the apply_to_branch
function from the shell script.

Signed-off-by: Joel Teichroeb 
---
 builtin/stash--helper.c | 51 +
 git-stash.sh| 17 ++---
 2 files changed, 53 insertions(+), 15 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 520cd746c4..486796bb6a 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -14,6 +14,7 @@
 static const char * const git_stash_helper_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   N_("git stash--helper branch  []"),
N_("git stash--helper clear"),
NULL
 };
@@ -28,6 +29,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 * const git_stash_helper_clear_usage[] = {
N_("git stash--helper clear"),
NULL
@@ -502,6 +508,49 @@ static int drop_stash(int argc, const char **argv, const 
char *prefix)
return ret;
 }
 
+static int branch_stash(int argc, const char **argv, const char *prefix)
+{
+   const char *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)
+   return error(_("No branch name specified"));
+
+   branch = argv[0];
+
+   if (get_stash_info(, argc - 1, argv + 1))
+   return -1;
+
+   /* Checkout does not currently provide a function for checking out a 
branch
+* as cmd_checkout does a large amount of sanity checks first that we
+* require here.
+*/
+   argv_array_pushl(, "checkout", "-b", NULL);
+   argv_array_push(, branch);
+   argv_array_push(, oid_to_hex(_commit));
+   ret = cmd_checkout(args.argc, args.argv, prefix);
+   if (ret) {
+   free_stash_info();
+   return -1;
+   }
+
+   ret = do_apply_stash(prefix, , 1);
+   if (!ret && info.is_stash_ref)
+   ret = do_drop_stash(prefix, );
+
+   free_stash_info();
+
+   return ret;
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
int result = 0;
@@ -528,6 +577,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], "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 0b8f07b38a..c5fd4c6c44 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -598,20 +598,6 @@ drop_stash () {
clear_stash
 }
 
-apply_to_branch () {
-   test -n "$1" || die "$(gettext "No branch name specified")"
-   branch=$1
-   shift 1
-
-   set -- --index "$@"
-   assert_stash_like "$@"
-
-   git checkout -b $branch $REV^ &&
-   apply_stash "$@" && {
-   test -z "$IS_STASH_REF" || drop_stash "$@"
-   }
-}
-
 test "$1" = "-p" && set "push" "$@"
 
 PARSE_CACHE='--not-parsed'
@@ -672,7 +658,8 @@ pop)
;;
 branch)
shift
-   apply_to_branch "$@"
+   cd "$START_DIR"
+   git stash--helper branch "$@"
;;
 *)
case $# in
-- 
2.16.3