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.

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

[Thomas Gummerer]

On 11/10, Joel Teichroeb wrote:
> 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 
> ---

Thanks for working on this!  I like the approach of converting this
one command at a time.  I think it would be even better if this was a
series of patches, converting stash one command at a time, instead of
adding multiple multiple commands to stash helper in one patch.  This
would make reviewing the patches a bit easier.

For example this could look like:
[2/5] stash: convert apply to builtin
[3/5] stash: convert branch to builtin
[4/5] stash: convert drop to builtin
[5/5] stash: convert pop to builtin

Some comments from me below.

>  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[] = {

s/stash/&_helper/ maybe?  This is how we call it in the other helpers
as well.

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

[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 
---
 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", 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) &&
+