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

[Joel Teichroeb]

On Thu, Mar 29, 2018 at 1:07 PM, Junio C Hamano  wrote:
> Joel Teichroeb  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)
>> +{
>> + int result = 0;
>> + pid_t pid = getpid();
>> + const char *index_file;
>> +
>> + struct option options[] = {
>> + OPT_END()
>> + };
>> +
>> + git_config(git_default_config, NULL);
>> +
>> + argc = parse_options(argc, argv, prefix, options, 
>> git_stash_helper_usage,
>> + PARSE_OPT_KEEP_UNKNOWN|PARSE_OPT_KEEP_DASHDASH);
>> +
>> + index_file = get_index_file();
>> + xsnprintf(stash_index_path, PATH_MAX, "%s.stash.%"PRIuMAX, index_file, 
>> (uintmax_t)pid);
>
> Wouldn't it make more sense to get rid of PATH_MAX and hold it in a
> strbuf instead?  I.e.
>
> static struct strbuf stash_index_path = STRBUF_INIT;
> ...
> strbuf_addf(_index_path, "%s.stash.%" PRIuMAX, index_file, 
> (uintmax_t)pid);
>

That makes it a lot cleaner, 

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

[Junio C Hamano]

Joel Teichroeb  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);

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

> +int cmd_stash__helper(int argc, const char **argv, const char *prefix)
> +{
> + int result = 0;
> + pid_t pid = getpid();
> + const char *index_file;
> +
> + struct option options[] = {
> + OPT_END()
> + };
> +
> + git_config(git_default_config, NULL);
> +
> + argc = parse_options(argc, argv, prefix, options, 
> git_stash_helper_usage,
> + PARSE_OPT_KEEP_UNKNOWN|PARSE_OPT_KEEP_DASHDASH);
> +
> + index_file = get_index_file();
> + xsnprintf(stash_index_path, PATH_MAX, "%s.stash.%"PRIuMAX, index_file, 
> (uintmax_t)pid);

Wouldn't it make more sense to get rid of PATH_MAX and hold it in a
strbuf instead?  I.e.

static struct strbuf stash_index_path = STRBUF_INIT;
...
strbuf_addf(_index_path, "%s.stash.%" PRIuMAX, index_file, 
(uintmax_t)pid);

> + cd "$START_DIR"
> + git stash--helper apply "$@"
> + res=$?
> + cd_to_toplevel
> + return $res
>  }

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