[PATCH v3 0/4] Implement git stash as a builtin command

[PATCH v3 0/4] 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.

Joel Teichroeb (4):
  stash: add test for stash create with no files
  stash: add test for stashing in a detached state
  close the index lock when not writing the new index
  stash: implement builtin stash

 Makefile                                      |    2 +-
 builtin.h                                     |    1 +
 builtin/add.c                                 |    3 +-
 builtin/mv.c                                  |    8 +-
 builtin/rm.c                                  |    3 +-
 builtin/stash.c                               | 1280 +++++++++++++++++++++++++
 git-stash.sh => contrib/examples/git-stash.sh |    0
 git.c                                         |    1 +
 merge-recursive.c                             |    8 +-
 t/t3903-stash.sh                              |   19 +
 10 files changed, 1316 insertions(+), 9 deletions(-)
 create mode 100644 builtin/stash.c
 rename git-stash.sh => contrib/examples/git-stash.sh (100%)

-- 
2.13.0

[PATCH v3 1/4] 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..aaae221304 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 $(cat actual | wc -l) -eq 0
+'
+
 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 v3 2/4] stash: add test for stashing in a detached state

[Joel Teichroeb]

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

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index aaae221304..b86851ef46 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -808,6 +808,17 @@ 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) &&
+	echo "WIP on (no branch): 47d5e0e 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 v3 3/4] close the index lock when not writing the new index

[Joel Teichroeb]

Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
---
 builtin/add.c     | 3 ++-
 builtin/mv.c      | 8 +++++---
 builtin/rm.c      | 3 ++-
 merge-recursive.c | 8 +++++---
 4 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 9f53f020d0..6b04eb2c71 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -461,7 +461,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	if (active_cache_changed) {
 		if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
 			die(_("Unable to write new index file"));
-	}
+	} else
+		rollback_lock_file(&lock_file);
 
 	return exit_status;
 }
diff --git a/builtin/mv.c b/builtin/mv.c
index 61d20037ad..ccf21de17f 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -293,9 +293,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	if (gitmodules_modified)
 		stage_updated_gitmodules();
 
-	if (active_cache_changed &&
-	    write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
-		die(_("Unable to write new index file"));
+	if (active_cache_changed) {
+		if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
+			die(_("Unable to write new index file"));
+	} else
+		rollback_lock_file(&lock_file);
 
 	return 0;
 }
diff --git a/builtin/rm.c b/builtin/rm.c
index fb79dcab18..4c7a91888b 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -389,7 +389,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 	if (active_cache_changed) {
 		if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
 			die(_("Unable to write new index file"));
-	}
+	} else
+		rollback_lock_file(&lock_file);
 
 	return 0;
 }
diff --git a/merge-recursive.c b/merge-recursive.c
index 62decd51cc..db841c0d38 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2145,9 +2145,11 @@ 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 v3 4/4] 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                               | 1280 +++++++++++++++++++++++++
 git-stash.sh => contrib/examples/git-stash.sh |    0
 git.c                                         |    1 +
 5 files changed, 1283 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 e35542e631..4af4ac41c7 100644
--- a/Makefile
+++ b/Makefile
@@ -523,7 +523,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
 
@@ -961,6 +960,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 9e4a89816d..16e8a437d4 100644
--- a/builtin.h
+++ b/builtin.h
@@ -121,6 +121,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..bf36ff8f9b
--- /dev/null
+++ b/builtin/stash.c
@@ -0,0 +1,1280 @@
+#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 *REV;
+	int is_stash_ref;
+	int has_u;
+	const char *patch;
+};
+
+int untracked_files(struct strbuf *out, int include_untracked,
+		const char **argv)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	cp.git_cmd = 1;
+	argv_array_push(&cp.args, "ls-files");
+	argv_array_push(&cp.args, "-o");
+	argv_array_push(&cp.args, "-z");
+	if (include_untracked != 2)
+		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,
+		const char **argv)
+{
+	struct argv_array args1;
+	struct argv_array args2;
+	struct strbuf out = STRBUF_INIT;
+
+	argv_array_init(&args1);
+	argv_array_push(&args1, "diff-index");
+	argv_array_push(&args1, "--quiet");
+	argv_array_push(&args1, "--cached");
+	argv_array_push(&args1, "HEAD");
+	argv_array_push(&args1, "--ignore-submodules");
+	argv_array_push(&args1, "--");
+	if (argv)
+		argv_array_pushv(&args1, argv);
+	argv_array_init(&args2);
+	argv_array_push(&args2, "diff-files");
+	argv_array_push(&args2, "--quiet");
+	argv_array_push(&args2, "--ignore-submodules");
+	argv_array_push(&args2, "--");
+	if (argv)
+		argv_array_pushv(&args2, argv);
+	if (include_untracked)
+		untracked_files(&out, include_untracked, argv);
+	return cmd_diff_index(args1.argc, args1.argv, prefix) == 0 &&
+			cmd_diff_files(args2.argc, args2.argv, prefix) == 0 &&
+			(!include_untracked || out.len == 0);
+}
+
+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 i_commit_rev = STRBUF_INIT;
+	struct strbuf u_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;
+	struct object_context unused;
+	char *str;
+	int ret;
+	const char *REV = commit;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	info->is_stash_ref = 0;
+
+	if (commit == NULL) {
+		strbuf_addf(&commit_buf, "%s@{0}", ref_stash);
+		REV = commit_buf.buf;
+	} else if (strlen(commit) < 3) {
+		strbuf_addf(&commit_buf, "%s@{%s}", ref_stash, commit);
+		REV = commit_buf.buf;
+	}
+	info->REV = REV;
+
+	strbuf_addf(&w_commit_rev, "%s", REV);
+	strbuf_addf(&b_commit_rev, "%s^1", REV);
+	strbuf_addf(&i_commit_rev, "%s^2", REV);
+	strbuf_addf(&u_commit_rev, "%s^3", REV);
+	strbuf_addf(&w_tree_rev, "%s:", REV);
+	strbuf_addf(&b_tree_rev, "%s^1:", REV);
+	strbuf_addf(&i_tree_rev, "%s^2:", REV);
+	strbuf_addf(&u_tree_rev, "%s^3:", REV);
+
+
+	ret = (
+		get_sha1_with_context(w_commit_rev.buf, 0, info->w_commit.hash, &unused) == 0 &&
+		get_sha1_with_context(b_commit_rev.buf, 0, info->b_commit.hash, &unused) == 0 &&
+		get_sha1_with_context(i_commit_rev.buf, 0, info->i_commit.hash, &unused) == 0 &&
+		get_sha1_with_context(w_tree_rev.buf, 0, info->w_tree.hash, &unused) == 0 &&
+		get_sha1_with_context(b_tree_rev.buf, 0, info->b_tree.hash, &unused) == 0 &&
+		get_sha1_with_context(i_tree_rev.buf, 0, info->i_tree.hash, &unused) == 0);
+
+	if (!ret) {
+		fprintf_ln(stderr, _("%s is not a valid reference"), REV);
+		return 1;
+	}
+
+
+	info->has_u = get_sha1_with_context(u_commit_rev.buf, 0, info->u_commit.hash, &unused) == 0 &&
+		get_sha1_with_context(u_tree_rev.buf, 0, info->u_tree.hash, &unused) == 0;
+
+
+	/* TODO: Improve this logic */
+	strbuf_addf(&symbolic, "%s", REV);
+	str = strstr(symbolic.buf, "@");
+	if (str)
+		str[0] = 0;
+	cp.git_cmd = 1;
+	argv_array_push(&cp.args, "rev-parse");
+	argv_array_push(&cp.args, "--symbolic-full-name");
+	argv_array_pushf(&cp.args, "%s", symbolic.buf);
+	pipe_command(&cp, NULL, 0, &out, 0, NULL, 0);
+	out.buf[out.len-1] = 0;
+
+	info->is_stash_ref = strcmp(out.buf, ref_stash) == 0;
+
+	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, struct strbuf *out,
+		int include_untracked, const char **argv)
+{
+	struct child_process cp2 = CHILD_PROCESS_INIT;
+	struct strbuf out3 = STRBUF_INIT;
+	struct strbuf out4 = STRBUF_INIT;
+	struct object_id orig_tree;
+
+	set_alternate_index_output(stash_index_path);
+	untracked_files(&out4, include_untracked, argv);
+
+	cp2.git_cmd = 1;
+	argv_array_push(&cp2.args, "update-index");
+	argv_array_push(&cp2.args, "-z");
+	argv_array_push(&cp2.args, "--add");
+	argv_array_push(&cp2.args, "--remove");
+	argv_array_push(&cp2.args, "--stdin");
+	argv_array_pushf(&cp2.env_array, "GIT_INDEX_FILE=%s", stash_index_path);
+
+	if (pipe_command(&cp2, out4.buf, out4.len, NULL, 0, NULL, 0))
+		return 1;
+
+	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(&out3, "untracked files on %s", out->buf);
+
+	if (commit_tree(out3.buf, out3.len, info->u_tree.hash, NULL, info->u_commit.hash, NULL, NULL))
+		return 1;
+
+	set_alternate_index_output(".git/index");
+	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.hash);
+	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.hash);
+	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)
+{
+	const char *stash_index_path = ".git/foocache2";
+	struct argv_array args;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct child_process cp2 = CHILD_PROCESS_INIT;
+	struct strbuf out = STRBUF_INIT;
+
+	argv_array_init(&args);
+	argv_array_push(&args, "read-tree");
+	argv_array_push(&args, "HEAD");
+	argv_array_pushf(&args, "--index-output=%s", stash_index_path);
+	cmd_read_tree(args.argc, args.argv, prefix);
+
+	cp2.git_cmd = 1;
+	argv_array_push(&cp2.args, "add--interactive");
+	argv_array_push(&cp2.args, "--patch=stash");
+	argv_array_push(&cp2.args, "--");
+	argv_array_pushf(&cp2.env_array, "GIT_INDEX_FILE=%s", stash_index_path);
+	if (run_command(&cp2))
+		return 1;
+
+	discard_cache();
+	read_cache_from(stash_index_path);
+
+	if (write_cache_as_tree(info->w_tree.hash, 0, NULL))
+		return 1;
+
+	cp.git_cmd = 1;
+	argv_array_push(&cp.args, "diff-tree");
+	argv_array_push(&cp.args, "-p");
+	argv_array_push(&cp.args, "HEAD");
+	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 = out.buf;
+
+	set_alternate_index_output(".git/index");
+	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 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 = STRBUF_INIT;
+	struct strbuf out3 = STRBUF_INIT;
+	struct pretty_print_context ctx = {0};
+
+	struct commit *c = NULL;
+	const char *hash;
+	struct strbuf out2 = STRBUF_INIT;
+
+	read_cache_preload(NULL);
+	refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL);
+	if (check_no_changes(prefix, include_untracked, argv))
+		return 1;
+
+	if (get_sha1_tree("HEAD", info->b_commit.hash))
+		die(_("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.hash);
+
+	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, "%s: %s ", branch_name, hash);
+
+	pretty_print_commit(&ctx, c, &out);
+
+	strbuf_addf(&out3, "index on %s\n", out.buf);
+
+	commit_list_insert(lookup_commit(info->b_commit.hash), &parents);
+
+	if (write_cache_as_tree(info->i_tree.hash, 0, NULL))
+		die(_("git write-tree failed to write a tree"));
+
+	if (commit_tree(out3.buf, out3.len, info->i_tree.hash, parents, info->i_commit.hash, NULL, NULL))
+		die(_("Cannot save the current index state"));
+
+
+	if (include_untracked) {
+		if (save_untracked(info, &out, include_untracked, argv))
+			die(_("Cannot save the untracked files"));
+	}
+
+
+	if (patch) {
+		if (patch_working_tree(info, prefix, argv))
+			die(_("Cannot save the current worktree state"));
+	} else {
+		if (save_working_tree(info, prefix, argv))
+			die(_("Cannot save the current worktree state"));
+	}
+	parents = NULL;
+
+	if (include_untracked) {
+		commit_list_insert(lookup_commit(info->u_commit.hash), &parents);
+	}
+
+	commit_list_insert(lookup_commit(info->i_commit.hash), &parents);
+	commit_list_insert(lookup_commit(info->b_commit.hash), &parents);
+
+	if (message != NULL && strlen(message) != 0)
+		strbuf_addf(&out2, "On %s: %s\n", branch_name, message);
+	else
+		strbuf_addf(&out2, "WIP on %s\n", out.buf);
+
+	if (commit_tree(out2.buf, out2.len, info->w_tree.hash, parents, info->w_commit.hash, NULL, NULL))
+		die(_("Cannot record working tree state"));
+
+	info->message = out2.buf;
+
+	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;
+	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) {
+		struct strbuf out = STRBUF_INIT;
+		int i;
+		for (i = 0; i < argc; ++i) {
+			if (i != 0) {
+				strbuf_addf(&out, " ");
+			}
+			strbuf_addf(&out, "%s", argv[i]);
+		}
+		message = out.buf;
+	}
+
+	if (do_create_stash(&info, prefix, message, include_untracked, 0, NULL))
+		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)
+		die(_("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 = NULL;
+	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 (message == NULL)
+		message = "Create via \"git stash store\".";
+
+	if (argc != 1)
+		die(_("\"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;
+	struct object_context unused;
+	if (get_sha1_with_context(ref_stash, 0, obj.hash, &unused))
+		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)
+		die(_("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.hash);
+	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 patch, const char **argv)
+{
+	int result;
+	struct stash_info info;
+
+	if (patch && include_untracked) {
+		fprintf_ln(stderr, _("can't use --patch and --include-untracked or --all at the same time"));
+		return 1;
+	}
+
+	if (!include_untracked) {
+		struct child_process cp = CHILD_PROCESS_INIT;
+		struct strbuf out = STRBUF_INIT;
+
+		cp.git_cmd = 1;
+		argv_array_push(&cp.args, "ls-files");
+		argv_array_push(&cp.args, "--error-unmatch");
+		argv_array_push(&cp.args, "--");
+		if (argv)
+			argv_array_pushv(&cp.args, argv);
+		result = pipe_command(&cp, NULL, 0, &out, 0, NULL, 0);
+		if (result)
+			return 1;
+	}
+
+	read_cache_preload(NULL);
+	refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL);
+	if (check_no_changes(prefix, include_untracked, argv)) {
+		printf(_("No local changes to save\n"));
+		return 0;
+	}
+
+	if (!reflog_exists(ref_stash)) {
+		if (do_clear_stash())
+			die(_("Cannot initialize stash"));
+	}
+
+
+	do_create_stash(&info, prefix, message, include_untracked, patch, argv);
+	result = do_store_stash(prefix, 1, info.message, info.w_commit);
+
+	if (result == 0 && !quiet) {
+		printf(_("Saved working directory and index state %s"), info.message);
+	}
+
+	if (!patch) {
+		if (argv && *argv) {
+			struct argv_array args;
+			struct child_process cp = CHILD_PROCESS_INIT;
+			struct child_process cp2 = CHILD_PROCESS_INIT;
+			struct strbuf out = STRBUF_INIT;
+			argv_array_init(&args);
+			argv_array_push(&args, "reset");
+			argv_array_push(&args, "--quiet");
+			argv_array_push(&args, "--");
+			argv_array_pushv(&args, argv);
+			cmd_reset(args.argc, args.argv, prefix);
+
+
+			cp.git_cmd = 1;
+			argv_array_push(&cp.args, "ls-files");
+			argv_array_push(&cp.args, "-z");
+			argv_array_push(&cp.args, "--modified");
+			argv_array_push(&cp.args, "--");
+			argv_array_pushv(&cp.args, argv);
+			pipe_command(&cp, NULL, 0, &out, 0, NULL, 0);
+
+			cp2.git_cmd = 1;
+			argv_array_push(&cp2.args, "checkout-index");
+			argv_array_push(&cp2.args, "-z");
+			argv_array_push(&cp2.args, "--force");
+			argv_array_push(&cp2.args, "--stdin");
+			pipe_command(&cp2, out.buf, out.len, NULL, 0, NULL, 0);
+
+			argv_array_init(&args);
+			argv_array_push(&args, "clean");
+			argv_array_push(&args, "--force");
+			argv_array_push(&args, "-d");
+			argv_array_push(&args, "--quiet");
+			argv_array_push(&args, "--");
+			argv_array_pushv(&args, argv);
+			cmd_clean(args.argc, args.argv, prefix);
+		} else {
+			struct argv_array args;
+			argv_array_init(&args);
+			argv_array_push(&args, "reset");
+			argv_array_push(&args, "--hard");
+			argv_array_push(&args, "--quiet");
+			cmd_reset(args.argc, args.argv, prefix);
+		}
+
+		if (include_untracked) {
+			struct argv_array args;
+			argv_array_init(&args);
+			argv_array_push(&args, "clean");
+			argv_array_push(&args, "--force");
+			argv_array_push(&args, "--quiet");
+			argv_array_push(&args, "-d");
+			if (include_untracked == 2)
+				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 child_process cp2 = CHILD_PROCESS_INIT;
+			struct strbuf out = STRBUF_INIT;
+
+			reset_tree(info.i_tree, 0, 1);
+
+			cp.git_cmd = 1;
+			argv_array_push(&cp.args, "ls-files");
+			argv_array_push(&cp.args, "-z");
+			argv_array_push(&cp.args, "--modified");
+			argv_array_push(&cp.args, "--");
+			argv_array_pushv(&cp.args, argv);
+			pipe_command(&cp, NULL, 0, &out, 0, NULL, 0);
+
+			cp2.git_cmd = 1;
+			argv_array_push(&cp2.args, "checkout-index");
+			argv_array_push(&cp2.args, "-z");
+			argv_array_push(&cp2.args, "--force");
+			argv_array_push(&cp2.args, "--stdin");
+			pipe_command(&cp2, out.buf, out.len, NULL, 0, NULL, 0);
+		}
+	} else {
+		struct child_process cp2 = CHILD_PROCESS_INIT;
+		cp2.git_cmd = 1;
+		argv_array_push(&cp2.args, "apply");
+		argv_array_push(&cp2.args, "-R");
+		if (pipe_command(&cp2, info.patch, strlen(info.patch), NULL, 0, NULL, 0))
+			die(_("Cannot remove worktree changes"));
+
+		if (!keep_index) {
+			struct argv_array args;
+			argv_array_init(&args);
+			argv_array_push(&args, "reset");
+			argv_array_push(&args, "--quiet");
+			argv_array_push(&args, "--");
+			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 all = 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", &all,
+			 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 (all) {
+		include_untracked = 2;
+	}
+
+	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, patch, argv);
+}
+
+static int save_stash(int argc, const char **argv, const char *prefix)
+{
+	const char *message = NULL;
+	int include_untracked = 0;
+	int all = 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", &all,
+			 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 (all) {
+		include_untracked = 2;
+	}
+
+	if (keep_index_set != -1) {
+		keep_index = keep_index_set;
+	} else if (patch) {
+		keep_index = 1;
+	}
+
+	if (argc != 0) {
+		struct strbuf out = STRBUF_INIT;
+		int i;
+		for (i = 0; i < argc; ++i) {
+			if (i != 0) {
+				strbuf_addf(&out, " ");
+			}
+			strbuf_addf(&out, "%s", argv[i]);
+		}
+		message = out.buf;
+	}
+
+	return do_push_stash(prefix, message, keep_index, include_untracked, patch, NULL);
+}
+
+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 child_process cp2 = CHILD_PROCESS_INIT;
+			struct strbuf out = STRBUF_INIT;
+			struct argv_array args;
+			cp.git_cmd = 1;
+			argv_array_push(&cp.args, "diff-tree");
+			argv_array_push(&cp.args, "--binary");
+			argv_array_pushf(&cp.args, "%s^2^..%s^2", sha1_to_hex(info->w_commit.hash), sha1_to_hex(info->w_commit.hash));
+			pipe_command(&cp, NULL, 0, &out, 0, NULL, 0);
+
+			cp2.git_cmd = 1;
+			argv_array_push(&cp2.args, "apply");
+			argv_array_push(&cp2.args, "--cached");
+			pipe_command(&cp2, out.buf, out.len, NULL, 0, NULL, 0);
+
+			discard_cache();
+			read_cache();
+			if (write_cache_as_tree(index_tree.hash, 0, NULL))
+				return 1;
+
+			argv_array_init(&args);
+			argv_array_push(&args, "reset");
+			cmd_reset(args.argc, args.argv, prefix);
+		}
+	}
+
+	if (info->has_u) {
+		struct argv_array args;
+		struct child_process cp2 = CHILD_PROCESS_INIT;
+
+		argv_array_init(&args);
+		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);
+
+
+		cp2.git_cmd = 1;
+		argv_array_push(&cp2.args, "checkout-index");
+		argv_array_push(&cp2.args, "--all");
+		argv_array_pushf(&cp2.env_array, "GIT_INDEX_FILE=%s", stash_index_path);
+
+		if (cmd_read_tree(args.argc, args.argv, prefix) ||
+			run_command(&cp2)) {
+			die(_("Could not restore untracked files from stash"));
+		}
+		set_alternate_index_output(".git/index");
+	}
+
+
+	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(_("Merging %s with %s\n"), 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(&args);
+		argv_array_push(&args, "rerere");
+		cmd_rerere(args.argc, args.argv, prefix);
+
+		return ret;
+	}
+
+	if (index) {
+		ret = reset_tree(index_tree, 0, 0);
+	} else {
+		struct child_process cp = CHILD_PROCESS_INIT;
+		struct child_process cp2 = CHILD_PROCESS_INIT;
+		struct strbuf out = STRBUF_INIT;
+		cp.git_cmd = 1;
+		argv_array_push(&cp.args, "diff-index");
+		argv_array_push(&cp.args, "--cached");
+		argv_array_push(&cp.args, "--name-only");
+		argv_array_push(&cp.args, "--diff-filter=A");
+		argv_array_push(&cp.args, sha1_to_hex(c_tree.hash));
+		ret = pipe_command(&cp, NULL, 0, &out, 0, NULL, 0);
+
+		ret = reset_tree(c_tree, 0, 1);
+
+		cp2.git_cmd = 1;
+		argv_array_push(&cp2.args, "update-index");
+		argv_array_push(&cp2.args, "--add");
+		argv_array_push(&cp2.args, "--stdin");
+		ret = pipe_command(&cp2, out.buf, out.len, NULL, 0, NULL, 0);
+		discard_cache();
+		read_cache();
+	}
+
+	if (!quiet) {
+		struct argv_array args;
+		argv_array_init(&args);
+		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;
+	int ret;
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	argv_array_init(&args);
+	argv_array_push(&args, "reflog");
+	argv_array_push(&args, "delete");
+	argv_array_push(&args, "--updateref");
+	argv_array_push(&args, "--rewrite");
+	argv_array_push(&args, info->REV);
+	ret = cmd_reflog(args.argc, args.argv, prefix);
+	if (ret == 0) {
+		if (!quiet) {
+			printf(_("Dropped %s (%s)\n"), info->REV, sha1_to_hex(info->w_commit.hash));
+		}
+	} else {
+		die(_("%s: Could not drop stash entry"), info->REV);
+	}
+
+	cp.git_cmd = 1;
+	/* Even though --quiet is specified, rev-parse still outputs the hash */
+	cp.no_stdout = 1;
+	argv_array_init(&cp.args);
+	argv_array_push(&cp.args, "rev-parse");
+	argv_array_push(&cp.args, "--verify");
+	argv_array_push(&cp.args, "--quiet");
+	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) {
+		fprintf_ln(stderr, _("'%s' is not a stash reference"), commit);
+		return 1;
+	}
+
+	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 object_context unused;
+	struct argv_array args;
+
+	argc = parse_options(argc, argv, prefix, options,
+				 git_stash_list_usage, PARSE_OPT_KEEP_UNKNOWN);
+
+	if (get_sha1_with_context(ref_stash, 0, obj.hash, &unused))
+		return 0;
+
+	argv_array_init(&args);
+	argv_array_push(&args, "log");
+	argv_array_push(&args, "--format=%gd: %gs");
+	argv_array_push(&args, "-g");
+	argv_array_push(&args, "--first-parent");
+	argv_array_push(&args, "-m");
+	argv_array_pushv(&args, argv);
+	argv_array_push(&args, ref_stash);
+	if (cmd_log(args.argc, args.argv, prefix))
+		return 1;
+
+	return 0;
+}
+
+static int show_stash(int argc, const char **argv, const char *prefix)
+{
+	struct argv_array args;
+	struct stash_info info;
+	const char *commit = NULL;
+	int numstat = 0;
+	int patch = 0;
+
+	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_init(&args);
+	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));
+	return cmd_diff(args.argc, args.argv, prefix);
+}
+
+static int pop_stash(int argc, const char **argv, const char *prefix)
+{
+	int index = 0;
+	const char *commit = NULL;
+	struct stash_info info;
+	struct option options[] = {
+		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
+		OPT_BOOL(0, "index", &index,
+			 N_("attempt to ininstate the index")),
+		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) {
+		fprintf_ln(stderr, _("'%s' is not a stash reference"), commit);
+		return 1;
+	}
+
+	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;
+	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_init(&args);
+	argv_array_push(&args, "checkout");
+	argv_array_push(&args, "-b");
+	argv_array_push(&args, branch);
+	argv_array_push(&args, sha1_to_hex(info.b_commit.hash));
+	ret = cmd_checkout(args.argc, args.argv, prefix);
+
+	ret = do_apply_stash(prefix, &info, 1);
+	if (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, 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(&args);
+			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 v3 1/4] stash: add test for stash create with no files

[Ævar Arnfjörð Bjarmason]

On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb <joel@teichroeb.net> wrote:
> 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..aaae221304 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 $(cat actual | wc -l) -eq 0

Looks fine like this, I don't think there's any actual portability
concern (as with some wrappers), but FWIW you can do this more briefly
with the test framework as:

    test_line_count = 0 actual

Although I wonder in this case whether you don't actually mean:

    [...]>actual &&
    ! test -s actual

I.e. isn't the test that there should be no output, not that there
shouldn't be a \n there? Or alternatively:

     test $(cat actual | wc -c) -eq 0

> +'
> +
>  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
>

Re: [PATCH v3 3/4] close the index lock when not writing the new index

[Ævar Arnfjörð Bjarmason]

On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb <joel@teichroeb.net> wrote:
> Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
> ---
>  builtin/add.c     | 3 ++-
>  builtin/mv.c      | 8 +++++---
>  builtin/rm.c      | 3 ++-
>  merge-recursive.c | 8 +++++---
>  4 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 9f53f020d0..6b04eb2c71 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -461,7 +461,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>         if (active_cache_changed) {
>                 if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
>                         die(_("Unable to write new index file"));
> -       }
> +       } else
> +               rollback_lock_file(&lock_file);

From Documentation/CodingGuidelines: "When there are multiple arms to
a conditional and some of them[...]".

>
>         return exit_status;
>  }
> diff --git a/builtin/mv.c b/builtin/mv.c
> index 61d20037ad..ccf21de17f 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -293,9 +293,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>         if (gitmodules_modified)
>                 stage_updated_gitmodules();
>
> -       if (active_cache_changed &&
> -           write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
> -               die(_("Unable to write new index file"));
> +       if (active_cache_changed) {
> +               if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
> +                       die(_("Unable to write new index file"));
> +       } else
> +               rollback_lock_file(&lock_file);
>
>         return 0;
>  }
> diff --git a/builtin/rm.c b/builtin/rm.c
> index fb79dcab18..4c7a91888b 100644
> --- a/builtin/rm.c
> +++ b/builtin/rm.c
> @@ -389,7 +389,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>         if (active_cache_changed) {
>                 if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
>                         die(_("Unable to write new index file"));
> -       }
> +       } else
> +               rollback_lock_file(&lock_file);
>
>         return 0;
>  }
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 62decd51cc..db841c0d38 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -2145,9 +2145,11 @@ 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
>

Re: [PATCH v3 4/4] stash: implement builtin stash

[Christian Couder]

On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb <joel@teichroeb.net> wrote:

[...]

> +int untracked_files(struct strbuf *out, int include_untracked,
> +               const char **argv)
> +{
> +       struct child_process cp = CHILD_PROCESS_INIT;
> +       cp.git_cmd = 1;
> +       argv_array_push(&cp.args, "ls-files");
> +       argv_array_push(&cp.args, "-o");
> +       argv_array_push(&cp.args, "-z");

You might want to use argv_array_pushl(), for example:

       argv_array_pushl(&cp.args, "ls-files", "-o", "-z", NULL);

> +       if (include_untracked != 2)
> +               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,
> +               const char **argv)
> +{
> +       struct argv_array args1;
> +       struct argv_array args2;
> +       struct strbuf out = STRBUF_INIT;
> +
> +       argv_array_init(&args1);
> +       argv_array_push(&args1, "diff-index");
> +       argv_array_push(&args1, "--quiet");
> +       argv_array_push(&args1, "--cached");
> +       argv_array_push(&args1, "HEAD");
> +       argv_array_push(&args1, "--ignore-submodules");
> +       argv_array_push(&args1, "--");

Here and in other places also you could use argv_array_pushl().

> +       if (argv)
> +               argv_array_pushv(&args1, argv);
> +       argv_array_init(&args2);
> +       argv_array_push(&args2, "diff-files");
> +       argv_array_push(&args2, "--quiet");
> +       argv_array_push(&args2, "--ignore-submodules");
> +       argv_array_push(&args2, "--");
> +       if (argv)
> +               argv_array_pushv(&args2, argv);
> +       if (include_untracked)
> +               untracked_files(&out, include_untracked, argv);
> +       return cmd_diff_index(args1.argc, args1.argv, prefix) == 0 &&
> +                       cmd_diff_files(args2.argc, args2.argv, prefix) == 0 &&
> +                       (!include_untracked || out.len == 0);
> +}
> +
> +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 i_commit_rev = STRBUF_INIT;
> +       struct strbuf u_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;
> +       struct object_context unused;
> +       char *str;
> +       int ret;
> +       const char *REV = commit;

We use lower case variable names.

> +       struct child_process cp = CHILD_PROCESS_INIT;
> +       info->is_stash_ref = 0;
> +
> +       if (commit == NULL) {
> +               strbuf_addf(&commit_buf, "%s@{0}", ref_stash);
> +               REV = commit_buf.buf;
> +       } else if (strlen(commit) < 3) {
> +               strbuf_addf(&commit_buf, "%s@{%s}", ref_stash, commit);
> +               REV = commit_buf.buf;
> +       }
> +       info->REV = REV;

Also the "REV" member of struct stash_info could be lower cased.

> +       strbuf_addf(&w_commit_rev, "%s", REV);
> +       strbuf_addf(&b_commit_rev, "%s^1", REV);
> +       strbuf_addf(&i_commit_rev, "%s^2", REV);
> +       strbuf_addf(&u_commit_rev, "%s^3", REV);
> +       strbuf_addf(&w_tree_rev, "%s:", REV);
> +       strbuf_addf(&b_tree_rev, "%s^1:", REV);
> +       strbuf_addf(&i_tree_rev, "%s^2:", REV);
> +       strbuf_addf(&u_tree_rev, "%s^3:", REV);
> +
> +

Spurious new line above.

> +       ret = (
> +               get_sha1_with_context(w_commit_rev.buf, 0, info->w_commit.hash, &unused) == 0 &&
> +               get_sha1_with_context(b_commit_rev.buf, 0, info->b_commit.hash, &unused) == 0 &&
> +               get_sha1_with_context(i_commit_rev.buf, 0, info->i_commit.hash, &unused) == 0 &&
> +               get_sha1_with_context(w_tree_rev.buf, 0, info->w_tree.hash, &unused) == 0 &&
> +               get_sha1_with_context(b_tree_rev.buf, 0, info->b_tree.hash, &unused) == 0 &&
> +               get_sha1_with_context(i_tree_rev.buf, 0, info->i_tree.hash, &unused) == 0);
> +
> +       if (!ret) {
> +               fprintf_ln(stderr, _("%s is not a valid reference"), REV);
> +               return 1;

Maybe use "return error(_("%s is not a valid reference"), REV);"

> +       }
> +
> +

Spurious new lines above.

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

Spurious new line above.

> +       }
> +}
> +
> +/* Untracked files are stored by themselves in a parentless commit, for
> + * ease of unpacking later.
> + */

This comment should rather be like:

/*
 * Untracked files are stored by themselves in a parentless commit, for
 * ease of unpacking later.
 */

> +static int save_untracked(struct stash_info *info, struct strbuf *out,
> +               int include_untracked, const char **argv)
> +{
> +       struct child_process cp2 = CHILD_PROCESS_INIT;
> +       struct strbuf out3 = STRBUF_INIT;
> +       struct strbuf out4 = STRBUF_INIT;

Please try to find more meaningful names for such variables.

> +       struct object_id orig_tree;
> +
> +       set_alternate_index_output(stash_index_path);
> +       untracked_files(&out4, include_untracked, argv);
> +
> +       cp2.git_cmd = 1;
> +       argv_array_push(&cp2.args, "update-index");
> +       argv_array_push(&cp2.args, "-z");
> +       argv_array_push(&cp2.args, "--add");
> +       argv_array_push(&cp2.args, "--remove");
> +       argv_array_push(&cp2.args, "--stdin");
> +       argv_array_pushf(&cp2.env_array, "GIT_INDEX_FILE=%s", stash_index_path);
> +
> +       if (pipe_command(&cp2, out4.buf, out4.len, NULL, 0, NULL, 0))
> +               return 1;
> +
> +       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(&out3, "untracked files on %s", out->buf);
> +
> +       if (commit_tree(out3.buf, out3.len, info->u_tree.hash, NULL, info->u_commit.hash, NULL, NULL))
> +               return 1;
> +
> +       set_alternate_index_output(".git/index");

Isn't there a variable, a function or a constant that could be used
instead of the hard coded ".git/index"?

> +       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.hash);
> +       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;

In general I think we tend to return -1 instead of 1 in case of errors in C.

[...]

> +static int do_create_stash(struct stash_info *info, const char *prefix,
> +       const char *message, int include_untracked, 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 = STRBUF_INIT;
> +       struct strbuf out3 = STRBUF_INIT;
> +       struct pretty_print_context ctx = {0};
> +
> +       struct commit *c = NULL;
> +       const char *hash;
> +       struct strbuf out2 = STRBUF_INIT;
> +
> +       read_cache_preload(NULL);
> +       refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL);
> +       if (check_no_changes(prefix, include_untracked, argv))
> +               return 1;
> +
> +       if (get_sha1_tree("HEAD", info->b_commit.hash))
> +               die(_("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.hash);
> +
> +       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, "%s: %s ", branch_name, hash);
> +
> +       pretty_print_commit(&ctx, c, &out);
> +
> +       strbuf_addf(&out3, "index on %s\n", out.buf);
> +
> +       commit_list_insert(lookup_commit(info->b_commit.hash), &parents);
> +
> +       if (write_cache_as_tree(info->i_tree.hash, 0, NULL))
> +               die(_("git write-tree failed to write a tree"));
> +
> +       if (commit_tree(out3.buf, out3.len, info->i_tree.hash, parents, info->i_commit.hash, NULL, NULL))
> +               die(_("Cannot save the current index state"));
> +
> +
> +       if (include_untracked) {
> +               if (save_untracked(info, &out, include_untracked, argv))
> +                       die(_("Cannot save the untracked files"));
> +       }
> +
> +

Spurious new line above.

> +       if (patch) {
> +               if (patch_working_tree(info, prefix, argv))
> +                       die(_("Cannot save the current worktree state"));
> +       } else {
> +               if (save_working_tree(info, prefix, argv))
> +                       die(_("Cannot save the current worktree state"));
> +       }
> +       parents = NULL;
> +
> +       if (include_untracked) {
> +               commit_list_insert(lookup_commit(info->u_commit.hash), &parents);
> +       }

Braces can be removed above.

> +       commit_list_insert(lookup_commit(info->i_commit.hash), &parents);
> +       commit_list_insert(lookup_commit(info->b_commit.hash), &parents);
> +
> +       if (message != NULL && strlen(message) != 0)
> +               strbuf_addf(&out2, "On %s: %s\n", branch_name, message);
> +       else
> +               strbuf_addf(&out2, "WIP on %s\n", out.buf);
> +
> +       if (commit_tree(out2.buf, out2.len, info->w_tree.hash, parents, info->w_commit.hash, NULL, NULL))
> +               die(_("Cannot record working tree state"));
> +
> +       info->message = out2.buf;
> +
> +       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;
> +       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) {
> +               struct strbuf out = STRBUF_INIT;
> +               int i;
> +               for (i = 0; i < argc; ++i) {
> +                       if (i != 0) {
> +                               strbuf_addf(&out, " ");
> +                       }

Braces can be removed.

[...]

> +static int store_stash(int argc, const char **argv, const char *prefix)
> +{
> +       const char *message = NULL;
> +       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 (message == NULL)
> +               message = "Create via \"git stash store\".";

Maybe you could have initialized "message" when you declared the variable above.

> +       if (argc != 1)
> +               die(_("\"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;

Maybe use error() or warning().

> +       }

[...]

> +static int do_push_stash(const char *prefix, const char *message,
> +               int keep_index, int include_untracked, int patch, const char **argv)
> +{
> +       int result;
> +       struct stash_info info;
> +
> +       if (patch && include_untracked) {
> +               fprintf_ln(stderr, _("can't use --patch and --include-untracked or --all at the same time"));
> +               return 1;

error()?

> +       }
> +
> +       if (!include_untracked) {
> +               struct child_process cp = CHILD_PROCESS_INIT;
> +               struct strbuf out = STRBUF_INIT;
> +
> +               cp.git_cmd = 1;
> +               argv_array_push(&cp.args, "ls-files");
> +               argv_array_push(&cp.args, "--error-unmatch");
> +               argv_array_push(&cp.args, "--");
> +               if (argv)
> +                       argv_array_pushv(&cp.args, argv);
> +               result = pipe_command(&cp, NULL, 0, &out, 0, NULL, 0);
> +               if (result)
> +                       return 1;
> +       }
> +
> +       read_cache_preload(NULL);
> +       refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL);
> +       if (check_no_changes(prefix, include_untracked, argv)) {
> +               printf(_("No local changes to save\n"));
> +               return 0;
> +       }
> +
> +       if (!reflog_exists(ref_stash)) {
> +               if (do_clear_stash())
> +                       die(_("Cannot initialize stash"));
> +       }
> +
> +

Spurious new line.

> +       do_create_stash(&info, prefix, message, include_untracked, patch, argv);
> +       result = do_store_stash(prefix, 1, info.message, info.w_commit);
> +
> +       if (result == 0 && !quiet) {
> +               printf(_("Saved working directory and index state %s"), info.message);
> +       }

Braces can be removed.

I stopped skimming here.

Thanks,
Christian.

Re: [PATCH v3 2/4] stash: add test for stashing in a detached state

[Ævar Arnfjörð Bjarmason]

On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb <joel@teichroeb.net> wrote:
> Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
> ---
>  t/t3903-stash.sh | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index aaae221304..b86851ef46 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -808,6 +808,17 @@ 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) &&
> +       echo "WIP on (no branch): 47d5e0e initial" >expect &&
> +       git show --pretty=%s -s ${STASH_ID} >actual &&
> +       test_cmp expect actual

I thought this needed test_i18ncmp, turns out it didn't, neither the
original stash code nor your replacement translates this. That's fine,
just a note to other reviewers...

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

Re: [PATCH v3 4/4] stash: implement builtin stash

[Ævar Arnfjörð Bjarmason]

On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb <joel@teichroeb.net> wrote:
> Implement all git stash functionality as a builtin command

First thanks for working on this, it's great. Applied it locally,
passes all tests for me. A couple of comments Christian didn't cover

> +       info->has_u = get_sha1_with_context(u_commit_rev.buf, 0, info->u_commit.hash, &unused) == 0 &&
> +               get_sha1_with_context(u_tree_rev.buf, 0, info->u_tree.hash, &unused) == 0;
> +
> +
> +       /* TODO: Improve this logic */
> +       strbuf_addf(&symbolic, "%s", REV);
> +       str = strstr(symbolic.buf, "@");

Could you elaborate on how this should be improved?


> +static int patch_working_tree(struct stash_info *info, const char *prefix,
> +               const char **argv)
> +{
> +       const char *stash_index_path = ".git/foocache2";

This foocache path isn't created by the shell code, if it's a new
thing that's needed (and I haven't followed this code in detail, don'n
know what it's for) shouldn't we give it a more descriptive name so
that if git crashes it's obvious what it is?

> +       const char *message = NULL;
> +       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);

Nit: In general in this patch the 2nd line of parse_options doesn't
align with a tabwidth of 8. Ditto for indenting function arguments
(e.g. for untracked_files).

Re: [PATCH v3 4/4] stash: implement builtin stash

[Joel Teichroeb]

On Sun, May 28, 2017 at 11:26 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb <joel@teichroeb.net> wrote:
>> Implement all git stash functionality as a builtin command
>
> First thanks for working on this, it's great. Applied it locally,
> passes all tests for me. A couple of comments Christian didn't cover
>
>> +       info->has_u = get_sha1_with_context(u_commit_rev.buf, 0, info->u_commit.hash, &unused) == 0 &&
>> +               get_sha1_with_context(u_tree_rev.buf, 0, info->u_tree.hash, &unused) == 0;
>> +
>> +
>> +       /* TODO: Improve this logic */
>> +       strbuf_addf(&symbolic, "%s", REV);
>> +       str = strstr(symbolic.buf, "@");
>
> Could you elaborate on how this should be improved?
>

I just figured there would be a builtin function that could help here,
but hadn't had the chance to look into it. It's something easy to do
in bash, but more complicated in C.

>
>> +static int patch_working_tree(struct stash_info *info, const char *prefix,
>> +               const char **argv)
>> +{
>> +       const char *stash_index_path = ".git/foocache2";
>
> This foocache path isn't created by the shell code, if it's a new
> thing that's needed (and I haven't followed this code in detail, don'n
> know what it's for) shouldn't we give it a more descriptive name so
> that if git crashes it's obvious what it is?
>

Opps, I had cleaned that part up locally, but I forgot to push it when
switching computers. It'll be better in the next patch set.

>> +       const char *message = NULL;
>> +       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);
>
> Nit: In general in this patch the 2nd line of parse_options doesn't
> align with a tabwidth of 8. Ditto for indenting function arguments
> (e.g. for untracked_files).

I'll fix my tab width. Forgot that long lines would change, haha.

Re: [PATCH v3 4/4] stash: implement builtin stash

[Ævar Arnfjörð Bjarmason]

On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb <joel@teichroeb.net> wrote:
> Implement all git stash functionality as a builtin command
>
> Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
> ---

General note on this that I missed in my first E-Mail, you have ~20
calls to argv_array_init() but none to argv_array_clear(). So you're
leaking memory, and it obscures potential other issues with valgrind.

A lot of that's easy to solve, but sometimes requires a temporary
variable since the code is now returning directly, e.g:

@@ -1091,6 +1094,7 @@ static int list_stash(int argc, const char
**argv, const char *prefix)
        struct object_id obj;
        struct object_context unused;
        struct argv_array args;
+       int ret = 0;

        argc = parse_options(argc, argv, prefix, options,
                                 git_stash_list_usage, PARSE_OPT_KEEP_UNKNOWN);
@@ -1107,9 +1111,9 @@ static int list_stash(int argc, const char
**argv, const char *prefix)
        argv_array_pushv(&args, argv);
        argv_array_push(&args, ref_stash);
        if (cmd_log(args.argc, args.argv, prefix))
-               return 1;
-
-       return 0;
+               ret = 1;
+       argv_array_clear(&args);
+       return ret;
 }

But more generally this goes a long way to resolving the issue where
you have variables like out1, out2 or cp1, cp2 etc. which Christian
pointed out. I.e. you're not freeing/clearing strbufs either, instead
just creating new ones that also aren't freed, or not clearing
child_process structs, e.g. this on top allows you to re-use the same
variable and stops leaking memory:

diff --git a/builtin/stash.c b/builtin/stash.c
index bf36ff8f9b..4e7344501a 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -729,7 +729,6 @@ static int do_push_stash(const char *prefix, const
char *message,

                if (keep_index) {
                        struct child_process cp = CHILD_PROCESS_INIT;
-                       struct child_process cp2 = CHILD_PROCESS_INIT;
                        struct strbuf out = STRBUF_INIT;

                        reset_tree(info.i_tree, 0, 1);
@@ -741,13 +740,18 @@ static int do_push_stash(const char *prefix,
const char *message,
                        argv_array_push(&cp.args, "--");
                        argv_array_pushv(&cp.args, argv);
                        pipe_command(&cp, NULL, 0, &out, 0, NULL, 0);
+                       argv_array_clear(&cp.args);
+                       child_process_clear(&cp);

-                       cp2.git_cmd = 1;
-                       argv_array_push(&cp2.args, "checkout-index");
-                       argv_array_push(&cp2.args, "-z");
-                       argv_array_push(&cp2.args, "--force");
-                       argv_array_push(&cp2.args, "--stdin");
-                       pipe_command(&cp2, out.buf, out.len, NULL, 0, NULL, 0);
+                       child_process_init(&cp);
+                       cp.git_cmd = 1;
+                       argv_array_push(&cp.args, "checkout-index");
+                       argv_array_push(&cp.args, "-z");
+                       argv_array_push(&cp.args, "--force");
+                       argv_array_push(&cp.args, "--stdin");
+                       pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0);
+                       argv_array_clear(&cp.args);
+                       child_process_clear(&cp);
                }
        } else {
                struct child_process cp2 = CHILD_PROCESS_INIT;

Re: [PATCH v3 0/4] Implement git stash as a builtin command

[Ævar Arnfjörð Bjarmason]

On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb <joel@teichroeb.net> 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.

Worth noting, with your patches the best of 3 run of all the stash tests:

$ for i in {1..3}; do (time prove -j1 t*-stash*.sh) 2>&1 | grep ^real;
done  |awk '{print $2}' | sort -n | head -n 1
0m3.293s

Without:

$ for i in {1..3}; do (time prove -j1 t*-stash*.sh) 2>&1 | grep ^real;
done  |awk '{print $2}' | sort -n | head -n 1
0m7.619s

Of course some individual invocations are much faster than that, this
includes all the shell overhead of the tests themselves.

Re: [PATCH v3 4/4] stash: implement builtin stash

[Jeff King]

On Sun, May 28, 2017 at 08:51:07PM +0200, Ævar Arnfjörð Bjarmason wrote:

> On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb <joel@teichroeb.net> wrote:
> > Implement all git stash functionality as a builtin command
> >
> > Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
> > ---
> 
> General note on this that I missed in my first E-Mail, you have ~20
> calls to argv_array_init() but none to argv_array_clear(). So you're
> leaking memory, and it obscures potential other issues with valgrind.

I haven't looked carefully at the patches, but note that the argv array
in a child_process is handled automatically by start/finish_command.

So:

> @@ -1107,9 +1111,9 @@ static int list_stash(int argc, const char
> **argv, const char *prefix)
>         argv_array_pushv(&args, argv);
>         argv_array_push(&args, ref_stash);
>         if (cmd_log(args.argc, args.argv, prefix))
> -               return 1;
> -
> -       return 0;
> +               ret = 1;
> +       argv_array_clear(&args);
> +       return ret;
>  }

This one does need a clear, because it's calling an internal function.
But...

> @@ -741,13 +740,18 @@ static int do_push_stash(const char *prefix,
> const char *message,
>                         argv_array_push(&cp.args, "--");
>                         argv_array_pushv(&cp.args, argv);
>                         pipe_command(&cp, NULL, 0, &out, 0, NULL, 0);
> +                       argv_array_clear(&cp.args);

This one does not, because the array will have been cleared by calling
pipe_command (because it does the start/finish block itself; and yes,
before you go checking, start_command() clears it even when it returns
error).

> +                       child_process_clear(&cp);

This should not be necessary for the same reason.

> -                       cp2.git_cmd = 1;
> -                       argv_array_push(&cp2.args, "checkout-index");
> -                       argv_array_push(&cp2.args, "-z");
> -                       argv_array_push(&cp2.args, "--force");
> -                       argv_array_push(&cp2.args, "--stdin");
> -                       pipe_command(&cp2, out.buf, out.len, NULL, 0, NULL, 0);
> +                       child_process_init(&cp);
> +                       cp.git_cmd = 1;
> +                       argv_array_push(&cp.args, "checkout-index");
> +                       argv_array_push(&cp.args, "-z");
> +                       argv_array_push(&cp.args, "--force");
> +                       argv_array_push(&cp.args, "--stdin");
> +                       pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0);
> +                       argv_array_clear(&cp.args);
> +                       child_process_clear(&cp);

And ditto here.

I'd also encourage using CHILD_PROCESS_INIT and ARGV_ARRAY_INIT constant
initializers instead of their function-call counterparts, as that
matches our usual style.

-Peff

Re: [PATCH v3 4/4] stash: implement builtin stash

[Jeff King]

On Sun, May 28, 2017 at 11:31:48AM -0700, Joel Teichroeb wrote:

> >> +       /* TODO: Improve this logic */
> >> +       strbuf_addf(&symbolic, "%s", REV);
> >> +       str = strstr(symbolic.buf, "@");
> >
> > Could you elaborate on how this should be improved?
> 
> I just figured there would be a builtin function that could help here,
> but hadn't had the chance to look into it. It's something easy to do
> in bash, but more complicated in C.

There's no strbuf function for "truncate at this character". But:

  - you can use strchr for a single-character match, which is more
    efficient; i.e.:

      str = strchr(symbolic.buf, '@');

  - instead of inserting a '\0' into the strbuf, use strbuf_setlen(),
    which also updates the symbolic.len; i.e.:

      strbuf_setlen(&symbolic, str - symbolic.buf);

  - it looks like you copy into the strbuf just to truncate, so you
    could actually get the final size before inserting into the strbuf
    using strchrnul. Like:

      end_of_rev = strchrnul(REV, '@');
      strbuf_add(&symbolic, REV, end_of_rev - REV);

-Peff

Re: [PATCH v3 1/4] stash: add test for stash create with no files

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> +       git stash create > actual &&
>> +       test $(cat actual | wc -l) -eq 0
> ...
> Although I wonder in this case whether you don't actually mean:
>
>     [...]>actual &&
>     ! test -s actual
>
> I.e. isn't the test that there should be no output, not that there
> shouldn't be a \n there?

Good suggestion.  There is "test_must_be_empty" you may want to use.

Also, we do not leave SP between the redirection operator and the
filename.  Your example ">actual" is good; the original goes against
our style.

Thanks.

Re: [PATCH v3 2/4] 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 | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index aaae221304..b86851ef46 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -808,6 +808,17 @@ 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) &&
> +	echo "WIP on (no branch): 47d5e0e initial" >expect &&

Is it easy to generate this (especially 47d5e0e) at runtime?  I know
that earlier tests in this script are brittle and will break when
object names change by using test vectors that hardcode object names
a bit too much, but if we can avoid making it worse, let's try to do
so for future developers who will have to do the work of adjusting
the tests for possible changes to the object name hashing algorithm.

> +	git show --pretty=%s -s ${STASH_ID} >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'stash -- <pathspec> stashes and restores the file' '
>  	>foo &&
>  	>bar &&

Re: [PATCH v3 3/4] close the index lock when not writing the new index

[Junio C Hamano]

Joel Teichroeb <joel@teichroeb.net> writes:

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

The title says what the patch does; it does not explain why it is a
good change.  Lockfiles will be closed automatically when we exit
anyway, so one can argue that the current code is good.

If you are planning to add more code to these "missing" else clauses
in future steps in this series, this change makes sort-of sense.  If
that is the case, please say that in your log message.

>  builtin/add.c     | 3 ++-
>  builtin/mv.c      | 8 +++++---
>  builtin/rm.c      | 3 ++-
>  merge-recursive.c | 8 +++++---
>  4 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 9f53f020d0..6b04eb2c71 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -461,7 +461,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  	if (active_cache_changed) {
>  		if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
>  			die(_("Unable to write new index file"));
> -	}
> +	} else
> +		rollback_lock_file(&lock_file);

I think Ævar already pointed out the style issue, i.e.

	if (a-c-c) {
		if (w-l-i())
			die(...);
	} else {
		rollback_lock_file(&lock_file);
	}

The same for all other hunks in this patch.

Thanks.

Re: [PATCH v3 4/4] stash: implement builtin stash

[Junio C Hamano]

Joel Teichroeb <joel@teichroeb.net> writes:

> +int untracked_files(struct strbuf *out, int include_untracked,
> +		const char **argv)

Does this need to be public?  

For a caller that wants to learn if there is _any_ untracked file,
having a strbuf that holds all output is overkill.  For a caller
that wants to learn what are the untracked paths, having a strbuf
that it needs to parse is not all that helpful.  Only for a caller
that does an unusual thing, namely, just grab the output and throw
it at another command as input, without checking what the output was
itself at all, would benefit.

So the interface to this function doesn't look like a very good one
if this wants to be a public helper.  Perhaps mark it as "static"?

> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	cp.git_cmd = 1;
> +	argv_array_push(&cp.args, "ls-files");
> +	argv_array_push(&cp.args, "-o");
> +	argv_array_push(&cp.args, "-z");
> +	if (include_untracked != 2)
> +		argv_array_push(&cp.args, "--exclude-standard");

This magic number "2" will be hard to grok by future readers.

> +	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,
> +		const char **argv)
> +{
> +	struct argv_array args1;
> +	struct argv_array args2;
> +	struct strbuf out = STRBUF_INIT;
> +
> +	argv_array_init(&args1);
> +	argv_array_push(&args1, "diff-index");
> +	argv_array_push(&args1, "--quiet");
> +	argv_array_push(&args1, "--cached");
> +	argv_array_push(&args1, "HEAD");
> +	argv_array_push(&args1, "--ignore-submodules");
> +	argv_array_push(&args1, "--");
> +	if (argv)
> +		argv_array_pushv(&args1, argv);
> +	argv_array_init(&args2);
> +	argv_array_push(&args2, "diff-files");
> +	argv_array_push(&args2, "--quiet");
> +	argv_array_push(&args2, "--ignore-submodules");
> +	argv_array_push(&args2, "--");
> +	if (argv)
> +		argv_array_pushv(&args2, argv);

> +	if (include_untracked)
> +		untracked_files(&out, include_untracked, argv);
> +	return cmd_diff_index(args1.argc, args1.argv, prefix) == 0 &&
> +			cmd_diff_files(args2.argc, args2.argv, prefix) == 0 &&
> +			(!include_untracked || out.len == 0);
> +}

Possible leak of out.buf here.

> +
> +static int get_stash_info(struct stash_info *info, const char *commit)
> +{

Judging from the callers of get_stash_info(), nobody passes a
"commit" as parameter; as far as they are concerned, they are
dealing with the name of one item in the stash (stash-id?).  They do
not care the fact that the item happens to be implemented as a
commit object.

Perhaps rename "commit" (parameter) to "stash_id" or something.

> +	struct strbuf w_commit_rev = STRBUF_INIT;
> +	struct strbuf b_commit_rev = STRBUF_INIT;
> +	struct strbuf i_commit_rev = STRBUF_INIT;
> +	struct strbuf u_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;
> +	struct object_context unused;
> +	char *str;
> +	int ret;
> +	const char *REV = commit;

Variables and field names that are all caps become misleading.
Avoid them.

> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	info->is_stash_ref = 0;
> +
> +	if (commit == NULL) {
> +		strbuf_addf(&commit_buf, "%s@{0}", ref_stash);
> +		REV = commit_buf.buf;
> +	} else if (strlen(commit) < 3) {
> +		strbuf_addf(&commit_buf, "%s@{%s}", ref_stash, commit);
> +		REV = commit_buf.buf;
> +	}
> +	info->REV = REV;
> +
> +	strbuf_addf(&w_commit_rev, "%s", REV);
> +	strbuf_addf(&b_commit_rev, "%s^1", REV);
> +	strbuf_addf(&i_commit_rev, "%s^2", REV);
> +	strbuf_addf(&u_commit_rev, "%s^3", REV);
> +	strbuf_addf(&w_tree_rev, "%s:", REV);
> +	strbuf_addf(&b_tree_rev, "%s^1:", REV);
> +	strbuf_addf(&i_tree_rev, "%s^2:", REV);
> +	strbuf_addf(&u_tree_rev, "%s^3:", REV);
> +
> +
> +	ret = (
> +		get_sha1_with_context(w_commit_rev.buf, 0, info->w_commit.hash, &unused) == 0 &&
> +		get_sha1_with_context(b_commit_rev.buf, 0, info->b_commit.hash, &unused) == 0 &&
> +		get_sha1_with_context(i_commit_rev.buf, 0, info->i_commit.hash, &unused) == 0 &&
> +		get_sha1_with_context(w_tree_rev.buf, 0, info->w_tree.hash, &unused) == 0 &&
> +		get_sha1_with_context(b_tree_rev.buf, 0, info->b_tree.hash, &unused) == 0 &&
> +		get_sha1_with_context(i_tree_rev.buf, 0, info->i_tree.hash, &unused) == 0);

Hmph.  What's the reason to use get_sha1_with_context() if you
declare the context is unused?  Wouldn't plain-vanilla get_sha1()
work better?

Having to take both commit and tree of these sounds somewhat
cumbersome.  Do we have to know all of them and do we use all of
them (not a complaint, just asking because I find it somewhat
surprising)?

> +
> +	if (!ret) {
> +		fprintf_ln(stderr, _("%s is not a valid reference"), REV);
> +		return 1;
> +	}
> +
> +
> +	info->has_u = get_sha1_with_context(u_commit_rev.buf, 0, info->u_commit.hash, &unused) == 0 &&
> +		get_sha1_with_context(u_tree_rev.buf, 0, info->u_tree.hash, &unused) == 0;

Ditto.

> +static int create_stash(int argc, const char **argv, const char *prefix)
> +{
> +	int include_untracked = 0;
> +	const char *message = NULL;
> +	struct stash_info info;
> +	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()
> +	};

OPT_BOOL(), unlike its now deprecated and removed predecessor OPT_BOOLEAN(),
does not count up, so the value of include_untracked is lmited to
either 0 or 1.  So this is not the source of mysterious magic number
"2" I noticed above.

I'll stop here for now.  

Thanks for working on this.

Re: [PATCH v3 4/4] stash: implement builtin stash

[Joel Teichroeb]

Once I have all those leaks fixed, is there a way to make sure I'm not
missing any? I tried using valgrind with leak-check enabled, but there
are too many leaks from other git commands.

Joel

Re: [PATCH v3 4/4] stash: implement builtin stash

[Ævar Arnfjörð Bjarmason]

On Mon, May 29, 2017 at 8:18 PM, Joel Teichroeb <joel@teichroeb.net> wrote:
> Once I have all those leaks fixed, is there a way to make sure I'm not
> missing any? I tried using valgrind with leak-check enabled, but there
> are too many leaks from other git commands.

I just used:

    valgrind --leak-check=full ./git-stash list

And then skimmed things that mentioned stash.c in the pager. There
might be some better way to do this (e.g. instrument the test suite to
run valgrind for all commands and summarize that somehow), but I don't
know how to do that offhand...

Re: [PATCH v3 4/4] stash: implement builtin stash

[Joel Teichroeb]

I'm running into a lot of trouble using argv_array_clear. It seems
that some of the builtin git cmd functions move the parameters around,
and write new pointers to argv. There's three options I have now, and
I'm not sure which is the best one.

1. Fix all the builtin cmd functions that I use to not mess around with argv
2. Stop using the builtin cmd functions, and use child processes exclusively
3. Don't worry about clearing the memory used for these function calls.

It looks like the rest of the code generally does #3.

Any advice here would be appreciated.

Re: [PATCH v3 4/4] stash: implement builtin stash

[Jeff King]

On Wed, May 31, 2017 at 08:29:43PM -0700, Joel Teichroeb wrote:

> I'm running into a lot of trouble using argv_array_clear. It seems
> that some of the builtin git cmd functions move the parameters around,
> and write new pointers to argv. There's three options I have now, and
> I'm not sure which is the best one.

Hrm. It's normal for parsing to reorder the parameters (e.g., shifting
non-options to the front), but that should still allow a clear at the
end. New pointers would definitely cause a problem, though. I don't know
of any cases where we do that, but on the other hand I wouldn't be too
surprised to find that the revision.c options parser does some nasty
tricks.

Do you have a specific example? I'd be curious to see if we can just fix
the parser to be less surprising (i.e., your (1) below).

> 1. Fix all the builtin cmd functions that I use to not mess around with argv

If it's just one or two spots, this might be viable.

> 2. Stop using the builtin cmd functions, and use child processes exclusively

That might not be the worst thing in the world for a first cut at a
shell to C transition, because it eliminates a whole class of possible
problems. But it really just side-steps the problem, as we'd want to
eventually deal with it and reduce the process count.

> 3. Don't worry about clearing the memory used for these function calls.

That might be do-able, as long as the leaks are O(1) for a program run
(and not say, a leak per commit). At the very least we should mark
those spots with a "NEEDSWORK" comment and an explanation of the issue
so that your work in finding them isn't wasted.

> It looks like the rest of the code generally does #3.

It looks like we don't actually pass argv arrays to setup_revisions()
all that often. The three I see are:

  - bisect_rev_setup(), which is a known leak. This is trickier, though,
    because we actually pass the initialized rev_info out of the
    function, and the memory needs to last until we're done with the
    traversal

  - http-push, which does seem to free the memory

  - stat_tracking_info(), which does seem to free

I could well believe there are places where we leak, though, especially
for top-level functions that exit the program when they're done.

A fourth option is to massage the argv array into something that can be
massaged by the callee, and retain the original array for freeing. I.e.,
something like:

  struct argv_array argv = ARGV_ARRAY_INIT;
  const char **massaged;

  argv_array_pushl(&argv, ...whatever...);

  ALLOC_ARRAY(massaged, argc);
  COPY_ARRAY(massaged, argv, argc);

  setup_revisions(argv.argc, massaged, &revs, NULL);

  /*
   * No clue what's in "massaged" now, as setup_revisions() may have
   * reordered things, added new elements, deleted some, etc. But we
   * don't have to care because any pointers we need to free are still
   * in the original argv struct, and we should be safe to free the
   * massaged array itself.
   */
  free(massaged);
  argv_array_clear(&argv);

That's pretty horrible, though. If setup_revisions() is requiring us to
do that, I'd really prefer to look into fixing it.

-Peff

Re: [PATCH v3 0/4] Implement git stash as a builtin command

[Johannes Schindelin]

Hi Joel,

On Sun, 28 May 2017, 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.

I am embarrassed to say that I never found the time to have a detailed
look at this, and it has been 5 months since you sent this. Sorry about
that.

Do you have an easy-to-fetch branch with this somewhere?

Thanks,
Dscho

Re: [PATCH v3 0/4] Implement git stash as a builtin command

[Joel Teichroeb]

Hi Johannes,

I fixed all the tests at that time, but another one was added that I
could not figure out how to fix. I was planning to break the commit up
into smaller parts and only convert one stash command at a time
calling them still from the shell script. I haven't had time to do
that yet though.

My latest change is here:
https://github.com/klusark/git/tree/rfc-c-stash

The test commits have already been merged, so just the last two are
needed. I haven't been keeping history of my changes, although I have
sent it to the mailing list around 4 times.

Joel

On Mon, Oct 23, 2017 at 4:09 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Joel,
>
> On Sun, 28 May 2017, 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.
>
> I am embarrassed to say that I never found the time to have a detailed
> look at this, and it has been 5 months since you sent this. Sorry about
> that.
>
> Do you have an easy-to-fetch branch with this somewhere?
>
> Thanks,
> Dscho