[PATCH 0/4] Convert some stash functionality to a builtin

[PATCH 0/4] Convert some stash functionality to a builtin

[Joel Teichroeb]

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

[PATCH 1/4] stash: convert apply to builtin

[Joel Teichroeb]

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

[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] [<stash>]"),
+	N_("git stash--helper branch <branchname> [<stash>]"),
 	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 <branchname> [<stash>]"),
+	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

[PATCH 3/4] stash: convert drop and clear to builtin

[Joel Teichroeb]

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

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

Re: [PATCH 1/4] stash: convert apply to builtin

[Christian Couder]

> +       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);
> +}

Re: [PATCH 3/4] stash: convert drop and clear to builtin

[Christian Couder]

> +       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);
> +       }

Re: [PATCH 1/4] stash: convert apply to builtin

[Eric Sunshine]

On Sat, Mar 24, 2018 at 1:37 PM, Joel Teichroeb <joel@teichroeb.net> 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);
> +}

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

[Eric Sunshine]

On Sat, Mar 24, 2018 at 1:37 PM, Joel Teichroeb <joel@teichroeb.net> 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;
> +}

Re: [PATCH 3/4] stash: convert drop and clear to builtin

[Eric Sunshine]

On Sat, Mar 24, 2018 at 1:37 PM, Joel Teichroeb <joel@teichroeb.net> 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);
> +}

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

[Eric Sunshine]

On Sat, Mar 24, 2018 at 1:37 PM, Joel Teichroeb <joel@teichroeb.net> 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);
> +}

Re: [PATCH 1/4] stash: convert apply to builtin

[Christian Couder]

On Sat, Mar 24, 2018 at 6:37 PM, Joel Teichroeb <joel@teichroeb.net> 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?

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

[Christian Couder]

On Sat, Mar 24, 2018 at 6:37 PM, Joel Teichroeb <joel@teichroeb.net> 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 1/4] stash: convert apply to builtin

[Christian Couder]

On Sun, Mar 25, 2018 at 8:40 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Mar 24, 2018 at 1:37 PM, Joel Teichroeb <joel@teichroeb.net> 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.

Re: [PATCH 1/4] stash: convert apply to builtin

[Thomas Gummerer]

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;
> +	}
> +
> [...]

Re: [PATCH 1/4] stash: convert apply to builtin

[Joel Teichroeb]

On Sun, Mar 25, 2018 at 1:09 AM, Christian Couder
<christian.couder@gmail.com> 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?

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] [<stash>]"),
> +	N_("git stash--helper branch <branchname> [<stash>]"),
>  	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 <branchname> [<stash>]"),
> +	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
> 

Re: [PATCH 1/4] stash: convert apply to builtin

[Thomas Gummerer]

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;
> +}
> +
> [...]

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] [<stash>]"),
> +	N_("git stash--helper pop [--index] [-q|--quiet] [<stash>]"),
>  	N_("git stash--helper apply [--index] [-q|--quiet] [<stash>]"),
>  	N_("git stash--helper branch <branchname> [<stash>]"),
>  	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] [<stash>]"),
> +	NULL
> +};
> +
>  static const char * const git_stash_helper_apply_usage[] = {
>  	N_("git stash--helper apply [--index] [-q|--quiet] [<stash>]"),
>  	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 0/4] Convert some stash functionality to a builtin

[Thomas Gummerer]

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
> 

Re: [PATCH 1/4] stash: convert apply to builtin

[Christian Couder]

On Sun, Mar 25, 2018 at 6:51 PM, Joel Teichroeb <joel@teichroeb.net> wrote:
> On Sun, Mar 25, 2018 at 1:09 AM, Christian Couder
> <christian.couder@gmail.com> 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.

[PATCH] Remove contrib/examples/*

[Ævar Arnfjörð Bjarmason]

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" (<CA+CzEk9QpmHK_TSBwQfEedNqrcVSBp3xY7bdv1YA_KxePiFeXw@mail.gmail.com>)
> 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 <avarab@gmail.com>
> ---
>
> On Sun, Mar 25, 2018 at 6:51 PM, Joel Teichroeb <joel@teichroeb.net> wrote:
>> On Sun, Mar 25, 2018 at 1:09 AM, Christian Couder
>> <christian.couder@gmail.com> 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.

Re: [PATCH] Remove contrib/examples/*

[Jeff King]

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" (<CA+CzEk9QpmHK_TSBwQfEedNqrcVSBp3xY7bdv1YA_KxePiFeXw@mail.gmail.com>)
> 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

Re: [PATCH] Remove contrib/examples/*

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.

Re: [PATCH 1/4] stash: convert apply to builtin

[Joel Teichroeb]

On Sun, Mar 25, 2018 at 9:43 AM, Thomas Gummerer <t.gummerer@gmail.com> 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.