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

[PATCH v2 0/6] 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.

Previous threads:
v1: https://public-inbox.org/git/20180325173916.GE10909@hank/T/

Changes from v1:
 - Fixed memory leaks
 - Fixed formatting
 - Fixed typos
 - Removed old .sh code that has been replaced
 - Fix differences in error messages
 - Added tests to ensure argument handling work when specifiying
   multiple refs, or no arguments for branch
 - Refactored the code a bit
 - Reordered the patches so they compile and test successfully

Joel Teichroeb (6):
  stash: Add tests for passing in too many refs
  stash: Add test for branch with no arguments
  stash: convert apply to builtin
  stash: convert drop and clear to builtin
  stash: convert branch to builtin
  stash: convert pop to builtin

 .gitignore              |   1 +
 Makefile                |   1 +
 builtin.h               |   1 +
 builtin/stash--helper.c | 620 ++++++++++++++++++++++++++++++++++++++++++++++++
 git-stash.sh            | 125 +---------
 git.c                   |   1 +
 t/t3903-stash.sh        |  26 ++
 7 files changed, 658 insertions(+), 117 deletions(-)
 create mode 100644 builtin/stash--helper.c

-- 
2.16.2

[PATCH v2 1/6] stash: Add tests for passing in too many refs

[Joel Teichroeb]

Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
---
 t/t3903-stash.sh | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index aefde7b172..7146e27bb5 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -45,6 +45,12 @@ test_expect_success 'applying bogus stash does nothing' '
 	test_cmp expect file
 '
 
+test_expect_success 'applying with too many agruments does nothing' '
+	test_must_fail git stash apply stash@{0} bar &&
+	echo 1 >expect &&
+	test_cmp expect file
+'
+
 test_expect_success 'apply does not need clean working directory' '
 	echo 4 >other-file &&
 	git stash apply &&
@@ -97,6 +103,10 @@ test_expect_success 'stash drop complains of extra options' '
 	test_must_fail git stash drop --foo
 '
 
+test_expect_success 'stash drop complains with too many refs' '
+	test_must_fail git stash drop stash@{1} stash@{2}
+'
+
 test_expect_success 'drop top stash' '
 	git reset --hard &&
 	git stash list > stashlist1 &&
@@ -160,6 +170,10 @@ test_expect_success 'stash pop' '
 	test 0 = $(git stash list | wc -l)
 '
 
+test_expect_success 'stash pop complains with too many refs' '
+	test_must_fail git stash pop stash@{1} stash@{2}
+'
+
 cat > expect << EOF
 diff --git a/file2 b/file2
 new file mode 100644
@@ -479,6 +493,10 @@ test_expect_success 'stash branch - stashes on stack, stash-like argument' '
 	test $(git ls-files --modified | wc -l) -eq 1
 '
 
+test_expect_success 'stash branch complains with too many refs' '
+	test_must_fail git stash branch stash-branch stash@{1} stash@{2}
+'
+
 test_expect_success 'stash show format defaults to --stat' '
 	git stash clear &&
 	test_when_finished "git reset --hard HEAD" &&
@@ -567,6 +585,10 @@ test_expect_success 'stash show -p - no stashes on stack, stash-like argument' '
 	test_cmp expected actual
 '
 
+test_expect_success 'stash show complains with too many refs' '
+	test_must_fail git stash show stash@{1} stash@{2}
+'
+
 test_expect_success 'stash drop - fail early if specified stash is not a stash reference' '
 	git stash clear &&
 	test_when_finished "git reset --hard HEAD && git stash clear" &&
-- 
2.16.2

[PATCH v2 2/6] stash: Add test for branch with no arguments

[Joel Teichroeb]

Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
---
 t/t3903-stash.sh | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 7146e27bb5..261571d02a 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -497,6 +497,10 @@ test_expect_success 'stash branch complains with too many refs' '
 	test_must_fail git stash branch stash-branch stash@{1} stash@{2}
 '
 
+test_expect_success 'stash branch complains with no arguments' '
+	test_must_fail git stash branch
+'
+
 test_expect_success 'stash show format defaults to --stat' '
 	git stash clear &&
 	test_when_finished "git reset --hard HEAD" &&
-- 
2.16.2

[PATCH v2 3/6] stash: convert apply to builtin

[Joel Teichroeb]

Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
---
 .gitignore              |   1 +
 Makefile                |   1 +
 builtin.h               |   1 +
 builtin/stash--helper.c | 431 ++++++++++++++++++++++++++++++++++++++++++++++++
 git-stash.sh            |  75 +--------
 git.c                   |   1 +
 6 files changed, 440 insertions(+), 70 deletions(-)
 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 96f6138f63..6cfdbe9a32 100644
--- a/Makefile
+++ b/Makefile
@@ -1028,6 +1028,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..050de415b4
--- /dev/null
+++ b/builtin/stash--helper.c
@@ -0,0 +1,431 @@
+#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;
+	struct strbuf revision;
+	int is_stash_ref;
+	int has_u;
+};
+
+static int get_symbolic_name(const char *symbolic, struct strbuf *out)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	cp.git_cmd = 1;
+	argv_array_pushl(&cp.args, "rev-parse", "--symbolic-full-name", NULL);
+	argv_array_pushf(&cp.args, "%s", symbolic);
+	return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
+}
+
+static int have_stash(void)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	cp.git_cmd = 1;
+	cp.no_stdout = 1;
+	argv_array_pushl(&cp.args, "rev-parse", "--verify", "--quiet", NULL);
+	argv_array_pushf(&cp.args, "%s", ref_stash);
+	return pipe_command(&cp, NULL, 0, NULL, 0, NULL, 0);
+}
+
+static void destroy_stash_info(struct stash_info *info)
+{
+	strbuf_release(&info->revision);
+}
+
+static int get_stash_info(struct stash_info *info, int argc, const char **argv)
+{
+	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 symbolic = STRBUF_INIT;
+	struct strbuf out = STRBUF_INIT;
+	int ret;
+	const char *revision;
+	const char *commit = NULL;
+	char *end_of_rev;
+	info->is_stash_ref = 0;
+
+	if (argc > 1) {
+		int i;
+		fprintf(stderr, _("Too many revisions specified:"));
+		for (i = 0; i < argc; ++i) {
+			fprintf(stderr, " '%s'", argv[i]);
+		}
+		fprintf(stderr, "\n");
+
+		return -1;
+	}
+
+	if (argc == 1)
+		commit = argv[0];
+
+	strbuf_init(&info->revision, 0);
+	if (commit == NULL) {
+		if (have_stash()) {
+			destroy_stash_info(info);
+			return error(_("No stash entries found."));
+		}
+
+		strbuf_addf(&info->revision, "%s@{0}", ref_stash);
+	} else if (strspn(commit, "0123456789") == strlen(commit)) {
+		strbuf_addf(&info->revision, "%s@{%s}", ref_stash, commit);
+	} else {
+		strbuf_addstr(&info->revision, commit);
+	}
+
+	revision = info->revision.buf;
+
+	strbuf_addf(&w_commit_rev, "%s", revision);
+
+
+	ret = !get_oid(w_commit_rev.buf, &info->w_commit);
+
+	strbuf_release(&w_commit_rev);
+
+	if (!ret) {
+		destroy_stash_info(info);
+		return error(_("%s is not a valid reference"), 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(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(&b_commit_rev);
+	strbuf_release(&w_tree_rev);
+	strbuf_release(&b_tree_rev);
+	strbuf_release(&i_tree_rev);
+
+	if (!ret) {
+		destroy_stash_info(info);
+		return error(_("'%s' is not a stash-like commit"), 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);
+
+	ret = get_symbolic_name(symbolic.buf, &out);
+	strbuf_release(&symbolic);
+	if (ret) {
+		destroy_stash_info(info);
+		strbuf_release(&out);
+		return -1;
+	}
+
+	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))
+		return error(_("unable to write new index file"));
+
+	return 0;
+}
+
+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);
+}
+
+static int reset_head(const char *prefix)
+{
+	struct argv_array args = ARGV_ARRAY_INIT;
+
+	argv_array_push(&args, "reset");
+	return cmd_reset(args.argc, args.argv, prefix);
+}
+
+static int diff_cached_index(struct strbuf *out, struct object_id *c_tree)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+    const char *c_tree_hex = oid_to_hex(c_tree);
+
+	cp.git_cmd = 1;
+	argv_array_pushl(&cp.args, "diff-index", "--cached", "--name-only", "--diff-filter=A", NULL);
+	argv_array_push(&cp.args, c_tree_hex);
+	return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
+}
+
+static int update_index(struct strbuf *out) {
+	struct child_process cp = CHILD_PROCESS_INIT;
+	cp.git_cmd = 1;
+	argv_array_pushl(&cp.args, "update-index", "--add", "--stdin", NULL);
+	return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 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, 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 strbuf out = STRBUF_INIT;
+
+			if (diff_tree_binary(&out, &info->w_commit)) {
+				strbuf_release(&out);
+				return -1;
+			}
+
+			ret = apply_cached(&out);
+			strbuf_release(&out);
+			if (ret)
+				return -1;
+
+			discard_cache();
+			read_cache();
+			if (write_cache_as_tree(&index_tree, 0, NULL))
+				return -1;
+
+			reset_head(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, oid_to_hex(&info->u_tree));
+		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 (!oidcmp(&info->b_tree, &c_tree))
+		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)
+			fprintf_ln(stderr, _("Index was not unstashed."));
+
+		return ret;
+	}
+
+	if (has_index) {
+		if (reset_tree(&index_tree, 0, 0))
+			return -1;
+	} else {
+		struct strbuf out = STRBUF_INIT;
+
+		if (diff_cached_index(&out, &c_tree)) {
+			strbuf_release(&out);
+			return -1;
+		}
+
+		if (reset_tree(&c_tree, 0, 1)) {
+			strbuf_release(&out);
+			return -1;
+		}
+
+		ret = update_index(&out);
+		strbuf_release(&out);
+		if (ret)
+			return -1;
+
+		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)
+{
+	int index = 0;
+	struct stash_info info;
+	int ret;
+	struct option options[] = {
+		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
+		OPT_BOOL(0, "index", &index,
+			N_("attempt to recreate the index")),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+			git_stash_helper_apply_usage, 0);
+
+	if (get_stash_info(&info, argc, argv))
+		return -1;
+
+	ret = do_apply_stash(prefix, &info, index);
+	destroy_stash_info(&info);
+	return ret;
+}
+
+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 94793c1a91..0b5d1f3743 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -566,76 +566,11 @@ assert_stash_ref() {
 }
 
 apply_stash () {
-
-	assert_stash_like "$@"
-
-	git update-index -q --refresh || die "$(gettext "unable to refresh index")"
-
-	# current index state
-	c_tree=$(git write-tree) ||
-		die "$(gettext "Cannot apply a stash in the middle of a merge")"
-
-	unstashed_index_tree=
-	if test -n "$INDEX_OPTION" && test "$b_tree" != "$i_tree" &&
-			test "$c_tree" != "$i_tree"
-	then
-		git diff-tree --binary $s^2^..$s^2 | git apply --cached
-		test $? -ne 0 &&
-			die "$(gettext "Conflicts in index. Try without --index.")"
-		unstashed_index_tree=$(git write-tree) ||
-			die "$(gettext "Could not save index tree")"
-		git reset
-	fi
-
-	if test -n "$u_tree"
-	then
-		GIT_INDEX_FILE="$TMPindex" git read-tree "$u_tree" &&
-		GIT_INDEX_FILE="$TMPindex" git checkout-index --all &&
-		rm -f "$TMPindex" ||
-		die "$(gettext "Could not restore untracked files from stash entry")"
-	fi
-
-	eval "
-		GITHEAD_$w_tree='Stashed changes' &&
-		GITHEAD_$c_tree='Updated upstream' &&
-		GITHEAD_$b_tree='Version stash was based on' &&
-		export GITHEAD_$w_tree GITHEAD_$c_tree GITHEAD_$b_tree
-	"
-
-	if test -n "$GIT_QUIET"
-	then
-		GIT_MERGE_VERBOSITY=0 && export GIT_MERGE_VERBOSITY
-	fi
-	if git merge-recursive $b_tree -- $c_tree $w_tree
-	then
-		# No conflict
-		if test -n "$unstashed_index_tree"
-		then
-			git read-tree "$unstashed_index_tree"
-		else
-			a="$TMP-added" &&
-			git diff-index --cached --name-only --diff-filter=A $c_tree >"$a" &&
-			git read-tree --reset $c_tree &&
-			git update-index --add --stdin <"$a" ||
-				die "$(gettext "Cannot unstage modified files")"
-			rm -f "$a"
-		fi
-		squelch=
-		if test -n "$GIT_QUIET"
-		then
-			squelch='>/dev/null 2>&1'
-		fi
-		(cd "$START_DIR" && eval "git status $squelch") || :
-	else
-		# Merge conflict; keep the exit status from merge-recursive
-		status=$?
-		git rerere
-		if test -n "$INDEX_OPTION"
-		then
-			gettextln "Index was not unstashed." >&2
-		fi
-		exit $status
-	fi
+	cd "$START_DIR"
+	git stash--helper apply "$@"
+	res=$?
+	cd_to_toplevel
+	return $res
 }
 
 pop_stash() {
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 v2 4/6] stash: convert drop and clear to builtin

[Joel Teichroeb]

Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
---
 builtin/stash--helper.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++
 git-stash.sh            |   4 +-
 2 files changed, 103 insertions(+), 2 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 050de415b4..640de6d0aa 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -11,7 +11,14 @@
 #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 clear"),
+	NULL
+};
+
+static const char * const git_stash_helper_drop_usage[] = {
+	N_("git stash--helper drop [-q|--quiet] [<stash>]"),
 	NULL
 };
 
@@ -20,6 +27,11 @@ static const char * const git_stash_helper_apply_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];
@@ -166,6 +178,29 @@ static int get_stash_info(struct stash_info *info, int argc, const char **argv)
 	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;
@@ -399,6 +434,68 @@ static int apply_stash(int argc, const char **argv, const char *prefix)
 	return ret;
 }
 
+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.buf);
+	ret = cmd_reflog(args.argc, args.argv, prefix);
+	if (!ret) {
+		if (!quiet)
+			printf(_("Dropped %s (%s)\n"), info->revision.buf, oid_to_hex(&info->w_commit));
+	} else {
+		return error(_("%s: Could not drop stash entry"), info->revision.buf);
+	}
+
+	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 assert_stash_ref(struct stash_info *info)
+{
+	if (!info->is_stash_ref)
+		return error(_("'%s' is not a stash reference"), info->revision.buf);
+
+	return 0;
+}
+
+static int drop_stash(int argc, const char **argv, const char *prefix)
+{
+	struct stash_info info;
+	int ret;
+	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 (get_stash_info(&info, argc, argv))
+		return -1;
+
+	if (assert_stash_ref(&info)) {
+		destroy_stash_info(&info);
+		return -1;
+	}
+
+	ret = do_drop_stash(prefix, &info);
+	destroy_stash_info(&info);
+	return ret;
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
 	int result = 0;
@@ -421,6 +518,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 {
 		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 0b5d1f3743..0b8f07b38a 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -652,7 +652,7 @@ apply)
 	;;
 clear)
 	shift
-	clear_stash "$@"
+	git stash--helper clear "$@"
 	;;
 create)
 	shift
@@ -664,7 +664,7 @@ store)
 	;;
 drop)
 	shift
-	drop_stash "$@"
+	git stash--helper drop "$@"
 	;;
 pop)
 	shift
-- 
2.16.2

[PATCH v2 5/6] stash: convert branch to builtin

[Joel Teichroeb]

Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
---
 builtin/stash--helper.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 git-stash.sh            | 17 ++---------------
 2 files changed, 49 insertions(+), 15 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 640de6d0aa..4226c13be3 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -13,6 +13,7 @@
 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
 };
@@ -27,6 +28,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 * const git_stash_helper_clear_usage[] = {
 	N_("git stash--helper clear"),
 	NULL
@@ -496,6 +502,45 @@ static int drop_stash(int argc, const char **argv, const char *prefix)
 	return ret;
 }
 
+static int branch_stash(int argc, const char **argv, const char *prefix)
+{
+	const char *branch = NULL;
+	int ret;
+	struct argv_array args = ARGV_ARRAY_INIT;
+	struct stash_info info;
+	struct option options[] = {
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+			git_stash_helper_branch_usage, 0);
+
+	if (argc == 0)
+		return error(_("No branch name specified"));
+
+	branch = argv[0];
+
+	if (get_stash_info(&info, argc - 1, argv + 1))
+		return -1;
+
+	argv_array_pushl(&args, "checkout", "-b", NULL);
+	argv_array_push(&args, branch);
+	argv_array_push(&args, oid_to_hex(&info.b_commit));
+	ret = cmd_checkout(args.argc, args.argv, prefix);
+	if (ret) {
+		destroy_stash_info(&info);
+		return -1;
+	}
+
+	ret = do_apply_stash(prefix, &info, 1);
+	if (!ret && info.is_stash_ref)
+		ret = do_drop_stash(prefix, &info);
+
+	destroy_stash_info(&info);
+
+	return ret;
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
 	int result = 0;
@@ -522,6 +567,8 @@ int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 		result = clear_stash(argc, argv, prefix);
 	else if (!strcmp(argv[0], "drop"))
 		result = drop_stash(argc, argv, prefix);
+	else if (!strcmp(argv[0], "branch"))
+		result = branch_stash(argc, argv, prefix);
 	else {
 		error(_("unknown subcommand: %s"), argv[0]);
 		usage_with_options(git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index 0b8f07b38a..c5fd4c6c44 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -598,20 +598,6 @@ drop_stash () {
 	clear_stash
 }
 
-apply_to_branch () {
-	test -n "$1" || die "$(gettext "No branch name specified")"
-	branch=$1
-	shift 1
-
-	set -- --index "$@"
-	assert_stash_like "$@"
-
-	git checkout -b $branch $REV^ &&
-	apply_stash "$@" && {
-		test -z "$IS_STASH_REF" || drop_stash "$@"
-	}
-}
-
 test "$1" = "-p" && set "push" "$@"
 
 PARSE_CACHE='--not-parsed'
@@ -672,7 +658,8 @@ pop)
 	;;
 branch)
 	shift
-	apply_to_branch "$@"
+	cd "$START_DIR"
+	git stash--helper branch "$@"
 	;;
 *)
 	case $# in
-- 
2.16.2

[PATCH v2 6/6] stash: convert pop to builtin

[Joel Teichroeb]

Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
---
 builtin/stash--helper.c | 41 +++++++++++++++++++++++++++++++++++++++++
 git-stash.sh            | 39 ++++-----------------------------------
 2 files changed, 45 insertions(+), 35 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 4226c13be3..36e7e660a0 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
@@ -502,6 +508,39 @@ static int drop_stash(int argc, const char **argv, const char *prefix)
 	return ret;
 }
 
+static int pop_stash(int argc, const char **argv, const char *prefix)
+{
+	int index = 0, ret;
+	struct stash_info info;
+	struct option options[] = {
+		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
+		OPT_BOOL(0, "index", &index,
+			N_("attempt to recreate the index")),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+			git_stash_helper_pop_usage, 0);
+
+	if (get_stash_info(&info, argc, argv))
+		return -1;
+
+	if (assert_stash_ref(&info)) {
+		destroy_stash_info(&info);
+		return -1;
+	}
+
+	if (do_apply_stash(prefix, &info, index)) {
+		printf_ln(_("The stash entry is kept in case you need it again."));
+		destroy_stash_info(&info);
+		return -1;
+	}
+
+	ret = do_drop_stash(prefix, &info);
+	destroy_stash_info(&info);
+	return ret;
+}
+
 static int branch_stash(int argc, const char **argv, const char *prefix)
 {
 	const char *branch = NULL;
@@ -567,6 +606,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 c5fd4c6c44..1b8c7c4f63 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -565,39 +565,6 @@ assert_stash_ref() {
 	}
 }
 
-apply_stash () {
-	cd "$START_DIR"
-	git stash--helper apply "$@"
-	res=$?
-	cd_to_toplevel
-	return $res
-}
-
-pop_stash() {
-	assert_stash_ref "$@"
-
-	if apply_stash "$@"
-	then
-		drop_stash "$@"
-	else
-		status=$?
-		say "$(gettext "The stash entry is kept in case you need it again.")"
-		exit $status
-	fi
-}
-
-drop_stash () {
-	assert_stash_ref "$@"
-
-	git reflog delete --updateref --rewrite "${REV}" &&
-		say "$(eval_gettext "Dropped \${REV} (\$s)")" ||
-		die "$(eval_gettext "\${REV}: Could not drop stash entry")"
-
-	# clear_stash if we just dropped the last stash entry
-	git rev-parse --verify --quiet "$ref_stash@{0}" >/dev/null ||
-	clear_stash
-}
-
 test "$1" = "-p" && set "push" "$@"
 
 PARSE_CACHE='--not-parsed'
@@ -634,7 +601,8 @@ push)
 	;;
 apply)
 	shift
-	apply_stash "$@"
+	cd "$START_DIR"
+	git stash--helper apply "$@"
 	;;
 clear)
 	shift
@@ -654,7 +622,8 @@ drop)
 	;;
 pop)
 	shift
-	pop_stash "$@"
+	cd "$START_DIR"
+	git stash--helper pop "$@"
 	;;
 branch)
 	shift
-- 
2.16.2

Re: [PATCH v2 3/6] stash: convert apply to builtin

[Christian Couder]

On Mon, Mar 26, 2018 at 3:14 AM, Joel Teichroeb <joel@teichroeb.net> wrote:
> Signed-off-by: Joel Teichroeb <joel@teichroeb.net>

The commit message in this patch and the following ones could be a bit
more verbose. It could at least tell that the end goal is to convert
git-stash.sh to a C builtin.

> +static void destroy_stash_info(struct stash_info *info)
> +{
> +       strbuf_release(&info->revision);
> +}

Not sure if "destroy" is the right word in the function name. I would
have used "free" instead.

> +static int get_stash_info(struct stash_info *info, int argc, const char **argv)
> +{
> +       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 symbolic = STRBUF_INIT;
> +       struct strbuf out = STRBUF_INIT;
> +       int ret;
> +       const char *revision;
> +       const char *commit = NULL;
> +       char *end_of_rev;
> +       info->is_stash_ref = 0;
> +
> +       if (argc > 1) {
> +               int i;
> +               fprintf(stderr, _("Too many revisions specified:"));
> +               for (i = 0; i < argc; ++i) {
> +                       fprintf(stderr, " '%s'", argv[i]);
> +               }

The brackets are not needed.

> +               fprintf(stderr, "\n");
> +
> +               return -1;
> +       }
> +
> +       if (argc == 1)
> +               commit = argv[0];
> +
> +       strbuf_init(&info->revision, 0);
> +       if (commit == NULL) {
> +               if (have_stash()) {
> +                       destroy_stash_info(info);
> +                       return error(_("No stash entries found."));
> +               }
> +
> +               strbuf_addf(&info->revision, "%s@{0}", ref_stash);
> +       } else if (strspn(commit, "0123456789") == strlen(commit)) {
> +               strbuf_addf(&info->revision, "%s@{%s}", ref_stash, commit);
> +       } else {
> +               strbuf_addstr(&info->revision, commit);
> +       }
> +
> +       revision = info->revision.buf;
> +
> +       strbuf_addf(&w_commit_rev, "%s", revision);

Maybe use strbuf_addstr()?

> +
> +

Spurious new line.

[...]

> +static int diff_cached_index(struct strbuf *out, struct object_id *c_tree)
> +{
> +       struct child_process cp = CHILD_PROCESS_INIT;
> +    const char *c_tree_hex = oid_to_hex(c_tree);

Indent looks weird.

> +
> +       cp.git_cmd = 1;
> +       argv_array_pushl(&cp.args, "diff-index", "--cached", "--name-only", "--diff-filter=A", NULL);
> +       argv_array_push(&cp.args, c_tree_hex);
> +       return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
> +}
> +
> +static int update_index(struct strbuf *out) {

The opening bracket should be on its own line.

> +       struct child_process cp = CHILD_PROCESS_INIT;

Maybe add a new line here to be more consistent with other such functions.

> +       cp.git_cmd = 1;
> +       argv_array_pushl(&cp.args, "update-index", "--add", "--stdin", NULL);
> +       return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);
> +}

[...]

> +       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, oid_to_hex(&info->u_tree));
> +               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);

Maybe use small functions for the above read-tree and checkout-index.

> +               res = run_command(&cp) || run_command(&cp2);
> +               remove_path(stash_index_path);
> +               if (res)
> +                       return error(_("Could not restore untracked files from stash"));
> +       }

Thanks.

Re: [PATCH v2 1/6] stash: Add tests for passing in too many refs

[Johannes Schindelin]

Hi Joel,

On Sun, 25 Mar 2018, Joel Teichroeb wrote:

> Signed-off-by: Joel Teichroeb <joel@teichroeb.net>

I could imagine that the commit message would benefit from this body:

	In preparation for converting the stash command incrementally to
	a builtin command, this patch improves test coverage of the option
	parsing.

> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index aefde7b172..7146e27bb5 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -45,6 +45,12 @@ test_expect_success 'applying bogus stash does nothing' '
>  	test_cmp expect file
>  '
>  
> +test_expect_success 'applying with too many agruments does nothing' '
> +	test_must_fail git stash apply stash@{0} bar &&
> +	echo 1 >expect &&
> +	test_cmp expect file
> +'

I suppose you encountered a problem where `stash apply a b` would modify
the file?

And if you really want to verify that the command does nothing, I guess
you will have to use

	test-chmtime =123456789 file &&
	test_must_fail git stash apply stash@{0} bar &&
	test 123456789 = $(test-chmtime -v +0 file | sed 's/[^0-9].*$//')

> @@ -97,6 +103,10 @@ test_expect_success 'stash drop complains of extra options' '
>  	test_must_fail git stash drop --foo
>  '
>  
> +test_expect_success 'stash drop complains with too many refs' '
> +	test_must_fail git stash drop stash@{1} stash@{2}

I wonder whether you might want to verify that the error message is
printed, e.g. via

	test_must_fail git stash drop stash@{1} stash@{2} 2>err &&
	test_i18ngrep "Too many" err

Also, since the added tests look very similar, it might make sense to use
a loop (with fixed revision arguments).

Ciao,
Dscho

Re: [PATCH v2 3/6] stash: convert apply to builtin

[Joel Teichroeb]

On Mon, Mar 26, 2018 at 12:05 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Mon, Mar 26, 2018 at 3:14 AM, Joel Teichroeb <joel@teichroeb.net> wrote:
>> Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
>
> The commit message in this patch and the following ones could be a bit
> more verbose. It could at least tell that the end goal is to convert
> git-stash.sh to a C builtin.
>
>> +static void destroy_stash_info(struct stash_info *info)
>> +{
>> +       strbuf_release(&info->revision);
>> +}
>
> Not sure if "destroy" is the right word in the function name. I would
> have used "free" instead.
>
>> +static int get_stash_info(struct stash_info *info, int argc, const char **argv)
>> +{
>> +       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 symbolic = STRBUF_INIT;
>> +       struct strbuf out = STRBUF_INIT;
>> +       int ret;
>> +       const char *revision;
>> +       const char *commit = NULL;
>> +       char *end_of_rev;
>> +       info->is_stash_ref = 0;
>> +
>> +       if (argc > 1) {
>> +               int i;
>> +               fprintf(stderr, _("Too many revisions specified:"));
>> +               for (i = 0; i < argc; ++i) {
>> +                       fprintf(stderr, " '%s'", argv[i]);
>> +               }
>
> The brackets are not needed.
>
>> +               fprintf(stderr, "\n");
>> +
>> +               return -1;
>> +       }
>> +
>> +       if (argc == 1)
>> +               commit = argv[0];
>> +
>> +       strbuf_init(&info->revision, 0);
>> +       if (commit == NULL) {
>> +               if (have_stash()) {
>> +                       destroy_stash_info(info);
>> +                       return error(_("No stash entries found."));
>> +               }
>> +
>> +               strbuf_addf(&info->revision, "%s@{0}", ref_stash);
>> +       } else if (strspn(commit, "0123456789") == strlen(commit)) {
>> +               strbuf_addf(&info->revision, "%s@{%s}", ref_stash, commit);
>> +       } else {
>> +               strbuf_addstr(&info->revision, commit);
>> +       }
>> +
>> +       revision = info->revision.buf;
>> +
>> +       strbuf_addf(&w_commit_rev, "%s", revision);
>
> Maybe use strbuf_addstr()?
>
>> +
>> +
>
> Spurious new line.
>
> [...]
>
>> +static int diff_cached_index(struct strbuf *out, struct object_id *c_tree)
>> +{
>> +       struct child_process cp = CHILD_PROCESS_INIT;
>> +    const char *c_tree_hex = oid_to_hex(c_tree);
>
> Indent looks weird.
>
>> +
>> +       cp.git_cmd = 1;
>> +       argv_array_pushl(&cp.args, "diff-index", "--cached", "--name-only", "--diff-filter=A", NULL);
>> +       argv_array_push(&cp.args, c_tree_hex);
>> +       return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
>> +}
>> +
>> +static int update_index(struct strbuf *out) {
>
> The opening bracket should be on its own line.
>
>> +       struct child_process cp = CHILD_PROCESS_INIT;
>
> Maybe add a new line here to be more consistent with other such functions.
>
>> +       cp.git_cmd = 1;
>> +       argv_array_pushl(&cp.args, "update-index", "--add", "--stdin", NULL);
>> +       return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);
>> +}
>
> [...]
>
>> +       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, oid_to_hex(&info->u_tree));
>> +               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);
>
> Maybe use small functions for the above read-tree and checkout-index.
>
>> +               res = run_command(&cp) || run_command(&cp2);
>> +               remove_path(stash_index_path);
>> +               if (res)
>> +                       return error(_("Could not restore untracked files from stash"));
>> +       }
>
> Thanks.

Thanks for your detailed comments! I've fixed them all in my next patch set.