Re: [PATCH v5 0/5] Convert some stash functionality to a builtin

[Joel Teichroeb]

Re-sending, but this time in plain text (why doesn't gmail for android
support that...)

On Mon, Apr 30, 2018 at 7:35 AM, Joel Teichroeb <j...@teichroeb.net> wrote:
> Hi Paul,
>
> That sounds great to me!
>
> Thanks,
> Joel
>
>
> On Sat, Apr 28, 2018, 3:06 PM Paul-Sebastian Ungureanu
> <ungureanupaulsebast...@gmail.com> wrote:
>>
>> Hello Joel,
>>
>> >> 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.
>>
>> First of all, I must thank you for submitting this series of patches. It
>> is a great starting point to convert 'git stash'.
>>
>> I would like to continue your work on "git stash" if that is fine with
>> you. I will continue to build on top of it, starting with applying some
>> patches in order to implement what was already suggested in this thread.
>>   During the summer, I am planning to finish this process and,
>> hopefully, have a 100% C, built-in 'git stash'.
>>
>> Best regards,
>> Paul

[PATCH v5 2/5] stash: convert apply to builtin

[Joel Teichroeb]

Add a bulitin helper for performing stash commands. Converting
all at once proved hard to review, so starting with just apply
let conversion get started without the other command being
finished.

The helper is being implemented as a drop in replacement for
stash so that when it is complete it can simply be renamed and
the shell script deleted.

Delete the contents of the apply_stash shell function and replace
it with a call to stash--helper apply until pop is also
converted.

Signed-off-by: Joel Teichroeb <j...@teichroeb.net>
---
 .gitignore  |   1 +
 Makefile|   1 +
 builtin.h   |   1 +
 builtin/stash--helper.c | 431 
 git-stash.sh|  75 +
 git.c   |   1 +
 6 files changed, 440 insertions(+), 70 deletions(-)
 create mode 100644 builtin/stash--helper.c

diff --git a/.gitignore b/.gitignore
index 833ef3b0b7..296d5f376d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -152,6 +152,7 @@
 /git-show-ref
 /git-stage
 /git-stash
+/git-stash--helper
 /git-status
 /git-stripspace
 /git-submodule
diff --git a/Makefile b/Makefile
index 96f6138f63..6cfdbe9a32 100644
--- a/Makefile
+++ b/Makefile
@@ -1028,6 +1028,7 @@ BUILTIN_OBJS += builtin/send-pack.o
 BUILTIN_OBJS += builtin/shortlog.o
 BUILTIN_OBJS += builtin/show-branch.o
 BUILTIN_OBJS += builtin/show-ref.o
+BUILTIN_OBJS += builtin/stash--helper.o
 BUILTIN_OBJS += builtin/stripspace.o
 BUILTIN_OBJS += builtin/submodule--helper.o
 BUILTIN_OBJS += builtin/symbolic-ref.o
diff --git a/builtin.h b/builtin.h
index 42378f3aa4..a14fd85b0e 100644
--- a/builtin.h
+++ b/builtin.h
@@ -219,6 +219,7 @@ extern int cmd_shortlog(int argc, const char **argv, const 
char *prefix);
 extern int cmd_show(int argc, const char **argv, const char *prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
 extern int cmd_status(int argc, const char **argv, const char *prefix);
+extern int cmd_stash__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
 extern int cmd_submodule__helper(int argc, const char **argv, const char 
*prefix);
 extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
new file mode 100644
index 00..9d00a9547d
--- /dev/null
+++ b/builtin/stash--helper.c
@@ -0,0 +1,431 @@
+#include "builtin.h"
+#include "config.h"
+#include "parse-options.h"
+#include "refs.h"
+#include "lockfile.h"
+#include "cache-tree.h"
+#include "unpack-trees.h"
+#include "merge-recursive.h"
+#include "argv-array.h"
+#include "run-command.h"
+#include "dir.h"
+#include "rerere.h"
+
+static const char * const git_stash_helper_usage[] = {
+   N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   NULL
+};
+
+static const char * const git_stash_helper_apply_usage[] = {
+   N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   NULL
+};
+
+static const char *ref_stash = "refs/stash";
+static int quiet;
+static struct strbuf stash_index_path = STRBUF_INIT;
+
+struct stash_info {
+   struct object_id w_commit;
+   struct object_id b_commit;
+   struct object_id i_commit;
+   struct object_id u_commit;
+   struct object_id w_tree;
+   struct object_id b_tree;
+   struct object_id i_tree;
+   struct object_id u_tree;
+   struct strbuf revision;
+   int is_stash_ref;
+   int has_u;
+};
+
+static int grab_oid(struct object_id *oid, const char *fmt, const char *rev)
+{
+   struct strbuf buf = STRBUF_INIT;
+   int ret;
+
+   strbuf_addf(, fmt, rev);
+   ret = get_oid(buf.buf, oid);
+   strbuf_release();
+   return ret;
+}
+
+static void free_stash_info(struct stash_info *info)
+{
+   strbuf_release(>revision);
+}
+
+static int get_stash_info(struct stash_info *info, int argc, const char **argv)
+{
+   struct strbuf symbolic = STRBUF_INIT;
+   int ret;
+   const char *revision;
+   const char *commit = NULL;
+   char *end_of_rev;
+   char *expanded_ref;
+   struct object_id discard;
+
+   if (argc > 1) {
+   int i;
+   struct strbuf refs_msg = STRBUF_INIT;
+   for (i = 0; i < argc; ++i)
+   strbuf_addf(_msg, " '%s'", argv[i]);
+
+   fprintf_ln(stderr, _("Too many revisions specified:%s"), 
refs_msg.buf);
+   strbuf_release(_msg);
+
+   return -1;
+   }
+
+   if (argc == 1)
+   commit = argv[0];
+
+   strbuf_init(>revision, 0);
+   if (commit == NULL) {
+   if (!ref_exists(ref_stash)) {
+   free_stash_info(info);
+ 

[PATCH v5 5/5] stash: convert pop to builtin

[Joel Teichroeb]

Add stash pop to the helper and delete the pop_stash, drop_stash,
assert_stash_ref and pop_stash functions from the shell script
now that they are no longer needed.

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

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 486796bb6a..7c8950bc90 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -13,6 +13,7 @@
 
 static const char * const git_stash_helper_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
+   N_("git stash--helper pop [--index] [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
N_("git stash--helper branch  []"),
N_("git stash--helper clear"),
@@ -24,6 +25,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] []"),
+   NULL
+};
+
 static const char * const git_stash_helper_apply_usage[] = {
N_("git stash--helper apply [--index] [-q|--quiet] []"),
NULL
@@ -508,6 +514,39 @@ static int drop_stash(int argc, const char **argv, const 
char *prefix)
return ret;
 }
 
+static int pop_stash(int argc, const char **argv, const char *prefix)
+{
+   int index = 0, ret;
+   struct stash_info info;
+   struct option options[] = {
+   OPT__QUIET(, N_("be quiet, only report errors")),
+   OPT_BOOL(0, "index", ,
+   N_("attempt to recreate the index")),
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+   git_stash_helper_pop_usage, 0);
+
+   if (get_stash_info(, argc, argv))
+   return -1;
+
+   if (assert_stash_ref()) {
+   free_stash_info();
+   return -1;
+   }
+
+   if (do_apply_stash(prefix, , index)) {
+   printf_ln(_("The stash entry is kept in case you need it 
again."));
+   free_stash_info();
+   return -1;
+   }
+
+   ret = do_drop_stash(prefix, );
+   free_stash_info();
+   return ret;
+}
+
 static int branch_stash(int argc, const char **argv, const char *prefix)
 {
const char *branch = NULL;
@@ -577,6 +616,8 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
result = clear_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "drop"))
result = drop_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "pop"))
+   result = pop_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "branch"))
result = branch_stash(argc, argv, prefix);
else {
diff --git a/git-stash.sh b/git-stash.sh
index c5fd4c6c44..8f2640fe90 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -554,50 +554,6 @@ assert_stash_like() {
}
 }
 
-is_stash_ref() {
-   is_stash_like "$@" && test -n "$IS_STASH_REF"
-}
-
-assert_stash_ref() {
-   is_stash_ref "$@" || {
-   args="$*"
-   die "$(eval_gettext "'\$args' is not a stash reference")"
-   }
-}
-
-apply_stash () {
-   cd "$START_DIR"
-   git stash--helper apply "$@"
-   res=$?
-   cd_to_toplevel
-   return $res
-}
-
-pop_stash() {
-   assert_stash_ref "$@"
-
-   if apply_stash "$@"
-   then
-   drop_stash "$@"
-   else
-   status=$?
-   say "$(gettext "The stash entry is kept in case you need it 
again.")"
-   exit $status
-   fi
-}
-
-drop_stash () {
-   assert_stash_ref "$@"
-
-   git reflog delete --updateref --rewrite "${REV}" &&
-   say "$(eval_gettext "Dropped \${REV} (\$s)")" ||
-   die "$(eval_gettext "\${REV}: Could not drop stash entry")"
-
-   # clear_stash if we just dropped the last stash entry
-   git rev-parse --verify --quiet "$ref_stash@{0}" >/dev/null ||
-   clear_stash
-}
-
 test "$1" = "-p" && set "push" "$@"
 
 PARSE_CACHE='--not-parsed'
@@ -634,7 +590,8 @@ push)
;;
 apply)
shift
-   apply_stash "$@"
+   cd "$START_DIR"
+   git stash--helper apply "$@"
;;
 clear)
shift
@@ -654,7 +611,8 @@ drop)
;;
 pop)
shift
-   pop_stash "$@"
+   cd "$START_DIR"
+   git stash--helper pop "$@"
;;
 branch)
shift
-- 
2.16.3

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

[Joel Teichroeb]

Add the drop and clear commands to the builtin helper. These two
are each simple, but are being added together as they are quite
related.

We have to unfortunately keep the drop and clear functions in the
shell script as functions are called with parameters internally
that are not valid when the commands are called externally. Once
pop is converted they can both be removed.

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

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 9d00a9547d..520cd746c4 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -12,7 +12,14 @@
 #include "rerere.h"
 
 static const char * const git_stash_helper_usage[] = {
+   N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   N_("git stash--helper clear"),
+   NULL
+};
+
+static const char * const git_stash_helper_drop_usage[] = {
+   N_("git stash--helper drop [-q|--quiet] []"),
NULL
 };
 
@@ -21,6 +28,11 @@ static const char * const git_stash_helper_apply_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_clear_usage[] = {
+   N_("git stash--helper clear"),
+   NULL
+};
+
 static const char *ref_stash = "refs/stash";
 static int quiet;
 static struct strbuf stash_index_path = STRBUF_INIT;
@@ -134,6 +146,29 @@ static int get_stash_info(struct stash_info *info, int 
argc, const char **argv)
return 0;
 }
 
+static int do_clear_stash(void)
+{
+   struct object_id obj;
+   if (get_oid(ref_stash, ))
+   return 0;
+
+   return delete_ref(NULL, ref_stash, , 0);
+}
+
+static int clear_stash(int argc, const char **argv, const char *prefix)
+{
+   struct option options[] = {
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options, 
git_stash_helper_clear_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+
+   if (argc != 0)
+   return error(_("git stash--helper clear with parameters is 
unimplemented"));
+
+   return do_clear_stash();
+}
+
 static int reset_tree(struct object_id *i_tree, int update, int reset)
 {
struct unpack_trees_options opts;
@@ -399,6 +434,74 @@ static int apply_stash(int argc, const char **argv, const 
char *prefix)
return ret;
 }
 
+static int do_drop_stash(const char *prefix, struct stash_info *info)
+{
+   struct argv_array args = ARGV_ARRAY_INIT;
+   int ret;
+   struct child_process cp = CHILD_PROCESS_INIT;
+
+   /* reflog does not provide a simple function for deleting refs. One will
+* need to be added to avoid implementing too much reflog code here
+*/
+   argv_array_pushl(, "reflog", "delete", "--updateref", "--rewrite", 
NULL);
+   argv_array_push(, info->revision.buf);
+   ret = cmd_reflog(args.argc, args.argv, prefix);
+   if (!ret) {
+   if (!quiet)
+   printf(_("Dropped %s (%s)\n"), info->revision.buf, 
oid_to_hex(>w_commit));
+   } else {
+   return error(_("%s: Could not drop stash entry"), 
info->revision.buf);
+   }
+
+   /* This could easily be replaced by get_oid, but currently it will 
throw a
+* fatal error when a reflog is empty, which we can not recover from
+*/
+   cp.git_cmd = 1;
+   /* Even though --quiet is specified, rev-parse still outputs the hash */
+   cp.no_stdout = 1;
+   argv_array_pushl(, "rev-parse", "--verify", "--quiet", NULL);
+   argv_array_pushf(, "%s@{0}", ref_stash);
+   ret = run_command();
+
+   if (ret)
+   do_clear_stash();
+
+   return 0;
+}
+
+static int assert_stash_ref(struct stash_info *info)
+{
+   if (!info->is_stash_ref)
+   return error(_("'%s' is not a stash reference"), 
info->revision.buf);
+
+   return 0;
+}
+
+static int drop_stash(int argc, const char **argv, const char *prefix)
+{
+   struct stash_info info;
+   int ret;
+   struct option options[] = {
+   OPT__QUIET(, N_("be quiet, only report errors")),
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+   git_stash_helper_drop_usage, 0);
+
+   if (get_stash_info(, argc, argv))
+   return -1;
+
+   if (assert_stash_ref()) {
+   free_stash_info();
+   return -1;
+   }
+
+   ret = do_drop_stash(prefix, );
+   free_stash_info();
+   return ret;
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
  

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

[Joel Teichroeb]

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

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

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

The code is based on next as write_cache_as_tree was changed to
take an object ID. It can easily be rebase on master by changing
the two calls to write_cache_as_tree to use tha hash.

Previous threads:
v1: https://public-inbox.org/git/20180325173916.GE10909@hank/T/
v2: https://public-inbox.org/git/20180326011426.19159-1-j...@teichroeb.net/
v3: https://public-inbox.org/git/20180327054432.26419-1-j...@teichroeb.net/
v4: https://public-inbox.org/git/20180328222129.22192-1-j...@teichroeb.net/

Changes from v4:
 - Fixed a typo (Thanks Eric)
 - Redid my test again with input from Junio
 - Cleaned up usages of get_oid (Thanks Junio)
 - Slightly reduced calls to builtin functions (rerere, rev-parse)
 - Added comments clarifying why when forking/cmd_* is used

Joel Teichroeb (5):
  stash: improve option parsing test coverage
  stash: convert apply to builtin
  stash: convert drop and clear to builtin
  stash: convert branch to builtin
  stash: convert pop to builtin

 .gitignore  |   1 +
 Makefile|   1 +
 builtin.h   |   1 +
 builtin/stash--helper.c | 630 
 git-stash.sh| 136 +--
 git.c   |   1 +
 t/t3903-stash.sh|  35 +++
 7 files changed, 677 insertions(+), 128 deletions(-)
 create mode 100644 builtin/stash--helper.c

-- 
2.16.3

[PATCH v5 1/5] stash: improve option parsing test coverage

[Joel Teichroeb]

In preparation for converting the stash command incrementally to
a builtin command, this patch improves test coverage of the option
parsing. Both for having too many parameters, or too few.

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

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index aefde7b172..4eaa4cae9a 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -444,6 +444,36 @@ test_expect_failure 'stash file to directory' '
test foo = "$(cat file/file)"
 '
 
+test_expect_success 'giving too many ref agruments does not modify files' '
+   git stash clear &&
+   test_when_finished "git reset --hard HEAD" &&
+   echo foo >file2 &&
+   git stash &&
+   echo bar >file2 &&
+   git stash &&
+   test-chmtime =123456789 file2 &&
+   for type in apply pop "branch stash-branch"
+   do
+   test_must_fail git stash $type stash@{0} stash@{1} 2>err &&
+   test_i18ngrep "Too many" err &&
+   test 123456789 = $(test-chmtime -v +0 file2 | sed 
's/[^0-9].*$//') || return 1
+   done
+'
+
+test_expect_success 'giving too many ref agruments to drop does not drop 
anything' '
+   git stash list > stashlist1 &&
+   test_must_fail git stash drop stash@{0} stash@{1} 2>err &&
+   test_i18ngrep "Too many" err &&
+   git stash list > stashlist2 &&
+   test_cmp stashlist1 stashlist2
+'
+
+test_expect_success 'giving too many ref agruments to show does not show 
anything' '
+   test_must_fail git stash show stash@{0} stash@{1} 2>err 1>out && # show 
must not show anything
+   test_i18ngrep "Too many" err &&
+   test_must_be_empty out
+'
+
 test_expect_success 'stash create - no changes' '
git stash clear &&
test_when_finished "git reset --hard HEAD" &&
@@ -479,6 +509,11 @@ test_expect_success 'stash branch - stashes on stack, 
stash-like argument' '
test $(git ls-files --modified | wc -l) -eq 1
 '
 
+test_expect_success 'stash branch complains with no arguments' '
+   test_must_fail git stash branch 2>err &&
+   test_i18ngrep "No branch name specified" err
+'
+
 test_expect_success 'stash show format defaults to --stat' '
git stash clear &&
test_when_finished "git reset --hard HEAD" &&
-- 
2.16.3

[PATCH v5 4/5] stash: convert branch to builtin

[Joel Teichroeb]

Add stash branch to the helper and delete the apply_to_branch
function from the shell script.

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

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 520cd746c4..486796bb6a 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -14,6 +14,7 @@
 static const char * const git_stash_helper_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   N_("git stash--helper branch  []"),
N_("git stash--helper clear"),
NULL
 };
@@ -28,6 +29,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  []"),
+   NULL
+};
+
 static const char * const git_stash_helper_clear_usage[] = {
N_("git stash--helper clear"),
NULL
@@ -502,6 +508,49 @@ static int drop_stash(int argc, const char **argv, const 
char *prefix)
return ret;
 }
 
+static int branch_stash(int argc, const char **argv, const char *prefix)
+{
+   const char *branch = NULL;
+   int ret;
+   struct argv_array args = ARGV_ARRAY_INIT;
+   struct stash_info info;
+   struct option options[] = {
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+   git_stash_helper_branch_usage, 0);
+
+   if (argc == 0)
+   return error(_("No branch name specified"));
+
+   branch = argv[0];
+
+   if (get_stash_info(, argc - 1, argv + 1))
+   return -1;
+
+   /* Checkout does not currently provide a function for checking out a 
branch
+* as cmd_checkout does a large amount of sanity checks first that we
+* require here.
+*/
+   argv_array_pushl(, "checkout", "-b", NULL);
+   argv_array_push(, branch);
+   argv_array_push(, oid_to_hex(_commit));
+   ret = cmd_checkout(args.argc, args.argv, prefix);
+   if (ret) {
+   free_stash_info();
+   return -1;
+   }
+
+   ret = do_apply_stash(prefix, , 1);
+   if (!ret && info.is_stash_ref)
+   ret = do_drop_stash(prefix, );
+
+   free_stash_info();
+
+   return ret;
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
int result = 0;
@@ -528,6 +577,8 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
result = clear_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "drop"))
result = drop_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "branch"))
+   result = branch_stash(argc, argv, prefix);
else {
error(_("unknown subcommand: %s"), argv[0]);
usage_with_options(git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index 0b8f07b38a..c5fd4c6c44 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -598,20 +598,6 @@ drop_stash () {
clear_stash
 }
 
-apply_to_branch () {
-   test -n "$1" || die "$(gettext "No branch name specified")"
-   branch=$1
-   shift 1
-
-   set -- --index "$@"
-   assert_stash_like "$@"
-
-   git checkout -b $branch $REV^ &&
-   apply_stash "$@" && {
-   test -z "$IS_STASH_REF" || drop_stash "$@"
-   }
-}
-
 test "$1" = "-p" && set "push" "$@"
 
 PARSE_CACHE='--not-parsed'
@@ -672,7 +658,8 @@ pop)
;;
 branch)
shift
-   apply_to_branch "$@"
+   cd "$START_DIR"
+   git stash--helper branch "$@"
;;
 *)
case $# in
-- 
2.16.3

Re: [PATCH v4 2/5] stash: convert apply to builtin

[Joel Teichroeb]

On Thu, Mar 29, 2018 at 1:07 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Joel Teichroeb <j...@teichroeb.net> writes:
>
>> +static int get_stash_info(struct stash_info *info, int argc, const char 
>> **argv)
>> +{
>
> So, this roughly corresponds to parse_flags_and_rev function, it seems.
>
>> + struct strbuf w_commit_rev = STRBUF_INIT;
>> + struct strbuf b_commit_rev = STRBUF_INIT;
>> + struct strbuf w_tree_rev = STRBUF_INIT;
>> + struct strbuf b_tree_rev = STRBUF_INIT;
>> + struct strbuf i_tree_rev = STRBUF_INIT;
>> + struct strbuf u_tree_rev = STRBUF_INIT;
>> + struct strbuf symbolic = STRBUF_INIT;
>> + struct strbuf out = STRBUF_INIT;
>> + int ret;
>> + const char *revision;
>> + const char *commit = NULL;
>> + char *end_of_rev;
>> + info->is_stash_ref = 0;
>> +
>> + if (argc > 1) {
>> + int i;
>> + struct strbuf refs_msg = STRBUF_INIT;
>> + for (i = 0; i < argc; ++i)
>> + strbuf_addf(_msg, " '%s'", argv[i]);
>> +
>> + fprintf_ln(stderr, _("Too many revisions specified:%s"), 
>> refs_msg.buf);
>> + strbuf_release(_msg);
>> +
>> + return -1;
>> + }
>> +
>> + if (argc == 1)
>> + commit = argv[0];
>> +
>> + strbuf_init(>revision, 0);
>> + if (commit == NULL) {
>> + if (have_stash()) {
>> + free_stash_info(info);
>> + return error(_("No stash entries found."));
>> + }
>> +
>> + strbuf_addf(>revision, "%s@{0}", ref_stash);
>> + } else if (strspn(commit, "0123456789") == strlen(commit)) {
>> + strbuf_addf(>revision, "%s@{%s}", ref_stash, commit);
>> + } else {
>> + strbuf_addstr(>revision, commit);
>> + }
>> +
>> + revision = info->revision.buf;
>> + strbuf_addstr(_commit_rev, revision);
>> + ret = !get_oid(w_commit_rev.buf, >w_commit);
>> + strbuf_release(_commit_rev);
>
> Use of strbuf w_commit_rev looks completely pointless here.  Am I
> mistaken to say that the above three lines are equivalent to:
>
> ret = !get_oid(revision, >w_commit);
>

Right, it was refactored to this in a previous version, but I didn't
quite think it through.

>> +
>> + if (!ret) {
>> + error(_("%s is not a valid reference"), revision);
>> + free_stash_info(info);
>> + return -1;
>> + }
>> +
>> + strbuf_addf(_commit_rev, "%s^1", revision);
>> + strbuf_addf(_tree_rev, "%s:", revision);
>> + strbuf_addf(_tree_rev, "%s^1:", revision);
>> + strbuf_addf(_tree_rev, "%s^2:", revision);
>> +
>> + ret = !get_oid(b_commit_rev.buf, >b_commit) &&
>> + !get_oid(w_tree_rev.buf, >w_tree) &&
>> + !get_oid(b_tree_rev.buf, >b_tree) &&
>> + !get_oid(i_tree_rev.buf, >i_tree);
>> +
>> + strbuf_release(_commit_rev);
>> + strbuf_release(_tree_rev);
>> + strbuf_release(_tree_rev);
>> + strbuf_release(_tree_rev);
>
> For the same reason, these strbuf's look pretty much pointless.  I
> wonder if a private helper
>
> static int grab_oid(struct oid *oid, const char *fmt, const char *rev)
> {
> struct strbuf buf = STRBUF_INIT;
> int ret;
>
> strbuf_addf(, fmt, rev);
> ret = get_oid(buf, oid);
> strbuf_release();
> return ret;
> }
>
> would help here?  Then you wouldn't be writing something like the
> above, and instead you'd grab four object names like so:
>
> if (grab_oid(>b_commit, "%s^1", revision) ||
> grab_oid(>w_tree, "%s:", revision) ||
> grab_oid(>b_tree, "%s&1:", revision) ||
> grab_oid(>i_tree, "%s&2:", revision)) {
> ... we found an error ...
> return -1;
> }
>
> which would be a lot easier to follow, no?

Very much agreed! I felt like that part of the code was the weakest
part of my patch before. I'm very happy to have it cleaned up like
this!

>
>> +int cmd_stash__helper(int argc, const char **argv, const char *prefix)
>> +{

[PATCH v4 4/5] stash: convert branch to builtin

[Joel Teichroeb]

Add stash branch to the helper and delete the apply_to_branch
function from the shell script.

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

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 640c545f5..51fe8cab7 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -13,6 +13,7 @@
 static const char * const git_stash_helper_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   N_("git stash--helper branch  []"),
N_("git stash--helper clear"),
NULL
 };
@@ -27,6 +28,11 @@ static const char * const git_stash_helper_apply_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_branch_usage[] = {
+   N_("git stash--helper branch  []"),
+   NULL
+};
+
 static const char * const git_stash_helper_clear_usage[] = {
N_("git stash--helper clear"),
NULL
@@ -509,6 +515,45 @@ static int drop_stash(int argc, const char **argv, const 
char *prefix)
return ret;
 }
 
+static int branch_stash(int argc, const char **argv, const char *prefix)
+{
+   const char *branch = NULL;
+   int ret;
+   struct argv_array args = ARGV_ARRAY_INIT;
+   struct stash_info info;
+   struct option options[] = {
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+   git_stash_helper_branch_usage, 0);
+
+   if (argc == 0)
+   return error(_("No branch name specified"));
+
+   branch = argv[0];
+
+   if (get_stash_info(, argc - 1, argv + 1))
+   return -1;
+
+   argv_array_pushl(, "checkout", "-b", NULL);
+   argv_array_push(, branch);
+   argv_array_push(, oid_to_hex(_commit));
+   ret = cmd_checkout(args.argc, args.argv, prefix);
+   if (ret) {
+   free_stash_info();
+   return -1;
+   }
+
+   ret = do_apply_stash(prefix, , 1);
+   if (!ret && info.is_stash_ref)
+   ret = do_drop_stash(prefix, );
+
+   free_stash_info();
+
+   return ret;
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
int result = 0;
@@ -535,6 +580,8 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
result = clear_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "drop"))
result = drop_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "branch"))
+   result = branch_stash(argc, argv, prefix);
else {
error(_("unknown subcommand: %s"), argv[0]);
usage_with_options(git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index 0b8f07b38..c5fd4c6c4 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -598,20 +598,6 @@ drop_stash () {
clear_stash
 }
 
-apply_to_branch () {
-   test -n "$1" || die "$(gettext "No branch name specified")"
-   branch=$1
-   shift 1
-
-   set -- --index "$@"
-   assert_stash_like "$@"
-
-   git checkout -b $branch $REV^ &&
-   apply_stash "$@" && {
-   test -z "$IS_STASH_REF" || drop_stash "$@"
-   }
-}
-
 test "$1" = "-p" && set "push" "$@"
 
 PARSE_CACHE='--not-parsed'
@@ -672,7 +658,8 @@ pop)
;;
 branch)
shift
-   apply_to_branch "$@"
+   cd "$START_DIR"
+   git stash--helper branch "$@"
;;
 *)
case $# in
-- 
2.16.2

[PATCH v4 5/5] stash: convert pop to builtin

[Joel Teichroeb]

Add stash pop to the helper and delete the pop_stash, drop_stash,
assert_stash_ref and pop_stash functions from the shell script
now that they are no longer needed.

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

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 51fe8cab7..aa8a2bb3a 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -12,6 +12,7 @@
 
 static const char * const git_stash_helper_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
+   N_("git stash--helper pop [--index] [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
N_("git stash--helper branch  []"),
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] []"),
+   NULL
+};
+
 static const char * const git_stash_helper_apply_usage[] = {
N_("git stash--helper apply [--index] [-q|--quiet] []"),
NULL
@@ -515,6 +521,39 @@ static int drop_stash(int argc, const char **argv, const 
char *prefix)
return ret;
 }
 
+static int pop_stash(int argc, const char **argv, const char *prefix)
+{
+   int index = 0, ret;
+   struct stash_info info;
+   struct option options[] = {
+   OPT__QUIET(, N_("be quiet, only report errors")),
+   OPT_BOOL(0, "index", ,
+   N_("attempt to recreate the index")),
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+   git_stash_helper_pop_usage, 0);
+
+   if (get_stash_info(, argc, argv))
+   return -1;
+
+   if (assert_stash_ref()) {
+   free_stash_info();
+   return -1;
+   }
+
+   if (do_apply_stash(prefix, , index)) {
+   printf_ln(_("The stash entry is kept in case you need it 
again."));
+   free_stash_info();
+   return -1;
+   }
+
+   ret = do_drop_stash(prefix, );
+   free_stash_info();
+   return ret;
+}
+
 static int branch_stash(int argc, const char **argv, const char *prefix)
 {
const char *branch = NULL;
@@ -580,6 +619,8 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
result = clear_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "drop"))
result = drop_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "pop"))
+   result = pop_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "branch"))
result = branch_stash(argc, argv, prefix);
else {
diff --git a/git-stash.sh b/git-stash.sh
index c5fd4c6c4..8f2640fe9 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -554,50 +554,6 @@ assert_stash_like() {
}
 }
 
-is_stash_ref() {
-   is_stash_like "$@" && test -n "$IS_STASH_REF"
-}
-
-assert_stash_ref() {
-   is_stash_ref "$@" || {
-   args="$*"
-   die "$(eval_gettext "'\$args' is not a stash reference")"
-   }
-}
-
-apply_stash () {
-   cd "$START_DIR"
-   git stash--helper apply "$@"
-   res=$?
-   cd_to_toplevel
-   return $res
-}
-
-pop_stash() {
-   assert_stash_ref "$@"
-
-   if apply_stash "$@"
-   then
-   drop_stash "$@"
-   else
-   status=$?
-   say "$(gettext "The stash entry is kept in case you need it 
again.")"
-   exit $status
-   fi
-}
-
-drop_stash () {
-   assert_stash_ref "$@"
-
-   git reflog delete --updateref --rewrite "${REV}" &&
-   say "$(eval_gettext "Dropped \${REV} (\$s)")" ||
-   die "$(eval_gettext "\${REV}: Could not drop stash entry")"
-
-   # clear_stash if we just dropped the last stash entry
-   git rev-parse --verify --quiet "$ref_stash@{0}" >/dev/null ||
-   clear_stash
-}
-
 test "$1" = "-p" && set "push" "$@"
 
 PARSE_CACHE='--not-parsed'
@@ -634,7 +590,8 @@ push)
;;
 apply)
shift
-   apply_stash "$@"
+   cd "$START_DIR"
+   git stash--helper apply "$@"
;;
 clear)
shift
@@ -654,7 +611,8 @@ drop)
;;
 pop)
shift
-   pop_stash "$@"
+   cd "$START_DIR"
+   git stash--helper pop "$@"
;;
 branch)
shift
-- 
2.16.2

[PATCH v4 2/5] stash: convert apply to builtin

[Joel Teichroeb]

Add a bulitin helper for performing stash commands. Converting
all at once proved hard to review, so starting with just apply
let conversion get started without the other command being
finished.

The helper is being implemented as a drop in replacement for
stash so that when it is complete it can simply be renamed and
the shell script deleted.

Delete the contents of the apply_stash shell function and replace
it with a call to stash--helper apply until pop is also
converted.

Signed-off-by: Joel Teichroeb <j...@teichroeb.net>
---
 .gitignore  |   1 +
 Makefile|   1 +
 builtin.h   |   1 +
 builtin/stash--helper.c | 444 
 git-stash.sh|  75 +---
 git.c   |   1 +
 6 files changed, 453 insertions(+), 70 deletions(-)
 create mode 100644 builtin/stash--helper.c

diff --git a/.gitignore b/.gitignore
index 833ef3b0b..296d5f376 100644
--- a/.gitignore
+++ b/.gitignore
@@ -152,6 +152,7 @@
 /git-show-ref
 /git-stage
 /git-stash
+/git-stash--helper
 /git-status
 /git-stripspace
 /git-submodule
diff --git a/Makefile b/Makefile
index 96f6138f6..6cfdbe9a3 100644
--- a/Makefile
+++ b/Makefile
@@ -1028,6 +1028,7 @@ BUILTIN_OBJS += builtin/send-pack.o
 BUILTIN_OBJS += builtin/shortlog.o
 BUILTIN_OBJS += builtin/show-branch.o
 BUILTIN_OBJS += builtin/show-ref.o
+BUILTIN_OBJS += builtin/stash--helper.o
 BUILTIN_OBJS += builtin/stripspace.o
 BUILTIN_OBJS += builtin/submodule--helper.o
 BUILTIN_OBJS += builtin/symbolic-ref.o
diff --git a/builtin.h b/builtin.h
index 42378f3aa..a14fd85b0 100644
--- a/builtin.h
+++ b/builtin.h
@@ -219,6 +219,7 @@ extern int cmd_shortlog(int argc, const char **argv, const 
char *prefix);
 extern int cmd_show(int argc, const char **argv, const char *prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
 extern int cmd_status(int argc, const char **argv, const char *prefix);
+extern int cmd_stash__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
 extern int cmd_submodule__helper(int argc, const char **argv, const char 
*prefix);
 extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
new file mode 100644
index 0..00e854734
--- /dev/null
+++ b/builtin/stash--helper.c
@@ -0,0 +1,444 @@
+#include "builtin.h"
+#include "config.h"
+#include "parse-options.h"
+#include "refs.h"
+#include "lockfile.h"
+#include "cache-tree.h"
+#include "unpack-trees.h"
+#include "merge-recursive.h"
+#include "argv-array.h"
+#include "run-command.h"
+#include "dir.h"
+
+static const char * const git_stash_helper_usage[] = {
+   N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   NULL
+};
+
+static const char * const git_stash_helper_apply_usage[] = {
+   N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   NULL
+};
+
+static const char *ref_stash = "refs/stash";
+static int quiet;
+static char stash_index_path[PATH_MAX];
+
+struct stash_info {
+   struct object_id w_commit;
+   struct object_id b_commit;
+   struct object_id i_commit;
+   struct object_id u_commit;
+   struct object_id w_tree;
+   struct object_id b_tree;
+   struct object_id i_tree;
+   struct object_id u_tree;
+   struct strbuf revision;
+   int is_stash_ref;
+   int has_u;
+};
+
+static int get_symbolic_name(const char *symbolic, struct strbuf *out)
+{
+   struct child_process cp = CHILD_PROCESS_INIT;
+
+   cp.git_cmd = 1;
+   argv_array_pushl(, "rev-parse", "--symbolic-full-name", NULL);
+   argv_array_push(, symbolic);
+   return pipe_command(, NULL, 0, out, 0, NULL, 0);
+}
+
+static int have_stash(void)
+{
+   struct child_process cp = CHILD_PROCESS_INIT;
+
+   cp.git_cmd = 1;
+   cp.no_stdout = 1;
+   argv_array_pushl(, "rev-parse", "--verify", "--quiet", NULL);
+   argv_array_push(, ref_stash);
+   return pipe_command(, NULL, 0, NULL, 0, NULL, 0);
+}
+
+static void free_stash_info(struct stash_info *info)
+{
+   strbuf_release(>revision);
+}
+
+static int get_stash_info(struct stash_info *info, int argc, const char **argv)
+{
+   struct strbuf w_commit_rev = STRBUF_INIT;
+   struct strbuf b_commit_rev = STRBUF_INIT;
+   struct strbuf w_tree_rev = STRBUF_INIT;
+   struct strbuf b_tree_rev = STRBUF_INIT;
+   struct strbuf i_tree_rev = STRBUF_INIT;
+   struct strbuf u_tree_rev = STRBUF_INIT;
+   struct strbuf symbolic = STRBUF_INIT;
+   struct strbuf out = STRBUF_INIT;
+   int ret;
+   const char *revision;
+   const char *commit = NULL;
+  

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

[Joel Teichroeb]

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

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

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

The code is based on next as write_cache_as_tree was changed to
take an object ID. It can easily be rebase on master by changing
the two calls to write_cache_as_tree to use tha hash.

The only comments on v3 were minor, so I feel this should be
ready to go in soon.

Previous threads:
v1: https://public-inbox.org/git/20180325173916.GE10909@hank/T/
v2: https://public-inbox.org/git/20180326011426.19159-1-j...@teichroeb.net/
v3: https://public-inbox.org/git/20180327054432.26419-1-j...@teichroeb.net/

Changes from v3:
 - Fix the Windows build
 - Fix a use after free in the error handling

Joel Teichroeb (5):
  stash: improve option parsing test coverage
  stash: convert apply to builtin
  stash: convert drop and clear to builtin
  stash: convert branch to builtin
  stash: convert pop to builtin

 .gitignore  |   1 +
 Makefile|   1 +
 builtin.h   |   1 +
 builtin/stash--helper.c | 633 
 git-stash.sh| 136 +--
 git.c   |   1 +
 t/t3903-stash.sh|  16 ++
 7 files changed, 661 insertions(+), 128 deletions(-)
 create mode 100644 builtin/stash--helper.c

-- 
2.16.2

[PATCH v4 1/5] stash: improve option parsing test coverage

[Joel Teichroeb]

In preparation for converting the stash command incrementally to
a builtin command, this patch improves test coverage of the option
parsing. Both for having too many paramerters, or too few.

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

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index aefde7b17..8a666c60c 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -84,6 +84,17 @@ test_expect_success 'apply stashed changes (including 
index)' '
test 1 = $(git show HEAD:file)
 '
 
+test_expect_success 'giving too many ref agruments does nothing' '
+
+   for type in apply drop pop show "branch stash-branch"
+   do
+   test-chmtime =123456789 file &&
+   test_must_fail git stash $type stash@{0} stash@{1} 2>err &&
+   test_i18ngrep "Too many" err &&
+   test 123456789 = $(test-chmtime -v +0 file | sed 
's/[^0-9].*$//') || return 1
+   done
+'
+
 test_expect_success 'unstashing in a subdirectory' '
git reset --hard HEAD &&
mkdir subdir &&
@@ -479,6 +490,11 @@ test_expect_success 'stash branch - stashes on stack, 
stash-like argument' '
test $(git ls-files --modified | wc -l) -eq 1
 '
 
+test_expect_success 'stash branch complains with no arguments' '
+   test_must_fail git stash branch 2>err &&
+   test_i18ngrep "No branch name specified" err
+'
+
 test_expect_success 'stash show format defaults to --stat' '
git stash clear &&
test_when_finished "git reset --hard HEAD" &&
-- 
2.16.2

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

[Joel Teichroeb]

Add the drop and clear commands to the builtin helper. These two
are each simple, but are being added together as they are quite
related.

We have to unfortunately keep the drop and clear functions in the
shell script as functions are called with parameters internally
that are not valid when the commands are called externally. Once
pop is converted they can both be removed.

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

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 00e854734..640c545f5 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -11,7 +11,14 @@
 #include "dir.h"
 
 static const char * const git_stash_helper_usage[] = {
+   N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   N_("git stash--helper clear"),
+   NULL
+};
+
+static const char * const git_stash_helper_drop_usage[] = {
+   N_("git stash--helper drop [-q|--quiet] []"),
NULL
 };
 
@@ -20,6 +27,11 @@ static const char * const git_stash_helper_apply_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_clear_usage[] = {
+   N_("git stash--helper clear"),
+   NULL
+};
+
 static const char *ref_stash = "refs/stash";
 static int quiet;
 static char stash_index_path[PATH_MAX];
@@ -168,6 +180,29 @@ static int get_stash_info(struct stash_info *info, int 
argc, const char **argv)
return 0;
 }
 
+static int do_clear_stash(void)
+{
+   struct object_id obj;
+   if (get_oid(ref_stash, ))
+   return 0;
+
+   return delete_ref(NULL, ref_stash, , 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;
@@ -412,6 +447,68 @@ static int apply_stash(int argc, const char **argv, const 
char *prefix)
return ret;
 }
 
+static int do_drop_stash(const char *prefix, struct stash_info *info)
+{
+   struct argv_array args = ARGV_ARRAY_INIT;
+   int ret;
+   struct child_process cp = CHILD_PROCESS_INIT;
+
+   argv_array_pushl(, "reflog", "delete", "--updateref", "--rewrite", 
NULL);
+   argv_array_push(, info->revision.buf);
+   ret = cmd_reflog(args.argc, args.argv, prefix);
+   if (!ret) {
+   if (!quiet)
+   printf(_("Dropped %s (%s)\n"), info->revision.buf, 
oid_to_hex(>w_commit));
+   } else {
+   return error(_("%s: Could not drop stash entry"), 
info->revision.buf);
+   }
+
+   cp.git_cmd = 1;
+   /* Even though --quiet is specified, rev-parse still outputs the hash */
+   cp.no_stdout = 1;
+   argv_array_pushl(, "rev-parse", "--verify", "--quiet", NULL);
+   argv_array_pushf(, "%s@{0}", ref_stash);
+   ret = run_command();
+
+   if (ret)
+   do_clear_stash();
+
+   return 0;
+}
+
+static int assert_stash_ref(struct stash_info *info)
+{
+   if (!info->is_stash_ref)
+   return error(_("'%s' is not a stash reference"), 
info->revision.buf);
+
+   return 0;
+}
+
+static int drop_stash(int argc, const char **argv, const char *prefix)
+{
+   struct stash_info info;
+   int ret;
+   struct option options[] = {
+   OPT__QUIET(, N_("be quiet, only report errors")),
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+   git_stash_helper_drop_usage, 0);
+
+   if (get_stash_info(, argc, argv))
+   return -1;
+
+   if (assert_stash_ref()) {
+   free_stash_info();
+   return -1;
+   }
+
+   ret = do_drop_stash(prefix, );
+   free_stash_info();
+   return ret;
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
int result = 0;
@@ -434,6 +531,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 =

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

[Joel Teichroeb]

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

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

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

[Joel Teichroeb]

On Sun, Mar 25, 2018 at 9:43 AM, Thomas Gummerer <t.gumme...@gmail.com> wrote:
> On 03/24, Joel Teichroeb wrote:
>> ---
>
> Missing sign-off?  I saw it's missing in the other patches as well.
>

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

>> [...]
>> +
>> + if (info->has_u) {
>> + struct child_process cp = CHILD_PROCESS_INIT;
>> + struct child_process cp2 = CHILD_PROCESS_INIT;
>> + int res;
>> +
>> + cp.git_cmd = 1;
>> + argv_array_push(, "read-tree");
>> + argv_array_push(, sha1_to_hex(info->u_tree.hash));
>> + argv_array_pushf(_array, "GIT_INDEX_FILE=%s", 
>> stash_index_path);
>> +
>> + cp2.git_cmd = 1;
>> + argv_array_pushl(, "checkout-index", "--all", NULL);
>> + argv_array_pushf(_array, "GIT_INDEX_FILE=%s", 
>> stash_index_path);
>> +
>> + res = run_command() || run_command();
>> + remove_path(stash_index_path);
>> + if (res)
>> + return error(_("Could not restore untracked files from 
>> stash"));
>
> A minor change in behaviour here is that we are removing the temporary
> index file unconditionally here, while we would previously only remove
> it if both 'read-tree' and 'checkout-index' would succeed.
>
> I don't think that's a bad thing, we probably don't want users to try
> and use that index file in any way, and I doubt that's part of anyones
> workflow, so I think cleaning it up makes sense.
>

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

Re: [PATCH v3 2/5] stash: convert apply to builtin

[Joel Teichroeb]

On Tue, Mar 27, 2018 at 9:02 AM, Johannes Schindelin
 wrote:
> Hi Joel,
>
> [...]
>> +
>> +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(_tree, 0, NULL) || reset_tree(_tree, 0, 0))
>
> When applied on top of current `master`, I need to replace the _tree by
> c_tree.hash.
>
> Likewise...
>

I based this revision off next because of the object_id changes. I
probably should have mentioned in my cover-letter.

>> [...]
>> +
>> + index_file = get_index_file();
>> + xsnprintf(stash_index_path, PATH_MAX, "%s.stash.%d", index_file, pid);
>
> Since `pid_t` is `unsigned long long` on Windows, I changed the %d" to
> %"PRIuMAX and cast `pid` to `(uintmax_t)`.

Thanks for testing on windows! I'll have that fixed in the next revision.

[PATCH v3 5/5] stash: convert pop to builtin

[Joel Teichroeb]

Add stash pop to the helper and delete the pop_stash, drop_stash,
assert_stash_ref and pop_stash functions from the shell script
now that they are no longer needed.

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

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index d755faf33..53c0d2171 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -12,6 +12,7 @@
 
 static const char * const git_stash_helper_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
+   N_("git stash--helper pop [--index] [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
N_("git stash--helper branch  []"),
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] []"),
+   NULL
+};
+
 static const char * const git_stash_helper_apply_usage[] = {
N_("git stash--helper apply [--index] [-q|--quiet] []"),
NULL
@@ -513,6 +519,39 @@ static int drop_stash(int argc, const char **argv, const 
char *prefix)
return ret;
 }
 
+static int pop_stash(int argc, const char **argv, const char *prefix)
+{
+   int index = 0, ret;
+   struct stash_info info;
+   struct option options[] = {
+   OPT__QUIET(, N_("be quiet, only report errors")),
+   OPT_BOOL(0, "index", ,
+   N_("attempt to recreate the index")),
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+   git_stash_helper_pop_usage, 0);
+
+   if (get_stash_info(, argc, argv))
+   return -1;
+
+   if (assert_stash_ref()) {
+   free_stash_info();
+   return -1;
+   }
+
+   if (do_apply_stash(prefix, , index)) {
+   printf_ln(_("The stash entry is kept in case you need it 
again."));
+   free_stash_info();
+   return -1;
+   }
+
+   ret = do_drop_stash(prefix, );
+   free_stash_info();
+   return ret;
+}
+
 static int branch_stash(int argc, const char **argv, const char *prefix)
 {
const char *branch = NULL;
@@ -578,6 +617,8 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
result = clear_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "drop"))
result = drop_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "pop"))
+   result = pop_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "branch"))
result = branch_stash(argc, argv, prefix);
else {
diff --git a/git-stash.sh b/git-stash.sh
index c5fd4c6c4..8f2640fe9 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -554,50 +554,6 @@ assert_stash_like() {
}
 }
 
-is_stash_ref() {
-   is_stash_like "$@" && test -n "$IS_STASH_REF"
-}
-
-assert_stash_ref() {
-   is_stash_ref "$@" || {
-   args="$*"
-   die "$(eval_gettext "'\$args' is not a stash reference")"
-   }
-}
-
-apply_stash () {
-   cd "$START_DIR"
-   git stash--helper apply "$@"
-   res=$?
-   cd_to_toplevel
-   return $res
-}
-
-pop_stash() {
-   assert_stash_ref "$@"
-
-   if apply_stash "$@"
-   then
-   drop_stash "$@"
-   else
-   status=$?
-   say "$(gettext "The stash entry is kept in case you need it 
again.")"
-   exit $status
-   fi
-}
-
-drop_stash () {
-   assert_stash_ref "$@"
-
-   git reflog delete --updateref --rewrite "${REV}" &&
-   say "$(eval_gettext "Dropped \${REV} (\$s)")" ||
-   die "$(eval_gettext "\${REV}: Could not drop stash entry")"
-
-   # clear_stash if we just dropped the last stash entry
-   git rev-parse --verify --quiet "$ref_stash@{0}" >/dev/null ||
-   clear_stash
-}
-
 test "$1" = "-p" && set "push" "$@"
 
 PARSE_CACHE='--not-parsed'
@@ -634,7 +590,8 @@ push)
;;
 apply)
shift
-   apply_stash "$@"
+   cd "$START_DIR"
+   git stash--helper apply "$@"
;;
 clear)
shift
@@ -654,7 +611,8 @@ drop)
;;
 pop)
shift
-   pop_stash "$@"
+   cd "$START_DIR"
+   git stash--helper pop "$@"
;;
 branch)
shift
-- 
2.16.2

[PATCH v3 4/5] stash: convert branch to builtin

[Joel Teichroeb]

Add stash branch to the helper and delete the apply_to_branch
function from the shell script.

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

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 0d7a0d55e..d755faf33 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -13,6 +13,7 @@
 static const char * const git_stash_helper_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   N_("git stash--helper branch  []"),
N_("git stash--helper clear"),
NULL
 };
@@ -27,6 +28,11 @@ static const char * const git_stash_helper_apply_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_branch_usage[] = {
+   N_("git stash--helper branch  []"),
+   NULL
+};
+
 static const char * const git_stash_helper_clear_usage[] = {
N_("git stash--helper clear"),
NULL
@@ -507,6 +513,45 @@ static int drop_stash(int argc, const char **argv, const 
char *prefix)
return ret;
 }
 
+static int branch_stash(int argc, const char **argv, const char *prefix)
+{
+   const char *branch = NULL;
+   int ret;
+   struct argv_array args = ARGV_ARRAY_INIT;
+   struct stash_info info;
+   struct option options[] = {
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+   git_stash_helper_branch_usage, 0);
+
+   if (argc == 0)
+   return error(_("No branch name specified"));
+
+   branch = argv[0];
+
+   if (get_stash_info(, argc - 1, argv + 1))
+   return -1;
+
+   argv_array_pushl(, "checkout", "-b", NULL);
+   argv_array_push(, branch);
+   argv_array_push(, oid_to_hex(_commit));
+   ret = cmd_checkout(args.argc, args.argv, prefix);
+   if (ret) {
+   free_stash_info();
+   return -1;
+   }
+
+   ret = do_apply_stash(prefix, , 1);
+   if (!ret && info.is_stash_ref)
+   ret = do_drop_stash(prefix, );
+
+   free_stash_info();
+
+   return ret;
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
int result = 0;
@@ -533,6 +578,8 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
result = clear_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "drop"))
result = drop_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "branch"))
+   result = branch_stash(argc, argv, prefix);
else {
error(_("unknown subcommand: %s"), argv[0]);
usage_with_options(git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index 0b8f07b38..c5fd4c6c4 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -598,20 +598,6 @@ drop_stash () {
clear_stash
 }
 
-apply_to_branch () {
-   test -n "$1" || die "$(gettext "No branch name specified")"
-   branch=$1
-   shift 1
-
-   set -- --index "$@"
-   assert_stash_like "$@"
-
-   git checkout -b $branch $REV^ &&
-   apply_stash "$@" && {
-   test -z "$IS_STASH_REF" || drop_stash "$@"
-   }
-}
-
 test "$1" = "-p" && set "push" "$@"
 
 PARSE_CACHE='--not-parsed'
@@ -672,7 +658,8 @@ pop)
;;
 branch)
shift
-   apply_to_branch "$@"
+   cd "$START_DIR"
+   git stash--helper branch "$@"
;;
 *)
case $# in
-- 
2.16.2

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

[Joel Teichroeb]

Add the drop and clear commands to the builtin helper. These two
are each simple, but are being added together as they are quite
related.

We have to unfortunately keep the drop and clear functions in the
shell script as functions are called with parameters internally
that are not valid when the commands are called externally. Once
pop is converted they can both be removed.

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

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index df3c8e1e6..0d7a0d55e 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -11,7 +11,14 @@
 #include "dir.h"
 
 static const char * const git_stash_helper_usage[] = {
+   N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   N_("git stash--helper clear"),
+   NULL
+};
+
+static const char * const git_stash_helper_drop_usage[] = {
+   N_("git stash--helper drop [-q|--quiet] []"),
NULL
 };
 
@@ -20,6 +27,11 @@ static const char * const git_stash_helper_apply_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_clear_usage[] = {
+   N_("git stash--helper clear"),
+   NULL
+};
+
 static const char *ref_stash = "refs/stash";
 static int quiet;
 static char stash_index_path[PATH_MAX];
@@ -166,6 +178,29 @@ static int get_stash_info(struct stash_info *info, int 
argc, const char **argv)
return 0;
 }
 
+static int do_clear_stash(void)
+{
+   struct object_id obj;
+   if (get_oid(ref_stash, ))
+   return 0;
+
+   return delete_ref(NULL, ref_stash, , 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;
@@ -410,6 +445,68 @@ static int apply_stash(int argc, const char **argv, const 
char *prefix)
return ret;
 }
 
+static int do_drop_stash(const char *prefix, struct stash_info *info)
+{
+   struct argv_array args = ARGV_ARRAY_INIT;
+   int ret;
+   struct child_process cp = CHILD_PROCESS_INIT;
+
+   argv_array_pushl(, "reflog", "delete", "--updateref", "--rewrite", 
NULL);
+   argv_array_push(, info->revision.buf);
+   ret = cmd_reflog(args.argc, args.argv, prefix);
+   if (!ret) {
+   if (!quiet)
+   printf(_("Dropped %s (%s)\n"), info->revision.buf, 
oid_to_hex(>w_commit));
+   } else {
+   return error(_("%s: Could not drop stash entry"), 
info->revision.buf);
+   }
+
+   cp.git_cmd = 1;
+   /* Even though --quiet is specified, rev-parse still outputs the hash */
+   cp.no_stdout = 1;
+   argv_array_pushl(, "rev-parse", "--verify", "--quiet", NULL);
+   argv_array_pushf(, "%s@{0}", ref_stash);
+   ret = run_command();
+
+   if (ret)
+   do_clear_stash();
+
+   return 0;
+}
+
+static int assert_stash_ref(struct stash_info *info)
+{
+   if (!info->is_stash_ref)
+   return error(_("'%s' is not a stash reference"), 
info->revision.buf);
+
+   return 0;
+}
+
+static int drop_stash(int argc, const char **argv, const char *prefix)
+{
+   struct stash_info info;
+   int ret;
+   struct option options[] = {
+   OPT__QUIET(, N_("be quiet, only report errors")),
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+   git_stash_helper_drop_usage, 0);
+
+   if (get_stash_info(, argc, argv))
+   return -1;
+
+   if (assert_stash_ref()) {
+   free_stash_info();
+   return -1;
+   }
+
+   ret = do_drop_stash(prefix, );
+   free_stash_info();
+   return ret;
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
int result = 0;
@@ -432,6 +529,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 =

[PATCH v3 2/5] stash: convert apply to builtin

[Joel Teichroeb]

Add a bulitin helper for performing stash commands. Converting
all at once proved hard to review, so starting with just apply
let conversion get started without the other command being
finished.

The helper is being implemented as a drop in replacement for
stash so that when it is complete it can simply be renamed and
the shell script deleted.

Delete the contents of the apply_stash shell function and replace
it with a call to stash--helper apply until pop is also
converted.

Signed-off-by: Joel Teichroeb <j...@teichroeb.net>
---
 .gitignore  |   1 +
 Makefile|   1 +
 builtin.h   |   1 +
 builtin/stash--helper.c | 442 
 git-stash.sh|  75 +---
 git.c   |   1 +
 6 files changed, 451 insertions(+), 70 deletions(-)
 create mode 100644 builtin/stash--helper.c

diff --git a/.gitignore b/.gitignore
index 833ef3b0b..296d5f376 100644
--- a/.gitignore
+++ b/.gitignore
@@ -152,6 +152,7 @@
 /git-show-ref
 /git-stage
 /git-stash
+/git-stash--helper
 /git-status
 /git-stripspace
 /git-submodule
diff --git a/Makefile b/Makefile
index 96f6138f6..6cfdbe9a3 100644
--- a/Makefile
+++ b/Makefile
@@ -1028,6 +1028,7 @@ BUILTIN_OBJS += builtin/send-pack.o
 BUILTIN_OBJS += builtin/shortlog.o
 BUILTIN_OBJS += builtin/show-branch.o
 BUILTIN_OBJS += builtin/show-ref.o
+BUILTIN_OBJS += builtin/stash--helper.o
 BUILTIN_OBJS += builtin/stripspace.o
 BUILTIN_OBJS += builtin/submodule--helper.o
 BUILTIN_OBJS += builtin/symbolic-ref.o
diff --git a/builtin.h b/builtin.h
index 42378f3aa..a14fd85b0 100644
--- a/builtin.h
+++ b/builtin.h
@@ -219,6 +219,7 @@ extern int cmd_shortlog(int argc, const char **argv, const 
char *prefix);
 extern int cmd_show(int argc, const char **argv, const char *prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
 extern int cmd_status(int argc, const char **argv, const char *prefix);
+extern int cmd_stash__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
 extern int cmd_submodule__helper(int argc, const char **argv, const char 
*prefix);
 extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
new file mode 100644
index 0..df3c8e1e6
--- /dev/null
+++ b/builtin/stash--helper.c
@@ -0,0 +1,442 @@
+#include "builtin.h"
+#include "config.h"
+#include "parse-options.h"
+#include "refs.h"
+#include "lockfile.h"
+#include "cache-tree.h"
+#include "unpack-trees.h"
+#include "merge-recursive.h"
+#include "argv-array.h"
+#include "run-command.h"
+#include "dir.h"
+
+static const char * const git_stash_helper_usage[] = {
+   N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   NULL
+};
+
+static const char * const git_stash_helper_apply_usage[] = {
+   N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   NULL
+};
+
+static const char *ref_stash = "refs/stash";
+static int quiet;
+static char stash_index_path[PATH_MAX];
+
+struct stash_info {
+   struct object_id w_commit;
+   struct object_id b_commit;
+   struct object_id i_commit;
+   struct object_id u_commit;
+   struct object_id w_tree;
+   struct object_id b_tree;
+   struct object_id i_tree;
+   struct object_id u_tree;
+   struct strbuf revision;
+   int is_stash_ref;
+   int has_u;
+};
+
+static int get_symbolic_name(const char *symbolic, struct strbuf *out)
+{
+   struct child_process cp = CHILD_PROCESS_INIT;
+
+   cp.git_cmd = 1;
+   argv_array_pushl(, "rev-parse", "--symbolic-full-name", NULL);
+   argv_array_push(, symbolic);
+   return pipe_command(, NULL, 0, out, 0, NULL, 0);
+}
+
+static int have_stash(void)
+{
+   struct child_process cp = CHILD_PROCESS_INIT;
+
+   cp.git_cmd = 1;
+   cp.no_stdout = 1;
+   argv_array_pushl(, "rev-parse", "--verify", "--quiet", NULL);
+   argv_array_push(, ref_stash);
+   return pipe_command(, NULL, 0, NULL, 0, NULL, 0);
+}
+
+static void free_stash_info(struct stash_info *info)
+{
+   strbuf_release(>revision);
+}
+
+static int get_stash_info(struct stash_info *info, int argc, const char **argv)
+{
+   struct strbuf w_commit_rev = STRBUF_INIT;
+   struct strbuf b_commit_rev = STRBUF_INIT;
+   struct strbuf w_tree_rev = STRBUF_INIT;
+   struct strbuf b_tree_rev = STRBUF_INIT;
+   struct strbuf i_tree_rev = STRBUF_INIT;
+   struct strbuf u_tree_rev = STRBUF_INIT;
+   struct strbuf symbolic = STRBUF_INIT;
+   struct strbuf out = STRBUF_INIT;
+   int ret;
+   const char *revision;
+   const char *commit = NULL;
+  

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

[Joel Teichroeb]

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

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

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

Previous threads:
v1: https://public-inbox.org/git/20180325173916.GE10909@hank/T/
v2: https://public-inbox.org/git/20180326011426.19159-1-j...@teichroeb.net/

Changes from v2:
 - Fixed formatting (I keep forgetting to set vim to tabs)
 - Renamed destroy to free
 - Redid my tests to validate more (Thanks Johannes)
 - Deleted more shell code that isn't needed anymore

Joel Teichroeb (5):
  stash: improve option parsing test coverage
  stash: convert apply to builtin
  stash: convert drop and clear to builtin
  stash: convert branch to builtin
  stash: convert pop to builtin

 .gitignore  |   1 +
 Makefile|   1 +
 builtin.h   |   1 +
 builtin/stash--helper.c | 631 
 git-stash.sh| 136 +--
 git.c   |   1 +
 t/t3903-stash.sh|  16 ++
 7 files changed, 659 insertions(+), 128 deletions(-)
 create mode 100644 builtin/stash--helper.c

-- 
2.16.2

[PATCH v3 1/5] stash: improve option parsing test coverage

[Joel Teichroeb]

In preparation for converting the stash command incrementally to
a builtin command, this patch improves test coverage of the option
parsing. Both for having too many paramerters, or too few.

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

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index aefde7b17..8a666c60c 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -84,6 +84,17 @@ test_expect_success 'apply stashed changes (including 
index)' '
test 1 = $(git show HEAD:file)
 '
 
+test_expect_success 'giving too many ref agruments does nothing' '
+
+   for type in apply drop pop show "branch stash-branch"
+   do
+   test-chmtime =123456789 file &&
+   test_must_fail git stash $type stash@{0} stash@{1} 2>err &&
+   test_i18ngrep "Too many" err &&
+   test 123456789 = $(test-chmtime -v +0 file | sed 
's/[^0-9].*$//') || return 1
+   done
+'
+
 test_expect_success 'unstashing in a subdirectory' '
git reset --hard HEAD &&
mkdir subdir &&
@@ -479,6 +490,11 @@ test_expect_success 'stash branch - stashes on stack, 
stash-like argument' '
test $(git ls-files --modified | wc -l) -eq 1
 '
 
+test_expect_success 'stash branch complains with no arguments' '
+   test_must_fail git stash branch 2>err &&
+   test_i18ngrep "No branch name specified" err
+'
+
 test_expect_success 'stash show format defaults to --stat' '
git stash clear &&
test_when_finished "git reset --hard HEAD" &&
-- 
2.16.2

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

[Joel Teichroeb]

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

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 050de415b4..640de6d0aa 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -11,7 +11,14 @@
 #include "dir.h"
 
 static const char * const git_stash_helper_usage[] = {
+   N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   N_("git stash--helper clear"),
+   NULL
+};
+
+static const char * const git_stash_helper_drop_usage[] = {
+   N_("git stash--helper drop [-q|--quiet] []"),
NULL
 };
 
@@ -20,6 +27,11 @@ static const char * const git_stash_helper_apply_usage[] = {
NULL
 };
 
+static const char * const git_stash_helper_clear_usage[] = {
+   N_("git stash--helper clear"),
+   NULL
+};
+
 static const char *ref_stash = "refs/stash";
 static int quiet;
 static char stash_index_path[PATH_MAX];
@@ -166,6 +178,29 @@ static int get_stash_info(struct stash_info *info, int 
argc, const char **argv)
return 0;
 }
 
+static int do_clear_stash(void)
+{
+   struct object_id obj;
+   if (get_oid(ref_stash, ))
+   return 0;
+
+   return delete_ref(NULL, ref_stash, , 0);
+}
+
+static int clear_stash(int argc, const char **argv, const char *prefix)
+{
+   struct option options[] = {
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options, 
git_stash_helper_clear_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+
+   if (argc != 0)
+   return error(_("git stash--helper clear with parameters is 
unimplemented"));
+
+   return do_clear_stash();
+}
+
 static int reset_tree(struct object_id *i_tree, int update, int reset)
 {
struct unpack_trees_options opts;
@@ -399,6 +434,68 @@ static int apply_stash(int argc, const char **argv, const 
char *prefix)
return ret;
 }
 
+static int do_drop_stash(const char *prefix, struct stash_info *info)
+{
+   struct argv_array args = ARGV_ARRAY_INIT;
+   int ret;
+   struct child_process cp = CHILD_PROCESS_INIT;
+
+   argv_array_pushl(, "reflog", "delete", "--updateref", "--rewrite", 
NULL);
+   argv_array_push(, info->revision.buf);
+   ret = cmd_reflog(args.argc, args.argv, prefix);
+   if (!ret) {
+   if (!quiet)
+   printf(_("Dropped %s (%s)\n"), info->revision.buf, 
oid_to_hex(>w_commit));
+   } else {
+   return error(_("%s: Could not drop stash entry"), 
info->revision.buf);
+   }
+
+   cp.git_cmd = 1;
+   /* Even though --quiet is specified, rev-parse still outputs the hash */
+   cp.no_stdout = 1;
+   argv_array_pushl(, "rev-parse", "--verify", "--quiet", NULL);
+   argv_array_pushf(, "%s@{0}", ref_stash);
+   ret = run_command();
+
+   if (ret)
+   do_clear_stash();
+
+   return 0;
+}
+
+static int assert_stash_ref(struct stash_info *info)
+{
+   if (!info->is_stash_ref)
+   return error(_("'%s' is not a stash reference"), 
info->revision.buf);
+
+   return 0;
+}
+
+static int drop_stash(int argc, const char **argv, const char *prefix)
+{
+   struct stash_info info;
+   int ret;
+   struct option options[] = {
+   OPT__QUIET(, N_("be quiet, only report errors")),
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+   git_stash_helper_drop_usage, 0);
+
+   if (get_stash_info(, argc, argv))
+   return -1;
+
+   if (assert_stash_ref()) {
+   destroy_stash_info();
+   return -1;
+   }
+
+   ret = do_drop_stash(prefix, );
+   destroy_stash_info();
+   return ret;
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
int result = 0;
@@ -421,6 +518,10 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
usage_with_options(git_stash_helper_usage, options);
else if (!strcmp(argv[0], "apply"))
result = apply_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "clear"))
+   result = clear_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "drop"))
+   result = drop_stash(argc, argv, prefix);
else {
error(_("unknown subcommand: %s"), argv[0]);
usage_with_options(git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index 0b5d1f3743..0b8f07

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

[Joel Teichroeb]

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

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

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

[Joel Teichroeb]

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

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

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

[Joel Teichroeb]

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

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

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

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

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

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

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

-- 
2.16.2

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

[Joel Teichroeb]

Signed-off-by: Joel Teichroeb <j...@teichroeb.net>
---
 .gitignore  |   1 +
 Makefile|   1 +
 builtin.h   |   1 +
 builtin/stash--helper.c | 431 
 git-stash.sh|  75 +
 git.c   |   1 +
 6 files changed, 440 insertions(+), 70 deletions(-)
 create mode 100644 builtin/stash--helper.c

diff --git a/.gitignore b/.gitignore
index 833ef3b0b7..296d5f376d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -152,6 +152,7 @@
 /git-show-ref
 /git-stage
 /git-stash
+/git-stash--helper
 /git-status
 /git-stripspace
 /git-submodule
diff --git a/Makefile b/Makefile
index 96f6138f63..6cfdbe9a32 100644
--- a/Makefile
+++ b/Makefile
@@ -1028,6 +1028,7 @@ BUILTIN_OBJS += builtin/send-pack.o
 BUILTIN_OBJS += builtin/shortlog.o
 BUILTIN_OBJS += builtin/show-branch.o
 BUILTIN_OBJS += builtin/show-ref.o
+BUILTIN_OBJS += builtin/stash--helper.o
 BUILTIN_OBJS += builtin/stripspace.o
 BUILTIN_OBJS += builtin/submodule--helper.o
 BUILTIN_OBJS += builtin/symbolic-ref.o
diff --git a/builtin.h b/builtin.h
index 42378f3aa4..a14fd85b0e 100644
--- a/builtin.h
+++ b/builtin.h
@@ -219,6 +219,7 @@ extern int cmd_shortlog(int argc, const char **argv, const 
char *prefix);
 extern int cmd_show(int argc, const char **argv, const char *prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
 extern int cmd_status(int argc, const char **argv, const char *prefix);
+extern int cmd_stash__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
 extern int cmd_submodule__helper(int argc, const char **argv, const char 
*prefix);
 extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
new file mode 100644
index 00..050de415b4
--- /dev/null
+++ b/builtin/stash--helper.c
@@ -0,0 +1,431 @@
+#include "builtin.h"
+#include "config.h"
+#include "parse-options.h"
+#include "refs.h"
+#include "lockfile.h"
+#include "cache-tree.h"
+#include "unpack-trees.h"
+#include "merge-recursive.h"
+#include "argv-array.h"
+#include "run-command.h"
+#include "dir.h"
+
+static const char * const git_stash_helper_usage[] = {
+   N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   NULL
+};
+
+static const char * const git_stash_helper_apply_usage[] = {
+   N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   NULL
+};
+
+static const char *ref_stash = "refs/stash";
+static int quiet;
+static char stash_index_path[PATH_MAX];
+
+struct stash_info {
+   struct object_id w_commit;
+   struct object_id b_commit;
+   struct object_id i_commit;
+   struct object_id u_commit;
+   struct object_id w_tree;
+   struct object_id b_tree;
+   struct object_id i_tree;
+   struct object_id u_tree;
+   struct strbuf revision;
+   int is_stash_ref;
+   int has_u;
+};
+
+static int get_symbolic_name(const char *symbolic, struct strbuf *out)
+{
+   struct child_process cp = CHILD_PROCESS_INIT;
+
+   cp.git_cmd = 1;
+   argv_array_pushl(, "rev-parse", "--symbolic-full-name", NULL);
+   argv_array_pushf(, "%s", symbolic);
+   return pipe_command(, NULL, 0, out, 0, NULL, 0);
+}
+
+static int have_stash(void)
+{
+   struct child_process cp = CHILD_PROCESS_INIT;
+
+   cp.git_cmd = 1;
+   cp.no_stdout = 1;
+   argv_array_pushl(, "rev-parse", "--verify", "--quiet", NULL);
+   argv_array_pushf(, "%s", ref_stash);
+   return pipe_command(, NULL, 0, NULL, 0, NULL, 0);
+}
+
+static void destroy_stash_info(struct stash_info *info)
+{
+   strbuf_release(>revision);
+}
+
+static int get_stash_info(struct stash_info *info, int argc, const char **argv)
+{
+   struct strbuf w_commit_rev = STRBUF_INIT;
+   struct strbuf b_commit_rev = STRBUF_INIT;
+   struct strbuf w_tree_rev = STRBUF_INIT;
+   struct strbuf b_tree_rev = STRBUF_INIT;
+   struct strbuf i_tree_rev = STRBUF_INIT;
+   struct strbuf u_tree_rev = STRBUF_INIT;
+   struct strbuf symbolic = STRBUF_INIT;
+   struct strbuf out = STRBUF_INIT;
+   int ret;
+   const char *revision;
+   const char *commit = NULL;
+   char *end_of_rev;
+   info->is_stash_ref = 0;
+
+   if (argc > 1) {
+   int i;
+   fprintf(stderr, _("Too many revisions specified:"));
+   for (i = 0; i < argc; ++i) {
+   fprintf(stderr, " '%s'", argv[i]);
+   }
+   fprintf(stderr, "\n");
+
+   return -1;
+   }
+
+   if (ar

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

[Joel Teichroeb]

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

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

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

[Joel Teichroeb]

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

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

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

[Joel Teichroeb]

On Sun, Mar 25, 2018 at 1:09 AM, Christian Couder
 wrote:
> It seems to me that the apply_stash() shell function is also used in
> pop_stash() and in apply_to_branch(). Can the new helper be used there
> too instead of apply_stash()? And then could apply_stash() be remove?

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

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

[Joel Teichroeb]

---
 builtin/stash--helper.c | 44 
 git-stash.sh|  3 ++-
 2 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index e9a9574f40..18c4aba665 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -12,6 +12,7 @@
 
 static const char * const git_stash_helper_usage[] = {
N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   N_("git stash--helper branch  []"),
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  []"),
+   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, , 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(, commit))
+   return -1;
+
+   argv_array_pushl(, "checkout", "-b", NULL);
+   argv_array_push(, branch);
+   argv_array_push(, sha1_to_hex(info.b_commit.hash));
+   ret = cmd_checkout(args.argc, args.argv, prefix);
+   if (ret)
+   return -1;
+
+   ret = do_apply_stash(prefix, , 1);
+   if (!ret && info.is_stash_ref)
+   ret = do_drop_stash(prefix, );
+
+   return ret;
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
int result = 0;
@@ -329,6 +371,8 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
usage_with_options(git_stash_helper_usage, options);
else if (!strcmp(argv[0], "apply"))
result = apply_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "branch"))
+   result = branch_stash(argc, argv, prefix);
else {
error(_("unknown subcommand: %s"), argv[0]);
usage_with_options(git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index 92c084eb17..360643ad4e 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -736,7 +736,8 @@ pop)
;;
 branch)
shift
-   apply_to_branch "$@"
+   cd "$START_DIR"
+   git stash--helper branch "$@"
;;
 *)
case $# in
-- 
2.16.2

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

[Joel Teichroeb]

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

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

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

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

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

-- 
2.16.2

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

[Joel Teichroeb]

---
 builtin/stash--helper.c | 93 +
 git-stash.sh|  4 +--
 2 files changed, 95 insertions(+), 2 deletions(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 18c4aba665..1598b82ac2 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -11,8 +11,15 @@
 #include "dir.h"
 
 static const char * const git_stash_helper_usage[] = {
+   N_("git stash--helper drop [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
N_("git stash--helper branch  []"),
+   N_("git stash--helper clear"),
+   NULL
+};
+
+static const char * const git_stash_helper_drop_usage[] = {
+   N_("git stash--helper drop [-q|--quiet] []"),
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, ))
+   return 0;
+
+   return delete_ref(NULL, ref_stash, , 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, , 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(, "reflog", "delete", "--updateref", "--rewrite", 
NULL);
+   argv_array_push(, 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(, "rev-parse", "--verify", "--quiet", NULL);
+   argv_array_pushf(, "%s@{0}", ref_stash);
+   ret = run_command();
+
+   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(, 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(, commit))
+   return -1;
+
+   if (!info.is_stash_ref)
+   return error(_("'%s' is not a stash reference"), commit);
+
+   return do_drop_stash(prefix, );
+}
+
 static int branch_stash(int argc, const char **argv, const char *prefix)
 {
const char *commit = NULL, *branch = NULL;
@@ -371,6 +460,10 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
usage_with_options(git_stash_helper_usage, options);
else if (!strcmp(argv[0], "apply"))
result = apply_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "clear"))
+   result = clear_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "drop"))
+   result = drop_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "branch"))
result = branch_stash(argc, argv, prefix);
else {
diff --git a/git-stash.sh b/git-stash.sh
index 360643ad4e..54d0a6c21f 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -716,7 +716,7 @@ apply)
;;
 clear)
shift
-   clear_stash "$@"
+   git stash--helper clear "$@"
;;
 create)
shift
@@ -728,7 +728,7 @@ store)
;;
 drop)
shift
-   drop_stash "$@"
+   git stash--helper drop "$@"
;;
 pop)
shift
-- 
2.16.2

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

[Joel Teichroeb]

---
 builtin/stash--helper.c | 38 ++
 git-stash.sh|  3 ++-
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 1598b82ac2..b912f84c97 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -12,6 +12,7 @@
 
 static const char * const git_stash_helper_usage[] = {
N_("git stash--helper drop [-q|--quiet] []"),
+   N_("git stash--helper pop [--index] [-q|--quiet] []"),
N_("git stash--helper apply [--index] [-q|--quiet] []"),
N_("git stash--helper branch  []"),
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] []"),
+   NULL
+};
+
 static const char * const git_stash_helper_apply_usage[] = {
N_("git stash--helper apply [--index] [-q|--quiet] []"),
NULL
@@ -402,6 +408,36 @@ static int drop_stash(int argc, const char **argv, const 
char *prefix)
return do_drop_stash(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(, N_("be quiet, only report errors")),
+   OPT_BOOL(0, "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(, commit))
+   return -1;
+
+   if (!info.is_stash_ref)
+   return error(_("'%s' is not a stash reference"), commit);
+
+   if (do_apply_stash(prefix, , index))
+   return -1;
+
+   return do_drop_stash(prefix, );
+}
+
 static int branch_stash(int argc, const char **argv, const char *prefix)
 {
const char *commit = NULL, *branch = NULL;
@@ -464,6 +500,8 @@ int cmd_stash__helper(int argc, const char **argv, const 
char *prefix)
result = clear_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "drop"))
result = drop_stash(argc, argv, prefix);
+   else if (!strcmp(argv[0], "pop"))
+   result = pop_stash(argc, argv, prefix);
else if (!strcmp(argv[0], "branch"))
result = branch_stash(argc, argv, prefix);
else {
diff --git a/git-stash.sh b/git-stash.sh
index 54d0a6c21f..d595bbaf64 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -732,7 +732,8 @@ drop)
;;
 pop)
shift
-   pop_stash "$@"
+   cd "$START_DIR"
+   git stash--helper pop "$@"
;;
 branch)
shift
-- 
2.16.2

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

[Joel Teichroeb]

---
 .gitignore  |   1 +
 Makefile|   1 +
 builtin.h   |   1 +
 builtin/stash--helper.c | 339 
 git-stash.sh|   3 +-
 git.c   |   1 +
 6 files changed, 345 insertions(+), 1 deletion(-)
 create mode 100644 builtin/stash--helper.c

diff --git a/.gitignore b/.gitignore
index 833ef3b0b7..296d5f376d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -152,6 +152,7 @@
 /git-show-ref
 /git-stage
 /git-stash
+/git-stash--helper
 /git-status
 /git-stripspace
 /git-submodule
diff --git a/Makefile b/Makefile
index a1d8775adb..8ca361c57a 100644
--- a/Makefile
+++ b/Makefile
@@ -1020,6 +1020,7 @@ BUILTIN_OBJS += builtin/send-pack.o
 BUILTIN_OBJS += builtin/shortlog.o
 BUILTIN_OBJS += builtin/show-branch.o
 BUILTIN_OBJS += builtin/show-ref.o
+BUILTIN_OBJS += builtin/stash--helper.o
 BUILTIN_OBJS += builtin/stripspace.o
 BUILTIN_OBJS += builtin/submodule--helper.o
 BUILTIN_OBJS += builtin/symbolic-ref.o
diff --git a/builtin.h b/builtin.h
index 42378f3aa4..a14fd85b0e 100644
--- a/builtin.h
+++ b/builtin.h
@@ -219,6 +219,7 @@ extern int cmd_shortlog(int argc, const char **argv, const 
char *prefix);
 extern int cmd_show(int argc, const char **argv, const char *prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
 extern int cmd_status(int argc, const char **argv, const char *prefix);
+extern int cmd_stash__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
 extern int cmd_submodule__helper(int argc, const char **argv, const char 
*prefix);
 extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
new file mode 100644
index 00..e9a9574f40
--- /dev/null
+++ b/builtin/stash--helper.c
@@ -0,0 +1,339 @@
+#include "builtin.h"
+#include "config.h"
+#include "parse-options.h"
+#include "refs.h"
+#include "lockfile.h"
+#include "cache-tree.h"
+#include "unpack-trees.h"
+#include "merge-recursive.h"
+#include "argv-array.h"
+#include "run-command.h"
+#include "dir.h"
+
+static const char * const git_stash_helper_usage[] = {
+   N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   NULL
+};
+
+static const char * const git_stash_helper_apply_usage[] = {
+   N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   NULL
+};
+
+static const char *ref_stash = "refs/stash";
+static int quiet;
+static char stash_index_path[PATH_MAX];
+
+struct stash_info {
+   struct object_id w_commit;
+   struct object_id b_commit;
+   struct object_id i_commit;
+   struct object_id u_commit;
+   struct object_id w_tree;
+   struct object_id b_tree;
+   struct object_id i_tree;
+   struct object_id u_tree;
+   const char *message;
+   const char *revision;
+   int is_stash_ref;
+   int has_u;
+   const char *patch;
+};
+
+static int get_stash_info(struct stash_info *info, const char *commit)
+{
+   struct strbuf w_commit_rev = STRBUF_INIT;
+   struct strbuf b_commit_rev = STRBUF_INIT;
+   struct strbuf w_tree_rev = STRBUF_INIT;
+   struct strbuf b_tree_rev = STRBUF_INIT;
+   struct strbuf i_tree_rev = STRBUF_INIT;
+   struct strbuf u_tree_rev = STRBUF_INIT;
+   struct strbuf commit_buf = STRBUF_INIT;
+   struct strbuf symbolic = STRBUF_INIT;
+   struct strbuf out = STRBUF_INIT;
+   int ret;
+   const char *revision = commit;
+   char *end_of_rev;
+   struct child_process cp = CHILD_PROCESS_INIT;
+   info->is_stash_ref = 0;
+
+   if (commit == NULL) {
+   strbuf_addf(_buf, "%s@{0}", ref_stash);
+   revision = commit_buf.buf;
+   } else if (strspn(commit, "0123456789") == strlen(commit)) {
+   strbuf_addf(_buf, "%s@{%s}", ref_stash, commit);
+   revision = commit_buf.buf;
+   }
+   info->revision = revision;
+
+   strbuf_addf(_commit_rev, "%s", revision);
+   strbuf_addf(_commit_rev, "%s^1", revision);
+   strbuf_addf(_tree_rev, "%s:", revision);
+   strbuf_addf(_tree_rev, "%s^1:", revision);
+   strbuf_addf(_tree_rev, "%s^2:", revision);
+
+   ret = !get_oid(w_commit_rev.buf, >w_commit) &&
+   !get_oid(b_commit_rev.buf, >b_commit) &&
+   !get_oid(w_tree_rev.buf, >w_tree) &&
+   !get_oid(b_tree_rev.buf, >b_tree) &&
+   !get_oid(i_tree_rev.buf, >i_tree);
+
+   strbuf_release(_commit_rev);
+   strbuf_release(_commit_rev);
+   strbuf_release(_tree_rev);
+   strbuf_release(_tree_rev);
+   strbuf_release(_tree_rev);
+
+   if (!ret)
+   return error(_("%s is not a valid reference"), revision);
+
+   strbuf_addf(_tree_rev, "%s^3:", revision);
+
+   info->has_u = !get_oid(u_tree_rev.buf, >u_tree);
+
+   strbuf_release(_tree_rev);
+
+   

Re: [PATCH 2/2] stash: implement builtin stash helper

[Joel Teichroeb]

Thanks for your comments! I've incorporated them all for the next patch set.

[PATCH 2/2] stash: implement builtin stash helper

[Joel Teichroeb]

Start moving stash functions over to builtin c code and call
them in the shell script, instead of converting it all at
once.

Signed-off-by: Joel Teichroeb <j...@teichroeb.net>
---
 Makefile|   1 +
 builtin.h   |   1 +
 builtin/stash--helper.c | 516 
 git-stash.sh| 134 +
 git.c   |   1 +
 5 files changed, 526 insertions(+), 127 deletions(-)
 create mode 100644 builtin/stash--helper.c

diff --git a/Makefile b/Makefile
index ee9d5eb11..3a9bd4d57 100644
--- a/Makefile
+++ b/Makefile
@@ -1000,6 +1000,7 @@ BUILTIN_OBJS += builtin/send-pack.o
 BUILTIN_OBJS += builtin/shortlog.o
 BUILTIN_OBJS += builtin/show-branch.o
 BUILTIN_OBJS += builtin/show-ref.o
+BUILTIN_OBJS += builtin/stash--helper.o
 BUILTIN_OBJS += builtin/stripspace.o
 BUILTIN_OBJS += builtin/submodule--helper.o
 BUILTIN_OBJS += builtin/symbolic-ref.o
diff --git a/builtin.h b/builtin.h
index 42378f3aa..a14fd85b0 100644
--- a/builtin.h
+++ b/builtin.h
@@ -219,6 +219,7 @@ extern int cmd_shortlog(int argc, const char **argv, const 
char *prefix);
 extern int cmd_show(int argc, const char **argv, const char *prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
 extern int cmd_status(int argc, const char **argv, const char *prefix);
+extern int cmd_stash__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
 extern int cmd_submodule__helper(int argc, const char **argv, const char 
*prefix);
 extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
new file mode 100644
index 0..c8cb667fe
--- /dev/null
+++ b/builtin/stash--helper.c
@@ -0,0 +1,516 @@
+#include "builtin.h"
+#include "config.h"
+#include "parse-options.h"
+#include "refs.h"
+#include "lockfile.h"
+#include "cache-tree.h"
+#include "unpack-trees.h"
+#include "merge-recursive.h"
+#include "argv-array.h"
+#include "run-command.h"
+
+static const char * const git_stash_usage[] = {
+   N_("git stash--helper drop [-q|--quiet] []"),
+   N_("git stash--helper pop [--index] [-q|--quiet] []"),
+   N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   N_("git stash--helper branch  []"),
+   N_("git stash--helper clear"),
+   NULL
+};
+
+static const char * const git_stash_drop_usage[] = {
+   N_("git stash--helper drop [-q|--quiet] []"),
+   NULL
+};
+
+static const char * const git_stash_pop_usage[] = {
+   N_("git stash--helper pop [--index] [-q|--quiet] []"),
+   NULL
+};
+
+static const char * const git_stash_apply_usage[] = {
+   N_("git stash--helper apply [--index] [-q|--quiet] []"),
+   NULL
+};
+
+static const char * const git_stash_branch_usage[] = {
+   N_("git stash--helper branch  []"),
+   NULL
+};
+
+static const char * const git_stash_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];
+
+struct stash_info {
+   struct object_id w_commit;
+   struct object_id b_commit;
+   struct object_id i_commit;
+   struct object_id u_commit;
+   struct object_id w_tree;
+   struct object_id b_tree;
+   struct object_id i_tree;
+   struct object_id u_tree;
+   const char *message;
+   const char *revision;
+   int is_stash_ref;
+   int has_u;
+   const char *patch;
+};
+
+static int get_stash_info(struct stash_info *info, const char *commit)
+{
+   struct strbuf w_commit_rev = STRBUF_INIT;
+   struct strbuf b_commit_rev = STRBUF_INIT;
+   struct strbuf w_tree_rev = STRBUF_INIT;
+   struct strbuf b_tree_rev = STRBUF_INIT;
+   struct strbuf i_tree_rev = STRBUF_INIT;
+   struct strbuf u_tree_rev = STRBUF_INIT;
+   struct strbuf commit_buf = STRBUF_INIT;
+   struct strbuf symbolic = STRBUF_INIT;
+   struct strbuf out = STRBUF_INIT;
+   int ret;
+   const char *revision = commit;
+   char *end_of_rev;
+   struct child_process cp = CHILD_PROCESS_INIT;
+   info->is_stash_ref = 0;
+
+   if (commit == NULL) {
+   strbuf_addf(_buf, "%s@{0}", ref_stash);
+   revision = commit_buf.buf;
+   } else if (strspn(commit, "0123456789") == strlen(commit)) {
+   strbuf_addf(_buf, "%s@{%s}", ref_stash, commit);
+   revision = commit_buf.buf;
+   }
+   info->revision = revision;
+
+   strbuf_addf(_commit_rev, "%s", revision);
+   strbuf_addf(_commit_rev, "%s^1",

[PATCH 1/2] merge: close the index lock when not writing the new index

[Joel Teichroeb]

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

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

diff --git a/merge-recursive.c b/merge-recursive.c
index 2ca8444c6..225ff3fb5 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2184,9 +2184,12 @@ int merge_recursive_generic(struct merge_options *o,
if (clean < 0)
return clean;
 
-   if (active_cache_changed &&
-   write_locked_index(_index, , COMMIT_LOCK))
-   return err(o, _("Unable to write index."));
+   if (active_cache_changed) {
+   if (write_locked_index(_index, , COMMIT_LOCK))
+   return err(o, _("Unable to write index."));
+   } else {
+   rollback_lock_file();
+   }
 
return clean ? 0 : 1;
 }
-- 
2.15.0

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

[Joel Teichroeb]

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

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

Joel Teichroeb (2):
  merge: close the index lock when not writing the new index
  stash: implement builtin stash helper

 Makefile|   1 +
 builtin.h   |   1 +
 builtin/stash--helper.c | 516 
 git-stash.sh| 134 +
 git.c   |   1 +
 merge-recursive.c   |   9 +-
 6 files changed, 532 insertions(+), 130 deletions(-)
 create mode 100644 builtin/stash--helper.c

-- 
2.15.0

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.schinde...@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

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

[Joel Teichroeb]

Hi Junio,

I was just too lazy to write a cover letter, and thought these would
make sense on their own. I'll make sure to include a cover letter next
time.

I just ripped them out of my patch series on implementing stash as a
builtin[1]. Since I haven't had time, I figured I could at least get
the additional tests I wrote into the codebase.

The tests mainly expand coverage of git stash, covering a few critical
error cases that didn't have tests.

[1] https://public-inbox.org/git/20170608005535.13080-1-j...@teichroeb.net/

On Sat, Aug 19, 2017 at 1:55 PM, Junio C Hamano  wrote:
> I see three patches that add tests, but it is hard to judge them
> without any explanation on what the point of them are.
>
> Are you documenting an existing breakage?  Are you extending test
> coverage for some breakage we recently fixed without adding tests to
> ensure that the fix will stay unbroken?  Are you planning to touch
> the implementation (perhaps to add yet another feature that uses
> some existing code) and adding new tests in advance to avoid breaking
> the existing code?  Some other motivation?
>
> These would have made fine material to write in the cover letter for
> this series.
>
> Thanks.

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

[Joel Teichroeb]

All that we are really testing here is that the message is
correct when we are not on any branch. All other functionality is
already tested elsewhere.

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

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

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

[Joel Teichroeb]

Ensure the command suceeds and outputs nothing

Signed-off-by: Joel Teichroeb <j...@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 4046817d70..f0708ced27 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -444,6 +444,14 @@ test_expect_failure 'stash file to directory' '
test foo = "$(cat file/file)"
 '
 
+test_expect_success 'stash create - no changes' '
+   git stash clear &&
+   test_when_finished "git reset --hard HEAD" &&
+   git reset --hard &&
+   git stash create >actual &&
+   test_must_be_empty actual
+'
+
 test_expect_success 'stash branch - no stashes on stack, stash-like argument' '
git stash clear &&
test_when_finished "git reset --hard HEAD" &&
-- 
2.14.1

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

[Joel Teichroeb]

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

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

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

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

[Joel Teichroeb]

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

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

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

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

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



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

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

[Joel Teichroeb]

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

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

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

[Joel Teichroeb]

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

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

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

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

[Joel Teichroeb]

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

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

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

Agreed.

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

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

[Joel Teichroeb]

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

Thanks,
Joel

On 6/7/2017 5:55 PM, Joel Teichroeb wrote:

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

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

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

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

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

Re: Re: git stash --include-untracked possible destructive behavior

[Joel Teichroeb]

Looks like that series was merged last week, fixing this issue.

Marc, the fix will probably be in git 2.14.

Joel

Re: git stash --include-untracked possible destructive behavior

[Joel Teichroeb]

It looks like it's an issue with git clean, as that's what stash uses 
internally. You can try your same test, but replace git stash with "git 
clean -df" and it will delete Data. Maybe clean should check if there 
are any ignored files in an untracked directory and only remove it if -x 
is specified?


Joel

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

[Joel Teichroeb]

Ensure the command gives the correct return code

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

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

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

[Joel Teichroeb]

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

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

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

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

[Joel Teichroeb]

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

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

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

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

[Joel Teichroeb]

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

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

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

[Joel Teichroeb]

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

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

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

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

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

-- 
2.13.0

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

[Joel Teichroeb]

Implement all git stash functionality as a builtin command

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

diff --git a/Makefile b/Makefile
index 7c621f7f76..3364d87630 100644
--- a/Makefile
+++ b/Makefile
@@ -525,7 +525,6 @@ SCRIPT_SH += git-quiltimport.sh
 SCRIPT_SH += git-rebase.sh
 SCRIPT_SH += git-remote-testgit.sh
 SCRIPT_SH += git-request-pull.sh
-SCRIPT_SH += git-stash.sh
 SCRIPT_SH += git-submodule.sh
 SCRIPT_SH += git-web--browse.sh
 
@@ -965,6 +964,7 @@ BUILTIN_OBJS += builtin/send-pack.o
 BUILTIN_OBJS += builtin/shortlog.o
 BUILTIN_OBJS += builtin/show-branch.o
 BUILTIN_OBJS += builtin/show-ref.o
+BUILTIN_OBJS += builtin/stash.o
 BUILTIN_OBJS += builtin/stripspace.o
 BUILTIN_OBJS += builtin/submodule--helper.o
 BUILTIN_OBJS += builtin/symbolic-ref.o
diff --git a/builtin.h b/builtin.h
index 498ac80d07..fa59481420 100644
--- a/builtin.h
+++ b/builtin.h
@@ -119,6 +119,7 @@ extern int cmd_shortlog(int argc, const char **argv, const 
char *prefix);
 extern int cmd_show(int argc, const char **argv, const char *prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
 extern int cmd_status(int argc, const char **argv, const char *prefix);
+extern int cmd_stash(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
 extern int cmd_submodule__helper(int argc, const char **argv, const char 
*prefix);
 extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
diff --git a/builtin/stash.c b/builtin/stash.c
new file mode 100644
index 00..a9680f2909
--- /dev/null
+++ b/builtin/stash.c
@@ -0,0 +1,1224 @@
+#include "builtin.h"
+#include "parse-options.h"
+#include "refs.h"
+#include "tree.h"
+#include "lockfile.h"
+#include "object.h"
+#include "tree-walk.h"
+#include "cache-tree.h"
+#include "unpack-trees.h"
+#include "diff.h"
+#include "revision.h"
+#include "commit.h"
+#include "diffcore.h"
+#include "merge-recursive.h"
+#include "argv-array.h"
+#include "run-command.h"
+
+static const char * const git_stash_usage[] = {
+   N_("git stash list []"),
+   N_("git stash show []"),
+   N_("git stash drop [-q|--quiet] []"),
+   N_("git stash ( pop | apply ) [--index] [-q|--quiet] []"),
+   N_("git stash branch  []"),
+   N_("git stash [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]"),
+   N_("[-u|--include-untracked] [-a|--all] []]"),
+   N_("git stash clear"),
+   N_("git stash create []"),
+   N_("git stash store [-m|--message ] [-q|--quiet] "),
+   NULL
+};
+
+static const char * const git_stash_list_usage[] = {
+   N_("git stash list []"),
+   NULL
+};
+
+static const char * const git_stash_show_usage[] = {
+   N_("git stash show []"),
+   NULL
+};
+
+static const char * const git_stash_drop_usage[] = {
+   N_("git stash drop [-q|--quiet] []"),
+   NULL
+};
+
+static const char * const git_stash_pop_usage[] = {
+   N_("git stash pop [--index] [-q|--quiet] []"),
+   NULL
+};
+
+static const char * const git_stash_apply_usage[] = {
+   N_("git stash apply [--index] [-q|--quiet] []"),
+   NULL
+};
+
+static const char * const git_stash_branch_usage[] = {
+   N_("git stash branch  []"),
+   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] []]"),
+   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 []"),
+   NULL
+};
+
+static const char * const git_stash_store_usage[] = {
+   N_("git stash store [-m|--message ] [-q|--quiet] "),
+   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;
+ 

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

[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

[Joel Teichroeb]

On Sun, May 28, 2017 at 11:26 AM, Ævar Arnfjörð Bjarmason
<ava...@gmail.com> wrote:
> On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb <j...@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, ) == 0 &&
>> +   get_sha1_with_context(u_tree_rev.buf, 0, info->u_tree.hash, 
>> ) == 0;
>> +
>> +
>> +   /* TODO: Improve this logic */
>> +   strbuf_addf(, "%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", , N_("message"),
>> +N_("stash commit message")),
>> +   OPT__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.

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

[Joel Teichroeb]

Signed-off-by: Joel Teichroeb <j...@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(_index, _file, COMMIT_LOCK))
die(_("Unable to write new index file"));
-   }
+   } else
+   rollback_lock_file(_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(_index, _file, COMMIT_LOCK))
-   die(_("Unable to write new index file"));
+   if (active_cache_changed) {
+   if (write_locked_index(_index, _file, COMMIT_LOCK))
+   die(_("Unable to write new index file"));
+   } else
+   rollback_lock_file(_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(_index, _file, COMMIT_LOCK))
die(_("Unable to write new index file"));
-   }
+   } else
+   rollback_lock_file(_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(_index, lock, COMMIT_LOCK))
-   return err(o, _("Unable to write index."));
+   if (active_cache_changed) {
+   if (write_locked_index(_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 2/4] stash: add test for stashing in a detached state

[Joel Teichroeb]

Signed-off-by: Joel Teichroeb <j...@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 --  stashes and restores the file' '
>foo &&
>bar &&
-- 
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 <j...@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 00..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 []"),
+   N_("git stash show []"),
+   N_("git stash drop [-q|--quiet] []"),
+   N_("git stash ( pop | apply ) [--index] [-q|--quiet] []"),
+   N_("git stash branch  []"),
+   N_("git stash [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]"),
+   N_("[-u|--include-untracked] [-a|--all] []]"),
+   N_("git stash clear"),
+   N_("git stash create []"),
+   N_("git stash store [-m|--message ] [-q|--quiet] "),
+   NULL
+};
+
+static const char * const git_stash_list_usage[] = {
+   N_("git stash list []"),
+   NULL
+};
+
+static const char * const git_stash_show_usage[] = {
+   N_("git stash show []"),
+   NULL
+};
+
+static const char * const git_stash_drop_usage[] = {
+   N_("git stash drop [-q|--quiet] []"),
+   NULL
+};
+
+static const char * const git_stash_pop_usage[] = {
+   N_("git stash pop [--index] [-q|--quiet] []"),
+   NULL
+};
+
+static const char * const git_stash_apply_usage[] = {
+   N_("git stash apply [--index] [-q|--quiet] []"),
+   NULL
+};
+
+static const char * const git_stash_branch_usage[] = {
+   N_("git stash branch  []"),
+   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] []]"),
+   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 []"),
+   NULL
+};
+
+static const char * const git_stash_store_usage[] = {
+   N_("git stash store [-m|--message ] [-q|--quiet] "),
+   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;
+   

[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 <j...@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 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/RFC V2] stash: implement builtin stash

[Joel Teichroeb]

Implement all git stash functionality as a builtin command

Signed-off-by: Joel Teichroeb <j...@teichroeb.net>
---
I've been working on rewriting git stash as a c builtin and I have all
but three tests passing. I'm having a bit of trouble fixing them, as
well as a few other issues, so I'd really appreciate some help. Don't
bother commenting on the small details yet as I still need to go
though the code to make sure it matches the code style guidelines.

Test Summary Report
---
t7601-merge-pull-config.sh   (Wstat: 256 Tests: 14
Failed: 2)
  Failed tests:  11-12
  Non-zero exit status: 1
t3903-stash.sh   (Wstat: 256 Tests: 74
Failed: 1)
  Failed test:  69
  Non-zero exit status: 1

It looks to be the same issue for both of these cases where
merge-recursive reports:
error: Your local changes to the following files would be overwritten by merge:
file
other-file

which doesn't make sense as those files didn't exist before the merge.
Furthermore if I take the existing git stash implementation and have
it stop before running the merge-recursive command and then run it on
the commandline manually, I get the same issue. I've tried setting all
the same environment variables that the existing git stash
implementation does, but it doesn't help. It seems like there could be
a bug in merge-recursive, but I'm not sure how to track it down.

git stash uses the GIT_INDEX_FILE environment variable in order to not
trash the main index. I ended up doing things like this:

discard_cache();
ret = read_cache_from(index_path);
write_index_as_tree(orig_tree.hash, _index, index_path, 0, NULL);
discard_cache();
ret = read_cache_from(index_path);

in order to have an empty cache. Could someone take a look at my uses
of the index and point out better ways to do it?

My main goal right now is replace as many of the cmd_* calls as
possible.

changes in v2:
* Better follow coding guidelines
* Improve error handling

 Makefile  |2 +-
 builtin.h |1 +
 builtin/stash.c   | 1266 +
 git-stash.sh  |  731 ---
 git.c |1 +
 merge-recursive.c |5 +-
 6 files changed, 1271 insertions(+), 735 deletions(-)
 create mode 100644 builtin/stash.c
 delete mode 100755 git-stash.sh

diff --git a/Makefile b/Makefile
index ba524f3a7..73915a2e0 100644
--- a/Makefile
+++ b/Makefile
@@ -516,7 +516,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
 
@@ -949,6 +948,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 9e4a89816..16e8a437d 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 0..785fc18d5
--- /dev/null
+++ b/builtin/stash.c
@@ -0,0 +1,1266 @@
+#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 []"),
+   N_("git stash show []"),
+   N_("git stash drop [-q|--quiet] []"),
+   N_("git stash ( pop | apply ) [--index] [-q|--quiet] []"),
+   N_("git stash branch  []"),
+   N_("git stash [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]"),
+   N_("[-u|--include-untracked] [-a|--all] []]"),
+   N_("git stash cl

[PATCH] [RFC] Converting git stash to a builtin

[Joel Teichroeb]

I've been working on rewriting git stash as a c builtin and I have all
but three tests passing. I'm having a bit of trouble fixing them, as
well as a few other issues, so I'd really appreciate some help. Don't
bother commenting on the small details yet as I still need to go
though the code to make sure it matches the code style guidelines.

Test Summary Report
---
t7601-merge-pull-config.sh   (Wstat: 256 Tests: 14
Failed: 2)
  Failed tests:  11-12
  Non-zero exit status: 1
t3903-stash.sh   (Wstat: 256 Tests: 74
Failed: 1)
  Failed test:  69
  Non-zero exit status: 1

It looks to be the same issue for both of these cases where
merge-recursive reports:
error: Your local changes to the following files would be overwritten by merge:
file
other-file

which doesn't make sense as those files didn't exist before the merge.
Furthermore if I take the existing git stash implementation and have
it stop before running the merge-recursive command and then run it on
the commandline manually, I get the same issue. I've tried setting all
the same environment variables that the existing git stash
implementation does, but it doesn't help. It seems like there could be
a bug in merge-recursive, but I'm not sure how to track it down.

git stash uses the GIT_INDEX_FILE environment variable in order to not
trash the main index. I ended up doing things like this:

discard_cache();
ret = read_cache_from(index_path);
write_index_as_tree(orig_tree.hash, _index, index_path, 0, NULL);
discard_cache();
ret = read_cache_from(index_path);

in order to have an empty cache. Could someone take a look at my uses
of the index and point out better ways to do it?

My main goal right now is to cleanup the code and replace more of the
commands with native code.
---
 Makefile  |2 +-
 builtin.h |1 +
 builtin/stash.c   | 1286 +
 git-stash.sh  |  731 --
 git.c |1 +
 merge-recursive.c |5 +-
 6 files changed, 1291 insertions(+), 735 deletions(-)
 create mode 100644 builtin/stash.c
 delete mode 100755 git-stash.sh

diff --git a/Makefile b/Makefile
index ed68700ac..3c673cc7e 100644
--- a/Makefile
+++ b/Makefile
@@ -516,7 +516,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
 
@@ -949,6 +948,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 9e4a89816..16e8a437d 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 0..c04d424f1
--- /dev/null
+++ b/builtin/stash.c
@@ -0,0 +1,1286 @@
+#include "builtin.h"
+#include "cache.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 []"),
+   N_("git stash show []"),
+   N_("git stash drop [-q|--quiet] []"),
+   N_("git stash ( pop | apply ) [--index] [-q|--quiet] []"),
+   N_("git stash branch  []"),
+   N_("git stash [save [--patch] [-k|--[no-]keep-index] [-q|--quiet]"),
+   N_("[-u|--include-untracked] [-a|--all] []]"),
+   N_("git stash clear"),
+   N_("git stash create []"),
+   N_("git stash store [-m|--message ] [-q|--quiet] "),
+   NULL
+};
+
+static const char * const git_stash_list_usage[] = {
+   N_("git stash list []"),
+   NULL
+};
+
+static const char * const git_stash_show_usage[] = {
+   N_("git stash show []"),
+   NULL
+};
+
+static const char * const git_stash_drop_usage[] = {

[RFC] Converting git stash to a builtin

[Joel Teichroeb]

I've been working on rewriting git stash as a c builtin and I have all
but three tests passing. I'm having a bit of trouble fixing them, as
well as a few other issues, so I'd really appreciate some help. Don't
bother commenting on the small details yet as I still need to go
though the code to make sure it matches the code style guidelines.

The commit is a bit big, so I'll just link to it on github.

https://github.com/klusark/git/commit/f74d65ae3e06d2c0ab000702ac5e756550e06454


Test Summary Report
---
t7601-merge-pull-config.sh   (Wstat: 256 Tests: 14
Failed: 2)
  Failed tests:  11-12
  Non-zero exit status: 1
t3903-stash.sh   (Wstat: 256 Tests: 74
Failed: 1)
  Failed test:  69
  Non-zero exit status: 1

It looks to be the same issue for both of these cases where
merge-recursive reports:
error: Your local changes to the following files would be overwritten by merge:
file
other-file

which doesn't make sense as those files didn't exist before the merge.
Furthermore if I take the existing git stash implementation and have
it stop before running the merge-recursive command and then run it on
the commandline manually, I get the same issue. I've tried setting all
the same environment variables that the existing git stash
implementation does, but it doesn't help. It seems like there could be
a bug in merge-recursive, but I'm not sure how to track it down.


git stash uses the GIT_INDEX_FILE environment variable in order to not
trash the main index. I ended up doing things like this:

discard_cache();
ret = read_cache_from(index_path);
write_index_as_tree(orig_tree.hash, _index, index_path, 0, NULL);
discard_cache();
ret = read_cache_from(index_path);

in order to have an empty cache. Could someone take a look at my uses
of the index and point out better ways to do it?

My main goal right now is to cleanup the code and replace more of the
commands with native code.

Thanks,
Joel