[PATCH v4 0/5] Implement git stash as a builtin command

[PATCH v4 0/5] Implement git stash as a builtin command

[Joel Teichroeb]

I've rewritten git stash as a builtin c command. All tests pass,
and I've added two new tests. Test coverage is around 95% with the
only things missing coverage being error handlers.

Changes since v3:
 * Fixed formatting issues
 * Fixed a bug with stash branch and added a new test for it
 * Fixed review comments

Outstanding issue:
 * Not all argv array memory is cleaned up

Joel Teichroeb (5):
  stash: add test for stash create with no files
  stash: Add a test for when apply fails during stash branch
  stash: add test for stashing in a detached state
  merge: close the index lock when not writing the new index
  stash: implement builtin stash

 Makefile                                      |    2 +-
 builtin.h                                     |    1 +
 builtin/stash.c                               | 1224 +++++++++++++++++++++++++
 git-stash.sh => contrib/examples/git-stash.sh |    0
 git.c                                         |    1 +
 merge-recursive.c                             |    9 +-
 t/t3903-stash.sh                              |   34 +
 7 files changed, 1267 insertions(+), 4 deletions(-)
 create mode 100644 builtin/stash.c
 rename git-stash.sh => contrib/examples/git-stash.sh (100%)

-- 
2.13.0

[PATCH v4 1/5] stash: add test for stash create with no files

[Joel Teichroeb]

Ensure the command gives the correct return code

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

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 3b4bed5c9a..cc923e6335 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -444,6 +444,14 @@ test_expect_failure 'stash file to directory' '
 	test foo = "$(cat file/file)"
 '
 
+test_expect_success 'stash create - no changes' '
+	git stash clear &&
+	test_when_finished "git reset --hard HEAD" &&
+	git reset --hard &&
+	git stash create >actual &&
+	test_must_be_empty actual
+'
+
 test_expect_success 'stash branch - no stashes on stack, stash-like argument' '
 	git stash clear &&
 	test_when_finished "git reset --hard HEAD" &&
-- 
2.13.0

[PATCH v4 2/5] stash: Add a test for when apply fails during stash branch

[Joel Teichroeb]

If the return value of merge recurisve is not checked, the stash could end
up being dropped even though it was not applied properly

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

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index cc923e6335..5399fb05ca 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -656,6 +656,20 @@ test_expect_success 'stash branch should not drop the stash if the branch exists
 	git rev-parse stash@{0} --
 '
 
+test_expect_success 'stash branch should not drop the stash if the apply fails' '
+	git stash clear &&
+	git reset HEAD~1 --hard &&
+	echo foo >file &&
+	git add file &&
+	git commit -m initial &&
+	echo bar >file &&
+	git stash &&
+	echo baz >file &&
+	test_when_finished "git checkout master" &&
+	test_must_fail git stash branch new_branch stash@{0} &&
+	git rev-parse stash@{0} --
+'
+
 test_expect_success 'stash apply shows status same as git status (relative to current directory)' '
 	git stash clear &&
 	echo 1 >subdir/subfile1 &&
-- 
2.13.0

[PATCH v4 3/5] stash: add test for stashing in a detached state

[Joel Teichroeb]

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

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 5399fb05ca..ce4c8fe3d6 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -822,6 +822,18 @@ test_expect_success 'create with multiple arguments for the message' '
 	test_cmp expect actual
 '
 
+test_expect_success 'create in a detached state' '
+	test_when_finished "git checkout master" &&
+	git checkout HEAD~1 &&
+	>foo &&
+	git add foo &&
+	STASH_ID=$(git stash create) &&
+	HEAD_ID=$(git rev-parse --short HEAD) &&
+	echo "WIP on (no branch): ${HEAD_ID} initial" >expect &&
+	git show --pretty=%s -s ${STASH_ID} >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'stash -- <pathspec> stashes and restores the file' '
 	>foo &&
 	>bar &&
-- 
2.13.0

[PATCH v4 4/5] merge: close the index lock when not writing the new index

[Joel Teichroeb]

If the merge does not have anything to do, it does not unlock the index,
causing any further index operations to fail. Thus, always unlock the index
regardless of outcome.

Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
---
 merge-recursive.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index ae5238d82c..16bb5512ef 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2145,9 +2145,12 @@ int merge_recursive_generic(struct merge_options *o,
 	if (clean < 0)
 		return clean;
 
-	if (active_cache_changed &&
-	    write_locked_index(&the_index, lock, COMMIT_LOCK))
-		return err(o, _("Unable to write index."));
+	if (active_cache_changed) {
+		if (write_locked_index(&the_index, lock, COMMIT_LOCK))
+			return err(o, _("Unable to write index."));
+	} else {
+		rollback_lock_file(lock);
+	}
 
 	return clean ? 0 : 1;
 }
-- 
2.13.0

[PATCH v4 5/5] stash: implement builtin stash

[Joel Teichroeb]

Implement all git stash functionality as a builtin command

Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
---
 Makefile                                      |    2 +-
 builtin.h                                     |    1 +
 builtin/stash.c                               | 1224 +++++++++++++++++++++++++
 git-stash.sh => contrib/examples/git-stash.sh |    0
 git.c                                         |    1 +
 5 files changed, 1227 insertions(+), 1 deletion(-)
 create mode 100644 builtin/stash.c
 rename git-stash.sh => contrib/examples/git-stash.sh (100%)

diff --git a/Makefile b/Makefile
index 7c621f7f76..3364d87630 100644
--- a/Makefile
+++ b/Makefile
@@ -525,7 +525,6 @@ SCRIPT_SH += git-quiltimport.sh
 SCRIPT_SH += git-rebase.sh
 SCRIPT_SH += git-remote-testgit.sh
 SCRIPT_SH += git-request-pull.sh
-SCRIPT_SH += git-stash.sh
 SCRIPT_SH += git-submodule.sh
 SCRIPT_SH += git-web--browse.sh
 
@@ -965,6 +964,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.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 498ac80d07..fa59481420 100644
--- a/builtin.h
+++ b/builtin.h
@@ -119,6 +119,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(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.c b/builtin/stash.c
new file mode 100644
index 0000000000..a9680f2909
--- /dev/null
+++ b/builtin/stash.c
@@ -0,0 +1,1224 @@
+#include "builtin.h"
+#include "parse-options.h"
+#include "refs.h"
+#include "tree.h"
+#include "lockfile.h"
+#include "object.h"
+#include "tree-walk.h"
+#include "cache-tree.h"
+#include "unpack-trees.h"
+#include "diff.h"
+#include "revision.h"
+#include "commit.h"
+#include "diffcore.h"
+#include "merge-recursive.h"
+#include "argv-array.h"
+#include "run-command.h"
+
+static const char * const git_stash_usage[] = {
+	N_("git stash list [<options>]"),
+	N_("git stash show [<stash>]"),
+	N_("git stash drop [-q|--quiet] [<stash>]"),
+	N_("git stash ( pop | apply ) [--index] [-q|--quiet] [<stash>]"),
+	N_("git stash branch <branchname> [<stash>]"),
+	N_("git stash [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]"),
+	N_("                [-u|--include-untracked] [-a|--all] [<message>]]"),
+	N_("git stash clear"),
+	N_("git stash create [<message>]"),
+	N_("git stash store [-m|--message <message>] [-q|--quiet] <commit>"),
+	NULL
+};
+
+static const char * const git_stash_list_usage[] = {
+	N_("git stash list [<options>]"),
+	NULL
+};
+
+static const char * const git_stash_show_usage[] = {
+	N_("git stash show [<stash>]"),
+	NULL
+};
+
+static const char * const git_stash_drop_usage[] = {
+	N_("git stash drop [-q|--quiet] [<stash>]"),
+	NULL
+};
+
+static const char * const git_stash_pop_usage[] = {
+	N_("git stash pop [--index] [-q|--quiet] [<stash>]"),
+	NULL
+};
+
+static const char * const git_stash_apply_usage[] = {
+	N_("git stash apply [--index] [-q|--quiet] [<stash>]"),
+	NULL
+};
+
+static const char * const git_stash_branch_usage[] = {
+	N_("git stash branch <branchname> [<stash>]"),
+	NULL
+};
+
+static const char * const git_stash_save_usage[] = {
+	N_("git stash [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]"),
+	N_("                [-u|--include-untracked] [-a|--all] [<message>]]"),
+	NULL
+};
+
+static const char * const git_stash_clear_usage[] = {
+	N_("git stash clear"),
+	NULL
+};
+
+static const char * const git_stash_create_usage[] = {
+	N_("git stash create [<message>]"),
+	NULL
+};
+
+static const char * const git_stash_store_usage[] = {
+	N_("git stash store [-m|--message <message>] [-q|--quiet] <commit>"),
+	NULL
+};
+
+static const char *ref_stash = "refs/stash";
+static int quiet = 0;
+static struct lock_file lock_file;
+static char stash_index_path[64];
+
+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 untracked_files(struct strbuf *out, int include_untracked,
+		int include_ignored, const char **argv)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	cp.git_cmd = 1;
+	argv_array_pushl(&cp.args, "ls-files", "-o", "-z", NULL);
+	if (include_untracked && !include_ignored)
+		argv_array_push(&cp.args, "--exclude-standard");
+	argv_array_push(&cp.args, "--");
+	if (argv)
+		argv_array_pushv(&cp.args, argv);
+	return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
+}
+
+static int check_no_changes(const char *prefix, int include_untracked,
+		int include_ignored, const char **argv)
+{
+	struct argv_array args1 = ARGV_ARRAY_INIT;
+	struct argv_array args2 = ARGV_ARRAY_INIT;
+	struct strbuf out = STRBUF_INIT;
+	int ret;
+
+	argv_array_pushl(&args1, "diff-index", "--quiet", "--cached", "HEAD",
+		"--ignore-submodules", "--", NULL);
+	if (argv)
+		argv_array_pushv(&args1, argv);
+
+	argv_array_pushl(&args2, "diff-files", "--quiet", "--ignore-submodules",
+		"--", NULL);
+	if (argv)
+		argv_array_pushv(&args2, argv);
+
+	if (include_untracked)
+		untracked_files(&out, include_untracked, include_ignored, argv);
+
+	ret = cmd_diff_index(args1.argc, args1.argv, prefix) == 0 &&
+			cmd_diff_files(args2.argc, args2.argv, prefix) == 0 &&
+			(!include_untracked || out.len == 0);
+	strbuf_release(&out);
+	return ret;
+}
+
+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 (strlen(commit) < 3) {
+		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);
+	strbuf_addf(&u_tree_rev, "%s^3:", revision);
+
+	ret = (get_sha1(w_commit_rev.buf, info->w_commit.hash) == 0 &&
+		get_sha1(b_commit_rev.buf, info->b_commit.hash) == 0 &&
+		get_sha1(w_tree_rev.buf, info->w_tree.hash) == 0 &&
+		get_sha1(b_tree_rev.buf, info->b_tree.hash) == 0 &&
+		get_sha1(i_tree_rev.buf, info->i_tree.hash) == 0);
+
+	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);
+
+	info->has_u = get_sha1(u_tree_rev.buf, info->u_tree.hash) == 0;
+
+	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) == 0;
+	strbuf_release(&out);
+
+	return !ret;
+}
+
+static void stash_create_callback(struct diff_queue_struct *q,
+				struct diff_options *opt, void *cbdata)
+{
+	int i;
+
+	for (i = 0; i < q->nr; i++) {
+		struct diff_filepair *p = q->queue[i];
+		const char *path = p->one->path;
+		struct stat st;
+		remove_file_from_index(&the_index, path);
+		if (!lstat(path, &st))
+			add_to_index(&the_index, path, &st, 0);
+	}
+}
+
+/*
+ * Untracked files are stored by themselves in a parentless commit, for
+ * ease of unpacking later.
+ */
+static int save_untracked(struct stash_info *info, const char *message,
+		int include_untracked, int include_ignored, const char **argv)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf out = STRBUF_INIT;
+	struct object_id orig_tree;
+	int ret;
+	const char *index_file = get_index_file();
+
+	set_alternate_index_output(stash_index_path);
+	untracked_files(&out, include_untracked, include_ignored, argv);
+
+	cp.git_cmd = 1;
+	argv_array_pushl(&cp.args, "update-index", "-z", "--add", "--remove",
+		"--stdin", NULL);
+	argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", stash_index_path);
+
+	if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0)) {
+		strbuf_release(&out);
+		return 1;
+	}
+
+	strbuf_reset(&out);
+
+	discard_cache();
+	read_cache_from(stash_index_path);
+
+	write_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0,NULL);
+	discard_cache();
+
+	read_cache_from(stash_index_path);
+
+	write_cache_as_tree(info->u_tree.hash, 0, NULL);
+	strbuf_addf(&out, "untracked files on %s", message);
+
+	ret = commit_tree(out.buf, out.len, info->u_tree.hash, NULL,
+			info->u_commit.hash, NULL, NULL);
+	strbuf_release(&out);
+	if (ret)
+		return 1;
+
+	set_alternate_index_output(index_file);
+	discard_cache();
+	read_cache();
+
+	return 0;
+}
+
+static int save_working_tree(struct stash_info *info, const char *prefix,
+		const char **argv)
+{
+	struct object_id orig_tree;
+	struct rev_info rev;
+	int nr_trees = 1;
+	struct tree_desc t[MAX_UNPACK_TREES];
+	struct tree *tree;
+	struct unpack_trees_options opts;
+	struct object *obj;
+
+	discard_cache();
+	tree = parse_tree_indirect(&info->i_tree);
+	prime_cache_tree(&the_index, tree);
+	write_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0, NULL);
+	discard_cache();
+
+	read_cache_from(stash_index_path);
+
+	memset(&opts, 0, sizeof(opts));
+
+	parse_tree(tree);
+
+	opts.head_idx = 1;
+	opts.src_index = &the_index;
+	opts.dst_index = &the_index;
+	opts.merge = 1;
+	opts.fn = oneway_merge;
+
+	init_tree_desc(t, tree->buffer, tree->size);
+
+	if (unpack_trees(nr_trees, t, &opts))
+		return 1;
+
+	init_revisions(&rev, prefix);
+	setup_revisions(0, NULL, &rev, NULL);
+	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
+	rev.diffopt.format_callback = stash_create_callback;
+	DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
+
+	parse_pathspec(&rev.prune_data, 0, 0, prefix, argv);
+
+	diff_setup_done(&rev.diffopt);
+	obj = parse_object(&info->b_commit);
+	add_pending_object(&rev, obj, "");
+	if (run_diff_index(&rev, 0))
+		return 1;
+
+	if (write_cache_as_tree(info->w_tree.hash, 0, NULL))
+		return 1;
+
+	discard_cache();
+	read_cache();
+
+	return 0;
+}
+
+static int patch_working_tree(struct stash_info *info, const char *prefix,
+		const char **argv)
+{
+	struct argv_array args = ARGV_ARRAY_INIT;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf out = STRBUF_INIT;
+	size_t unused;
+	const char *index_file = get_index_file();
+
+	argv_array_pushl(&args, "read-tree", "HEAD", NULL);
+	argv_array_pushf(&args, "--index-output=%s", stash_index_path);
+	cmd_read_tree(args.argc, args.argv, prefix);
+
+	cp.git_cmd = 1;
+	argv_array_pushl(&cp.args, "add--interactive", "--patch=stash", "--", NULL);
+	argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", stash_index_path);
+	if (run_command(&cp))
+		return 1;
+
+	discard_cache();
+	read_cache_from(stash_index_path);
+
+	if (write_cache_as_tree(info->w_tree.hash, 0, NULL))
+		return 1;
+
+	child_process_init(&cp);
+	cp.git_cmd = 1;
+	argv_array_pushl(&cp.args, "diff-tree", "-p", "HEAD", NULL);
+	argv_array_push(&cp.args, sha1_to_hex(info->w_tree.hash));
+	argv_array_push(&cp.args, "--");
+	if (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0) || out.len == 0)
+		return 1;
+
+	info->patch = strbuf_detach(&out, &unused);
+
+	set_alternate_index_output(index_file);
+	discard_cache();
+	read_cache();
+
+	return 0;
+}
+
+static int do_create_stash(struct stash_info *info, const char *prefix,
+		const char *message, int include_untracked, int include_ignored,
+		int patch, const char **argv)
+{
+	struct object_id curr_head;
+	char *branch_path = NULL;
+	const char *branch_name = NULL;
+	struct commit_list *parents = NULL;
+	struct strbuf out_message = STRBUF_INIT;
+	struct strbuf out = STRBUF_INIT;
+	struct pretty_print_context ctx = {0};
+
+	struct commit *c = NULL;
+	const char *hash;
+
+	read_cache_preload(NULL);
+	refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL);
+	if (check_no_changes(prefix, include_untracked, include_ignored, argv))
+		return 1;
+
+	if (get_sha1_tree("HEAD", info->b_commit.hash))
+		return error(_("You do not have the initial commit yet"));
+
+	branch_path = resolve_refdup("HEAD", 0, curr_head.hash, NULL);
+
+	if (branch_path == NULL || strcmp(branch_path, "HEAD") == 0)
+		branch_name = "(no branch)";
+	else
+		skip_prefix(branch_path, "refs/heads/", &branch_name);
+
+	c = lookup_commit(&info->b_commit);
+
+	ctx.output_encoding = get_log_output_encoding();
+	ctx.abbrev = 1;
+	ctx.fmt = CMIT_FMT_ONELINE;
+	hash = find_unique_abbrev(c->object.oid.hash, DEFAULT_ABBREV);
+
+	strbuf_addf(&out_message, "%s: %s ", branch_name, hash);
+
+	pretty_print_commit(&ctx, c, &out_message);
+
+	strbuf_addf(&out, "index on %s\n", out_message.buf);
+
+	commit_list_insert(lookup_commit(&info->b_commit), &parents);
+
+	if (write_cache_as_tree(info->i_tree.hash, 0, NULL))
+		return error(_("git write-tree failed to write a tree"));
+
+	if (commit_tree(out.buf, out.len, info->i_tree.hash, parents, info->i_commit.hash, NULL, NULL))
+		return error(_("Cannot save the current index state"));
+
+	strbuf_reset(&out);
+
+	if (include_untracked) {
+		if (save_untracked(info, out_message.buf, include_untracked, include_ignored, argv))
+			return error(_("Cannot save the untracked files"));
+	}
+
+	if (patch) {
+		if (patch_working_tree(info, prefix, argv))
+			return error(_("Cannot save the current worktree state"));
+	} else {
+		if (save_working_tree(info, prefix, argv))
+			return error(_("Cannot save the current worktree state"));
+	}
+	parents = NULL;
+
+	if (include_untracked)
+		commit_list_insert(lookup_commit(&info->u_commit), &parents);
+
+	commit_list_insert(lookup_commit(&info->i_commit), &parents);
+	commit_list_insert(lookup_commit(&info->b_commit), &parents);
+
+	if (message != NULL && strlen(message) != 0)
+		strbuf_addf(&out, "On %s: %s\n", branch_name, message);
+	else
+		strbuf_addf(&out, "WIP on %s\n", out_message.buf);
+
+	if (commit_tree(out.buf, out.len, info->w_tree.hash, parents, info->w_commit.hash, NULL, NULL))
+		return error(_("Cannot record working tree state"));
+
+	info->message = out.buf;
+
+	strbuf_release(&out_message);
+	free(branch_path);
+
+	return 0;
+}
+
+static int create_stash(int argc, const char **argv, const char *prefix)
+{
+	int include_untracked = 0;
+	const char *message = NULL;
+	struct stash_info info;
+	int ret;
+	struct strbuf out = STRBUF_INIT;
+	struct option options[] = {
+		OPT_BOOL('u', "include-untracked", &include_untracked,
+			N_("stash untracked filed")),
+		OPT_STRING('m', "message", &message, N_("message"),
+			N_("stash commit message")),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+			git_stash_create_usage, 0);
+
+	if (argc != 0) {
+		int i;
+		for (i = 0; i < argc; ++i) {
+			if (i != 0) {
+				strbuf_addf(&out, " ");
+			}
+			strbuf_addf(&out, "%s", argv[i]);
+		}
+		message = out.buf;
+	}
+
+	ret = do_create_stash(&info, prefix, message, include_untracked, 0, 0, NULL);
+
+	strbuf_release(&out);
+
+	if (ret)
+		return 0;
+
+	printf("%s\n", sha1_to_hex(info.w_commit.hash));
+	return 0;
+}
+
+static int do_store_stash(const char *prefix, int quiet, const char *message,
+		struct object_id commit)
+{
+	int ret;
+	ret = update_ref(message, ref_stash, commit.hash, NULL,
+			REF_FORCE_CREATE_REFLOG, UPDATE_REFS_DIE_ON_ERR);
+
+	if (ret && !quiet)
+		return error(_("Cannot update %s with %s"), ref_stash, sha1_to_hex(commit.hash));
+
+	return ret;
+}
+
+static int store_stash(int argc, const char **argv, const char *prefix)
+{
+	const char *message = "Create via \"git stash store\".";
+	const char *commit = NULL;
+	struct object_id obj;
+	struct option options[] = {
+		OPT_STRING('m', "message", &message, N_("message"),
+			N_("stash commit message")),
+		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
+		OPT_END()
+	};
+	argc = parse_options(argc, argv, prefix, options, git_stash_store_usage, 0);
+
+	if (argc != 1)
+		return error(_("\"git stash store\" requires one <commit> argument"));
+
+	commit = argv[0];
+
+	if (get_sha1(commit, obj.hash)) {
+		fprintf_ln(stderr, _("fatal: %s: not a valid SHA1"), commit);
+		fprintf_ln(stderr, _("cannot update %s with %s"), ref_stash, commit);
+		return 1;
+	}
+
+	return do_store_stash(prefix, quiet, message, obj);
+}
+
+static int do_clear_stash(void)
+{
+	struct object_id obj;
+	if (get_sha1(ref_stash, obj.hash))
+		return 0;
+
+	return delete_ref(NULL, ref_stash, obj.hash, 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_clear_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+
+	if (argc != 0)
+		return error(_("git stash 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;
+	int nr_trees = 1;
+	struct tree_desc t[MAX_UNPACK_TREES];
+	struct tree *tree;
+
+	read_cache_preload(NULL);
+	if (refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL))
+		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_push_stash(const char *prefix, const char *message,
+	int keep_index, int include_untracked, int include_ignored, int patch,
+	const char **argv)
+{
+	int res;
+	struct stash_info info;
+
+	if (patch && include_untracked)
+		return error(_("can't use --patch and --include-untracked or --all at the same time"));
+
+	if (!include_untracked) {
+		struct child_process cp = CHILD_PROCESS_INIT;
+
+		cp.git_cmd = 1;
+		cp.no_stdout = 1;
+		argv_array_pushl(&cp.args, "ls-files", "--error-unmatch", "--", NULL);
+		if (argv)
+			argv_array_pushv(&cp.args, argv);
+		res = run_command(&cp);
+		if (res)
+			return 1;
+	}
+
+	read_cache_preload(NULL);
+	refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL);
+	if (check_no_changes(prefix, include_untracked, include_ignored, argv)) {
+		printf_ln(_("No local changes to save"));
+		return 0;
+	}
+
+	if (!reflog_exists(ref_stash)) {
+		if (do_clear_stash())
+			return error(_("Cannot initialize stash"));
+	}
+
+	if (do_create_stash(&info, prefix, message, include_untracked, include_ignored, patch, argv))
+		return 1;
+	res = do_store_stash(prefix, 1, info.message, info.w_commit);
+
+	if (res == 0 && !quiet)
+		printf(_("Saved working directory and index state %s"), info.message);
+
+	if (!patch) {
+		if (argv && *argv) {
+			struct argv_array args = ARGV_ARRAY_INIT;
+			struct argv_array args2 = ARGV_ARRAY_INIT;
+			struct child_process cp = CHILD_PROCESS_INIT;
+			struct strbuf out = STRBUF_INIT;
+			argv_array_pushl(&args, "reset", "--quiet", "--", NULL);
+			argv_array_pushv(&args, argv);
+			cmd_reset(args.argc, args.argv, prefix);
+
+			cp.git_cmd = 1;
+			argv_array_pushl(&cp.args, "ls-files", "-z", "--modified", "--",
+				NULL);
+			argv_array_pushv(&cp.args, argv);
+			pipe_command(&cp, NULL, 0, &out, 0, NULL, 0);
+
+			child_process_init(&cp);
+			cp.git_cmd = 1;
+			argv_array_pushl(&cp.args, "checkout-index", "-z", "--force",
+				"--stdin", NULL);
+			pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0);
+			strbuf_release(&out);
+
+			argv_array_pushl(&args2, "clean", "--force", "-d", "--quiet", "--",
+				NULL);
+			argv_array_pushv(&args2, argv);
+			cmd_clean(args2.argc, args2.argv, prefix);
+		} else {
+			struct argv_array args = ARGV_ARRAY_INIT;
+			argv_array_pushl(&args, "reset", "--hard", "--quiet", NULL);
+			cmd_reset(args.argc, args.argv, prefix);
+		}
+
+		if (include_untracked) {
+			struct argv_array args = ARGV_ARRAY_INIT;
+			argv_array_pushl(&args, "clean", "--force", "--quiet", "-d", NULL);
+			if (include_ignored)
+				argv_array_push(&args, "-x");
+			argv_array_push(&args, "--");
+			if (argv)
+				argv_array_pushv(&args, argv);
+			cmd_clean(args.argc, args.argv, prefix);
+		}
+
+		if (keep_index) {
+			struct child_process cp = CHILD_PROCESS_INIT;
+			struct strbuf out = STRBUF_INIT;
+
+			reset_tree(info.i_tree, 0, 1);
+
+			cp.git_cmd = 1;
+			argv_array_pushl(&cp.args, "ls-files", "-z", "--modified", "--",
+				NULL);
+			argv_array_pushv(&cp.args, argv);
+			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, "checkout-index", "-z", "--force",
+				"--stdin", NULL);
+			if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0))
+				return 1;
+			strbuf_release(&out);
+		}
+	} else {
+		struct child_process cp = CHILD_PROCESS_INIT;
+		cp.git_cmd = 1;
+		argv_array_pushl(&cp.args, "apply", "-R", NULL);
+		if (pipe_command(&cp, info.patch, strlen(info.patch), NULL, 0, NULL, 0))
+			return error(_("Cannot remove worktree changes"));
+
+		if (!keep_index) {
+			struct argv_array args = ARGV_ARRAY_INIT;
+			argv_array_pushl(&args, "reset", "--quiet", "--", NULL);
+			if (argv)
+				argv_array_pushv(&args, argv);
+			cmd_reset(args.argc, args.argv, prefix);
+		}
+	}
+
+	return 0;
+}
+
+static int push_stash(int argc, const char **argv, const char *prefix)
+{
+	const char *message = NULL;
+	int include_untracked = 0;
+	int include_ignored = 0;
+	int patch = 0;
+	int keep_index_set = -1;
+	int keep_index = 0;
+	struct option options[] = {
+		OPT_BOOL('u', "include-untracked", &include_untracked,
+			N_("stash untracked filed")),
+		OPT_BOOL('a', "all", &include_ignored,
+			N_("stash ignored untracked files")),
+		OPT_BOOL('k', "keep-index", &keep_index_set,
+			N_("restore the index after applying the stash")),
+		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
+		OPT_STRING('m', "message", &message, N_("message"),
+			N_("stash commit message")),
+		OPT_BOOL('p', "patch", &patch,
+			N_("edit current diff and apply")),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+				git_stash_save_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+
+	if (include_ignored)
+		include_untracked = 1;
+
+	if (keep_index_set != -1)
+		keep_index = keep_index_set;
+	else if (patch)
+		keep_index = 1;
+
+	return do_push_stash(prefix, message, keep_index, include_untracked, include_ignored, patch, argv);
+}
+
+static int save_stash(int argc, const char **argv, const char *prefix)
+{
+	const char *message = NULL;
+	int include_untracked = 0;
+	int include_ignored = 0;
+	int patch = 0;
+	int keep_index_set = -1;
+	int keep_index = 0;
+	int ret;
+	struct strbuf out = STRBUF_INIT;
+	struct option options[] = {
+		OPT_BOOL('u', "include-untracked", &include_untracked,
+			N_("stash untracked filed")),
+		OPT_BOOL('a', "all", &include_ignored,
+			N_("stash ignored untracked files")),
+		OPT_BOOL('k', "keep-index", &keep_index_set,
+			N_("restore the index after applying the stash")),
+		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
+		OPT_STRING('m', "message", &message, N_("message"),
+			N_("stash commit message")),
+		OPT_BOOL('p', "patch", &patch,
+			N_("edit current diff and apply")),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+			git_stash_save_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+
+	if (include_ignored)
+		include_untracked = 1;
+
+	if (keep_index_set != -1)
+		keep_index = keep_index_set;
+	else if (patch)
+		keep_index = 1;
+
+	if (argc != 0) {
+		int i;
+		for (i = 0; i < argc; ++i) {
+			if (i != 0)
+				strbuf_addf(&out, " ");
+			strbuf_addf(&out, "%s", argv[i]);
+		}
+		message = out.buf;
+	}
+
+	ret = do_push_stash(prefix, message, keep_index, include_untracked,
+			include_ignored, patch, NULL);
+	strbuf_release(&out);
+	return ret;
+}
+
+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;
+
+	read_cache_preload(NULL);
+	if (refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL))
+		return 1;
+
+	if (write_cache_as_tree(c_tree.hash, 0, NULL))
+		return 1;
+
+	if (index) {
+		if (hashcmp(info->b_tree.hash, info->i_tree.hash) == 0 || hashcmp(c_tree.hash, info->i_tree.hash) == 0) {
+			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 argv_array args = ARGV_ARRAY_INIT;
+		struct child_process cp = CHILD_PROCESS_INIT;
+		const char *index_file = get_index_file();
+
+		argv_array_push(&args, "read-tree");
+		argv_array_push(&args, sha1_to_hex(info->u_tree.hash));
+		argv_array_pushf(&args, "--index-output=%s", stash_index_path);
+
+		cp.git_cmd = 1;
+		argv_array_pushl(&cp.args, "checkout-index", "--all", NULL);
+		argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", stash_index_path);
+
+		if (cmd_read_tree(args.argc, args.argv, prefix) ||
+				run_command(&cp)) {
+			return error(_("Could not restore untracked files from stash"));
+		}
+		set_alternate_index_output(index_file);
+	}
+
+	init_merge_options(&o);
+
+	o.branch1 = "Updated upstream";
+	o.branch2 = "Stashed changes";
+
+	if (hashcmp(info->b_tree.hash, c_tree.hash) == 0)
+		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 (index) {
+		ret = reset_tree(index_tree, 0, 0);
+	} 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;
+
+		ret = reset_tree(c_tree, 0, 1);
+		if (ret)
+			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();
+		read_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_apply_usage, 0);
+
+	if (argc == 1) {
+		commit = argv[0];
+	}
+
+	if (get_stash_info(&info, commit))
+		return 1;
+
+	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 == 0) {
+		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_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 list_stash(int argc, const char **argv, const char *prefix)
+{
+	struct option options[] = {
+		OPT_END()
+	};
+
+	struct object_id obj;
+	struct argv_array args = ARGV_ARRAY_INIT;
+	int ret;
+
+	argc = parse_options(argc, argv, prefix, options,
+			git_stash_list_usage, PARSE_OPT_KEEP_UNKNOWN);
+
+	if (get_sha1(ref_stash, obj.hash))
+		return 0;
+
+	argv_array_pushl(&args, "log", "--format=%gd: %gs", "-g", "--first-parent", "-m", NULL);
+	argv_array_pushv(&args, argv);
+	argv_array_push(&args, ref_stash);
+	ret = cmd_log(args.argc, args.argv, prefix);
+	if (ret)
+		return 1;
+
+	return 0;
+}
+
+static int show_stash(int argc, const char **argv, const char *prefix)
+{
+	struct argv_array args = ARGV_ARRAY_INIT;
+	struct stash_info info;
+	const char *commit = NULL;
+	int numstat = 0;
+	int patch = 0;
+	int ret;
+
+	struct option options[] = {
+		OPT_BOOL(0, "numstat", &numstat,
+			N_("Shows number of added and deleted lines in decimal notation")),
+		OPT_BOOL('p', "patch", &patch,
+			N_("Generate patch")),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+			git_stash_show_usage, 0);
+
+	if (argc == 1)
+		commit = argv[0];
+
+	if (get_stash_info(&info, commit))
+		return 1;
+
+	argv_array_push(&args, "diff");
+	if (numstat)
+		argv_array_push(&args, "--numstat");
+	else if (patch)
+		argv_array_push(&args, "-p");
+	else
+		argv_array_push(&args, "--stat");
+
+	argv_array_push(&args, sha1_to_hex(info.b_commit.hash));
+	argv_array_push(&args, sha1_to_hex(info.w_commit.hash));
+	ret = cmd_diff(args.argc, args.argv, prefix);
+	return ret;
+}
+
+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_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;
+	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_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 == 0 && info.is_stash_ref)
+		ret = do_drop_stash(prefix, &info);
+
+	return ret;
+}
+
+int cmd_stash(int argc, const char **argv, const char *prefix)
+{
+	int result = 0;
+	pid_t pid = getpid();
+
+	struct option options[] = {
+		OPT_END()
+	};
+
+	git_config(git_default_config, NULL);
+
+	xsnprintf(stash_index_path, 64, ".git/index.stash.%d", pid);
+
+	argc = parse_options(argc, argv, prefix, options, git_stash_usage,
+		PARSE_OPT_KEEP_UNKNOWN|PARSE_OPT_KEEP_DASHDASH);
+
+	if (argc < 1) {
+		result = do_push_stash(NULL, prefix, 0, 0, 0, 0, NULL);
+	} else if (!strcmp(argv[0], "list"))
+		result = list_stash(argc, argv, prefix);
+	else if (!strcmp(argv[0], "show"))
+		result = show_stash(argc, argv, prefix);
+	else if (!strcmp(argv[0], "save"))
+		result = save_stash(argc, argv, prefix);
+	else if (!strcmp(argv[0], "push"))
+		result = push_stash(argc, argv, prefix);
+	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], "create"))
+		result = create_stash(argc, argv, prefix);
+	else if (!strcmp(argv[0], "store"))
+		result = store_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 {
+		if (argv[0][0] == '-') {
+			struct argv_array args = ARGV_ARRAY_INIT;
+			argv_array_push(&args, "push");
+			argv_array_pushv(&args, argv);
+			result = push_stash(args.argc, args.argv, prefix);
+			if (!result)
+				printf_ln(_("To restore them type \"git stash apply\""));
+		} else {
+			error(_("unknown subcommand: %s"), argv[0]);
+			result = 1;
+		}
+	}
+
+	return result;
+}
diff --git a/git-stash.sh b/contrib/examples/git-stash.sh
similarity index 100%
rename from git-stash.sh
rename to contrib/examples/git-stash.sh
diff --git a/git.c b/git.c
index 8ff44f081d..4531011cdc 100644
--- a/git.c
+++ b/git.c
@@ -491,6 +491,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", cmd_stash, 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.13.0

Re: [PATCH v4 0/5] Implement git stash as a builtin command

[Joel Teichroeb]

I haven't seen any response. Would it be possible for anyone to review?

Thanks,
Joel

On 6/7/2017 5:55 PM, Joel Teichroeb wrote:
> I've rewritten git stash as a builtin c command. All tests pass,
> and I've added two new tests. Test coverage is around 95% with the
> only things missing coverage being error handlers.
>
> Changes since v3:
>   * Fixed formatting issues
>   * Fixed a bug with stash branch and added a new test for it
>   * Fixed review comments
>
> Outstanding issue:
>   * Not all argv array memory is cleaned up
>
> Joel Teichroeb (5):
>    stash: add test for stash create with no files
>    stash: Add a test for when apply fails during stash branch
>    stash: add test for stashing in a detached state
>    merge: close the index lock when not writing the new index
>    stash: implement builtin stash
>
>   Makefile                                      |    2 +-
>   builtin.h                                     |    1 +
>   builtin/stash.c                               | 1224 +++++++++++++++++++++++++
>   git-stash.sh => contrib/examples/git-stash.sh |    0
>   git.c                                         |    1 +
>   merge-recursive.c                             |    9 +-
>   t/t3903-stash.sh                              |   34 +
>   7 files changed, 1267 insertions(+), 4 deletions(-)
>   create mode 100644 builtin/stash.c
>   rename git-stash.sh => contrib/examples/git-stash.sh (100%)
>

Re: [PATCH v4 5/5] stash: implement builtin stash

[Thomas Gummerer]

On 06/07, Joel Teichroeb wrote:
> Implement all git stash functionality as a builtin command
> 
> Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
> ---

Thanks for working on this.  A few comments from me below.  Mainly on
stash push, as that's what I'm most familiar with, and all I had time
for today.  Hope it helps :)

>  Makefile                                      |    2 +-
>  builtin.h                                     |    1 +
>  builtin/stash.c                               | 1224 +++++++++++++++++++++++++
>  git-stash.sh => contrib/examples/git-stash.sh |    0
>  git.c                                         |    1 +
>  5 files changed, 1227 insertions(+), 1 deletion(-)
>  create mode 100644 builtin/stash.c
>  rename git-stash.sh => contrib/examples/git-stash.sh (100%)

[...]

> 
> +
> +static const char * const git_stash_usage[] = {
> +	N_("git stash list [<options>]"),
> +	N_("git stash show [<stash>]"),
> +	N_("git stash drop [-q|--quiet] [<stash>]"),
> +	N_("git stash ( pop | apply ) [--index] [-q|--quiet] [<stash>]"),
> +	N_("git stash branch <branchname> [<stash>]"),
> +	N_("git stash [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]"),
> +	N_("                [-u|--include-untracked] [-a|--all] [<message>]]"),

This is missing the newly introduced push command.

> +	N_("git stash clear"),
> +	N_("git stash create [<message>]"),
> +	N_("git stash store [-m|--message <message>] [-q|--quiet] <commit>"),

create and store are not currently advertised in the usage.  I think
this is intentional, because those commands are intended to be used
only in scripts.  I don't have a particularly strong opinion on
whether they should be added or not, but if we do add them I think we
should do so consciously in a separate commit, instead of adding them
on in this commit.

> +	NULL
> +};
> +
> +static const char * const git_stash_list_usage[] = {
> +	N_("git stash list [<options>]"),
> +	NULL
> +};
> +
> +static const char * const git_stash_show_usage[] = {
> +	N_("git stash show [<stash>]"),
> +	NULL
> +};
> +
> +static const char * const git_stash_drop_usage[] = {
> +	N_("git stash drop [-q|--quiet] [<stash>]"),
> +	NULL
> +};
> +
> +static const char * const git_stash_pop_usage[] = {
> +	N_("git stash pop [--index] [-q|--quiet] [<stash>]"),
> +	NULL
> +};
> +
> +static const char * const git_stash_apply_usage[] = {
> +	N_("git stash apply [--index] [-q|--quiet] [<stash>]"),
> +	NULL
> +};
> +
> +static const char * const git_stash_branch_usage[] = {
> +	N_("git stash branch <branchname> [<stash>]"),
> +	NULL
> +};
> +
> +static const char * const git_stash_save_usage[] = {
> +	N_("git stash [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]"),
> +	N_("                [-u|--include-untracked] [-a|--all] [<message>]]"),
> +	NULL
> +};
> +
> +static const char * const git_stash_clear_usage[] = {
> +	N_("git stash clear"),
> +	NULL
> +};
> +
> +static const char * const git_stash_create_usage[] = {
> +	N_("git stash create [<message>]"),
> +	NULL
> +};
> +
> +static const char * const git_stash_store_usage[] = {
> +	N_("git stash store [-m|--message <message>] [-q|--quiet] <commit>"),
> +	NULL
> +};
> +

[...]

> +
> +static int do_push_stash(const char *prefix, const char *message,
> +	int keep_index, int include_untracked, int include_ignored, int patch,
> +	const char **argv)

argv here is a list of pathspecs.  I think this would be a bit easier
to follow if the argument was called "pathspecs".  

> +{
> +	int res;
> +	struct stash_info info;
> +
> +	if (patch && include_untracked)
> +		return error(_("can't use --patch and --include-untracked or --all at the same time"));
> +
> +	if (!include_untracked) {
> +		struct child_process cp = CHILD_PROCESS_INIT;
> +
> +		cp.git_cmd = 1;
> +		cp.no_stdout = 1;
> +		argv_array_pushl(&cp.args, "ls-files", "--error-unmatch", "--", NULL);
> +		if (argv)
> +			argv_array_pushv(&cp.args, argv);
> +		res = run_command(&cp);
> +		if (res)
> +			return 1;
> +	}
> +
> +	read_cache_preload(NULL);
> +	refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL);
> +	if (check_no_changes(prefix, include_untracked, include_ignored, argv)) {
> +		printf_ln(_("No local changes to save"));
> +		return 0;
> +	}
> +
> +	if (!reflog_exists(ref_stash)) {
> +		if (do_clear_stash())
> +			return error(_("Cannot initialize stash"));
> +	}
> +
> +	if (do_create_stash(&info, prefix, message, include_untracked, include_ignored, patch, argv))
> +		return 1;
> +	res = do_store_stash(prefix, 1, info.message, info.w_commit);
> +
> +	if (res == 0 && !quiet)

Sometimes the function is used directly in the if, and sometimes the
res variable is used.  I think it would be nicer to consistently use
one or the other.  My preference would be to always use the functions
directly, as res is not used anywhere other than the if.

Also I think we prefer using (!res) instead of (res == 0) for checking
return values.

> +		printf(_("Saved working directory and index state %s"), info.message);
> +
> +	if (!patch) {
> +		if (argv && *argv) {
> +			struct argv_array args = ARGV_ARRAY_INIT;
> +			struct argv_array args2 = ARGV_ARRAY_INIT;
> +			struct child_process cp = CHILD_PROCESS_INIT;
> +			struct strbuf out = STRBUF_INIT;
> +			argv_array_pushl(&args, "reset", "--quiet", "--", NULL);
> +			argv_array_pushv(&args, argv);
> +			cmd_reset(args.argc, args.argv, prefix);
> +
> +			cp.git_cmd = 1;
> +			argv_array_pushl(&cp.args, "ls-files", "-z", "--modified", "--",
> +				NULL);
> +			argv_array_pushv(&cp.args, argv);
> +			pipe_command(&cp, NULL, 0, &out, 0, NULL, 0);
> +
> +			child_process_init(&cp);
> +			cp.git_cmd = 1;
> +			argv_array_pushl(&cp.args, "checkout-index", "-z", "--force",
> +				"--stdin", NULL);
> +			pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0);
> +			strbuf_release(&out);
> +
> +			argv_array_pushl(&args2, "clean", "--force", "-d", "--quiet", "--",
> +				NULL);
> +			argv_array_pushv(&args2, argv);
> +			cmd_clean(args2.argc, args2.argv, prefix);
> +		} else {
> +			struct argv_array args = ARGV_ARRAY_INIT;
> +			argv_array_pushl(&args, "reset", "--hard", "--quiet", NULL);
> +			cmd_reset(args.argc, args.argv, prefix);
> +		}
> +
> +		if (include_untracked) {
> +			struct argv_array args = ARGV_ARRAY_INIT;
> +			argv_array_pushl(&args, "clean", "--force", "--quiet", "-d", NULL);
> +			if (include_ignored)
> +				argv_array_push(&args, "-x");
> +			argv_array_push(&args, "--");
> +			if (argv)
> +				argv_array_pushv(&args, argv);
> +			cmd_clean(args.argc, args.argv, prefix);
> +		}
> +
> +		if (keep_index) {
> +			struct child_process cp = CHILD_PROCESS_INIT;
> +			struct strbuf out = STRBUF_INIT;
> +
> +			reset_tree(info.i_tree, 0, 1);
> +
> +			cp.git_cmd = 1;
> +			argv_array_pushl(&cp.args, "ls-files", "-z", "--modified", "--",
> +				NULL);
> +			argv_array_pushv(&cp.args, argv);
> +			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, "checkout-index", "-z", "--force",
> +				"--stdin", NULL);
> +			if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0))
> +				return 1;
> +			strbuf_release(&out);
> +		}
> +	} else {
> +		struct child_process cp = CHILD_PROCESS_INIT;
> +		cp.git_cmd = 1;
> +		argv_array_pushl(&cp.args, "apply", "-R", NULL);
> +		if (pipe_command(&cp, info.patch, strlen(info.patch), NULL, 0, NULL, 0))
> +			return error(_("Cannot remove worktree changes"));
> +
> +		if (!keep_index) {
> +			struct argv_array args = ARGV_ARRAY_INIT;
> +			argv_array_pushl(&args, "reset", "--quiet", "--", NULL);
> +			if (argv)
> +				argv_array_pushv(&args, argv);
> +			cmd_reset(args.argc, args.argv, prefix);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int push_stash(int argc, const char **argv, const char *prefix)
> +{
> +	const char *message = NULL;
> +	int include_untracked = 0;
> +	int include_ignored = 0;
> +	int patch = 0;
> +	int keep_index_set = -1;
> +	int keep_index = 0;
> +	struct option options[] = {
> +		OPT_BOOL('u', "include-untracked", &include_untracked,
> +			N_("stash untracked filed")),
> +		OPT_BOOL('a', "all", &include_ignored,
> +			N_("stash ignored untracked files")),
> +		OPT_BOOL('k', "keep-index", &keep_index_set,
> +			N_("restore the index after applying the stash")),
> +		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
> +		OPT_STRING('m', "message", &message, N_("message"),
> +			N_("stash commit message")),
> +		OPT_BOOL('p', "patch", &patch,
> +			N_("edit current diff and apply")),
> +		OPT_END()
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options,
> +				git_stash_save_usage, PARSE_OPT_STOP_AT_NON_OPTION);

"git_stash_save_usage" is slightly different from the usage for
push, which we should display here.  We probably should introduce
"git_stash_push_usage" for this.

> +	if (include_ignored)
> +		include_untracked = 1;
> +
> +	if (keep_index_set != -1)
> +		keep_index = keep_index_set;
> +	else if (patch)
> +		keep_index = 1;
> +
> +	return do_push_stash(prefix, message, keep_index, include_untracked, include_ignored, patch, argv);
> +}
> +

[...]

> +
> +int cmd_stash(int argc, const char **argv, const char *prefix)
> +{
> +	int result = 0;
> +	pid_t pid = getpid();
> +
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +
> +	git_config(git_default_config, NULL);
> +
> +	xsnprintf(stash_index_path, 64, ".git/index.stash.%d", pid);
> +
> +	argc = parse_options(argc, argv, prefix, options, git_stash_usage,
> +		PARSE_OPT_KEEP_UNKNOWN|PARSE_OPT_KEEP_DASHDASH);
> +
> +	if (argc < 1) {
> +		result = do_push_stash(NULL, prefix, 0, 0, 0, 0, NULL);
> +	} else if (!strcmp(argv[0], "list"))
> +		result = list_stash(argc, argv, prefix);
> +	else if (!strcmp(argv[0], "show"))
> +		result = show_stash(argc, argv, prefix);
> +	else if (!strcmp(argv[0], "save"))
> +		result = save_stash(argc, argv, prefix);
> +	else if (!strcmp(argv[0], "push"))
> +		result = push_stash(argc, argv, prefix);
> +	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], "create"))
> +		result = create_stash(argc, argv, prefix);
> +	else if (!strcmp(argv[0], "store"))
> +		result = store_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 {
> +		if (argv[0][0] == '-') {
> +			struct argv_array args = ARGV_ARRAY_INIT;
> +			argv_array_push(&args, "push");
> +			argv_array_pushv(&args, argv);
> +			result = push_stash(args.argc, args.argv, prefix);

This is a bit of a change in behaviour to what we currently have.

The rules we decided on are as follows:

 - "git stash -p" is an alias for "git stash push -p".
 - "git stash" with only option arguments is an alias for "git stash
   push" with those same arguments.  non-option arguments can be
   specified after a "--" for disambiguation.

The above makes "git stash -*" a alias for "git stash push -*".  This
would result in a change of behaviour, for example in the case where
someone would use "git stash -this is a test-".  In that case the
current behaviour is to create a stash with the message "-this is a
test-", while the above would end up making git stash error out.  The
discussion on how we came up with those rules can be found at
http://public-inbox.org/git/20170206161432.zvpsqegjspaa2l5l@sigill.intra.peff.net/. 

> +			if (!result)
> +				printf_ln(_("To restore them type \"git stash apply\""));

In the shell script this is only displayed when the stash_push in the
case where git stash is invoked with no arguments, not in the push
case if I read this correctly.  So the two lines above should go in
the (argc < 1) case I think.

> +		} else {
> +			error(_("unknown subcommand: %s"), argv[0]);

Currently we're displaying the whole usage string in this case.  I
think we should keep doing that.

> +			result = 1;
> +		}
> +	}
> +
> +	return result;
> +}
> diff --git a/git-stash.sh b/contrib/examples/git-stash.sh
> similarity index 100%
> rename from git-stash.sh
> rename to contrib/examples/git-stash.sh
> diff --git a/git.c b/git.c
> index 8ff44f081d..4531011cdc 100644
> --- a/git.c
> +++ b/git.c
> @@ -491,6 +491,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", cmd_stash, 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.13.0
> 

Re: [PATCH v4 1/5] stash: add test for stash create with no files

[Junio C Hamano]

Joel Teichroeb <joel@teichroeb.net> writes:

> Ensure the command gives the correct return code

OK.  When you know what the correct return code is, we'd prefer to
see it spelled out, i.e.

    Ensure that the command succeeds.

Or did you mean that the command outputs nothing?

The test itself looks obviously correct ;-)

> Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
> ---
>  t/t3903-stash.sh | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 3b4bed5c9a..cc923e6335 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -444,6 +444,14 @@ test_expect_failure 'stash file to directory' '
>  	test foo = "$(cat file/file)"
>  '
>  
> +test_expect_success 'stash create - no changes' '
> +	git stash clear &&
> +	test_when_finished "git reset --hard HEAD" &&
> +	git reset --hard &&
> +	git stash create >actual &&
> +	test_must_be_empty actual
> +'
> +
>  test_expect_success 'stash branch - no stashes on stack, stash-like argument' '
>  	git stash clear &&
>  	test_when_finished "git reset --hard HEAD" &&

Re: [PATCH v4 2/5] stash: Add a test for when apply fails during stash branch

[Junio C Hamano]

Joel Teichroeb <joel@teichroeb.net> writes:

> If the return value of merge recurisve is not checked, the stash could end
> up being dropped even though it was not applied properly

s/recurisve/recursive/

> Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
> ---
>  t/t3903-stash.sh | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index cc923e6335..5399fb05ca 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -656,6 +656,20 @@ test_expect_success 'stash branch should not drop the stash if the branch exists
>  	git rev-parse stash@{0} --
>  '
>  
> +test_expect_success 'stash branch should not drop the stash if the apply fails' '
> +	git stash clear &&
> +	git reset HEAD~1 --hard &&
> +	echo foo >file &&
> +	git add file &&
> +	git commit -m initial &&

It's not quite intuitive to call a non-root commit "initial" ;-)

> +	echo bar >file &&
> +	git stash &&
> +	echo baz >file &&

OK, so 'file' has 'foo' in HEAD, 'bar' in the stash@{0}.

> +	test_when_finished "git checkout master" &&
> +	test_must_fail git stash branch new_branch stash@{0} &&

Hmph.  Do we blindly checkout new_branch out of stash@{0}^1 and
unstash, but because 'file' in the working tree is dirty, we fail to
apply the stash and stop?

This sounds like a bug to me.  Shouldn't we be staying on 'master',
and fail without even creating 'new_branch', when this happens?

In any case we should be testing what branch we are on after this
step.  What branch should we be on after "git stash branch" fails?

> +	git rev-parse stash@{0} --
> +'
> +
>  test_expect_success 'stash apply shows status same as git status (relative to current directory)' '
>  	git stash clear &&
>  	echo 1 >subdir/subfile1 &&

Re: [PATCH v4 3/5] stash: add test for stashing in a detached state

[Junio C Hamano]

Joel Teichroeb <joel@teichroeb.net> writes:

> Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
> ---
>  t/t3903-stash.sh | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 5399fb05ca..ce4c8fe3d6 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -822,6 +822,18 @@ test_expect_success 'create with multiple arguments for the message' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'create in a detached state' '
> +	test_when_finished "git checkout master" &&
> +	git checkout HEAD~1 &&
> +	>foo &&
> +	git add foo &&
> +	STASH_ID=$(git stash create) &&
> +	HEAD_ID=$(git rev-parse --short HEAD) &&
> +	echo "WIP on (no branch): ${HEAD_ID} initial" >expect &&
> +	git show --pretty=%s -s ${STASH_ID} >actual &&
> +	test_cmp expect actual
> +'

Hmph.  Is the title automatically given to the stash the
only/primary thing that is of interest to us in this test?  I think
we care more about that we record the right thing in the resulting
stash and also after creating the stash the working tree and the
index becomes clean.  Shouldn't we be testing that?

If "git stash create" fails to make the working tree and the index
clean, then "git checkout master" run by when-finished will carry
the local modifications with us, which probably is not what you
meant.  You'd need "reset --hard" there, too, perhaps?

>  test_expect_success 'stash -- <pathspec> stashes and restores the file' '
>  	>foo &&
>  	>bar &&

Re: [PATCH v4 4/5] merge: close the index lock when not writing the new index

[Junio C Hamano]

Joel Teichroeb <joel@teichroeb.net> writes:

> If the merge does not have anything to do, it does not unlock the index,
> causing any further index operations to fail. Thus, always unlock the index
> regardless of outcome.
>
> Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
> ---

This one makes sense.  

So far, nobody who calls this function performs further index
manipulations and letting the atexit handlers automatically release
the lock was sufficient.  This allows new callers to do more work on
the index after a merge finishes.


>  merge-recursive.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index ae5238d82c..16bb5512ef 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -2145,9 +2145,12 @@ int merge_recursive_generic(struct merge_options *o,
>  	if (clean < 0)
>  		return clean;
>  
> -	if (active_cache_changed &&
> -	    write_locked_index(&the_index, lock, COMMIT_LOCK))
> -		return err(o, _("Unable to write index."));
> +	if (active_cache_changed) {
> +		if (write_locked_index(&the_index, lock, COMMIT_LOCK))
> +			return err(o, _("Unable to write index."));
> +	} else {
> +		rollback_lock_file(lock);
> +	}
>  
>  	return clean ? 0 : 1;
>  }

Re: [PATCH v4 3/5] stash: add test for stashing in a detached state

[Joel Teichroeb]

On Tue, Jun 13, 2017 at 12:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Joel Teichroeb <joel@teichroeb.net> writes:
>
>> Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
>> ---
>>  t/t3903-stash.sh | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
>> index 5399fb05ca..ce4c8fe3d6 100755
>> --- a/t/t3903-stash.sh
>> +++ b/t/t3903-stash.sh
>> @@ -822,6 +822,18 @@ test_expect_success 'create with multiple arguments for the message' '
>>       test_cmp expect actual
>>  '
>>
>> +test_expect_success 'create in a detached state' '
>> +     test_when_finished "git checkout master" &&
>> +     git checkout HEAD~1 &&
>> +     >foo &&
>> +     git add foo &&
>> +     STASH_ID=$(git stash create) &&
>> +     HEAD_ID=$(git rev-parse --short HEAD) &&
>> +     echo "WIP on (no branch): ${HEAD_ID} initial" >expect &&
>> +     git show --pretty=%s -s ${STASH_ID} >actual &&
>> +     test_cmp expect actual
>> +'
>
> Hmph.  Is the title automatically given to the stash the
> only/primary thing that is of interest to us in this test?  I think
> we care more about that we record the right thing in the resulting
> stash and also after creating the stash the working tree and the
> index becomes clean.  Shouldn't we be testing that?

In this case, the title is really what I wanted to test. There are
other tests already to make sure that stash create works, but there
were no tests to ensure that a stash was created with the correct
title when not on a branch. That being said though, I'll add more
validation as more validation is always better.

>
> If "git stash create" fails to make the working tree and the index
> clean, then "git checkout master" run by when-finished will carry
> the local modifications with us, which probably is not what you
> meant.  You'd need "reset --hard" there, too, perhaps?

Agreed.

>
>>  test_expect_success 'stash -- <pathspec> stashes and restores the file' '
>>       >foo &&
>>       >bar &&

Re: [PATCH v4 2/5] stash: Add a test for when apply fails during stash branch

[Joel Teichroeb]

On Tue, Jun 13, 2017 at 12:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Joel Teichroeb <joel@teichroeb.net> writes:
>
>> If the return value of merge recurisve is not checked, the stash could end
>> up being dropped even though it was not applied properly
>
> s/recurisve/recursive/
>
>> Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
>> ---
>>  t/t3903-stash.sh | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
>> index cc923e6335..5399fb05ca 100755
>> --- a/t/t3903-stash.sh
>> +++ b/t/t3903-stash.sh
>> @@ -656,6 +656,20 @@ test_expect_success 'stash branch should not drop the stash if the branch exists
>>       git rev-parse stash@{0} --
>>  '
>>
>> +test_expect_success 'stash branch should not drop the stash if the apply fails' '
>> +     git stash clear &&
>> +     git reset HEAD~1 --hard &&
>> +     echo foo >file &&
>> +     git add file &&
>> +     git commit -m initial &&
>
> It's not quite intuitive to call a non-root commit "initial" ;-)
>
>> +     echo bar >file &&
>> +     git stash &&
>> +     echo baz >file &&
>
> OK, so 'file' has 'foo' in HEAD, 'bar' in the stash@{0}.
>
>> +     test_when_finished "git checkout master" &&
>> +     test_must_fail git stash branch new_branch stash@{0} &&
>
> Hmph.  Do we blindly checkout new_branch out of stash@{0}^1 and
> unstash, but because 'file' in the working tree is dirty, we fail to
> apply the stash and stop?
>
> This sounds like a bug to me.  Shouldn't we be staying on 'master',
> and fail without even creating 'new_branch', when this happens?

Good point. The existing behavior is to create new_branch and check it
out. I'm not sure what the correct state should be then. Create
new_branch, checkout new_branch, fail to apply, checkout master?
Should it then delete new_branch? Is there a way instead to test
applying the stash before creating the branch without actually
applying it? Something like putting merge_recursive into some kind of
dry-run mode?

>
> In any case we should be testing what branch we are on after this
> step.  What branch should we be on after "git stash branch" fails?
>
>> +     git rev-parse stash@{0} --
>> +'
>> +
>>  test_expect_success 'stash apply shows status same as git status (relative to current directory)' '
>>       git stash clear &&
>>       echo 1 >subdir/subfile1 &&

Re: [PATCH v4 3/5] stash: add test for stashing in a detached state

[Junio C Hamano]

Joel Teichroeb <joel@teichroeb.net> writes:

>>> +test_expect_success 'create in a detached state' '
>>> +     test_when_finished "git checkout master" &&
>>> +     git checkout HEAD~1 &&
>>> +     >foo &&
>>> +     git add foo &&
>>> +     STASH_ID=$(git stash create) &&
>>> +     HEAD_ID=$(git rev-parse --short HEAD) &&
>>> +     echo "WIP on (no branch): ${HEAD_ID} initial" >expect &&
>>> +     git show --pretty=%s -s ${STASH_ID} >actual &&
>>> +     test_cmp expect actual
>>> +'
>>
>> Hmph.  Is the title automatically given to the stash the
>> only/primary thing that is of interest to us in this test?  I think
>> we care more about that we record the right thing in the resulting
>> stash and also after creating the stash the working tree and the
>> index becomes clean.  Shouldn't we be testing that?
>
> In this case, the title is really what I wanted to test. There are
> other tests already to make sure that stash create works, but there
> were no tests to ensure that a stash was created with the correct
> title when not on a branch.

Ah, OK.

Thanks.

Re: [PATCH v4 5/5] stash: implement builtin stash

[Junio C Hamano]

Joel Teichroeb <joel@teichroeb.net> writes:

> diff --git a/builtin/stash.c b/builtin/stash.c
> new file mode 100644
> index 0000000000..a9680f2909
> --- /dev/null
> +++ b/builtin/stash.c
> ...
> +static const char *ref_stash = "refs/stash";
> +static int quiet = 0;

Let BSS take care of zero-initialization, i.e. drop " = 0".

> +static int untracked_files(struct strbuf *out, int include_untracked,
> +		int include_ignored, const char **argv)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	cp.git_cmd = 1;
> +	argv_array_pushl(&cp.args, "ls-files", "-o", "-z", NULL);
> +	if (include_untracked && !include_ignored)
> +		argv_array_push(&cp.args, "--exclude-standard");
> +	argv_array_push(&cp.args, "--");
> +	if (argv)
> +		argv_array_pushv(&cp.args, argv);
> +	return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
> +}

Seeing that include_untracked and include_ignored always come in a
pair throughout the program, I wondered if it may be better to use
a single "unsigned include" with two bits

    #define INCLUDE_UNTRACKED 01
    #define INCLUDE_IGNORED 02

to pass around.  As long as we envision that we will not gain other
kind of "do we include X?" in the future, what your patch does is
fine, I would say.

> +static int check_no_changes(const char *prefix, int include_untracked,
> +		int include_ignored, const char **argv)
> +{
> +	struct argv_array args1 = ARGV_ARRAY_INIT;
> +	struct argv_array args2 = ARGV_ARRAY_INIT;
> +	struct strbuf out = STRBUF_INIT;
> +	int ret;
> +
> +	argv_array_pushl(&args1, "diff-index", "--quiet", "--cached", "HEAD",
> +		"--ignore-submodules", "--", NULL);
> +	if (argv)
> +		argv_array_pushv(&args1, argv);
> +
> +	argv_array_pushl(&args2, "diff-files", "--quiet", "--ignore-submodules",
> +		"--", NULL);
> +	if (argv)
> +		argv_array_pushv(&args2, argv);
> +
> +	if (include_untracked)
> +		untracked_files(&out, include_untracked, include_ignored, argv);
> +
> +	ret = cmd_diff_index(args1.argc, args1.argv, prefix) == 0 &&
> +			cmd_diff_files(args2.argc, args2.argv, prefix) == 0 &&
> +			(!include_untracked || out.len == 0);

When diff_index() finds there are modified paths, you do not have to
call diff_files() or untracked_files() at all (and you do not even
have to set-up args2).  Doesn't the above leak args.argv[] when &&
short circuits?

    This is a tangent, but it is somewhat unusual to call cmd_foo()
    as a subroutine.  I think cmd_diff_*() are written reasonably
    well to allow them to be called in this way safely, and there
    are a few existing commands that already do so, so it may be OK.

> +	strbuf_release(&out);
> +	return ret;
> +}
> +
> +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 (strlen(commit) < 3) {

This is a bit sloppy (the original is even sloppier but it is in
shell, so it is more excusable ;-).  This code thinks that anything
with @{<num>} must be longer than 3 because it has to have @{},
and that is where the strlen() comes from, I think, but the magic
number 3 appears without explanation here.

What the code actually needs to do is to see if the stash entry
specification came in "commit" (which by the way is a bit misnamed
parameter) is a bare number and use refs/stash@{<that number>} only
in that case, I think.  strspn() might be useful.

> +		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);
> +	strbuf_addf(&u_tree_rev, "%s^3:", revision);
> +
> +	ret = (get_sha1(w_commit_rev.buf, info->w_commit.hash) == 0 &&
> +		get_sha1(b_commit_rev.buf, info->b_commit.hash) == 0 &&
> +		get_sha1(w_tree_rev.buf, info->w_tree.hash) == 0 &&
> +		get_sha1(b_tree_rev.buf, info->b_tree.hash) == 0 &&
> +		get_sha1(i_tree_rev.buf, info->i_tree.hash) == 0);

It's more conventional to check for errors with !get_sha1(params),
not a long-hand comparision with 0.

> +	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 are leaking u_tree_rev.buf upon early return.

> +	info->has_u = get_sha1(u_tree_rev.buf, info->u_tree.hash) == 0;
> +
> +	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) == 0;

Favor !strncmp(params) over comparision with 0.

Style: Have SP around both sides of binary operator "-".

> +	strbuf_release(&out);
> +	return !ret;

Hmph, where did we last assign ret in this code?  Didn't we check
that value and returned error already, which means we know what !ret
is when the control reaches here?

> +}

I have to move to another building, so I'll stop here for now, but
will continue later.

Thanks.

Re: [PATCH v4 5/5] stash: implement builtin stash

[Junio C Hamano]

Joel Teichroeb <joel@teichroeb.net> writes:

> +static void stash_create_callback(struct diff_queue_struct *q,
> +				struct diff_options *opt, void *cbdata)
> +{
> +	int i;
> +
> +	for (i = 0; i < q->nr; i++) {
> +		struct diff_filepair *p = q->queue[i];
> +		const char *path = p->one->path;
> +		struct stat st;

The order is somewhat ugly.  Move "struct stat st;" that does not
have any initialization at the beginning.

> +		remove_file_from_index(&the_index, path);
> +		if (!lstat(path, &st))
> +			add_to_index(&the_index, path, &st, 0);
> +	}
> +}

So this will be called with list of paths that are different from
the working tree and the index, and adds all the paths the index
knows about to the index from the working tree?  Sounds OK, but I am
not sure if that is "stash_create_callback()".  Surely it is _part_
of creating a stash, but it would be better to name it to reflect
which part of creating a stash this helper is about.  I think this
is about recording the working tree state, so I would have expected
"record" and/or "working_tree" in its name.

> +/*
> + * Untracked files are stored by themselves in a parentless commit, for
> + * ease of unpacking later.
> + */
> +static int save_untracked(struct stash_info *info, const char *message,
> +		int include_untracked, int include_ignored, const char **argv)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	struct strbuf out = STRBUF_INIT;
> +	struct object_id orig_tree;
> +	int ret;
> +	const char *index_file = get_index_file();
> +
> +	set_alternate_index_output(stash_index_path);
> +	untracked_files(&out, include_untracked, include_ignored, argv);
> +
> +	cp.git_cmd = 1;
> +	argv_array_pushl(&cp.args, "update-index", "-z", "--add", "--remove",
> +		"--stdin", NULL);
> +	argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", stash_index_path);
> +
> +	if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0)) {
> +		strbuf_release(&out);
> +		return 1;
> +	}
> +

OK, that's a very straight-forward way of doing this, and as we do
not care too much about performance in this initial conversion to C,
it is even sensible.  In a later update after this patch lands, you
may want to use dir.c's fill_directory() API to find the untracked
files and add them yourself internally, without running ls-files (in
untracked_files()) or update-index (here) as subprocesses, but that
is in the future.  Let's get this round finished.

> +	strbuf_reset(&out);
> +
> +	discard_cache();
> +	read_cache_from(stash_index_path);
> +
> +	write_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0,NULL);

SP before "NULL".

> +	discard_cache();
> +
> +	read_cache_from(stash_index_path);

Hmph, what did anybody change in the on-disk stash_index (or
contents in the_index) since you read_cache_from()?

> +	write_cache_as_tree(info->u_tree.hash, 0, NULL);

Then you write exactly the same index contents again, this time to
info->u_tree here.  I am not sure why you need to do this twice, and
I do not see how orig_tree.hash you wrote earlier is used?

> +	strbuf_addf(&out, "untracked files on %s", message);
> +
> +	ret = commit_tree(out.buf, out.len, info->u_tree.hash, NULL,
> +			info->u_commit.hash, NULL, NULL);
> +	strbuf_release(&out);
> +	if (ret)
> +		return 1;
> +
> +	set_alternate_index_output(index_file);
> +	discard_cache();
> +	read_cache();
> +
> +	return 0;
> +}

OK, except for minor nits, this seems to correctly replicate what
u_commit=$(...) does in create_stash shell function in the original.

> +static int save_working_tree(struct stash_info *info, const char *prefix,
> +		const char **argv)
> +{
> +	struct object_id orig_tree;
> +	struct rev_info rev;
> +	int nr_trees = 1;
> +	struct tree_desc t[MAX_UNPACK_TREES];
> +	struct tree *tree;
> +	struct unpack_trees_options opts;
> +	struct object *obj;
> +
> +	discard_cache();
> +	tree = parse_tree_indirect(&info->i_tree);
> +	prime_cache_tree(&the_index, tree);
> +	write_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0, NULL);
> +	discard_cache();

Hmph, the caller of this function did read_cache(), refresh_index(),
and write_cache_as_tree(), and the result is in info->i_tree.
The above sequence discards, reads that tree into the index and
writes the same tree again.  Which seems like a huge no-op.  IIUC,
the write_cache_as_tree() the caller already did should have already 
primed the cache-tree structure, too.  These five lines are puzzling.

> +	read_cache_from(stash_index_path);

Hmph, it is unclear who wrote what state to this $TMPindex from this
codeflow.  Do you really want to read from there?  I am guessing
that this part corresponds to w_tree=$( ... ) in create_stash shell
function, which does "read-tree --index-output=$TMPindex -m $i_tree"
starting from the real $GIT_DIR/index and the call to unpack_tree()
that follows here is that "read-tree".

A one-way "read-tree -m" is purely a performance measure and the
resulting index will have the entries in $i_tree no matter what
index contents you start from, so you may not have seen an incorrect
result per-se, but I suspect that you do not want to be reading from
$TMPindex here.  Puzzled...

> +
> +	memset(&opts, 0, sizeof(opts));
> +
> +	parse_tree(tree);
> +
> +	opts.head_idx = 1;
> +	opts.src_index = &the_index;
> +	opts.dst_index = &the_index;
> +	opts.merge = 1;
> +	opts.fn = oneway_merge;
> +
> +	init_tree_desc(t, tree->buffer, tree->size);
> +
> +	if (unpack_trees(nr_trees, t, &opts))
> +		return 1;
> +
> +	init_revisions(&rev, prefix);
> +	setup_revisions(0, NULL, &rev, NULL);
> +	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
> +	rev.diffopt.format_callback = stash_create_callback;
> +	DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
> +
> +	parse_pathspec(&rev.prune_data, 0, 0, prefix, argv);
> +
> +	diff_setup_done(&rev.diffopt);
> +	obj = parse_object(&info->b_commit);
> +	add_pending_object(&rev, obj, "");
> +	if (run_diff_index(&rev, 0))
> +		return 1;
> +
> +	if (write_cache_as_tree(info->w_tree.hash, 0, NULL))
> +		return 1;
> +
> +	discard_cache();
> +	read_cache();
> +
> +	return 0;
> +}

This part otherwise looks like a correct way to grab changes to the
working tree into w_tree.

Again, I need to stop here for now.  Will continue later.

Re: [PATCH v4 5/5] stash: implement builtin stash

[Johannes Schindelin]

Hi Junio,

On Fri, 16 Jun 2017, Junio C Hamano wrote:

> Joel Teichroeb <joel@teichroeb.net> writes:
> 
> > +static void stash_create_callback(struct diff_queue_struct *q,
> > +				struct diff_options *opt, void *cbdata)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < q->nr; i++) {
> > +		struct diff_filepair *p = q->queue[i];
> > +		const char *path = p->one->path;
> > +		struct stat st;
> 
> The order is somewhat ugly.  Move "struct stat st;" that does not
> have any initialization at the beginning.

Let's not call it "ugly". You may find it ugly, but maybe you may want to
avoid contributors feeling judged negatively, either.

Instead, let's say that it is preferred in Git's source code to declare
uninitialized variables first, and then declare variables which are
initialized at the same time.

This convention, however, would need to be documented in CodingGuidelines
first. We do not want to make contributors feel dumb now, do we?

In this particular case, I also wonder whether it is worth the time to
point out an unwritten (and not always obeyed) rule. The variable block is
small enough that it does not matter much in which order the variables are
declared.

However, trying to be very strict even in such a small matter may well
cost us contributors (and it is dubious whether the most critical parts of
our technical debt has anything to do with small code style issues similar
to this one). It's not like our bar of entry to new contributors is very
low, exactly...

And if you disagree with this assessment, you should point out the same
issues in literally all of my patches, as I always put initialized
variables first, uninitialized last.

> > +	strbuf_reset(&out);
> > +
> > +	discard_cache();
> > +	read_cache_from(stash_index_path);
> > +
> > +	write_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0,NULL);
> 
> SP before "NULL".

If only we had automated source code formatting, saving us from these
distractions during patch review.

The rest of the review, modulo all the "Hmpf"s, seems helpful enough that
I will try to find time to review the next iteration of this patch series
(with a fresh mind, as I only skimmed the previous iteration) instead of
adding my comments here.

Ciao,
Dscho

Re: [PATCH v4 5/5] stash: implement builtin stash

[Jeff King]

On Mon, Jun 19, 2017 at 03:16:48PM +0200, Johannes Schindelin wrote:

> And if you disagree with this assessment, you should point out the same
> issues in literally all of my patches, as I always put initialized
> variables first, uninitialized last.

Yeah, I am scratching my head here. If we do have a convention for
ordering, I'd have thought it is that (and I'd probably put statics even
above initialized variables).

-Peff

Re: [PATCH v4 5/5] stash: implement builtin stash

[Joel Teichroeb]

On Fri, Jun 16, 2017 at 3:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Joel Teichroeb <joel@teichroeb.net> writes:
>> +/*
>> + * Untracked files are stored by themselves in a parentless commit, for
>> + * ease of unpacking later.
>> + */
>> +static int save_untracked(struct stash_info *info, const char *message,
>> +             int include_untracked, int include_ignored, const char **argv)
>> +{
>> +     struct child_process cp = CHILD_PROCESS_INIT;
>> +     struct strbuf out = STRBUF_INIT;
>> +     struct object_id orig_tree;
>> +     int ret;
>> +     const char *index_file = get_index_file();
>> +
>> +     set_alternate_index_output(stash_index_path);
>> +     untracked_files(&out, include_untracked, include_ignored, argv);
>> +
>> +     cp.git_cmd = 1;
>> +     argv_array_pushl(&cp.args, "update-index", "-z", "--add", "--remove",
>> +             "--stdin", NULL);
>> +     argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", stash_index_path);
>> +
>> +     if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0)) {
>> +             strbuf_release(&out);
>> +             return 1;
>> +     }
>> +
>
> OK, that's a very straight-forward way of doing this, and as we do
> not care too much about performance in this initial conversion to C,
> it is even sensible.  In a later update after this patch lands, you
> may want to use dir.c's fill_directory() API to find the untracked
> files and add them yourself internally, without running ls-files (in
> untracked_files()) or update-index (here) as subprocesses, but that
> is in the future.  Let's get this round finished.
>
>> +     strbuf_reset(&out);
>> +
>> +     discard_cache();
>> +     read_cache_from(stash_index_path);
>> +
>> +     write_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0,NULL);
>
> SP before "NULL".
>
>> +     discard_cache();
>> +
>> +     read_cache_from(stash_index_path);
>
> Hmph, what did anybody change in the on-disk stash_index (or
> contents in the_index) since you read_cache_from()?
>
>> +     write_cache_as_tree(info->u_tree.hash, 0, NULL);
>
> Then you write exactly the same index contents again, this time to
> info->u_tree here.  I am not sure why you need to do this twice, and
> I do not see how orig_tree.hash you wrote earlier is used?
>

I'm not sure I understand what's happening here either. When I was
writing this, it was essentially a lot of trial and error in order to
get the index handling correct. Getting rid of any single one of these
lines makes the test fail. At some point I'd like to redo all the
index handling parts here, as I think I can do without an additional
index, but I'd need to make sure the error handling is perfect first.

Re: [PATCH v4 5/5] stash: implement builtin stash

[Joel Teichroeb]

On Sun, Jun 11, 2017 at 2:27 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
>> +
>> +int cmd_stash(int argc, const char **argv, const char *prefix)
>> +{
>> +     int result = 0;
>> +     pid_t pid = getpid();
>> +
>> +     struct option options[] = {
>> +             OPT_END()
>> +     };
>> +
>> +     git_config(git_default_config, NULL);
>> +
>> +     xsnprintf(stash_index_path, 64, ".git/index.stash.%d", pid);
>> +
>> +     argc = parse_options(argc, argv, prefix, options, git_stash_usage,
>> +             PARSE_OPT_KEEP_UNKNOWN|PARSE_OPT_KEEP_DASHDASH);
>> +
>> +     if (argc < 1) {
>> +             result = do_push_stash(NULL, prefix, 0, 0, 0, 0, NULL);
>> +     } else if (!strcmp(argv[0], "list"))
>> +             result = list_stash(argc, argv, prefix);
>> +     else if (!strcmp(argv[0], "show"))
>> +             result = show_stash(argc, argv, prefix);
>> +     else if (!strcmp(argv[0], "save"))
>> +             result = save_stash(argc, argv, prefix);
>> +     else if (!strcmp(argv[0], "push"))
>> +             result = push_stash(argc, argv, prefix);
>> +     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], "create"))
>> +             result = create_stash(argc, argv, prefix);
>> +     else if (!strcmp(argv[0], "store"))
>> +             result = store_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 {
>> +             if (argv[0][0] == '-') {
>> +                     struct argv_array args = ARGV_ARRAY_INIT;
>> +                     argv_array_push(&args, "push");
>> +                     argv_array_pushv(&args, argv);
>> +                     result = push_stash(args.argc, args.argv, prefix);
>
> This is a bit of a change in behaviour to what we currently have.
>
> The rules we decided on are as follows:
>
>  - "git stash -p" is an alias for "git stash push -p".
>  - "git stash" with only option arguments is an alias for "git stash
>    push" with those same arguments.  non-option arguments can be
>    specified after a "--" for disambiguation.
>
> The above makes "git stash -*" a alias for "git stash push -*".  This
> would result in a change of behaviour, for example in the case where
> someone would use "git stash -this is a test-".  In that case the
> current behaviour is to create a stash with the message "-this is a
> test-", while the above would end up making git stash error out.  The
> discussion on how we came up with those rules can be found at
> http://public-inbox.org/git/20170206161432.zvpsqegjspaa2l5l@sigill.intra.peff.net/.

I don't really like the "argv[0][0] == '-'" logic, but it doesn't seem
to have the flaw you pointed out:
$ ./git stash -this is a test-
error: unknown switch `t'
usage: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
[...]

I'm not sure this is the best possible error message, but it's just as
useful as the message from the old version.

>
>> +                     if (!result)
>> +                             printf_ln(_("To restore them type \"git stash apply\""));
>
> In the shell script this is only displayed when the stash_push in the
> case where git stash is invoked with no arguments, not in the push
> case if I read this correctly.  So the two lines above should go in
> the (argc < 1) case I think.

I think it's correct as is. One of the tests checks for this string to
be output, and if I move the line, the test fails.



I agreed with all the other points you raised, and they will be fixed
in my next revision.

Re: [PATCH v4 5/5] stash: implement builtin stash

[Junio C Hamano]

Joel Teichroeb <joel@teichroeb.net> writes:

> +static int patch_working_tree(struct stash_info *info, const char *prefix,
> +		const char **argv)
> +{
> +	struct argv_array args = ARGV_ARRAY_INIT;
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	struct strbuf out = STRBUF_INIT;
> +	size_t unused;
> +	const char *index_file = get_index_file();
> +
> +	argv_array_pushl(&args, "read-tree", "HEAD", NULL);
> +	argv_array_pushf(&args, "--index-output=%s", stash_index_path);
> +	cmd_read_tree(args.argc, args.argv, prefix);

I do not think if cmd_read_tree() is prepared to be called like
this, and even if it happens to be OK, I do not think we should rely
on it.  

In general, cmd_foo() that implements subcommand "foo" expects only
to be called from main(), and expects that the calling main() will
exit with its return status.  This has implications that you, who
abuse a cmd_foo() function as if it is a reusable helper function,
need to watch out for.  For example, cmd_foo() may use static global
variables in builtin/foo.c that are initialized in a certain way
before it starts, so calling cmd_foo() twice may not work correctly
(the first invocation may change these variables and they won't be
reset when it returns).  cmd_foo() can and do leave resources
unreclaimed, because it expects the calling main() to exit
immediately, leaving descriptors it creates open or chunks of memory
it allocates unfreed.  cmd_foo() also can and do die() without
returning the control to the caller (this last item does not make
much difference to this particular codepath, as you'd end up dying
soon if this read-tree fails anyway, but in general you'd want to
give a more specific error message when it happens, i.e. instead of
an error message cmd_read_tree() internally gives, you want to say
"Cannot save the current worktree state").

> +
> +	cp.git_cmd = 1;
> +	argv_array_pushl(&cp.args, "add--interactive", "--patch=stash", "--", NULL);
> +	argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", stash_index_path);
> +	if (run_command(&cp))
> +		return 1;

Unlike the above direct call to cmd_read_tree(), the way this
invokes "git add--interactive" is kosher.  By returning non-zero (by
the way, the prevailing convention in this codebase is to return
negative for an error), you give a chance to the caller of this
helper function to say "Cannot save the current worktree state".

> +	discard_cache();
> +	read_cache_from(stash_index_path);
> +
> +	if (write_cache_as_tree(info->w_tree.hash, 0, NULL))
> +		return 1;

OK.

> +	child_process_init(&cp);
> +	cp.git_cmd = 1;
> +	argv_array_pushl(&cp.args, "diff-tree", "-p", "HEAD", NULL);
> +	argv_array_push(&cp.args, sha1_to_hex(info->w_tree.hash));
> +	argv_array_push(&cp.args, "--");
> +	if (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0) || out.len == 0)
> +		return 1;

This "diff-tree" call is also reasonable.  Instead of getting the
patch text into a temporary file (which is what the original did),
we slurp it into a strbuf "out", and pass it out to the caller by
storing in info->patch.  OK.

> +	info->patch = strbuf_detach(&out, &unused);

You can pass NULL instead of a throw-away variable &unused.

> +
> +	set_alternate_index_output(index_file);
> +	discard_cache();
> +	read_cache();
> +
> +	return 0;
> +}

So this looks fairly faithful rewrite to C of a half of the
create_stash shell function (we already reviewed the other half done
in your save_working_tree() function).

> +static int do_create_stash(struct stash_info *info, const char *prefix,
> +		const char *message, int include_untracked, int include_ignored,
> +		int patch, const char **argv)
> +{
> +	struct object_id curr_head;
> +	char *branch_path = NULL;
> +	const char *branch_name = NULL;
> +	struct commit_list *parents = NULL;
> +	struct strbuf out_message = STRBUF_INIT;
> +	struct strbuf out = STRBUF_INIT;
> +	struct pretty_print_context ctx = {0};
> +
> +	struct commit *c = NULL;
> +	const char *hash;
> +
> +	read_cache_preload(NULL);
> +	refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL);
> +	if (check_no_changes(prefix, include_untracked, include_ignored, argv))
> +		return 1;

We find there is nothing to stash, so we tell the caller that fact
by returning 1.  The caller can tell that this is a "different kind
of success" and is not an error by checking the sign of the return
value.

> +	if (get_sha1_tree("HEAD", info->b_commit.hash))
> +		return error(_("You do not have the initial commit yet"));

And this is an error from the caller's point of view (error()
returns -1).

> +	branch_path = resolve_refdup("HEAD", 0, curr_head.hash, NULL);
> +
> +	if (branch_path == NULL || strcmp(branch_path, "HEAD") == 0)
> +		branch_name = "(no branch)";
> +	else
> +		skip_prefix(branch_path, "refs/heads/", &branch_name);
> +
> +	c = lookup_commit(&info->b_commit);
> +
> +	ctx.output_encoding = get_log_output_encoding();
> +	ctx.abbrev = 1;
> +	ctx.fmt = CMIT_FMT_ONELINE;
> +	hash = find_unique_abbrev(c->object.oid.hash, DEFAULT_ABBREV);
> +
> +	strbuf_addf(&out_message, "%s: %s ", branch_name, hash);
> +
> +	pretty_print_commit(&ctx, c, &out_message);
> +
> +	strbuf_addf(&out, "index on %s\n", out_message.buf);

OK, the above roughly correspond to "# state of the base commit"
part of the original.  This message is created with

	msg=$(printf '%s: %s' "$branch" "$head")

and your "branch_name" is computed to be the same as $branch, and
$head in the original is head=$(git rev-list --oneline -n 1 HEAD--)
which is your "hash" with the oneline.  The whole $msg corresponds
to your "out_message.buf".  Looks correct.

> +	commit_list_insert(lookup_commit(&info->b_commit), &parents);
> +
> +	if (write_cache_as_tree(info->i_tree.hash, 0, NULL))
> +		return error(_("git write-tree failed to write a tree"));

Shouldn't the user also see "Cannot save the current index state"
message in this case?  A failure by write-tree is an implementation
detail and the latter is what matters more to the end user.

> +	if (commit_tree(out.buf, out.len, info->i_tree.hash, parents, info->i_commit.hash, NULL, NULL))
> +		return error(_("Cannot save the current index state"));
> +
> +	strbuf_reset(&out);
> +
> +	if (include_untracked) {
> +		if (save_untracked(info, out_message.buf, include_untracked, include_ignored, argv))
> +			return error(_("Cannot save the untracked files"));
> +	}
> +
> +	if (patch) {
> +		if (patch_working_tree(info, prefix, argv))
> +			return error(_("Cannot save the current worktree state"));
> +	} else {
> +		if (save_working_tree(info, prefix, argv))
> +			return error(_("Cannot save the current worktree state"));
> +	}
> +	parents = NULL;

The elements on the parents list here are leaked here, by early
returns we see above and also at the end of this function.

> +	if (include_untracked)
> +		commit_list_insert(lookup_commit(&info->u_commit), &parents);
> +
> +	commit_list_insert(lookup_commit(&info->i_commit), &parents);
> +	commit_list_insert(lookup_commit(&info->b_commit), &parents);
> +
> +	if (message != NULL && strlen(message) != 0)
> +		strbuf_addf(&out, "On %s: %s\n", branch_name, message);
> +	else
> +		strbuf_addf(&out, "WIP on %s\n", out_message.buf);
> +
> +	if (commit_tree(out.buf, out.len, info->w_tree.hash, parents, info->w_commit.hash, NULL, NULL))
> +		return error(_("Cannot record working tree state"));
> +
> +	info->message = out.buf;
> +
> +	strbuf_release(&out_message);
> +	free(branch_path);
> +
> +	return 0;
> +}

> +static int create_stash(int argc, const char **argv, const char *prefix)
> +{
> +	int include_untracked = 0;
> +	const char *message = NULL;
> +	struct stash_info info;
> +	int ret;
> +	struct strbuf out = STRBUF_INIT;
> +	struct option options[] = {
> +		OPT_BOOL('u', "include-untracked", &include_untracked,
> +			N_("stash untracked filed")),
> +		OPT_STRING('m', "message", &message, N_("message"),
> +			N_("stash commit message")),
> +		OPT_END()
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options,
> +			git_stash_create_usage, 0);
> +
> +	if (argc != 0) {
> +		int i;
> +		for (i = 0; i < argc; ++i) {
> +			if (i != 0) {
> +				strbuf_addf(&out, " ");
> +			}

Style tips:
    Do not enclose a single statement inside a block.  
    We prefer "if (i)" over "if (i != 0)".
    We prefer "if (!i)" over "if (i == 0)".
    We prefer "i++" over "++i", UNLESS you use the value before increment.

> +			strbuf_addf(&out, "%s", argv[i]);
> +		}
> +		message = out.buf;
> +	}
> +
> +	ret = do_create_stash(&info, prefix, message, include_untracked, 0, 0, NULL);
> +
> +	strbuf_release(&out);

I do not see a need for "message" variable; you can just pass
out.buf to do_create_stash().

> +
> +	if (ret)
> +		return 0;
> +
> +	printf("%s\n", sha1_to_hex(info.w_commit.hash));
> +	return 0;
> +}

Re: [PATCH v4 5/5] stash: implement builtin stash

[Junio C Hamano]

Joel Teichroeb <joel@teichroeb.net> writes:

> On Fri, Jun 16, 2017 at 3:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> Then you write exactly the same index contents again, this time to
>> info->u_tree here.  I am not sure why you need to do this twice, and
>> I do not see how orig_tree.hash you wrote earlier is used?
>
> I'm not sure I understand what's happening here either. When I was
> writing this, it was essentially a lot of trial and error in order to
> get the index handling correct....

Thanks for being honest.  I agree that we do not want to say "we do
not yet know the exact mechanism how X happens, but X does happen"
for any value of X (in this case "the code happens to do the same
thing as the original").  In biology or physics experiments, that
may be how science advances, but it is different when it comes for
us to explain our own code ;-).  After all, its our creation.

I haven't followed the big picture in your codepath, but if you had
something like this, I can see how you need a seemingly unneeded
reading of the index:

    function A
	discard and read index
	do A's thing

    function B
	discard and read index
	do B's thing

    function C
	discard and read index
	if (some condition)
		do things that involves smudging the index
		call A
	else
		call B

    function D
	read index
	if (some other condition)
		call A
	else
		do things that involves smudging the index
		call B

That is, when the division of labor for preparing the in-core index
is not very well defined between the caller and the callee.  When
function C calls function B, the index is unnecessarily discarded
and read at the beginning of function B, but if you remove it
without changing anything else, the call to it from function D would
break.  One way to fix it would be to make the two helpers work from
the given in-core index, iow, make their callers responsible for
preparing the in-core index to desired state, i.e.

    function A
	do A's thing

    function B
	do B's thing

    function C
	discard and read index
	if (some condition)
		do things that involves smudging the index
		discard and read index
		call A
	else
		call B

    function D
	read index
	if (some other condition)
		call A
	else
		do things that involves smudging the index
                discard and read index
		call B

Again, I didn't follow the big picture callpath in your patch, so
the above may not be why your extra read-index calls are needed, and
I do not know which of your functions correspond to A, B, C and D in
the above illustration.  But I think you get the idea.

Re: [PATCH v4 5/5] stash: implement builtin stash

[Thomas Gummerer]

On 06/19, Joel Teichroeb wrote:
> On Sun, Jun 11, 2017 at 2:27 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
> >> +
> >> +int cmd_stash(int argc, const char **argv, const char *prefix)
> >> +{
> >> +     int result = 0;
> >> +     pid_t pid = getpid();
> >> +
> >> +     struct option options[] = {
> >> +             OPT_END()
> >> +     };
> >> +
> >> +     git_config(git_default_config, NULL);
> >> +
> >> +     xsnprintf(stash_index_path, 64, ".git/index.stash.%d", pid);
> >> +
> >> +     argc = parse_options(argc, argv, prefix, options, git_stash_usage,
> >> +             PARSE_OPT_KEEP_UNKNOWN|PARSE_OPT_KEEP_DASHDASH);
> >> +
> >> +     if (argc < 1) {
> >> +             result = do_push_stash(NULL, prefix, 0, 0, 0, 0, NULL);
> >> +     } else if (!strcmp(argv[0], "list"))
> >> +             result = list_stash(argc, argv, prefix);
> >> +     else if (!strcmp(argv[0], "show"))
> >> +             result = show_stash(argc, argv, prefix);
> >> +     else if (!strcmp(argv[0], "save"))
> >> +             result = save_stash(argc, argv, prefix);
> >> +     else if (!strcmp(argv[0], "push"))
> >> +             result = push_stash(argc, argv, prefix);
> >> +     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], "create"))
> >> +             result = create_stash(argc, argv, prefix);
> >> +     else if (!strcmp(argv[0], "store"))
> >> +             result = store_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 {
> >> +             if (argv[0][0] == '-') {
> >> +                     struct argv_array args = ARGV_ARRAY_INIT;
> >> +                     argv_array_push(&args, "push");
> >> +                     argv_array_pushv(&args, argv);
> >> +                     result = push_stash(args.argc, args.argv, prefix);
> >
> > This is a bit of a change in behaviour to what we currently have.
> >
> > The rules we decided on are as follows:
> >
> >  - "git stash -p" is an alias for "git stash push -p".
> >  - "git stash" with only option arguments is an alias for "git stash
> >    push" with those same arguments.  non-option arguments can be
> >    specified after a "--" for disambiguation.
> >
> > The above makes "git stash -*" a alias for "git stash push -*".  This
> > would result in a change of behaviour, for example in the case where
> > someone would use "git stash -this is a test-".  In that case the
> > current behaviour is to create a stash with the message "-this is a
> > test-", while the above would end up making git stash error out.  The
> > discussion on how we came up with those rules can be found at
> > http://public-inbox.org/git/20170206161432.zvpsqegjspaa2l5l@sigill.intra.peff.net/.
> 
> I don't really like the "argv[0][0] == '-'" logic, but it doesn't seem
> to have the flaw you pointed out:
> $ ./git stash -this is a test-
> error: unknown switch `t'
> usage: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
> [...]

I just went through the thread again, to remind myself why we did it
this way.  The example I had above was the wrong example, sorry.  The
message at [1] explains it better.  Essentially by implementing the
rules I mentioned we wanted to avoid the potential confusion of what
does 'git stash -q drop' mean.  Before the rewrite this fails and
shows the usage.  After the rewrite this would try to stash everything
matching the pathspec drop, which might be confusing for users.  

[1]: http://public-inbox.org/git/20170213214521.pkjesijdlus36tnp@sigill.intra.peff.net/

> I'm not sure this is the best possible error message, but it's just as
> useful as the message from the old version.
> 
> >
> >> +                     if (!result)
> >> +                             printf_ln(_("To restore them type \"git stash apply\""));
> >
> > In the shell script this is only displayed when the stash_push in the
> > case where git stash is invoked with no arguments, not in the push
> > case if I read this correctly.  So the two lines above should go in
> > the (argc < 1) case I think.
> 
> I think it's correct as is. One of the tests checks for this string to
> be output, and if I move the line, the test fails.

Right, that test that fails only when the "To restore..." string is
printed to stdout.  So moving the "printf_ln()" to the line you did
only makes sure it's not printed there.  Reading the code again and
trying to trigger this print in the shell script stash makes me think
this is not even possible to trigger there anymore.

After the line

test -n "$seen_non_option" || set "push" "$@"

it's not possible that $# is 0 anymore, so this will never be
printed.  From a quick look at the history it seems like it wasn't
possible to trigger that codepath for a while.  If I'm reading things
correctly 3c2eb80fe3 ("stash: simplify defaulting to "save" and reject
unknown options", 2009-08-18) seems to have introduced the small
change in behaviour.   As I don't think anyone has complained since
then, I'd just leave it as is, which makes git stash with no options a
little less verbose.  [Adding Matthieu to cc as author of the above
mentioned commit]

> I agreed with all the other points you raised, and they will be fixed
> in my next revision.

Thanks!

Re: [PATCH v4 5/5] stash: implement builtin stash

[Matthieu Moy]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> After the line
>
> test -n "$seen_non_option" || set "push" "$@"
>
> it's not possible that $# is 0 anymore, so this will never be
> printed.  From a quick look at the history it seems like it wasn't
> possible to trigger that codepath for a while.  If I'm reading things
> correctly 3c2eb80fe3 ("stash: simplify defaulting to "save" and reject
> unknown options", 2009-08-18) seems to have introduced the small
> change in behaviour.

Indeed. That wasn't on purpose, but I seem to have turned this

	case $# in
	0)
		push_stash &&
		say "$(gettext "(To restore them type \"git stash apply\")")"
		;;

into dead code.

> As I don't think anyone has complained since then, I'd just leave it
> as is, which makes git stash with no options a little less verbose.

I agree it's OK to keep is as-is, but the original logic (give a bit
more advice when "stash push" was DWIM-ed) made sense too, so it can
make sense to re-activate it while porting to C.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v4 5/5] stash: implement builtin stash

[Thomas Gummerer]

On Mon, Jun 26, 2017 at 8:53 AM Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
>
> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > After the line
> >
> > test -n "$seen_non_option" || set "push" "$@"
> >
> > it's not possible that $# is 0 anymore, so this will never be
> > printed.  From a quick look at the history it seems like it wasn't
> > possible to trigger that codepath for a while.  If I'm reading things
> > correctly 3c2eb80fe3 ("stash: simplify defaulting to "save" and reject
> > unknown options", 2009-08-18) seems to have introduced the small
> > change in behaviour.
>
> Indeed. That wasn't on purpose, but I seem to have turned this
>
>         case $# in
>         0)
>                 push_stash &&
>                 say "$(gettext "(To restore them type \"git stash apply\")")"
>                 ;;
>
> into dead code.
>
> > As I don't think anyone has complained since then, I'd just leave it
> > as is, which makes git stash with no options a little less verbose.
>
> I agree it's OK to keep is as-is, but the original logic (give a bit
> more advice when "stash push" was DWIM-ed) made sense too, so it can
> make sense to re-activate it while porting to C.

I'd be happy either way.  If we decide to restore the original behaviour, I
think it should be done in a separate patch, as the test case will need
some adjustments.  That way we can keep this patch purely as the
conversion step, which makes it a bit easier to review.

> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/