Re: [PATCH v5 4/5] stash: convert branch to builtin
From: Johannes Schindelin <hidden>
Date: 2018-04-06 15:50:43
Hi Joel, On Wed, 4 Apr 2018, Joel Teichroeb wrote:
quoted hunk ↗ jump to hunk
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(&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(&args, "checkout", "-b", NULL);
+ argv_array_push(&args, branch);
+ argv_array_push(&args, oid_to_hex(&info.b_commit));
+ ret = cmd_checkout(args.argc, args.argv, prefix);
+ if (ret) {
+ free_stash_info(&info);
+ return -1;
+ }
+
+ ret = do_apply_stash(prefix, &info, 1);
+ if (!ret && info.is_stash_ref)
+ ret = do_drop_stash(prefix, &info);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, &info, 1); if (!ret && info.is_stash_ref) ret = do_drop_stash(prefix, &info); ... which might be a bit easier to read and to maintain?
quoted hunk ↗ jump to hunk
+ + free_stash_info(&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