# [PATCH 0/4] Convert some stash functionality to a builtin 25 messages from 2018-03-24 to 2018-03-28. Participants: Joel Teichroeb, Christian Couder, Eric Sunshine, Thomas Gummerer, Ævar Arnfjörð Bjarmason, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/48132 ## Joel Teichroeb, 2018-03-24 17:37 Subject: [PATCH 0/4] Convert some stash functionality to a builtin Message-ID: <20180324173707.17699-1-joel@teichroeb.net> URL: https://gitlist.dev/e/20180324173707.17699-1-joel%40teichroeb.net ``` I've been working on converting all of git stash to be a builtin, however it's hard to get it all working at once with limited time, so I've moved around half of it to a new stash--helper builtin and called these functions from the shell script. Once this is stabalized, it should be easier to convert the rest of the commands one at a time without breaking anything. I've sent most of this code before, but that was targetting a full replacement of stash. The code is overall the same, but with some code review changes and updates for internal api changes. Since there seems to be interest from GSOC students who want to work on converting builtins, I figured I should finish what I have that works now so they could build on top of it. Joel Teichroeb (4): stash: convert apply to builtin stash: convert branch to builtin stash: convert drop and clear to builtin stash: convert pop to builtin .gitignore | 1 + Makefile | 1 + builtin.h | 1 + builtin/stash--helper.c | 514 ++++++++++++++++++++++++++++++++++++++++++++++++ git-stash.sh | 13 +- git.c | 1 + 6 files changed, 526 insertions(+), 5 deletions(-) create mode 100644 builtin/stash--helper.c -- 2.16.2 ``` ## Joel Teichroeb, 2018-03-24 17:37 Subject: [PATCH 1/4] stash: convert apply to builtin Message-ID: <20180324173707.17699-2-joel@teichroeb.net> URL: https://gitlist.dev/e/20180324173707.17699-2-joel%40teichroeb.net In-Reply-To: <20180324173707.17699-1-joel@teichroeb.net> ``` --- .gitignore | 1 + Makefile | 1 + builtin.h | 1 + builtin/stash--helper.c | 339 ++++++++++++++++++++++++++++++++++++++++++++++++ git-stash.sh | 3 +- git.c | 1 + 6 files changed, 345 insertions(+), 1 deletion(-) create mode 100644 builtin/stash--helper.c diff --git a/.gitignore b/.gitignore index 833ef3b0b7..296d5f376d 100644 --- a/.gitignore +++ b/.gitignore @@ -152,6 +152,7 @@ /git-show-ref /git-stage /git-stash +/git-stash--helper /git-status /git-stripspace /git-submodule diff --git a/Makefile b/Makefile index a1d8775adb..8ca361c57a 100644 --- a/Makefile +++ b/Makefile @@ -1020,6 +1020,7 @@ BUILTIN_OBJS += builtin/send-pack.o BUILTIN_OBJS += builtin/shortlog.o BUILTIN_OBJS += builtin/show-branch.o BUILTIN_OBJS += builtin/show-ref.o +BUILTIN_OBJS += builtin/stash--helper.o BUILTIN_OBJS += builtin/stripspace.o BUILTIN_OBJS += builtin/submodule--helper.o BUILTIN_OBJS += builtin/symbolic-ref.o diff --git a/builtin.h b/builtin.h index 42378f3aa4..a14fd85b0e 100644 --- a/builtin.h +++ b/builtin.h @@ -219,6 +219,7 @@ extern int cmd_shortlog(int argc, const char **argv, const char *prefix); extern int cmd_show(int argc, const char **argv, const char *prefix); extern int cmd_show_branch(int argc, const char **argv, const char *prefix); extern int cmd_status(int argc, const char **argv, const char *prefix); +extern int cmd_stash__helper(int argc, const char **argv, const char *prefix); extern int cmd_stripspace(int argc, const char **argv, const char *prefix); extern int cmd_submodule__helper(int argc, const char **argv, const char *prefix); extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix); diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c new file mode 100644 index 0000000000..e9a9574f40 --- /dev/null +++ b/builtin/stash--helper.c @@ -0,0 +1,339 @@ +#include "builtin.h" +#include "config.h" +#include "parse-options.h" +#include "refs.h" +#include "lockfile.h" +#include "cache-tree.h" +#include "unpack-trees.h" +#include "merge-recursive.h" +#include "argv-array.h" +#include "run-command.h" +#include "dir.h" + +static const char * const git_stash_helper_usage[] = { + N_("git stash--helper apply [--index] [-q|--quiet] []"), + NULL +}; + +static const char * const git_stash_helper_apply_usage[] = { + N_("git stash--helper apply [--index] [-q|--quiet] []"), + NULL +}; + +static const char *ref_stash = "refs/stash"; +static int quiet; +static char stash_index_path[PATH_MAX]; + +struct stash_info { + struct object_id w_commit; + struct object_id b_commit; + struct object_id i_commit; + struct object_id u_commit; + struct object_id w_tree; + struct object_id b_tree; + struct object_id i_tree; + struct object_id u_tree; + const char *message; + const char *revision; + int is_stash_ref; + int has_u; + const char *patch; +}; + +static int get_stash_info(struct stash_info *info, const char *commit) +{ + struct strbuf w_commit_rev = STRBUF_INIT; + struct strbuf b_commit_rev = STRBUF_INIT; + struct strbuf w_tree_rev = STRBUF_INIT; + struct strbuf b_tree_rev = STRBUF_INIT; + struct strbuf i_tree_rev = STRBUF_INIT; + struct strbuf u_tree_rev = STRBUF_INIT; + struct strbuf commit_buf = STRBUF_INIT; + struct strbuf symbolic = STRBUF_INIT; + struct strbuf out = STRBUF_INIT; + int ret; + const char *revision = commit; + char *end_of_rev; + struct child_process cp = CHILD_PROCESS_INIT; + info->is_stash_ref = 0; + + if (commit == NULL) { + strbuf_addf(&commit_buf, "%s@{0}", ref_stash); + revision = commit_buf.buf; + } else if (strspn(commit, "0123456789") == strlen(commit)) { + strbuf_addf(&commit_buf, "%s@{%s}", ref_stash, commit); + revision = commit_buf.buf; + } + info->revision = revision; + + strbuf_addf(&w_commit_rev, "%s", revision); + strbuf_addf(&b_commit_rev, "%s^1", revision); + strbuf_addf(&w_tree_rev, "%s:", revision); + strbuf_addf(&b_tree_rev, "%s^1:", revision); + strbuf_addf(&i_tree_rev, "%s^2:", revision); + + ret = !get_oid(w_commit_rev.buf, &info->w_commit) && + !get_oid(b_commit_rev.buf, &info->b_commit) && + !get_oid(w_tree_rev.buf, &info->w_tree) && + !get_oid(b_tree_rev.buf, &info->b_tree) && + !get_oid(i_tree_rev.buf, &info->i_tree); + + strbuf_release(&w_commit_rev); + strbuf_release(&b_commit_rev); + strbuf_release(&w_tree_rev); + strbuf_release(&b_tree_rev); + strbuf_release(&i_tree_rev); + + if (!ret) + return error(_("%s is not a valid reference"), revision); + + strbuf_addf(&u_tree_rev, "%s^3:", revision); + + info->has_u = !get_oid(u_tree_rev.buf, &info->u_tree); + + strbuf_release(&u_tree_rev); + + end_of_rev = strchrnul(revision, '@'); + strbuf_add(&symbolic, revision, end_of_rev - revision); + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "rev-parse", "--symbolic-full-name", NULL); + argv_array_pushf(&cp.args, "%s", symbolic.buf); + strbuf_release(&symbolic); + pipe_command(&cp, NULL, 0, &out, 0, NULL, 0); + + if (out.len - 1 == strlen(ref_stash)) + info->is_stash_ref = !strncmp(out.buf, ref_stash, out.len - 1); + strbuf_release(&out); + + return 0; +} + +static int reset_tree(struct object_id i_tree, int update, int reset) +{ + struct unpack_trees_options opts; + int nr_trees = 1; + struct tree_desc t[MAX_UNPACK_TREES]; + struct tree *tree; + struct lock_file lock_file = LOCK_INIT; + + read_cache_preload(NULL); + if (refresh_cache(REFRESH_QUIET)) + return -1; + + hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR); + + memset(&opts, 0, sizeof(opts)); + + tree = parse_tree_indirect(&i_tree); + if (parse_tree(tree)) + return -1; + + init_tree_desc(t, tree->buffer, tree->size); + + opts.head_idx = 1; + opts.src_index = &the_index; + opts.dst_index = &the_index; + opts.merge = 1; + opts.reset = reset; + opts.update = update; + opts.fn = oneway_merge; + + if (unpack_trees(nr_trees, t, &opts)) + return -1; + + if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) { + error(_("unable to write new index file")); + return -1; + } + + return 0; +} + +static int do_apply_stash(const char *prefix, struct stash_info *info, int index) +{ + struct merge_options o; + struct object_id c_tree; + struct object_id index_tree; + const struct object_id *bases[1]; + int bases_count = 1; + struct commit *result; + int ret; + int has_index = index; + + read_cache_preload(NULL); + if (refresh_cache(REFRESH_QUIET)) + return -1; + + if (write_cache_as_tree(c_tree.hash, 0, NULL) || reset_tree(c_tree, 0, 0)) + return error(_("Cannot apply a stash in the middle of a merge")); + + if (index) { + if (!oidcmp(&info->b_tree, &info->i_tree) || !oidcmp(&c_tree, &info->i_tree)) { + has_index = 0; + } else { + struct child_process cp = CHILD_PROCESS_INIT; + struct strbuf out = STRBUF_INIT; + struct argv_array args = ARGV_ARRAY_INIT; + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "diff-tree", "--binary", NULL); + argv_array_pushf(&cp.args, "%s^2^..%s^2", sha1_to_hex(info->w_commit.hash), sha1_to_hex(info->w_commit.hash)); + if (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0)) + return -1; + + child_process_init(&cp); + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "apply", "--cached", NULL); + if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0)) + return -1; + + strbuf_release(&out); + discard_cache(); + read_cache(); + if (write_cache_as_tree(index_tree.hash, 0, NULL)) + return -1; + + argv_array_push(&args, "reset"); + cmd_reset(args.argc, args.argv, prefix); + } + } + + if (info->has_u) { + struct child_process cp = CHILD_PROCESS_INIT; + struct child_process cp2 = CHILD_PROCESS_INIT; + int res; + + cp.git_cmd = 1; + argv_array_push(&cp.args, "read-tree"); + argv_array_push(&cp.args, sha1_to_hex(info->u_tree.hash)); + argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", stash_index_path); + + cp2.git_cmd = 1; + argv_array_pushl(&cp2.args, "checkout-index", "--all", NULL); + argv_array_pushf(&cp2.env_array, "GIT_INDEX_FILE=%s", stash_index_path); + + res = run_command(&cp) || run_command(&cp2); + remove_path(stash_index_path); + if (res) + return error(_("Could not restore untracked files from stash")); + } + + init_merge_options(&o); + + o.branch1 = "Updated upstream"; + o.branch2 = "Stashed changes"; + + if (!hashcmp(info->b_tree.hash, c_tree.hash)) + o.branch1 = "Version stash was based on"; + + if (quiet) + o.verbosity = 0; + + if (o.verbosity >= 3) + printf_ln(_("Merging %s with %s"), o.branch1, o.branch2); + + bases[0] = &info->b_tree; + + ret = merge_recursive_generic(&o, &c_tree, &info->w_tree, bases_count, bases, &result); + if (ret != 0) { + struct argv_array args = ARGV_ARRAY_INIT; + argv_array_push(&args, "rerere"); + cmd_rerere(args.argc, args.argv, prefix); + + if (index) + printf_ln(_("Index was not unstashed.")); + + return ret; + } + + if (has_index) { + if (reset_tree(index_tree, 0, 0)) + return -1; + } else { + struct child_process cp = CHILD_PROCESS_INIT; + struct strbuf out = STRBUF_INIT; + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "diff-index", "--cached", "--name-only", "--diff-filter=A", NULL); + argv_array_push(&cp.args, sha1_to_hex(c_tree.hash)); + ret = pipe_command(&cp, NULL, 0, &out, 0, NULL, 0); + if (ret) + return -1; + + if (reset_tree(c_tree, 0, 1)) + return -1; + + child_process_init(&cp); + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "update-index", "--add", "--stdin", NULL); + ret = pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0); + if (ret) + return -1; + + strbuf_release(&out); + discard_cache(); + } + + if (!quiet) { + struct argv_array args = ARGV_ARRAY_INIT; + argv_array_push(&args, "status"); + cmd_status(args.argc, args.argv, prefix); + } + + return 0; +} + +static int apply_stash(int argc, const char **argv, const char *prefix) +{ + const char *commit = NULL; + int index = 0; + 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_apply_usage, 0); + + if (argc == 1) { + commit = argv[0]; + } + + if (get_stash_info(&info, commit)) + return -1; + + + return do_apply_stash(prefix, &info, index); +} + +int cmd_stash__helper(int argc, const char **argv, const char *prefix) +{ + int result = 0; + pid_t pid = getpid(); + const char *index_file; + + struct option options[] = { + OPT_END() + }; + + git_config(git_default_config, NULL); + + argc = parse_options(argc, argv, prefix, options, git_stash_helper_usage, + PARSE_OPT_KEEP_UNKNOWN|PARSE_OPT_KEEP_DASHDASH); + + index_file = get_index_file(); + xsnprintf(stash_index_path, PATH_MAX, "%s.stash.%d", index_file, pid); + + if (argc < 1) + usage_with_options(git_stash_helper_usage, options); + else if (!strcmp(argv[0], "apply")) + result = apply_stash(argc, argv, prefix); + else { + error(_("unknown subcommand: %s"), argv[0]); + usage_with_options(git_stash_helper_usage, options); + result = 1; + } + + return result; +} diff --git a/git-stash.sh b/git-stash.sh index fc8f8ae640..92c084eb17 100755 --- a/git-stash.sh +++ b/git-stash.sh @@ -711,7 +711,8 @@ push) ;; apply) shift - apply_stash "$@" + cd "$START_DIR" + git stash--helper apply "$@" ;; clear) shift diff --git a/git.c b/git.c index ceaa58ef40..6ffe6364ac 100644 --- a/git.c +++ b/git.c @@ -466,6 +466,7 @@ static struct cmd_struct commands[] = { { "show-branch", cmd_show_branch, RUN_SETUP }, { "show-ref", cmd_show_ref, RUN_SETUP }, { "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE }, + { "stash--helper", cmd_stash__helper, RUN_SETUP | NEED_WORK_TREE }, { "status", cmd_status, RUN_SETUP | NEED_WORK_TREE }, { "stripspace", cmd_stripspace }, { "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX}, -- 2.16.2 ``` ## Joel Teichroeb, 2018-03-24 17:37 Subject: [PATCH 2/4] stash: convert branch to builtin Message-ID: <20180324173707.17699-3-joel@teichroeb.net> URL: https://gitlist.dev/e/20180324173707.17699-3-joel%40teichroeb.net In-Reply-To: <20180324173707.17699-1-joel@teichroeb.net> ``` --- 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, &info, 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(&info, commit)) + return -1; + + argv_array_pushl(&args, "checkout", "-b", NULL); + argv_array_push(&args, branch); + argv_array_push(&args, sha1_to_hex(info.b_commit.hash)); + ret = cmd_checkout(args.argc, args.argv, prefix); + if (ret) + return -1; + + ret = do_apply_stash(prefix, &info, 1); + if (!ret && info.is_stash_ref) + ret = do_drop_stash(prefix, &info); + + 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 ``` ## Joel Teichroeb, 2018-03-24 17:37 Subject: [PATCH 3/4] stash: convert drop and clear to builtin Message-ID: <20180324173707.17699-4-joel@teichroeb.net> URL: https://gitlist.dev/e/20180324173707.17699-4-joel%40teichroeb.net In-Reply-To: <20180324173707.17699-1-joel@teichroeb.net> ``` --- builtin/stash--helper.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++ git-stash.sh | 4 +-- 2 files changed, 95 insertions(+), 2 deletions(-) diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c index 18c4aba665..1598b82ac2 100644 --- a/builtin/stash--helper.c +++ b/builtin/stash--helper.c @@ -11,8 +11,15 @@ #include "dir.h" 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 +}; + +static const char * const git_stash_helper_drop_usage[] = { + N_("git stash--helper drop [-q|--quiet] []"), NULL }; @@ -26,6 +33,11 @@ static const char * const git_stash_helper_branch_usage[] = { NULL }; +static const char * const git_stash_helper_clear_usage[] = { + N_("git stash--helper clear"), + NULL +}; + static const char *ref_stash = "refs/stash"; static int quiet; static char stash_index_path[PATH_MAX]; @@ -114,6 +126,29 @@ static int get_stash_info(struct stash_info *info, const char *commit) return 0; } +static int do_clear_stash(void) +{ + struct object_id obj; + if (get_oid(ref_stash, &obj)) + return 0; + + return delete_ref(NULL, ref_stash, &obj, 0); +} + +static int clear_stash(int argc, const char **argv, const char *prefix) +{ + struct option options[] = { + OPT_END() + }; + + argc = parse_options(argc, argv, prefix, options, git_stash_helper_clear_usage, PARSE_OPT_STOP_AT_NON_OPTION); + + if (argc != 0) + return error(_("git stash--helper clear with parameters is unimplemented")); + + return do_clear_stash(); +} + static int reset_tree(struct object_id i_tree, int update, int reset) { struct unpack_trees_options opts; @@ -313,6 +348,60 @@ static int apply_stash(int argc, const char **argv, const char *prefix) return do_apply_stash(prefix, &info, index); } +static int do_drop_stash(const char *prefix, struct stash_info *info) +{ + struct argv_array args = ARGV_ARRAY_INIT; + int ret; + struct child_process cp = CHILD_PROCESS_INIT; + + argv_array_pushl(&args, "reflog", "delete", "--updateref", "--rewrite", NULL); + argv_array_push(&args, info->revision); + ret = cmd_reflog(args.argc, args.argv, prefix); + if (!ret) { + if (!quiet) { + printf(_("Dropped %s (%s)\n"), info->revision, sha1_to_hex(info->w_commit.hash)); + } + } else { + return error(_("%s: Could not drop stash entry"), info->revision); + } + + cp.git_cmd = 1; + /* Even though --quiet is specified, rev-parse still outputs the hash */ + cp.no_stdout = 1; + argv_array_pushl(&cp.args, "rev-parse", "--verify", "--quiet", NULL); + argv_array_pushf(&cp.args, "%s@{0}", ref_stash); + ret = run_command(&cp); + + if (ret) + do_clear_stash(); + + return 0; +} + +static int drop_stash(int argc, const char **argv, const char *prefix) +{ + const char *commit = NULL; + struct stash_info info; + struct option options[] = { + OPT__QUIET(&quiet, N_("be quiet, only report errors")), + OPT_END() + }; + + argc = parse_options(argc, argv, prefix, options, + git_stash_helper_drop_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); + + return do_drop_stash(prefix, &info); +} + static int branch_stash(int argc, const char **argv, const char *prefix) { const char *commit = NULL, *branch = NULL; @@ -371,6 +460,10 @@ 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], "clear")) + 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 { diff --git a/git-stash.sh b/git-stash.sh index 360643ad4e..54d0a6c21f 100755 --- a/git-stash.sh +++ b/git-stash.sh @@ -716,7 +716,7 @@ apply) ;; clear) shift - clear_stash "$@" + git stash--helper clear "$@" ;; create) shift @@ -728,7 +728,7 @@ store) ;; drop) shift - drop_stash "$@" + git stash--helper drop "$@" ;; pop) shift -- 2.16.2 ``` ## Joel Teichroeb, 2018-03-24 17:37 Subject: [PATCH 4/4] stash: convert pop to builtin Message-ID: <20180324173707.17699-5-joel@teichroeb.net> URL: https://gitlist.dev/e/20180324173707.17699-5-joel%40teichroeb.net In-Reply-To: <20180324173707.17699-1-joel@teichroeb.net> ``` --- 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 ``` ## Christian Couder, 2018-03-24 18:19 Subject: Re: [PATCH 1/4] stash: convert apply to builtin Message-ID: URL: https://gitlist.dev/e/CAP8UFD37DgfQ63pPrN2CBH0VjTUuc-N4fq-34bTuyQTH0fR9mA%40mail.gmail.com In-Reply-To: <20180324173707.17699-2-joel@teichroeb.net> ``` > + if (unpack_trees(nr_trees, t, &opts)) > + return -1; > + > + if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) { > + error(_("unable to write new index file")); > + return -1; Maybe: return error(...); > + } > + > + return 0; > +} [...] > + argc = parse_options(argc, argv, prefix, options, > + git_stash_helper_apply_usage, 0); > + > + if (argc == 1) { > + commit = argv[0]; > + } The brackets are not needed. > + if (get_stash_info(&info, commit)) > + return -1; > + > + Spurious new line. > + return do_apply_stash(prefix, &info, index); > +} ``` ## Christian Couder, 2018-03-24 18:22 Subject: Re: [PATCH 3/4] stash: convert drop and clear to builtin Message-ID: URL: https://gitlist.dev/e/CAP8UFD2yudVPNya8sTTd5UUq7Doxp2VqSf%2BaCecJPHE_c1VoqA%40mail.gmail.com In-Reply-To: <20180324173707.17699-4-joel@teichroeb.net> ``` > + argv_array_pushl(&args, "reflog", "delete", "--updateref", "--rewrite", NULL); > + argv_array_push(&args, info->revision); > + ret = cmd_reflog(args.argc, args.argv, prefix); > + if (!ret) { > + if (!quiet) { > + printf(_("Dropped %s (%s)\n"), info->revision, sha1_to_hex(info->w_commit.hash)); > + } The brackets are not needed. > + } else { > + return error(_("%s: Could not drop stash entry"), info->revision); > + } ``` ## Eric Sunshine, 2018-03-25 06:40 Subject: Re: [PATCH 1/4] stash: convert apply to builtin Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSkQLSvOroB0bLLLBAXy9UBDN%2Bs%3Di97COtNDpO0FbLJkg%40mail.gmail.com In-Reply-To: <20180324173707.17699-2-joel@teichroeb.net> ``` On Sat, Mar 24, 2018 at 1:37 PM, Joel Teichroeb wrote: > diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c > @@ -0,0 +1,339 @@ > +static int get_stash_info(struct stash_info *info, const char *commit) > +{ > + struct strbuf w_commit_rev = STRBUF_INIT; > + struct strbuf b_commit_rev = STRBUF_INIT; > + struct strbuf w_tree_rev = STRBUF_INIT; > + struct strbuf b_tree_rev = STRBUF_INIT; > + struct strbuf i_tree_rev = STRBUF_INIT; > + struct strbuf u_tree_rev = STRBUF_INIT; > + struct strbuf commit_buf = STRBUF_INIT; > + struct strbuf symbolic = STRBUF_INIT; > + struct strbuf out = STRBUF_INIT; 'commit_buf' is being leaked. All the others seem to be covered (even in the case of early 'return'). > + if (commit == NULL) { > + strbuf_addf(&commit_buf, "%s@{0}", ref_stash); > + revision = commit_buf.buf; > + } else if (strspn(commit, "0123456789") == strlen(commit)) { > + strbuf_addf(&commit_buf, "%s@{%s}", ref_stash, commit); > + revision = commit_buf.buf; > + } > +static int do_apply_stash(const char *prefix, struct stash_info *info, int index) > +{ > + if (index) { > + if (!oidcmp(&info->b_tree, &info->i_tree) || !oidcmp(&c_tree, &info->i_tree)) { > + has_index = 0; > + } else { > + struct child_process cp = CHILD_PROCESS_INIT; > + struct strbuf out = STRBUF_INIT; > + struct argv_array args = ARGV_ARRAY_INIT; > + cp.git_cmd = 1; > + argv_array_pushl(&cp.args, "diff-tree", "--binary", NULL); > + argv_array_pushf(&cp.args, "%s^2^..%s^2", sha1_to_hex(info->w_commit.hash), sha1_to_hex(info->w_commit.hash)); > + if (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0)) > + return -1; Leaking 'out'? > + > + child_process_init(&cp); > + cp.git_cmd = 1; > + argv_array_pushl(&cp.args, "apply", "--cached", NULL); > + if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0)) > + return -1; Leaking 'out'. > + > + strbuf_release(&out); > + discard_cache(); > + read_cache(); > + if (write_cache_as_tree(index_tree.hash, 0, NULL)) > + return -1; > + > + argv_array_push(&args, "reset"); > + cmd_reset(args.argc, args.argv, prefix); > + } > + } > + if (has_index) { > + if (reset_tree(index_tree, 0, 0)) > + return -1; > + } else { > + struct child_process cp = CHILD_PROCESS_INIT; > + struct strbuf out = STRBUF_INIT; > + cp.git_cmd = 1; > + argv_array_pushl(&cp.args, "diff-index", "--cached", "--name-only", "--diff-filter=A", NULL); > + argv_array_push(&cp.args, sha1_to_hex(c_tree.hash)); > + ret = pipe_command(&cp, NULL, 0, &out, 0, NULL, 0); > + if (ret) > + return -1; > + > + if (reset_tree(c_tree, 0, 1)) > + return -1; Leaking 'out' at these two 'return's? > + child_process_init(&cp); > + cp.git_cmd = 1; > + argv_array_pushl(&cp.args, "update-index", "--add", "--stdin", NULL); > + ret = pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0); > + if (ret) > + return -1; And here. > + > + strbuf_release(&out); > + discard_cache(); > + } > + > + if (!quiet) { > + struct argv_array args = ARGV_ARRAY_INIT; > + argv_array_push(&args, "status"); > + cmd_status(args.argc, args.argv, prefix); > + } > + > + return 0; > +} > + > +static int apply_stash(int argc, const char **argv, const char *prefix) > +{ > + const char *commit = NULL; > + int index = 0; > + 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_apply_usage, 0); > + > + if (argc == 1) { > + commit = argv[0]; > + } > + > + if (get_stash_info(&info, commit)) > + return -1; > + > + > + return do_apply_stash(prefix, &info, index); > +} ``` ## Eric Sunshine, 2018-03-25 06:44 Subject: Re: [PATCH 2/4] stash: convert branch to builtin Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRyoKZN9osXXXuqTVXn27twLy--BXsHE1jLqKqXJ6DwAA%40mail.gmail.com In-Reply-To: <20180324173707.17699-3-joel@teichroeb.net> ``` 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(&info, commit)) > + return -1; > + > + argv_array_pushl(&args, "checkout", "-b", NULL); > + argv_array_push(&args, branch); > + argv_array_push(&args, sha1_to_hex(info.b_commit.hash)); > + ret = cmd_checkout(args.argc, args.argv, prefix); > + if (ret) > + return -1; > + > + ret = do_apply_stash(prefix, &info, 1); > + if (!ret && info.is_stash_ref) > + ret = do_drop_stash(prefix, &info); > + > + return ret; > +} ``` ## Eric Sunshine, 2018-03-25 06:49 Subject: Re: [PATCH 3/4] stash: convert drop and clear to builtin Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSC93bEoUZBtg3b%2BU_%3D3O%2BD2T1-0x-mH2LykyMsM2SROg%40mail.gmail.com In-Reply-To: <20180324173707.17699-4-joel@teichroeb.net> ``` On Sat, Mar 24, 2018 at 1:37 PM, Joel Teichroeb wrote: > diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c > @@ -313,6 +348,60 @@ static int apply_stash(int argc, const char **argv, const char *prefix) > +static int drop_stash(int argc, const char **argv, const char *prefix) > +{ > + const char *commit = NULL; > + struct stash_info info; > + struct option options[] = { > + OPT__QUIET(&quiet, N_("be quiet, only report errors")), > + OPT_END() > + }; > + > + argc = parse_options(argc, argv, prefix, options, > + git_stash_helper_drop_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); > + > + return do_drop_stash(prefix, &info); > +} ``` ## Eric Sunshine, 2018-03-25 06:51 Subject: Re: [PATCH 4/4] stash: convert pop to builtin Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQ7XdEMyL%3DmT4RGfnsz-fjs6RxG%3DztGC3oLprxKUBnJoA%40mail.gmail.com In-Reply-To: <20180324173707.17699-5-joel@teichroeb.net> ``` 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); > +} ``` ## Christian Couder, 2018-03-25 08:09 Subject: Re: [PATCH 1/4] stash: convert apply to builtin Message-ID: URL: https://gitlist.dev/e/CAP8UFD3Qxt2YMqTtHwU8n7EDvD66QjGSywRQoxJDnncv7%3D2BUg%40mail.gmail.com In-Reply-To: <20180324173707.17699-2-joel@teichroeb.net> ``` On Sat, Mar 24, 2018 at 6:37 PM, Joel Teichroeb wrote: > diff --git a/git-stash.sh b/git-stash.sh > index fc8f8ae640..92c084eb17 100755 > --- a/git-stash.sh > +++ b/git-stash.sh > @@ -711,7 +711,8 @@ push) > ;; > apply) > shift > - apply_stash "$@" > + cd "$START_DIR" > + git stash--helper apply "$@" > ;; > clear) > shift It seems to me that the apply_stash() shell function is also used in pop_stash() and in apply_to_branch(). Can the new helper be used there too instead of apply_stash()? And then could apply_stash() be remove? ``` ## Christian Couder, 2018-03-25 08:22 Subject: Re: [PATCH 2/4] stash: convert branch to builtin Message-ID: URL: https://gitlist.dev/e/CAP8UFD0Rmyiom0pEY6u7OVzCZbs9rg3VjEsW6Rs8S6m40uKrPw%40mail.gmail.com In-Reply-To: <20180324173707.17699-3-joel@teichroeb.net> ``` 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? ``` ## Christian Couder, 2018-03-25 09:27 Subject: Re: [PATCH 1/4] stash: convert apply to builtin Message-ID: URL: https://gitlist.dev/e/CAP8UFD1f%3DVM7VLWGPOzn5PSwHr_m2PohP0gOCM6wk7FdoZ3-Yg%40mail.gmail.com In-Reply-To: ``` On Sun, Mar 25, 2018 at 8:40 AM, Eric Sunshine wrote: > On Sat, Mar 24, 2018 at 1:37 PM, Joel Teichroeb wrote: >> +static int do_apply_stash(const char *prefix, struct stash_info *info, int index) >> +{ >> + if (index) { >> + if (!oidcmp(&info->b_tree, &info->i_tree) || !oidcmp(&c_tree, &info->i_tree)) { >> + has_index = 0; >> + } else { >> + struct child_process cp = CHILD_PROCESS_INIT; >> + struct strbuf out = STRBUF_INIT; >> + struct argv_array args = ARGV_ARRAY_INIT; >> + cp.git_cmd = 1; >> + argv_array_pushl(&cp.args, "diff-tree", "--binary", NULL); >> + argv_array_pushf(&cp.args, "%s^2^..%s^2", sha1_to_hex(info->w_commit.hash), sha1_to_hex(info->w_commit.hash)); >> + if (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0)) >> + return -1; > > Leaking 'out'? > >> + >> + child_process_init(&cp); >> + cp.git_cmd = 1; >> + argv_array_pushl(&cp.args, "apply", "--cached", NULL); >> + if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0)) >> + return -1; > > Leaking 'out'. It might be a good idea to have small functions encapsulating the forks of git commands. For example: static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit) { struct child_process cp = CHILD_PROCESS_INIT; const char *w_commit_hex = oid_to_hex(w_commit); cp.git_cmd = 1; argv_array_pushl(&cp.args, "diff-tree", "--binary", NULL); argv_array_pushf(&cp.args, "%s^2^..%s^2", w_commit_hex, w_commit_hex); return pipe_command(&cp, NULL, 0, out, 0, NULL, 0); } static int apply_cached(struct strbuf *out) { struct child_process cp = CHILD_PROCESS_INIT; cp.git_cmd = 1; argv_array_pushl(&cp.args, "apply", "--cached", NULL); return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0); } This could help find leaks and maybe later make it easier to call libified code instead of forking a git command. ``` ## Thomas Gummerer, 2018-03-25 16:43 Subject: Re: [PATCH 1/4] stash: convert apply to builtin Message-ID: <20180325164300.GA10909@hank> URL: https://gitlist.dev/e/20180325164300.GA10909%40hank In-Reply-To: <20180324173707.17699-2-joel@teichroeb.net> ``` On 03/24, Joel Teichroeb wrote: > --- Missing sign-off? I saw it's missing in the other patches as well. > [...] > +static int do_apply_stash(const char *prefix, struct stash_info *info, int index) > +{ > + struct merge_options o; > + struct object_id c_tree; > + struct object_id index_tree; > + const struct object_id *bases[1]; > + int bases_count = 1; > + struct commit *result; > + int ret; > + int has_index = index; > + > + read_cache_preload(NULL); > + if (refresh_cache(REFRESH_QUIET)) > + return -1; > + > + if (write_cache_as_tree(c_tree.hash, 0, NULL) || reset_tree(c_tree, 0, 0)) > + return error(_("Cannot apply a stash in the middle of a merge")); > + > + if (index) { > + if (!oidcmp(&info->b_tree, &info->i_tree) || !oidcmp(&c_tree, &info->i_tree)) { > + has_index = 0; > + } else { > + struct child_process cp = CHILD_PROCESS_INIT; > + struct strbuf out = STRBUF_INIT; > + struct argv_array args = ARGV_ARRAY_INIT; > + cp.git_cmd = 1; > + argv_array_pushl(&cp.args, "diff-tree", "--binary", NULL); > + argv_array_pushf(&cp.args, "%s^2^..%s^2", sha1_to_hex(info->w_commit.hash), sha1_to_hex(info->w_commit.hash)); > + if (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0)) > + return -1; > + > + child_process_init(&cp); > + cp.git_cmd = 1; > + argv_array_pushl(&cp.args, "apply", "--cached", NULL); > + if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0)) > + return -1; > + > + strbuf_release(&out); > + discard_cache(); > + read_cache(); > + if (write_cache_as_tree(index_tree.hash, 0, NULL)) > + return -1; > + > + argv_array_push(&args, "reset"); > + cmd_reset(args.argc, args.argv, prefix); > + } > + } > + > + if (info->has_u) { > + struct child_process cp = CHILD_PROCESS_INIT; > + struct child_process cp2 = CHILD_PROCESS_INIT; > + int res; > + > + cp.git_cmd = 1; > + argv_array_push(&cp.args, "read-tree"); > + argv_array_push(&cp.args, sha1_to_hex(info->u_tree.hash)); > + argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", stash_index_path); > + > + cp2.git_cmd = 1; > + argv_array_pushl(&cp2.args, "checkout-index", "--all", NULL); > + argv_array_pushf(&cp2.env_array, "GIT_INDEX_FILE=%s", stash_index_path); > + > + res = run_command(&cp) || run_command(&cp2); > + remove_path(stash_index_path); > + if (res) > + return error(_("Could not restore untracked files from stash")); A minor change in behaviour here is that we are removing the temporary index file unconditionally here, while we would previously only remove it if both 'read-tree' and 'checkout-index' would succeed. I don't think that's a bad thing, we probably don't want users to try and use that index file in any way, and I doubt that's part of anyones workflow, so I think cleaning it up makes sense. > + } > + > + init_merge_options(&o); > + > + o.branch1 = "Updated upstream"; > + o.branch2 = "Stashed changes"; > + > + if (!hashcmp(info->b_tree.hash, c_tree.hash)) > + o.branch1 = "Version stash was based on"; > + > + if (quiet) > + o.verbosity = 0; > + > + if (o.verbosity >= 3) > + printf_ln(_("Merging %s with %s"), o.branch1, o.branch2); > + > + bases[0] = &info->b_tree; > + > + ret = merge_recursive_generic(&o, &c_tree, &info->w_tree, bases_count, bases, &result); > + if (ret != 0) { > + struct argv_array args = ARGV_ARRAY_INIT; > + argv_array_push(&args, "rerere"); > + cmd_rerere(args.argc, args.argv, prefix); > + if (index) > + printf_ln(_("Index was not unstashed.")); Minor nit: I think the above should be 'fprintf_ln(stderr, ...)' to match what we currently have. > + > + return ret; > + } > + > [...] ``` ## Joel Teichroeb, 2018-03-25 16:51 Subject: Re: [PATCH 1/4] stash: convert apply to builtin Message-ID: URL: https://gitlist.dev/e/CA%2BCzEk9QpmHK_TSBwQfEedNqrcVSBp3xY7bdv1YA_KxePiFeXw%40mail.gmail.com In-Reply-To: ``` On Sun, Mar 25, 2018 at 1:09 AM, Christian Couder wrote: > It seems to me that the apply_stash() shell function is also used in > pop_stash() and in apply_to_branch(). Can the new helper be used there > too instead of apply_stash()? And then could apply_stash() be remove? I wasn't really sure if I should remove code from the .sh file as it seems in the past the old .sh files have been kept around as examples. Has that been done for previous conversions? ``` ## Thomas Gummerer, 2018-03-25 17:02 Subject: Re: [PATCH 2/4] stash: convert branch to builtin Message-ID: <20180325170236.GB10909@hank> URL: https://gitlist.dev/e/20180325170236.GB10909%40hank In-Reply-To: <20180324173707.17699-3-joel@teichroeb.net> ``` 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, &info, 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(&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(&args, "checkout", "-b", NULL); > + argv_array_push(&args, branch); > + argv_array_push(&args, sha1_to_hex(info.b_commit.hash)); > + ret = cmd_checkout(args.argc, args.argv, prefix); > + if (ret) > + return -1; > + > + ret = do_apply_stash(prefix, &info, 1); > + if (!ret && info.is_stash_ref) > + ret = do_drop_stash(prefix, &info); '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 > ``` ## Thomas Gummerer, 2018-03-25 17:23 Subject: Re: [PATCH 1/4] stash: convert apply to builtin Message-ID: <20180325172309.GC10909@hank> URL: https://gitlist.dev/e/20180325172309.GC10909%40hank In-Reply-To: <20180324173707.17699-2-joel@teichroeb.net> ``` On 03/24, Joel Teichroeb wrote: > --- > [...] > + > +static const char *ref_stash = "refs/stash"; > +static int quiet; > +static char stash_index_path[PATH_MAX]; > + > +struct stash_info { > + struct object_id w_commit; > + struct object_id b_commit; > + struct object_id i_commit; > + struct object_id u_commit; > + struct object_id w_tree; > + struct object_id b_tree; > + struct object_id i_tree; > + struct object_id u_tree; > + const char *message; > + const char *revision; > + int is_stash_ref; > + int has_u; > + const char *patch; > +}; > + > +static int get_stash_info(struct stash_info *info, const char *commit) > +{ > + struct strbuf w_commit_rev = STRBUF_INIT; > + struct strbuf b_commit_rev = STRBUF_INIT; > + struct strbuf w_tree_rev = STRBUF_INIT; > + struct strbuf b_tree_rev = STRBUF_INIT; > + struct strbuf i_tree_rev = STRBUF_INIT; > + struct strbuf u_tree_rev = STRBUF_INIT; > + struct strbuf commit_buf = STRBUF_INIT; > + struct strbuf symbolic = STRBUF_INIT; > + struct strbuf out = STRBUF_INIT; > + int ret; > + const char *revision = commit; > + char *end_of_rev; > + struct child_process cp = CHILD_PROCESS_INIT; > + info->is_stash_ref = 0; > + > + if (commit == NULL) { > + strbuf_addf(&commit_buf, "%s@{0}", ref_stash); > + revision = commit_buf.buf; Before setting up the revisions here, as is done below, we used to check if a stash even exists, if no commit was given. So in a repository with no stashes we would die with "No stash entries found", while now we die with "error: refs/stash@{0} is not a valid reference". I think the error message we had previously was slightly nicer, and we should try to keep it. > + } else if (strspn(commit, "0123456789") == strlen(commit)) { > + strbuf_addf(&commit_buf, "%s@{%s}", ref_stash, commit); > + revision = commit_buf.buf; > + } > + info->revision = revision; > + > + strbuf_addf(&w_commit_rev, "%s", revision); > + strbuf_addf(&b_commit_rev, "%s^1", revision); > + strbuf_addf(&w_tree_rev, "%s:", revision); > + strbuf_addf(&b_tree_rev, "%s^1:", revision); > + strbuf_addf(&i_tree_rev, "%s^2:", revision); > + > + ret = !get_oid(w_commit_rev.buf, &info->w_commit) && > + !get_oid(b_commit_rev.buf, &info->b_commit) && > + !get_oid(w_tree_rev.buf, &info->w_tree) && > + !get_oid(b_tree_rev.buf, &info->b_tree) && > + !get_oid(i_tree_rev.buf, &info->i_tree); > + > + strbuf_release(&w_commit_rev); > + strbuf_release(&b_commit_rev); > + strbuf_release(&w_tree_rev); > + strbuf_release(&b_tree_rev); > + strbuf_release(&i_tree_rev); > + > + if (!ret) > + return error(_("%s is not a valid reference"), revision); We used to distinguish between "not a valid reference" and "not a stash-like commit" here. I think just doing the first 'get_oid' before the others, and returning the error if that fails, and then doing the rest and returning the "not a stash-like commit" if one of the other 'get_oid' calls fails would work, although I did not test it. > + > + strbuf_addf(&u_tree_rev, "%s^3:", revision); > + > + info->has_u = !get_oid(u_tree_rev.buf, &info->u_tree); > + > + strbuf_release(&u_tree_rev); > + > + end_of_rev = strchrnul(revision, '@'); > + strbuf_add(&symbolic, revision, end_of_rev - revision); > + cp.git_cmd = 1; > + argv_array_pushl(&cp.args, "rev-parse", "--symbolic-full-name", NULL); > + argv_array_pushf(&cp.args, "%s", symbolic.buf); > + strbuf_release(&symbolic); > + pipe_command(&cp, NULL, 0, &out, 0, NULL, 0); > + > + if (out.len - 1 == strlen(ref_stash)) > + info->is_stash_ref = !strncmp(out.buf, ref_stash, out.len - 1); > + strbuf_release(&out); > + > + return 0; > +} > + > [...] ``` ## Thomas Gummerer, 2018-03-25 17:36 Subject: Re: [PATCH 4/4] stash: convert pop to builtin Message-ID: <20180325173635.GD10909@hank> URL: https://gitlist.dev/e/20180325173635.GD10909%40hank In-Reply-To: <20180324173707.17699-5-joel@teichroeb.net> ``` 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 > ``` ## Thomas Gummerer, 2018-03-25 17:39 Subject: Re: [PATCH 0/4] Convert some stash functionality to a builtin Message-ID: <20180325173916.GE10909@hank> URL: https://gitlist.dev/e/20180325173916.GE10909%40hank In-Reply-To: <20180324173707.17699-1-joel@teichroeb.net> ``` On 03/24, Joel Teichroeb wrote: > I've been working on converting all of git stash to be a > builtin, however it's hard to get it all working at once with > limited time, so I've moved around half of it to a new > stash--helper builtin and called these functions from the shell > script. Once this is stabalized, it should be easier to convert > the rest of the commands one at a time without breaking > anything. > > I've sent most of this code before, but that was targetting a > full replacement of stash. The code is overall the same, but > with some code review changes and updates for internal api > changes. Thanks for splitting this up into multiple patches, I found that much more pleasant to review, and thanks for your continued work on this :) > Since there seems to be interest from GSOC students who want to > work on converting builtins, I figured I should finish what I > have that works now so they could build on top of it. > > Joel Teichroeb (4): > stash: convert apply to builtin > stash: convert branch to builtin > stash: convert drop and clear to builtin > stash: convert pop to builtin > > .gitignore | 1 + > Makefile | 1 + > builtin.h | 1 + > builtin/stash--helper.c | 514 ++++++++++++++++++++++++++++++++++++++++++++++++ > git-stash.sh | 13 +- > git.c | 1 + > 6 files changed, 526 insertions(+), 5 deletions(-) > create mode 100644 builtin/stash--helper.c > > -- > 2.16.2 > ``` ## Christian Couder, 2018-03-25 19:58 Subject: Re: [PATCH 1/4] stash: convert apply to builtin Message-ID: URL: https://gitlist.dev/e/CAP8UFD04LF68LONP3hxpjc4oQokSab4HYvTkazeBq8STzN6E-A%40mail.gmail.com In-Reply-To: ``` On Sun, Mar 25, 2018 at 6:51 PM, Joel Teichroeb wrote: > On Sun, Mar 25, 2018 at 1:09 AM, Christian Couder > wrote: >> It seems to me that the apply_stash() shell function is also used in >> pop_stash() and in apply_to_branch(). Can the new helper be used there >> too instead of apply_stash()? And then could apply_stash() be remove? > > I wasn't really sure if I should remove code from the .sh file as it > seems in the past the old .sh files have been kept around as examples. Yeah, some original shell scripts that have been converted are kept in contrib/examples/, but the shell code has still been removed from the .sh files when they were being converted. > Has that been done for previous conversions? I don't think there were some cases when the shell code was not removed. I haven't looked at all the conversions in details though. ``` ## Ævar Arnfjörð Bjarmason, 2018-03-25 20:57 Subject: [PATCH] Remove contrib/examples/* Message-ID: <878tafyito.fsf@evledraar.gmail.com> URL: https://gitlist.dev/e/878tafyito.fsf%40evledraar.gmail.com In-Reply-To: <20180325204653.1470-1-avarab@gmail.com> ``` On Sun, Mar 25 2018, Ævar Arnfjörð Bjarmason wrote: > There were some side discussions at Git Merge this year about how we > should just update the README to tell users they can dig these up from > the history if the need them, do that. > > Looking at the "git log" for this directory we get quite a bit more > patch churn than we should here, mainly from things fixing various > tree-wide issues. > > There's also confusion on the list occasionally about how these should > be treated, "Re: [PATCH 1/4] stash: convert apply to > builtin" () > being the latest example of that. The people on CC got this, but it seems the git ML rejected the message as it's too big. The abbreviated patches is here quoted inline, and at: https://github.com/avar/git/commit/cc578c81c2cb2999b1a0b73954610bd74951c37b > Signed-off-by: Ævar Arnfjörð Bjarmason > --- > > On Sun, Mar 25, 2018 at 6:51 PM, Joel Teichroeb wrote: >> On Sun, Mar 25, 2018 at 1:09 AM, Christian Couder >> wrote: >>> It seems to me that the apply_stash() shell function is also used in >>> pop_stash() and in apply_to_branch(). Can the new helper be used there >>> too instead of apply_stash()? And then could apply_stash() be remove? >> >> I wasn't really sure if I should remove code from the .sh file as it >> seems in the past the old .sh files have been kept around as examples. >> Has that been done for previous conversions? > > I was skimming this patch and it seemed to me like it would be more > readable if the *.sh code was removed in the same change (if > possible). It's easier to review like that. > > Also, we should just stop maintainign contrib/examples/*. > > contrib/examples/README | 23 +- > contrib/examples/builtin-fetch--tool.c | 575 --------------- > contrib/examples/git-am.sh | 975 ------------------------ > contrib/examples/git-checkout.sh | 302 -------- > contrib/examples/git-clean.sh | 118 --- > contrib/examples/git-clone.sh | 525 ------------- > contrib/examples/git-commit.sh | 639 ---------------- > contrib/examples/git-difftool.perl | 481 ------------ > contrib/examples/git-fetch.sh | 379 ---------- > contrib/examples/git-gc.sh | 37 - > contrib/examples/git-log.sh | 15 - > contrib/examples/git-ls-remote.sh | 142 ---- > contrib/examples/git-merge-ours.sh | 14 - > contrib/examples/git-merge.sh | 620 ---------------- > contrib/examples/git-notes.sh | 121 --- > contrib/examples/git-pull.sh | 381 ---------- > contrib/examples/git-remote.perl | 474 ------------ > contrib/examples/git-repack.sh | 194 ----- > contrib/examples/git-rerere.perl | 284 ------- > contrib/examples/git-reset.sh | 106 --- > contrib/examples/git-resolve.sh | 112 --- > contrib/examples/git-revert.sh | 207 ------ > contrib/examples/git-svnimport.perl | 976 ------------------------- > contrib/examples/git-svnimport.txt | 179 ----- > contrib/examples/git-tag.sh | 205 ------ > contrib/examples/git-verify-tag.sh | 45 -- > contrib/examples/git-whatchanged.sh | 28 - > 27 files changed, 20 insertions(+), 8137 deletions(-) > delete mode 100644 contrib/examples/builtin-fetch--tool.c > delete mode 100755 contrib/examples/git-am.sh > delete mode 100755 contrib/examples/git-checkout.sh > delete mode 100755 contrib/examples/git-clean.sh > delete mode 100755 contrib/examples/git-clone.sh > delete mode 100755 contrib/examples/git-commit.sh > delete mode 100755 contrib/examples/git-difftool.perl > delete mode 100755 contrib/examples/git-fetch.sh > delete mode 100755 contrib/examples/git-gc.sh > delete mode 100755 contrib/examples/git-log.sh > delete mode 100755 contrib/examples/git-ls-remote.sh > delete mode 100755 contrib/examples/git-merge-ours.sh > delete mode 100755 contrib/examples/git-merge.sh > delete mode 100755 contrib/examples/git-notes.sh > delete mode 100755 contrib/examples/git-pull.sh > delete mode 100755 contrib/examples/git-remote.perl > delete mode 100755 contrib/examples/git-repack.sh > delete mode 100755 contrib/examples/git-rerere.perl > delete mode 100755 contrib/examples/git-reset.sh > delete mode 100755 contrib/examples/git-resolve.sh > delete mode 100755 contrib/examples/git-revert.sh > delete mode 100755 contrib/examples/git-svnimport.perl > delete mode 100644 contrib/examples/git-svnimport.txt > delete mode 100755 contrib/examples/git-tag.sh > delete mode 100755 contrib/examples/git-verify-tag.sh > delete mode 100755 contrib/examples/git-whatchanged.sh > > diff --git a/contrib/examples/README b/contrib/examples/README > index 6946f3dd2a..18bc60b021 100644 > --- a/contrib/examples/README > +++ b/contrib/examples/README > @@ -1,3 +1,20 @@ > -These are original scripted implementations, kept primarily for their > -reference value to any aspiring plumbing users who want to learn how > -pieces can be fit together. > +This directory used to contain scripted implementations of builtins > +that have since been rewritten in C. > + > +They have now been removed, but can be retrieved from an older commit > +that removed them from this directory. > + > +They're interesting for their reference value to any aspiring plumbing > +users who want to learn how pieces can be fit together, but in many > +cases have drifted enough from the actual implementations Git uses to > +be instructive. > + > +Other things that can be useful: > + > + * Some commands such as git-gc wrap other commands, and what they're > + doing behind the scenes can be seen by running them under > + GIT_TRACE=1 > + > + * Doing `git log` on paths matching '*--helper.c' will show > + incremental effort in the direction of moving existing shell > + scripts to C. > [...] The rest of this patch is deleting everything in contrib/examples/ that isn't the README. ``` ## Jeff King, 2018-03-26 06:01 Subject: Re: [PATCH] Remove contrib/examples/* Message-ID: <20180326060135.GB7594@sigill.intra.peff.net> URL: https://gitlist.dev/e/20180326060135.GB7594%40sigill.intra.peff.net In-Reply-To: <878tafyito.fsf@evledraar.gmail.com> ``` On Sun, Mar 25, 2018 at 08:46:53PM +0000, Ævar Arnfjörð Bjarmason wrote: > There were some side discussions at Git Merge this year about how we > should just update the README to tell users they can dig these up from > the history if the need them, do that. > > Looking at the "git log" for this directory we get quite a bit more > patch churn than we should here, mainly from things fixing various > tree-wide issues. > > There's also confusion on the list occasionally about how these should > be treated, "Re: [PATCH 1/4] stash: convert apply to > builtin" () > being the latest example of that. I'm in favor of this. I don't think I've ever come across this directory and _not_ been annoyed (because it was the result of a grep and was just cluttering the results). And I think your README change leaves a nice signpost for people who might be digging around for plumbing examples. > The people on CC got this, but it seems the git ML rejected the message > as it's too big. The abbreviated patches is here quoted inline, and at: > https://github.com/avar/git/commit/cc578c81c2cb2999b1a0b73954610bd74951c37b I was going to suggest re-sending with "-D", but it looks like "git apply" will not apply such a patch (even though it could in theory realize that the current blob matches the preimage sha1 and it would be safe to remove it). -Peff ``` ## Junio C Hamano, 2018-03-26 20:58 Subject: Re: [PATCH] Remove contrib/examples/* Message-ID: URL: https://gitlist.dev/e/xmqqpo3qbll7.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20180325204653.1470-1-avarab@gmail.com> ``` Ævar Arnfjörð Bjarmason writes: > + * Doing `git log` on paths matching '*--helper.c' will show > + incremental effort in the direction of moving existing shell > + scripts to C. It may be benefitial to remind readers of "--full-diff", e.g. $ git log --full-diff --stat -p -- "${foo}--helper.c" here. ``` ## Joel Teichroeb, 2018-03-28 03:30 Subject: Re: [PATCH 1/4] stash: convert apply to builtin Message-ID: URL: https://gitlist.dev/e/CA%2BCzEk_vRc2AQ%2BCxn66TmqbYjDcMsgy0-QXLsJwKRLE70nip_A%40mail.gmail.com In-Reply-To: <20180325164300.GA10909@hank> ``` On Sun, Mar 25, 2018 at 9:43 AM, Thomas Gummerer wrote: > On 03/24, Joel Teichroeb wrote: >> --- > > Missing sign-off? I saw it's missing in the other patches as well. > Thanks! I always forget to add a sign-off. >> [...] >> + >> + if (info->has_u) { >> + struct child_process cp = CHILD_PROCESS_INIT; >> + struct child_process cp2 = CHILD_PROCESS_INIT; >> + int res; >> + >> + cp.git_cmd = 1; >> + argv_array_push(&cp.args, "read-tree"); >> + argv_array_push(&cp.args, sha1_to_hex(info->u_tree.hash)); >> + argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", stash_index_path); >> + >> + cp2.git_cmd = 1; >> + argv_array_pushl(&cp2.args, "checkout-index", "--all", NULL); >> + argv_array_pushf(&cp2.env_array, "GIT_INDEX_FILE=%s", stash_index_path); >> + >> + res = run_command(&cp) || run_command(&cp2); >> + remove_path(stash_index_path); >> + if (res) >> + return error(_("Could not restore untracked files from stash")); > > A minor change in behaviour here is that we are removing the temporary > index file unconditionally here, while we would previously only remove > it if both 'read-tree' and 'checkout-index' would succeed. > > I don't think that's a bad thing, we probably don't want users to try > and use that index file in any way, and I doubt that's part of anyones > workflow, so I think cleaning it up makes sense. > I'm not sure about that. The shell script has a trap near the start in order to clean up the temp index, unless I'm understanding the shell script incorrectly. ```