Skip to contentSkip to conversation

Convert some stash functionality to a builtin

PATCH Superseded Joel Teichroeb 7 participants 25 messages Mar 24, 2018 – Mar 28, 2018 1 message not in the archive Kbuild

v1 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Joel Teichroeb

author
#0 lore raw

cover Convert some stash functionality to a builtin

I've been working on converting all of git stash to be a builtin, however it's hard to get it all working at once with limited time, so I've moved around half of it to a new stash--helper builtin and called these functions from the shell script. Once this is stabalized, it should be easier to convert the rest of the commands one at a time without breaking anything.

I've sent most of this code before, but that was targetting a full replacement of stash. The code is overall the same, but with some code review changes and updates for internal api changes.

Since there seems to be interest from GSOC students who want to work on converting builtins, I figured I should finish what I have that works now so they could build on top of it.

Joel Teichroeb (4):
stash: convert apply to builtin
stash: convert branch to builtin
stash: convert drop and clear to builtin
stash: convert pop to builtin

 .gitignore              |   1 +
 Makefile                |   1 +
 builtin.h               |   1 +
 builtin/stash--helper.c | 514 ++++++++++++++++++++++++++++++++++++++++++++++++
 git-stash.sh            |  13 +-
 git.c                   |   1 +
 6 files changed, 526 insertions(+), 5 deletions(-)
 create mode 100644 builtin/stash--helper.c
Signature · 1 line
2.16.2
author
#1 lore raw

PATCH 1/4 stash: convert apply to builtin

---
 .gitignore              |   1 +
 Makefile                |   1 +
 builtin.h               |   1 +
 builtin/stash--helper.c | 339 ++++++++++++++++++++++++++++++++++++++++++++++++
 git-stash.sh            |   3 +-
 git.c                   |   1 +
 6 files changed, 345 insertions(+), 1 deletion(-)
 create mode 100644 builtin/stash--helper.c
.gitignore +1 -0
index 833ef3b0b7..296d5f376d 100644
@@ -152,6 +152,7 @@
 /git-show-ref
 /git-stage
 /git-stash
+/git-stash--helper
 /git-status
 /git-stripspace
 /git-submodule
Makefile +1 -0
index a1d8775adb..8ca361c57a 100644
@@ -1020,6 +1020,7 @@ BUILTIN_OBJS += builtin/send-pack.o
 BUILTIN_OBJS += builtin/shortlog.o
 BUILTIN_OBJS += builtin/show-branch.o
 BUILTIN_OBJS += builtin/show-ref.o
+BUILTIN_OBJS += builtin/stash--helper.o
 BUILTIN_OBJS += builtin/stripspace.o
 BUILTIN_OBJS += builtin/submodule--helper.o
 BUILTIN_OBJS += builtin/symbolic-ref.o
builtin.h +1 -0
index 42378f3aa4..a14fd85b0e 100644
@@ -219,6 +219,7 @@ extern int cmd_shortlog(int argc, const char **argv, const char *prefix);
 extern int cmd_show(int argc, const char **argv, const char *prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
 extern int cmd_status(int argc, const char **argv, const char *prefix);
+extern int cmd_stash__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
 extern int cmd_submodule__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
builtin/stash--helper.c new file +339 -0

339 lines not on this page · raw message

git-stash.sh +2 -1
index fc8f8ae640..92c084eb17 100755
@@ -711,7 +711,8 @@ push)
 	;;
 apply)
 	shift
-	apply_stash "$@"
+	cd "$START_DIR"
+	git stash--helper apply "$@"
 	;;
 clear)
 	shift
git.c +1 -0
index ceaa58ef40..6ffe6364ac 100644
@@ -466,6 +466,7 @@ static struct cmd_struct commands[] = {
 	{ "show-branch", cmd_show_branch, RUN_SETUP },
 	{ "show-ref", cmd_show_ref, RUN_SETUP },
 	{ "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
+	{ "stash--helper", cmd_stash__helper, RUN_SETUP | NEED_WORK_TREE },
 	{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
 	{ "stripspace", cmd_stripspace },
 	{ "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX},
Signature · 1 line
2.16.2
#2 lore raw

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

builtin/stash--helper.c line 146

Quoted 6 lines
builtin/stash--helper.c new file +6 -0
+       if (unpack_trees(nr_trees, t, &opts))
+               return -1;
+
+       if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) {
+               error(_("unable to write new index file"));
+               return -1;

Maybe: return error(...);

Quoted 4 lines
builtin/stash--helper.c new file +4 -0
+       }
+
+       return 0;
+}

[...]

builtin/stash--helper.c line 301

Quoted 6 lines
builtin/stash--helper.c new file +6 -0
+       argc = parse_options(argc, argv, prefix, options,
+                       git_stash_helper_apply_usage, 0);
+
+       if (argc == 1) {
+               commit = argv[0];
+       }

The brackets are not needed.

builtin/stash--helper.c line 306

Quoted 4 lines
builtin/stash--helper.c new file +4 -0
+       if (get_stash_info(&info, commit))
+               return -1;
+
+

Spurious new line.

Quoted 2 lines
(quoted diff) +2 -0
+       return do_apply_stash(prefix, &info, index);
+}
#3 lore raw

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

In reply to Joel Teichroeb

builtin/stash--helper.c line 11

Quoted 13 lines · Joel Teichroeb
builtin/stash--helper.c +11 -0
@@ -0,0 +1,339 @@
+static int get_stash_info(struct stash_info *info, const char *commit)
+{
+       struct strbuf w_commit_rev = STRBUF_INIT;
+       struct strbuf b_commit_rev = STRBUF_INIT;
+       struct strbuf w_tree_rev = STRBUF_INIT;
+       struct strbuf b_tree_rev = STRBUF_INIT;
+       struct strbuf i_tree_rev = STRBUF_INIT;
+       struct strbuf u_tree_rev = STRBUF_INIT;
+       struct strbuf commit_buf = STRBUF_INIT;
+       struct strbuf symbolic = STRBUF_INIT;
+       struct strbuf out = STRBUF_INIT;

'commit_buf' is being leaked. All the others seem to be covered (even in the case of early 'return').

Quoted 11 lines
builtin/stash--helper.c +11 -0
+       if (commit == NULL) {
+               strbuf_addf(&commit_buf, "%s@{0}", ref_stash);
+               revision = commit_buf.buf;
+       } else if (strspn(commit, "0123456789") == strlen(commit)) {
+               strbuf_addf(&commit_buf, "%s@{%s}", ref_stash, commit);
+               revision = commit_buf.buf;
+       }
+static int do_apply_stash(const char *prefix, struct stash_info *info, int index)
+{
+       if (index) {
+               if (!oidcmp(&info->b_tree, &info->i_tree) || !oidcmp(&c_tree, &info->i_tree)) {

builtin/stash--helper.c line 181

Quoted 10 lines
builtin/stash--helper.c +10 -0
+                       has_index = 0;
+               } else {
+                       struct child_process cp = CHILD_PROCESS_INIT;
+                       struct strbuf out = STRBUF_INIT;
+                       struct argv_array args = ARGV_ARRAY_INIT;
+                       cp.git_cmd = 1;
+                       argv_array_pushl(&cp.args, "diff-tree", "--binary", NULL);
+                       argv_array_pushf(&cp.args, "%s^2^..%s^2", sha1_to_hex(info->w_commit.hash), sha1_to_hex(info->w_commit.hash));
+                       if (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0))
+                               return -1;

Leaking 'out'?

builtin/stash--helper.c line 187

Quoted 6 lines
builtin/stash--helper.c +6 -0
+
+                       child_process_init(&cp);
+                       cp.git_cmd = 1;
+                       argv_array_pushl(&cp.args, "apply", "--cached", NULL);
+                       if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0))
+                               return -1;

Leaking 'out'.

Quoted 16 lines
builtin/stash--helper.c +16 -0
+
+                       strbuf_release(&out);
+                       discard_cache();
+                       read_cache();
+                       if (write_cache_as_tree(index_tree.hash, 0, NULL))
+                               return -1;
+
+                       argv_array_push(&args, "reset");
+                       cmd_reset(args.argc, args.argv, prefix);
+               }
+       }
+       if (has_index) {
+               if (reset_tree(index_tree, 0, 0))
+                       return -1;
+       } else {
+               struct child_process cp = CHILD_PROCESS_INIT;

builtin/stash--helper.c line 262

Quoted 10 lines
builtin/stash--helper.c +10 -0
+               struct strbuf out = STRBUF_INIT;
+               cp.git_cmd = 1;
+               argv_array_pushl(&cp.args, "diff-index", "--cached", "--name-only", "--diff-filter=A", NULL);
+               argv_array_push(&cp.args, sha1_to_hex(c_tree.hash));
+               ret = pipe_command(&cp, NULL, 0, &out, 0, NULL, 0);
+               if (ret)
+                       return -1;
+
+               if (reset_tree(c_tree, 0, 1))
+                       return -1;

Leaking 'out' at these two 'return's?

builtin/stash--helper.c line 269

Quoted 6 lines
builtin/stash--helper.c +6 -0
+               child_process_init(&cp);
+               cp.git_cmd = 1;
+               argv_array_pushl(&cp.args, "update-index", "--add", "--stdin", NULL);
+               ret = pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0);
+               if (ret)
+                       return -1;

And here.

Quoted 13 lines
builtin/stash--helper.c +13 -0
+
+               strbuf_release(&out);
+               discard_cache();
+       }
+
+       if (!quiet) {
+               struct argv_array args = ARGV_ARRAY_INIT;
+               argv_array_push(&args, "status");
+               cmd_status(args.argc, args.argv, prefix);
+       }
+
+       return 0;
+}

builtin/stash--helper.c line 292

Quoted 10 lines
builtin/stash--helper.c +10 -0
+
+static int apply_stash(int argc, const char **argv, const char *prefix)
+{
+       const char *commit = NULL;
+       int index = 0;
+       struct stash_info info;
+       struct option options[] = {
+               OPT__QUIET(&quiet, N_("be quiet, only report errors")),
+               OPT_BOOL(0, "index", &index,
+                       N_("attempt to ininstate the index")),

"ininstate"??

Quoted 16 lines
builtin/stash--helper.c +16 -0
+               OPT_END()
+       };
+
+       argc = parse_options(argc, argv, prefix, options,
+                       git_stash_helper_apply_usage, 0);
+
+       if (argc == 1) {
+               commit = argv[0];
+       }
+
+       if (get_stash_info(&info, commit))
+               return -1;
+
+
+       return do_apply_stash(prefix, &info, index);
+}
#4 lore raw

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

Quoted 19 lines · Eric Sunshine
Quoted 4 lines · Joel Teichroeb
builtin/stash--helper.c new file +4 -0
+static int do_apply_stash(const char *prefix, struct stash_info *info, int index)
+{
+       if (index) {
+               if (!oidcmp(&info->b_tree, &info->i_tree) || !oidcmp(&c_tree, &info->i_tree)) {

builtin/stash--helper.c line 181

Quoted 10 lines
builtin/stash--helper.c new file +10 -0
+                       has_index = 0;
+               } else {
+                       struct child_process cp = CHILD_PROCESS_INIT;
+                       struct strbuf out = STRBUF_INIT;
+                       struct argv_array args = ARGV_ARRAY_INIT;
+                       cp.git_cmd = 1;
+                       argv_array_pushl(&cp.args, "diff-tree", "--binary", NULL);
+                       argv_array_pushf(&cp.args, "%s^2^..%s^2", sha1_to_hex(info->w_commit.hash), sha1_to_hex(info->w_commit.hash));
+                       if (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0))
+                               return -1;

Leaking 'out'?

Quoted 8 lines

builtin/stash--helper.c line 187

Quoted 6 lines
builtin/stash--helper.c new file +6 -0
+
+                       child_process_init(&cp);
+                       cp.git_cmd = 1;
+                       argv_array_pushl(&cp.args, "apply", "--cached", NULL);
+                       if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0))
+                               return -1;

Leaking 'out'.

It might be a good idea to have small functions encapsulating the forks of git commands. For example:

static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)

{
    struct child_process cp = CHILD_PROCESS_INIT;
    const char *w_commit_hex = oid_to_hex(w_commit);

    cp.git_cmd = 1;
    argv_array_pushl(&cp.args, "diff-tree", "--binary", NULL);
    argv_array_pushf(&cp.args, "%s^2^..%s^2", w_commit_hex, w_commit_hex);

    return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
}

static int apply_cached(struct strbuf *out)

{
    struct child_process cp = CHILD_PROCESS_INIT;

    cp.git_cmd = 1;
    argv_array_pushl(&cp.args, "apply", "--cached", NULL);
    return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);
}

This could help find leaks and maybe later make it easier to call libified code instead of forking a git command.

#5 lore raw

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

In reply to Joel Teichroeb
Quoted 5 lines · Joel Teichroeb
git-stash.sh +0 -0
index fc8f8ae640..92c084eb17 100755

git-stash.sh push) · line 718

Quoted 9 lines
git-stash.sh +2 -1
@@ -711,7 +711,8 @@ push)
        ;;
 apply)
        shift
-       apply_stash "$@"
+       cd "$START_DIR"
+       git stash--helper apply "$@"
        ;;
 clear)
        shift

It seems to me that the apply_stash() shell function is also used in pop_stash() and in apply_to_branch(). Can the new helper be used there too instead of apply_stash()? And then could apply_stash() be remove?

author
#6 lore raw

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

Quoted 3 lines · Christian Couder

It seems to me that the apply_stash() shell function is also used in pop_stash() and in apply_to_branch(). Can the new helper be used there too instead of apply_stash()? And then could apply_stash() be remove?

I wasn't really sure if I should remove code from the .sh file as it seems in the past the old .sh files have been kept around as examples. Has that been done for previous conversions?

#7 lore raw

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

Quoted 8 lines · Joel Teichroeb
Quoted 3 lines · Christian Couder

It seems to me that the apply_stash() shell function is also used in pop_stash() and in apply_to_branch(). Can the new helper be used there too instead of apply_stash()? And then could apply_stash() be remove?

I wasn't really sure if I should remove code from the .sh file as it seems in the past the old .sh files have been kept around as examples.

Yeah, some original shell scripts that have been converted are kept in contrib/examples/, but the shell code has still been removed from the .sh files when they were being converted.

Quoted 1 line

Has that been done for previous conversions?

I don't think there were some cases when the shell code was not removed. I haven't looked at all the conversions in details though.

A message that is not in the archive (sent off-list or not yet received): [email protected]

#9 lore raw

PATCH 1/1 Remove contrib/examples/*

Quoted 4 lines · Ævar Arnfjörð Bjarmason

There were some side discussions at Git Merge this year about how we should just update the README to tell users they can dig these up from the history if the need them, do that.

Quoted 8 lines

Looking at the "git log" for this directory we get quite a bit more patch churn than we should here, mainly from things fixing various tree-wide issues.

There's also confusion on the list occasionally about how these should be treated, "Re: [PATCH 1/4] stash: convert apply to
builtin" (<CA+CzEk9QpmHK_TSBwQfEedNqrcVSBp3xY7bdv1YA_KxePiFeXw@mail.gmail.com>) being the latest example of that.

The people on CC got this, but it seems the git ML rejected the message as it's too big. The abbreviated patches is here quoted inline, and at:
https://github.com/avar/git/commit/cc578c81c2cb2999b1a0b73954610bd74951c37b

Quoted 94 lines
Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>

---

Quoted 9 lines · Joel Teichroeb
Quoted 3 lines · Christian Couder

It seems to me that the apply_stash() shell function is also used in pop_stash() and in apply_to_branch(). Can the new helper be used there too instead of apply_stash()? And then could apply_stash() be remove?

I wasn't really sure if I should remove code from the .sh file as it seems in the past the old .sh files have been kept around as examples. Has that been done for previous conversions?

I was skimming this patch and it seemed to me like it would be more readable if the *.sh code was removed in the same change (if possible). It's easier to review like that.

Also, we should just stop maintainign contrib/examples/*.

 contrib/examples/README                |  23 +-
 contrib/examples/builtin-fetch--tool.c | 575 ---------------
 contrib/examples/git-am.sh             | 975 ------------------------
 contrib/examples/git-checkout.sh       | 302 --------
 contrib/examples/git-clean.sh          | 118 ---
 contrib/examples/git-clone.sh          | 525 -------------
 contrib/examples/git-commit.sh         | 639 ----------------
 contrib/examples/git-difftool.perl     | 481 ------------
 contrib/examples/git-fetch.sh          | 379 ----------
 contrib/examples/git-gc.sh             |  37 -
 contrib/examples/git-log.sh            |  15 -
 contrib/examples/git-ls-remote.sh      | 142 ----
 contrib/examples/git-merge-ours.sh     |  14 -
 contrib/examples/git-merge.sh          | 620 ----------------
 contrib/examples/git-notes.sh          | 121 ---
 contrib/examples/git-pull.sh           | 381 ----------
 contrib/examples/git-remote.perl       | 474 ------------
 contrib/examples/git-repack.sh         | 194 -----
 contrib/examples/git-rerere.perl       | 284 -------
 contrib/examples/git-reset.sh          | 106 ---
 contrib/examples/git-resolve.sh        | 112 ---
 contrib/examples/git-revert.sh         | 207 ------
 contrib/examples/git-svnimport.perl    | 976 -------------------------
 contrib/examples/git-svnimport.txt     | 179 -----
 contrib/examples/git-tag.sh            | 205 ------
 contrib/examples/git-verify-tag.sh     |  45 --
 contrib/examples/git-whatchanged.sh    |  28 -
 27 files changed, 20 insertions(+), 8137 deletions(-)
 delete mode 100644 contrib/examples/builtin-fetch--tool.c
 delete mode 100755 contrib/examples/git-am.sh
 delete mode 100755 contrib/examples/git-checkout.sh
 delete mode 100755 contrib/examples/git-clean.sh
 delete mode 100755 contrib/examples/git-clone.sh
 delete mode 100755 contrib/examples/git-commit.sh
 delete mode 100755 contrib/examples/git-difftool.perl
 delete mode 100755 contrib/examples/git-fetch.sh
 delete mode 100755 contrib/examples/git-gc.sh
 delete mode 100755 contrib/examples/git-log.sh
 delete mode 100755 contrib/examples/git-ls-remote.sh
 delete mode 100755 contrib/examples/git-merge-ours.sh
 delete mode 100755 contrib/examples/git-merge.sh
 delete mode 100755 contrib/examples/git-notes.sh
 delete mode 100755 contrib/examples/git-pull.sh
 delete mode 100755 contrib/examples/git-remote.perl
 delete mode 100755 contrib/examples/git-repack.sh
 delete mode 100755 contrib/examples/git-rerere.perl
 delete mode 100755 contrib/examples/git-reset.sh
 delete mode 100755 contrib/examples/git-resolve.sh
 delete mode 100755 contrib/examples/git-revert.sh
 delete mode 100755 contrib/examples/git-svnimport.perl
 delete mode 100644 contrib/examples/git-svnimport.txt
 delete mode 100755 contrib/examples/git-tag.sh
 delete mode 100755 contrib/examples/git-verify-tag.sh
 delete mode 100755 contrib/examples/git-whatchanged.sh
contrib/examples/README +11 -3
index 6946f3dd2a..18bc60b021 100644
@@ -1,3 +1,20 @@
-These are original scripted implementations, kept primarily for their
-reference value to any aspiring plumbing users who want to learn how
-pieces can be fit together.
+This directory used to contain scripted implementations of builtins
+that have since been rewritten in C.
+
+They have now been removed, but can be retrieved from an older commit
+that removed them from this directory.
+
+They're interesting for their reference value to any aspiring plumbing
+users who want to learn how pieces can be fit together, but in many
+cases have drifted enough from the actual implementations Git uses to
+be instructive.
+

contrib/examples/README line 20

Quoted 10 lines
contrib/examples/README +9 -0
+Other things that can be useful:
+
+ * Some commands such as git-gc wrap other commands, and what they're
+   doing behind the scenes can be seen by running them under
+   GIT_TRACE=1
+
+ * Doing `git log` on paths matching '*--helper.c' will show
+   incremental effort in the direction of moving existing shell
+   scripts to C.
[...]

The rest of this patch is deleting everything in contrib/examples/ that isn't the README.

#10 lore raw

Re: [PATCH] Remove contrib/examples/*

Quoted 4 lines · Ævar Arnfjörð Bjarmason

There were some side discussions at Git Merge this year about how we should just update the README to tell users they can dig these up from the history if the need them, do that.

Quoted 8 lines

Looking at the "git log" for this directory we get quite a bit more patch churn than we should here, mainly from things fixing various tree-wide issues.

There's also confusion on the list occasionally about how these should be treated, "Re: [PATCH 1/4] stash: convert apply to
builtin" (<CA+CzEk9QpmHK_TSBwQfEedNqrcVSBp3xY7bdv1YA_KxePiFeXw@mail.gmail.com>) being the latest example of that.

I'm in favor of this. I don't think I've ever come across this directory and _not_ been annoyed (because it was the result of a grep and was just cluttering the results). And I think your README change leaves a nice signpost for people who might be digging around for plumbing examples.

Quoted 3 lines

The people on CC got this, but it seems the git ML rejected the message as it's too big. The abbreviated patches is here quoted inline, and at:
https://github.com/avar/git/commit/cc578c81c2cb2999b1a0b73954610bd74951c37b

I was going to suggest re-sending with "-D", but it looks like "git apply" will not apply such a patch (even though it could in theory realize that the current blob matches the preimage sha1 and it would be safe to remove it).

-Peff

#11 lore raw

Re: [PATCH] Remove contrib/examples/*

In reply to a message not in the archive

Review

Quoted 3 lines · Ævar Arnfjörð Bjarmason

+ * Doing git log on paths matching '*--helper.c' will show

(quoted diff) +2 -0
+   incremental effort in the direction of moving existing shell
+   scripts to C.

It may be benefitial to remind readers of "--full-diff", e.g.

    $ git log --full-diff --stat -p -- "${foo}--helper.c"

here.

#12 lore raw

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

In reply to Joel Teichroeb
Quoted 1 line · Joel Teichroeb

---

Missing sign-off? I saw it's missing in the other patches as well.

Quoted 57 lines

[...]

builtin/stash--helper.c new file +56 -0
+static int do_apply_stash(const char *prefix, struct stash_info *info, int index)
+{
+	struct merge_options o;
+	struct object_id c_tree;
+	struct object_id index_tree;
+	const struct object_id *bases[1];
+	int bases_count = 1;
+	struct commit *result;
+	int ret;
+	int has_index = index;
+
+	read_cache_preload(NULL);
+	if (refresh_cache(REFRESH_QUIET))
+		return -1;
+
+	if (write_cache_as_tree(c_tree.hash, 0, NULL) || reset_tree(c_tree, 0, 0))
+		return error(_("Cannot apply a stash in the middle of a merge"));
+
+	if (index) {
+		if (!oidcmp(&info->b_tree, &info->i_tree) || !oidcmp(&c_tree, &info->i_tree)) {
+			has_index = 0;
+		} else {
+			struct child_process cp = CHILD_PROCESS_INIT;
+			struct strbuf out = STRBUF_INIT;
+			struct argv_array args = ARGV_ARRAY_INIT;
+			cp.git_cmd = 1;
+			argv_array_pushl(&cp.args, "diff-tree", "--binary", NULL);
+			argv_array_pushf(&cp.args, "%s^2^..%s^2", sha1_to_hex(info->w_commit.hash), sha1_to_hex(info->w_commit.hash));
+			if (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0))
+				return -1;
+
+			child_process_init(&cp);
+			cp.git_cmd = 1;
+			argv_array_pushl(&cp.args, "apply", "--cached", NULL);
+			if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0))
+				return -1;
+
+			strbuf_release(&out);
+			discard_cache();
+			read_cache();
+			if (write_cache_as_tree(index_tree.hash, 0, NULL))
+				return -1;
+
+			argv_array_push(&args, "reset");
+			cmd_reset(args.argc, args.argv, prefix);
+		}
+	}
+
+	if (info->has_u) {
+		struct child_process cp = CHILD_PROCESS_INIT;
+		struct child_process cp2 = CHILD_PROCESS_INIT;
+		int res;
+
+		cp.git_cmd = 1;
+		argv_array_push(&cp.args, "read-tree");
+		argv_array_push(&cp.args, sha1_to_hex(info->u_tree.hash));

builtin/stash--helper.c line 217

Quoted 10 lines
builtin/stash--helper.c new file +10 -0
+		argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", stash_index_path);
+
+		cp2.git_cmd = 1;
+		argv_array_pushl(&cp2.args, "checkout-index", "--all", NULL);
+		argv_array_pushf(&cp2.env_array, "GIT_INDEX_FILE=%s", stash_index_path);
+
+		res = run_command(&cp) || run_command(&cp2);
+		remove_path(stash_index_path);
+		if (res)
+			return error(_("Could not restore untracked files from stash"));

A minor change in behaviour here is that we are removing the temporary index file unconditionally here, while we would previously only remove it if both 'read-tree' and 'checkout-index' would succeed.

I don't think that's a bad thing, we probably don't want users to try and use that index file in any way, and I doubt that's part of anyones workflow, so I think cleaning it up makes sense.

Quoted 15 lines
builtin/stash--helper.c new file +15 -0
+	}
+
+	init_merge_options(&o);
+
+	o.branch1 = "Updated upstream";
+	o.branch2 = "Stashed changes";
+
+	if (!hashcmp(info->b_tree.hash, c_tree.hash))
+		o.branch1 = "Version stash was based on";
+
+	if (quiet)
+		o.verbosity = 0;
+
+	if (o.verbosity >= 3)
+		printf_ln(_("Merging %s with %s"), o.branch1, o.branch2);

builtin/stash--helper.c line 243

Quoted 10 lines
builtin/stash--helper.c new file +10 -0
+
+	bases[0] = &info->b_tree;
+
+	ret = merge_recursive_generic(&o, &c_tree, &info->w_tree, bases_count, bases, &result);
+	if (ret != 0) {
+		struct argv_array args = ARGV_ARRAY_INIT;
+		argv_array_push(&args, "rerere");
+		cmd_rerere(args.argc, args.argv, prefix);
+		if (index)
+			printf_ln(_("Index was not unstashed."));

Minor nit: I think the above should be 'fprintf_ln(stderr, ...)' to match what we currently have.

Quoted 5 lines
builtin/stash--helper.c new file +4 -0
+
+		return ret;
+	}
+
[...]
author
#13 lore raw

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

Quoted 5 lines · Thomas Gummerer
Quoted 1 line · Joel Teichroeb

---

Missing sign-off? I saw it's missing in the other patches as well.

Thanks! I always forget to add a sign-off.

Quoted 25 lines
Quoted 10 lines

[...]

builtin/stash--helper.c new file +9 -0
+
+     if (info->has_u) {
+             struct child_process cp = CHILD_PROCESS_INIT;
+             struct child_process cp2 = CHILD_PROCESS_INIT;
+             int res;
+
+             cp.git_cmd = 1;
+             argv_array_push(&cp.args, "read-tree");
+             argv_array_push(&cp.args, sha1_to_hex(info->u_tree.hash));

builtin/stash--helper.c line 217

Quoted 10 lines
builtin/stash--helper.c new file +10 -0
+             argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", stash_index_path);
+
+             cp2.git_cmd = 1;
+             argv_array_pushl(&cp2.args, "checkout-index", "--all", NULL);
+             argv_array_pushf(&cp2.env_array, "GIT_INDEX_FILE=%s", stash_index_path);
+
+             res = run_command(&cp) || run_command(&cp2);
+             remove_path(stash_index_path);
+             if (res)
+                     return error(_("Could not restore untracked files from stash"));

A minor change in behaviour here is that we are removing the temporary index file unconditionally here, while we would previously only remove it if both 'read-tree' and 'checkout-index' would succeed.

Quoted 4 lines

I don't think that's a bad thing, we probably don't want users to try and use that index file in any way, and I doubt that's part of anyones workflow, so I think cleaning it up makes sense.

I'm not sure about that. The shell script has a trap near the start in order to clean up the temp index, unless I'm understanding the shell script incorrectly.

#14 lore raw

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

In reply to Joel Teichroeb
Quoted 33 lines · Joel Teichroeb

---
[...]

builtin/stash--helper.c new file +31 -0
+
+static const char *ref_stash = "refs/stash";
+static int quiet;
+static char stash_index_path[PATH_MAX];
+
+struct stash_info {
+	struct object_id w_commit;
+	struct object_id b_commit;
+	struct object_id i_commit;
+	struct object_id u_commit;
+	struct object_id w_tree;
+	struct object_id b_tree;
+	struct object_id i_tree;
+	struct object_id u_tree;
+	const char *message;
+	const char *revision;
+	int is_stash_ref;
+	int has_u;
+	const char *patch;
+};
+
+static int get_stash_info(struct stash_info *info, const char *commit)
+{
+	struct strbuf w_commit_rev = STRBUF_INIT;
+	struct strbuf b_commit_rev = STRBUF_INIT;
+	struct strbuf w_tree_rev = STRBUF_INIT;
+	struct strbuf b_tree_rev = STRBUF_INIT;
+	struct strbuf i_tree_rev = STRBUF_INIT;
+	struct strbuf u_tree_rev = STRBUF_INIT;
+	struct strbuf commit_buf = STRBUF_INIT;
+	struct strbuf symbolic = STRBUF_INIT;

builtin/stash--helper.c line 62

Quoted 10 lines
builtin/stash--helper.c new file +10 -0
+	struct strbuf out = STRBUF_INIT;
+	int ret;
+	const char *revision = commit;
+	char *end_of_rev;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	info->is_stash_ref = 0;
+
+	if (commit == NULL) {
+		strbuf_addf(&commit_buf, "%s@{0}", ref_stash);
+		revision = commit_buf.buf;

Before setting up the revisions here, as is done below, we used to check if a stash even exists, if no commit was given. So in a repository with no stashes we would die with "No stash entries found", while now we die with "error: refs/stash@{0} is not a valid reference". I think the error message we had previously was slightly nicer, and we should try to keep it.

Quoted 16 lines
builtin/stash--helper.c new file +16 -0
+	} else if (strspn(commit, "0123456789") == strlen(commit)) {
+		strbuf_addf(&commit_buf, "%s@{%s}", ref_stash, commit);
+		revision = commit_buf.buf;
+	}
+	info->revision = revision;
+
+	strbuf_addf(&w_commit_rev, "%s", revision);
+	strbuf_addf(&b_commit_rev, "%s^1", revision);
+	strbuf_addf(&w_tree_rev, "%s:", revision);
+	strbuf_addf(&b_tree_rev, "%s^1:", revision);
+	strbuf_addf(&i_tree_rev, "%s^2:", revision);
+
+	ret = !get_oid(w_commit_rev.buf, &info->w_commit) &&
+		!get_oid(b_commit_rev.buf, &info->b_commit) &&
+		!get_oid(w_tree_rev.buf, &info->w_tree) &&
+		!get_oid(b_tree_rev.buf, &info->b_tree) &&

builtin/stash--helper.c line 88

Quoted 10 lines
builtin/stash--helper.c new file +10 -0
+		!get_oid(i_tree_rev.buf, &info->i_tree);
+
+	strbuf_release(&w_commit_rev);
+	strbuf_release(&b_commit_rev);
+	strbuf_release(&w_tree_rev);
+	strbuf_release(&b_tree_rev);
+	strbuf_release(&i_tree_rev);
+
+	if (!ret)
+		return error(_("%s is not a valid reference"), revision);

We used to distinguish between "not a valid reference" and "not a stash-like commit" here. I think just doing the first 'get_oid' before the others, and returning the error if that fails, and then doing the rest and returning the "not a stash-like commit" if one of the other 'get_oid' calls fails would work, although I did not test it.

Quoted 23 lines
builtin/stash--helper.c new file +22 -0
+
+	strbuf_addf(&u_tree_rev, "%s^3:", revision);
+
+	info->has_u = !get_oid(u_tree_rev.buf, &info->u_tree);
+
+	strbuf_release(&u_tree_rev);
+
+	end_of_rev = strchrnul(revision, '@');
+	strbuf_add(&symbolic, revision, end_of_rev - revision);
+	cp.git_cmd = 1;
+	argv_array_pushl(&cp.args, "rev-parse", "--symbolic-full-name", NULL);
+	argv_array_pushf(&cp.args, "%s", symbolic.buf);
+	strbuf_release(&symbolic);
+	pipe_command(&cp, NULL, 0, &out, 0, NULL, 0);
+
+	if (out.len - 1 == strlen(ref_stash))
+		info->is_stash_ref = !strncmp(out.buf, ref_stash, out.len - 1);
+	strbuf_release(&out);
+
+	return 0;
+}
+
[...]
author
#15 lore raw

PATCH 2/4 stash: convert branch to builtin

In reply to Joel Teichroeb
---
 builtin/stash--helper.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 git-stash.sh            |  3 ++-
 2 files changed, 46 insertions(+), 1 deletion(-)
builtin/stash--helper.c +44 -0
index e9a9574f40..18c4aba665 100644
@@ -12,6 +12,7 @@
 
 static const char * const git_stash_helper_usage[] = {
 	N_("git stash--helper apply [--index] [-q|--quiet] [<stash>]"),
+	N_("git stash--helper branch <branchname> [<stash>]"),
 	NULL
 };
 

@@ -20,6 +21,11 @@ static const char * const git_stash_helper_apply_usage[] = {
 	NULL
 };
 
+static const char * const git_stash_helper_branch_usage[] = {
+	N_("git stash--helper branch <branchname> [<stash>]"),
+	NULL
+};
+
 static const char *ref_stash = "refs/stash";
 static int quiet;
 static char stash_index_path[PATH_MAX];

@@ -307,6 +313,42 @@ static int apply_stash(int argc, const char **argv, const char *prefix)
 	return do_apply_stash(prefix, &info, index);
 }
 
+static int branch_stash(int argc, const char **argv, const char *prefix)
+{
+	const char *commit = NULL, *branch = NULL;
+	int ret;
+	struct argv_array args = ARGV_ARRAY_INIT;
+	struct stash_info info;
+	struct option options[] = {
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+			git_stash_helper_branch_usage, 0);
+
+	if (argc != 0) {
+		branch = argv[0];
+		if (argc == 2)
+			commit = argv[1];
+	}
+
+	if (get_stash_info(&info, commit))
+		return -1;
+
+	argv_array_pushl(&args, "checkout", "-b", NULL);
+	argv_array_push(&args, branch);
+	argv_array_push(&args, sha1_to_hex(info.b_commit.hash));
+	ret = cmd_checkout(args.argc, args.argv, prefix);
+	if (ret)
+		return -1;
+
+	ret = do_apply_stash(prefix, &info, 1);
+	if (!ret && info.is_stash_ref)
+		ret = do_drop_stash(prefix, &info);
+
+	return ret;
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
 	int result = 0;

@@ -329,6 +371,8 @@ int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_stash_helper_usage, options);
 	else if (!strcmp(argv[0], "apply"))
 		result = apply_stash(argc, argv, prefix);
+	else if (!strcmp(argv[0], "branch"))
+		result = branch_stash(argc, argv, prefix);
 	else {
 		error(_("unknown subcommand: %s"), argv[0]);
 		usage_with_options(git_stash_helper_usage, options);
git-stash.sh +2 -1
index 92c084eb17..360643ad4e 100755
@@ -736,7 +736,8 @@ pop)
 	;;
 branch)
 	shift
-	apply_to_branch "$@"
+	cd "$START_DIR"
+	git stash--helper branch "$@"
 	;;
 *)
 	case $# in
Signature · 1 line
2.16.2
#16 lore raw

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

Quoted 4 lines · Joel Teichroeb
builtin/stash--helper.c +2 -0
@@ -307,6 +313,42 @@ static int apply_stash(int argc, const char **argv, const char *prefix)
+static int branch_stash(int argc, const char **argv, const char *prefix)
+{

builtin/stash--helper.c static int apply_stash(int argc, const char **argv, const char *prefix) · line 324

Quoted 10 lines
builtin/stash--helper.c +10 -0
+       const char *commit = NULL, *branch = NULL;
+
+       argc = parse_options(argc, argv, prefix, options,
+                       git_stash_helper_branch_usage, 0);
+
+       if (argc != 0) {
+               branch = argv[0];
+               if (argc == 2)
+                       commit = argv[1];
+       }

This seems fragile. What happens if there are three args?

Quoted 16 lines
builtin/stash--helper.c +16 -0
+       if (get_stash_info(&info, commit))
+               return -1;
+
+       argv_array_pushl(&args, "checkout", "-b", NULL);
+       argv_array_push(&args, branch);
+       argv_array_push(&args, sha1_to_hex(info.b_commit.hash));
+       ret = cmd_checkout(args.argc, args.argv, prefix);
+       if (ret)
+               return -1;
+
+       ret = do_apply_stash(prefix, &info, 1);
+       if (!ret && info.is_stash_ref)
+               ret = do_drop_stash(prefix, &info);
+
+       return ret;
+}
#17 lore raw

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

In reply to Joel Teichroeb
Quoted 5 lines · Joel Teichroeb
git-stash.sh +0 -0
index 92c084eb17..360643ad4e 100755

git-stash.sh pop) · line 743

Quoted 9 lines
git-stash.sh +2 -1
@@ -736,7 +736,8 @@ pop)
        ;;
 branch)
        shift
-       apply_to_branch "$@"
+       cd "$START_DIR"
+       git stash--helper branch "$@"
        ;;
 *)
        case $# in

Can the apply_to_branch() shell function be removed from git-stash.sh?

#18 lore raw

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

In reply to Joel Teichroeb
Quoted 44 lines · Joel Teichroeb
---
 builtin/stash--helper.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 git-stash.sh            |  3 ++-
 2 files changed, 46 insertions(+), 1 deletion(-)
builtin/stash--helper.c +17 -0
index e9a9574f40..18c4aba665 100644
@@ -12,6 +12,7 @@
 
 static const char * const git_stash_helper_usage[] = {
 	N_("git stash--helper apply [--index] [-q|--quiet] [<stash>]"),
+	N_("git stash--helper branch <branchname> [<stash>]"),
 	NULL
 };
 

@@ -20,6 +21,11 @@ static const char * const git_stash_helper_apply_usage[] = {
 	NULL
 };
 
+static const char * const git_stash_helper_branch_usage[] = {
+	N_("git stash--helper branch <branchname> [<stash>]"),
+	NULL
+};
+
 static const char *ref_stash = "refs/stash";
 static int quiet;
 static char stash_index_path[PATH_MAX];

@@ -307,6 +313,42 @@ static int apply_stash(int argc, const char **argv, const char *prefix)
 	return do_apply_stash(prefix, &info, index);
 }
 
+static int branch_stash(int argc, const char **argv, const char *prefix)
+{
+	const char *commit = NULL, *branch = NULL;
+	int ret;
+	struct argv_array args = ARGV_ARRAY_INIT;
+	struct stash_info info;
+	struct option options[] = {
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,

builtin/stash--helper.c static int apply_stash(int argc, const char **argv, const char *prefix) · line 336

Quoted 10 lines
builtin/stash--helper.c +10 -0
+			git_stash_helper_branch_usage, 0);
+
+	if (argc != 0) {
+		branch = argv[0];
+		if (argc == 2)
+			commit = argv[1];
+	}
+
+	if (get_stash_info(&info, commit))
+		return -1;

I see this is supposed to do something similar to what 'assert_stash_like' was doing. However we never end up die'ing with "... is not a a stash-like commit" here from what I can see. I think I can see where this is coming from, and I missed it when reading over 1/4 here. I'll go back and comment there, where I think we're going slightly wrong.

Either way while I tripped over the 'get_stash_info' call here, I think it's the right thing to do.

builtin/stash--helper.c static int apply_stash(int argc, const char **argv, const char *prefix) · line 347

Quoted 10 lines
builtin/stash--helper.c +10 -0
+	argv_array_pushl(&args, "checkout", "-b", NULL);
+	argv_array_push(&args, branch);
+	argv_array_push(&args, sha1_to_hex(info.b_commit.hash));
+	ret = cmd_checkout(args.argc, args.argv, prefix);
+	if (ret)
+		return -1;
+
+	ret = do_apply_stash(prefix, &info, 1);
+	if (!ret && info.is_stash_ref)
+		ret = do_drop_stash(prefix, &info);

'do_drop_stash' is only defined in the next patch. Maybe maybe 2/4 and 3/4 need to swap places?

All patches should compile individually, and all tests should pass for each patch, so we maintain bisectability of the codebase.

Quoted 33 lines
builtin/stash--helper.c +6 -0
+
+	return ret;
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
 	int result = 0;

@@ -329,6 +371,8 @@ int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_stash_helper_usage, options);
 	else if (!strcmp(argv[0], "apply"))
 		result = apply_stash(argc, argv, prefix);
+	else if (!strcmp(argv[0], "branch"))
+		result = branch_stash(argc, argv, prefix);
 	else {
 		error(_("unknown subcommand: %s"), argv[0]);
 		usage_with_options(git_stash_helper_usage, options);
git-stash.sh +2 -1
index 92c084eb17..360643ad4e 100755
@@ -736,7 +736,8 @@ pop)
 	;;
 branch)
 	shift
-	apply_to_branch "$@"
+	cd "$START_DIR"
+	git stash--helper branch "$@"
 	;;
 *)
 	case $# in
Signature · 1 line
2.16.2
author
#19 lore raw

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

In reply to Joel Teichroeb
---
 builtin/stash--helper.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++
 git-stash.sh            |  4 +--
 2 files changed, 95 insertions(+), 2 deletions(-)
builtin/stash--helper.c +93 -0
index 18c4aba665..1598b82ac2 100644
@@ -11,8 +11,15 @@
 #include "dir.h"
 
 static const char * const git_stash_helper_usage[] = {
+	N_("git stash--helper drop [-q|--quiet] [<stash>]"),
 	N_("git stash--helper apply [--index] [-q|--quiet] [<stash>]"),
 	N_("git stash--helper branch <branchname> [<stash>]"),
+	N_("git stash--helper clear"),
+	NULL
+};
+
+static const char * const git_stash_helper_drop_usage[] = {
+	N_("git stash--helper drop [-q|--quiet] [<stash>]"),
 	NULL
 };
 

@@ -26,6 +33,11 @@ static const char * const git_stash_helper_branch_usage[] = {
 	NULL
 };
 
+static const char * const git_stash_helper_clear_usage[] = {
+	N_("git stash--helper clear"),
+	NULL
+};
+
 static const char *ref_stash = "refs/stash";
 static int quiet;
 static char stash_index_path[PATH_MAX];

@@ -114,6 +126,29 @@ static int get_stash_info(struct stash_info *info, const char *commit)
 	return 0;
 }
 
+static int do_clear_stash(void)
+{
+	struct object_id obj;
+	if (get_oid(ref_stash, &obj))
+		return 0;
+
+	return delete_ref(NULL, ref_stash, &obj, 0);
+}
+
+static int clear_stash(int argc, const char **argv, const char *prefix)
+{
+	struct option options[] = {
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options, git_stash_helper_clear_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+
+	if (argc != 0)
+		return error(_("git stash--helper clear with parameters is unimplemented"));
+
+	return do_clear_stash();
+}
+
 static int reset_tree(struct object_id i_tree, int update, int reset)
 {
 	struct unpack_trees_options opts;

@@ -313,6 +348,60 @@ static int apply_stash(int argc, const char **argv, const char *prefix)
 	return do_apply_stash(prefix, &info, index);
 }
 
+static int do_drop_stash(const char *prefix, struct stash_info *info)
+{
+	struct argv_array args = ARGV_ARRAY_INIT;
+	int ret;
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	argv_array_pushl(&args, "reflog", "delete", "--updateref", "--rewrite", NULL);
+	argv_array_push(&args, info->revision);
+	ret = cmd_reflog(args.argc, args.argv, prefix);
+	if (!ret) {
+		if (!quiet) {
+			printf(_("Dropped %s (%s)\n"), info->revision, sha1_to_hex(info->w_commit.hash));
+		}
+	} else {
+		return error(_("%s: Could not drop stash entry"), info->revision);
+	}
+
+	cp.git_cmd = 1;
+	/* Even though --quiet is specified, rev-parse still outputs the hash */
+	cp.no_stdout = 1;
+	argv_array_pushl(&cp.args, "rev-parse", "--verify", "--quiet", NULL);
+	argv_array_pushf(&cp.args, "%s@{0}", ref_stash);
+	ret = run_command(&cp);
+
+	if (ret)
+		do_clear_stash();
+
+	return 0;
+}
+
+static int drop_stash(int argc, const char **argv, const char *prefix)
+{
+	const char *commit = NULL;
+	struct stash_info info;
+	struct option options[] = {
+		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+			git_stash_helper_drop_usage, 0);
+
+	if (argc == 1)
+		commit = argv[0];
+
+	if (get_stash_info(&info, commit))
+		return -1;
+
+	if (!info.is_stash_ref)
+		return error(_("'%s' is not a stash reference"), commit);
+
+	return do_drop_stash(prefix, &info);
+}
+
 static int branch_stash(int argc, const char **argv, const char *prefix)
 {
 	const char *commit = NULL, *branch = NULL;

@@ -371,6 +460,10 @@ int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_stash_helper_usage, options);
 	else if (!strcmp(argv[0], "apply"))
 		result = apply_stash(argc, argv, prefix);
+	else if (!strcmp(argv[0], "clear"))
+		result = clear_stash(argc, argv, prefix);
+	else if (!strcmp(argv[0], "drop"))
+		result = drop_stash(argc, argv, prefix);
 	else if (!strcmp(argv[0], "branch"))
 		result = branch_stash(argc, argv, prefix);
 	else {
git-stash.sh +2 -2
index 360643ad4e..54d0a6c21f 100755
@@ -716,7 +716,7 @@ apply)
 	;;
 clear)
 	shift
-	clear_stash "$@"
+	git stash--helper clear "$@"
 	;;
 create)
 	shift

@@ -728,7 +728,7 @@ store)
 	;;
 drop)
 	shift
-	drop_stash "$@"
+	git stash--helper drop "$@"
 	;;
 pop)
 	shift
Signature · 1 line
2.16.2
#20 lore raw

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

builtin/stash--helper.c static int apply_stash(int argc, const char **argv, const char *prefix) · line 363

Quoted 7 lines
builtin/stash--helper.c +7 -0
+       argv_array_pushl(&args, "reflog", "delete", "--updateref", "--rewrite", NULL);
+       argv_array_push(&args, info->revision);
+       ret = cmd_reflog(args.argc, args.argv, prefix);
+       if (!ret) {
+               if (!quiet) {
+                       printf(_("Dropped %s (%s)\n"), info->revision, sha1_to_hex(info->w_commit.hash));
+               }

The brackets are not needed.

Quoted 3 lines
builtin/stash--helper.c +3 -0
+       } else {
+               return error(_("%s: Could not drop stash entry"), info->revision);
+       }
#21 lore raw

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

In reply to Joel Teichroeb
Quoted 6 lines · Joel Teichroeb
builtin/stash--helper.c +4 -0
@@ -313,6 +348,60 @@ static int apply_stash(int argc, const char **argv, const char *prefix)
+static int drop_stash(int argc, const char **argv, const char *prefix)
+{
+       const char *commit = NULL;
+       struct stash_info info;

builtin/stash--helper.c static int apply_stash(int argc, const char **argv, const char *prefix) · line 361

Quoted 10 lines
builtin/stash--helper.c +10 -0
+       struct option options[] = {
+               OPT__QUIET(&quiet, N_("be quiet, only report errors")),
+               OPT_END()
+       };
+
+       argc = parse_options(argc, argv, prefix, options,
+                       git_stash_helper_drop_usage, 0);
+
+       if (argc == 1)
+               commit = argv[0];

Seems fragile. What if there are two arguments?

Quoted 8 lines
builtin/stash--helper.c +8 -0
+       if (get_stash_info(&info, commit))
+               return -1;
+
+       if (!info.is_stash_ref)
+               return error(_("'%s' is not a stash reference"), commit);
+
+       return do_drop_stash(prefix, &info);
+}
author
#22 lore raw

PATCH 4/4 stash: convert pop to builtin

In reply to Joel Teichroeb
---
 builtin/stash--helper.c | 38 ++++++++++++++++++++++++++++++++++++++
 git-stash.sh            |  3 ++-
 2 files changed, 40 insertions(+), 1 deletion(-)
builtin/stash--helper.c +38 -0
index 1598b82ac2..b912f84c97 100644
@@ -12,6 +12,7 @@
 
 static const char * const git_stash_helper_usage[] = {
 	N_("git stash--helper drop [-q|--quiet] [<stash>]"),
+	N_("git stash--helper pop [--index] [-q|--quiet] [<stash>]"),
 	N_("git stash--helper apply [--index] [-q|--quiet] [<stash>]"),
 	N_("git stash--helper branch <branchname> [<stash>]"),
 	N_("git stash--helper clear"),

@@ -23,6 +24,11 @@ static const char * const git_stash_helper_drop_usage[] = {
 	NULL
 };
 
+static const char * const git_stash_helper_pop_usage[] = {
+	N_("git stash--helper pop [--index] [-q|--quiet] [<stash>]"),
+	NULL
+};
+
 static const char * const git_stash_helper_apply_usage[] = {
 	N_("git stash--helper apply [--index] [-q|--quiet] [<stash>]"),
 	NULL

@@ -402,6 +408,36 @@ static int drop_stash(int argc, const char **argv, const char *prefix)
 	return do_drop_stash(prefix, &info);
 }
 
+static int pop_stash(int argc, const char **argv, const char *prefix)
+{
+	int index = 0;
+	const char *commit = NULL;
+	struct stash_info info;
+	struct option options[] = {
+		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
+		OPT_BOOL(0, "index", &index,
+			N_("attempt to ininstate the index")),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+			git_stash_helper_pop_usage, 0);
+
+	if (argc == 1)
+		commit = argv[0];
+
+	if (get_stash_info(&info, commit))
+		return -1;
+
+	if (!info.is_stash_ref)
+		return error(_("'%s' is not a stash reference"), commit);
+
+	if (do_apply_stash(prefix, &info, index))
+		return -1;
+
+	return do_drop_stash(prefix, &info);
+}
+
 static int branch_stash(int argc, const char **argv, const char *prefix)
 {
 	const char *commit = NULL, *branch = NULL;

@@ -464,6 +500,8 @@ int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 		result = clear_stash(argc, argv, prefix);
 	else if (!strcmp(argv[0], "drop"))
 		result = drop_stash(argc, argv, prefix);
+	else if (!strcmp(argv[0], "pop"))
+		result = pop_stash(argc, argv, prefix);
 	else if (!strcmp(argv[0], "branch"))
 		result = branch_stash(argc, argv, prefix);
 	else {
git-stash.sh +2 -1
index 54d0a6c21f..d595bbaf64 100755
@@ -732,7 +732,8 @@ drop)
 	;;
 pop)
 	shift
-	pop_stash "$@"
+	cd "$START_DIR"
+	git stash--helper pop "$@"
 	;;
 branch)
 	shift
Signature · 1 line
2.16.2
#23 lore raw

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

builtin/stash--helper.c static int drop_stash(int argc, const char **argv, const char *prefix) · line 416

Quoted 11 lines · Joel Teichroeb
builtin/stash--helper.c +9 -0
@@ -402,6 +408,36 @@ static int drop_stash(int argc, const char **argv, const char *prefix)
+static int pop_stash(int argc, const char **argv, const char *prefix)
+{
+       int index = 0;
+       const char *commit = NULL;
+       struct stash_info info;
+       struct option options[] = {
+               OPT__QUIET(&quiet, N_("be quiet, only report errors")),
+               OPT_BOOL(0, "index", &index,
+                       N_("attempt to ininstate the index")),

"ininstate"??

builtin/stash--helper.c static int drop_stash(int argc, const char **argv, const char *prefix) · line 427

Quoted 8 lines
builtin/stash--helper.c +8 -0
+               OPT_END()
+       };
+
+       argc = parse_options(argc, argv, prefix, options,
+                       git_stash_helper_pop_usage, 0);
+
+       if (argc == 1)
+               commit = argv[0];

Seems fragile. What if there are two arguments?

Quoted 11 lines
builtin/stash--helper.c +11 -0
+       if (get_stash_info(&info, commit))
+               return -1;
+
+       if (!info.is_stash_ref)
+               return error(_("'%s' is not a stash reference"), commit);
+
+       if (do_apply_stash(prefix, &info, index))
+               return -1;
+
+       return do_drop_stash(prefix, &info);
+}
#24 lore raw

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

In reply to Joel Teichroeb
Quoted 46 lines · Joel Teichroeb
---
 builtin/stash--helper.c | 38 ++++++++++++++++++++++++++++++++++++++
 git-stash.sh            |  3 ++-
 2 files changed, 40 insertions(+), 1 deletion(-)
builtin/stash--helper.c +19 -0
index 1598b82ac2..b912f84c97 100644
@@ -12,6 +12,7 @@
 
 static const char * const git_stash_helper_usage[] = {
 	N_("git stash--helper drop [-q|--quiet] [<stash>]"),
+	N_("git stash--helper pop [--index] [-q|--quiet] [<stash>]"),
 	N_("git stash--helper apply [--index] [-q|--quiet] [<stash>]"),
 	N_("git stash--helper branch <branchname> [<stash>]"),
 	N_("git stash--helper clear"),

@@ -23,6 +24,11 @@ static const char * const git_stash_helper_drop_usage[] = {
 	NULL
 };
 
+static const char * const git_stash_helper_pop_usage[] = {
+	N_("git stash--helper pop [--index] [-q|--quiet] [<stash>]"),
+	NULL
+};
+
 static const char * const git_stash_helper_apply_usage[] = {
 	N_("git stash--helper apply [--index] [-q|--quiet] [<stash>]"),
 	NULL

@@ -402,6 +408,36 @@ static int drop_stash(int argc, const char **argv, const char *prefix)
 	return do_drop_stash(prefix, &info);
 }
 
+static int pop_stash(int argc, const char **argv, const char *prefix)
+{
+	int index = 0;
+	const char *commit = NULL;
+	struct stash_info info;
+	struct option options[] = {
+		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
+		OPT_BOOL(0, "index", &index,
+			N_("attempt to ininstate the index")),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,

builtin/stash--helper.c static int drop_stash(int argc, const char **argv, const char *prefix) · line 433

Quoted 10 lines
builtin/stash--helper.c +10 -0
+			git_stash_helper_pop_usage, 0);
+
+	if (argc == 1)
+		commit = argv[0];
+
+	if (get_stash_info(&info, commit))
+		return -1;
+
+	if (!info.is_stash_ref)
+		return error(_("'%s' is not a stash reference"), commit);

The pattern above appears twice now, is it worth factoring it out into a separate function, similar to 'assert_stash_ref'?

builtin/stash--helper.c static int drop_stash(int argc, const char **argv, const char *prefix) · line 436

Quoted 3 lines
builtin/stash--helper.c +3 -0
+
+	if (do_apply_stash(prefix, &info, index))
+		return -1;

If we fail, currently we print "The stash entry is kept in case you need it again", which we are loosing here. I think that's useful output in case the 'apply' command fails, especially in the case of a merge conflict, where I think the 'apply' will fail as well, and the user may be confused whether/why the stash is not dropped.

Quoted 33 lines
builtin/stash--helper.c +6 -0
+
+	return do_drop_stash(prefix, &info);
+}
+
 static int branch_stash(int argc, const char **argv, const char *prefix)
 {
 	const char *commit = NULL, *branch = NULL;

@@ -464,6 +500,8 @@ int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 		result = clear_stash(argc, argv, prefix);
 	else if (!strcmp(argv[0], "drop"))
 		result = drop_stash(argc, argv, prefix);
+	else if (!strcmp(argv[0], "pop"))
+		result = pop_stash(argc, argv, prefix);
 	else if (!strcmp(argv[0], "branch"))
 		result = branch_stash(argc, argv, prefix);
 	else {
git-stash.sh +2 -1
index 54d0a6c21f..d595bbaf64 100755
@@ -732,7 +732,8 @@ drop)
 	;;
 pop)
 	shift
-	pop_stash "$@"
+	cd "$START_DIR"
+	git stash--helper pop "$@"
 	;;
 branch)
 	shift
Signature · 1 line
2.16.2
#25 lore raw
In reply to Joel Teichroeb
Quoted 8 lines · Joel Teichroeb

I've been working on converting all of git stash to be a builtin, however it's hard to get it all working at once with limited time, so I've moved around half of it to a new stash--helper builtin and called these functions from the shell script. Once this is stabalized, it should be easier to convert the rest of the commands one at a time without breaking anything.

Quoted 4 lines

I've sent most of this code before, but that was targetting a full replacement of stash. The code is overall the same, but with some code review changes and updates for internal api changes.

Thanks for splitting this up into multiple patches, I found that much more pleasant to review, and thanks for your continued work on this :)

Quoted 22 lines

Since there seems to be interest from GSOC students who want to work on converting builtins, I figured I should finish what I have that works now so they could build on top of it.

Joel Teichroeb (4):
stash: convert apply to builtin
stash: convert branch to builtin
stash: convert drop and clear to builtin
stash: convert pop to builtin

 .gitignore              |   1 +
 Makefile                |   1 +
 builtin.h               |   1 +
 builtin/stash--helper.c | 514 ++++++++++++++++++++++++++++++++++++++++++++++++
 git-stash.sh            |  13 +-
 git.c                   |   1 +
 6 files changed, 526 insertions(+), 5 deletions(-)
 create mode 100644 builtin/stash--helper.c
Signature · 1 line
2.16.2