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

[Christian Couder <christian.couder@gmail.com>]

On Thu, Apr 5, 2018 at 4:28 AM, Joel Teichroeb <joel@teichroeb.net> wrote:
> 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 <joel@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 0000000000..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] [<stash>]"),
> +       NULL
> +};
> +
> +static const char * const git_stash_helper_apply_usage[] = {
> +       N_("git stash--helper apply [--index] [-q|--quiet] [<stash>]"),
> +       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(&buf, fmt, rev);
> +       ret = get_oid(buf.buf, oid);
> +       strbuf_release(&buf);
> +       return ret;
> +}
> +
> +static void free_stash_info(struct stash_info *info)
> +{
> +       strbuf_release(&info->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(&refs_msg, " '%s'", argv[i]);
> +
> +               fprintf_ln(stderr, _("Too many revisions specified:%s"), refs_msg.buf);
> +               strbuf_release(&refs_msg);
> +
> +               return -1;
> +       }
> +
> +       if (argc == 1)
> +               commit = argv[0];
> +
> +       strbuf_init(&info->revision, 0);
> +       if (commit == NULL) {
> +               if (!ref_exists(ref_stash)) {
> +                       free_stash_info(info);
> +                       return error(_("No stash entries found."));

Here `info` is freed first and then `return error(...)` is used.

> +               }
> +
> +               strbuf_addf(&info->revision, "%s@{0}", ref_stash);
> +       } else if (strspn(commit, "0123456789") == strlen(commit)) {
> +               strbuf_addf(&info->revision, "%s@{%s}", ref_stash, commit);
> +       } else {
> +               strbuf_addstr(&info->revision, commit);
> +       }
> +
> +       revision = info->revision.buf;
> +
> +       if (get_oid(revision, &info->w_commit)) {
> +               error(_("%s is not a valid reference"), revision);
> +               free_stash_info(info);
> +               return -1;

Maybe:

               free_stash_info(info);
               return error(_("%s is not a valid reference"), revision);

to save one line and be more consistent with above.

> +       }
> +
> +       if (grab_oid(&info->b_commit, "%s^1", revision) ||
> +               grab_oid(&info->w_tree, "%s:", revision) ||
> +               grab_oid(&info->b_tree, "%s^1:", revision) ||
> +               grab_oid(&info->i_tree, "%s^2:", revision)) {
> +
> +               error(_("'%s' is not a stash-like commit"), revision);
> +               free_stash_info(info);
> +               return -1;

Here also.

> +       }
> +
> +       info->has_u = !grab_oid(&info->u_tree, "%s^3:", revision);
> +
> +       end_of_rev = strchrnul(revision, '@');
> +       strbuf_add(&symbolic, revision, end_of_rev - revision);
> +
> +       ret = dwim_ref(symbolic.buf, symbolic.len, &discard, &expanded_ref);
> +       strbuf_release(&symbolic);
> +       switch (ret) {
> +       case 0: /* Not found, but valid ref */
> +               info->is_stash_ref = 0;
> +               break;
> +       case 1:
> +               info->is_stash_ref = !strcmp(expanded_ref, ref_stash);
> +               break;
> +       default: /* Invalid or ambiguous */
> +               free_stash_info(info);
> +               return -1;
> +       }
> +
> +       return 0;
> +}
> +
> +static int reset_tree(struct object_id *i_tree, int update, int reset)
> +{
> +       struct unpack_trees_options opts;
> +       int nr_trees = 1;
> +       struct tree_desc t[MAX_UNPACK_TREES];
> +       struct tree *tree;
> +       struct lock_file lock_file = LOCK_INIT;
> +
> +       read_cache_preload(NULL);
> +       if (refresh_cache(REFRESH_QUIET))
> +               return -1;
> +
> +       hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
> +
> +       memset(&opts, 0, sizeof(opts));
> +
> +       tree = parse_tree_indirect(i_tree);
> +       if (parse_tree(tree))
> +               return -1;
> +
> +       init_tree_desc(t, tree->buffer, tree->size);
> +
> +       opts.head_idx = 1;
> +       opts.src_index = &the_index;
> +       opts.dst_index = &the_index;
> +       opts.merge = 1;
> +       opts.reset = reset;
> +       opts.update = update;
> +       opts.fn = oneway_merge;
> +
> +       if (unpack_trees(nr_trees, t, &opts))
> +               return -1;
> +
> +       if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
> +               return error(_("unable to write new index file"));
> +
> +       return 0;
> +}
> +
> +static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)
> +{
> +       struct child_process cp = CHILD_PROCESS_INIT;
> +       const char *w_commit_hex = oid_to_hex(w_commit);
> +
> +       /* Diff-tree would not be very hard to replace with a native function,
> +        * however it should be done together with apply_cached.
> +        */

Our multi line comments are more like:

       /*
        * Diff-tree would not be very hard to replace with a native function,
        * however it should be done together with apply_cached.
        */

> +       cp.git_cmd = 1;
> +       argv_array_pushl(&cp.args, "diff-tree", "--binary", NULL);
> +       argv_array_pushf(&cp.args, "%s^2^..%s^2", w_commit_hex, w_commit_hex);
> +
> +       return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
> +}
> +
> +static int apply_cached(struct strbuf *out)
> +{
> +       struct child_process cp = CHILD_PROCESS_INIT;
> +
> +       /* Apply currently only reads either from stdin or a file, thus
> +        * apply_all_patches would have to be updated to optionally take a buffer
> +        */

Here also and below the multi line comment style is not the recommended one.

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

[...]

> +static int restore_untracked(struct object_id *u_tree)
> +{
> +       struct child_process cp = CHILD_PROCESS_INIT;
> +       int res;
> +
> +       /*
> +        * We need to run restore files from a given index, but without affecting
> +        * the current index, so we use GIT_INDEX_FILE with run_command to fork
> +        * processes that will not interfere.
> +        */

Here it is the recommended style.

> +       cp.git_cmd = 1;
> +       argv_array_push(&cp.args, "read-tree");
> +       argv_array_push(&cp.args, oid_to_hex(u_tree));
> +       argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", stash_index_path.buf);
> +       if (run_command(&cp)) {
> +               remove_path(stash_index_path.buf);
> +               return -1;
> +       }
> +
> +       child_process_init(&cp);
> +       cp.git_cmd = 1;
> +       argv_array_pushl(&cp.args, "checkout-index", "--all", NULL);
> +       argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", stash_index_path.buf);
> +
> +       res = run_command(&cp);
> +       remove_path(stash_index_path.buf);
> +       return res;
> +}
> +
> +static int do_apply_stash(const char *prefix, struct stash_info *info, int index)
> +{
> +       struct merge_options o;
> +       struct object_id c_tree;
> +       struct object_id index_tree;
> +       const struct object_id *bases[1];
> +       int bases_count = 1;
> +       struct commit *result;
> +       int ret;
> +       int has_index = index;
> +
> +       read_cache_preload(NULL);
> +       if (refresh_cache(REFRESH_QUIET))
> +               return -1;
> +
> +       if (write_cache_as_tree(&c_tree, 0, NULL) || reset_tree(&c_tree, 0, 0))
> +               return error(_("Cannot apply a stash in the middle of a merge"));
> +
> +       if (index) {
> +               if (!oidcmp(&info->b_tree, &info->i_tree) || !oidcmp(&c_tree, &info->i_tree)) {
> +                       has_index = 0;
> +               } else {
> +                       struct strbuf out = STRBUF_INIT;
> +
> +                       if (diff_tree_binary(&out, &info->w_commit)) {
> +                               strbuf_release(&out);
> +                               return -1;
> +                       }
> +
> +                       ret = apply_cached(&out);
> +                       strbuf_release(&out);
> +                       if (ret)
> +                               return -1;
> +
> +                       discard_cache();
> +                       read_cache();
> +                       if (write_cache_as_tree(&index_tree, 0, NULL))
> +                               return -1;
> +
> +                       reset_head(prefix);
> +               }
> +       }
> +
> +       if (info->has_u) {
> +               if (restore_untracked(&info->u_tree))
> +                       return error(_("Could not restore untracked files from stash"));
> +       }

Maybe:

       if (info->has_u && restore_untracked(&info->u_tree))
               return error(_("Could not restore untracked files from stash"));

> +       init_merge_options(&o);
> +
> +       o.branch1 = "Updated upstream";
> +       o.branch2 = "Stashed changes";
> +
> +       if (!oidcmp(&info->b_tree, &c_tree))
> +               o.branch1 = "Version stash was based on";
> +
> +       if (quiet)
> +               o.verbosity = 0;
> +
> +       if (o.verbosity >= 3)
> +               printf_ln(_("Merging %s with %s"), o.branch1, o.branch2);
> +
> +       bases[0] = &info->b_tree;
> +
> +       ret = merge_recursive_generic(&o, &c_tree, &info->w_tree, bases_count, bases, &result);
> +       if (ret != 0) {
> +               rerere(0);
> +
> +               if (index)
> +                       fprintf_ln(stderr, _("Index was not unstashed."));
> +
> +               return ret;
> +       }
> +
> +       if (has_index) {
> +               if (reset_tree(&index_tree, 0, 0))
> +                       return -1;
> +       } else {
> +               struct strbuf out = STRBUF_INIT;
> +
> +               if (diff_cached_index(&out, &c_tree)) {
> +                       strbuf_release(&out);
> +                       return -1;
> +               }
> +
> +               if (reset_tree(&c_tree, 0, 1)) {
> +                       strbuf_release(&out);
> +                       return -1;
> +               }
> +
> +               ret = update_index(&out);
> +               strbuf_release(&out);
> +               if (ret)
> +                       return -1;
> +
> +               discard_cache();
> +       }
> +
> +       if (!quiet) {
> +               struct argv_array args = ARGV_ARRAY_INIT;
> +               /* Status is quite simple and could be replaced with calls to wt_status
> +                * in the future, but it adds complexities which may require more tests
> +                */

Multi line comment style.

> +               argv_array_push(&args, "status");
> +               cmd_status(args.argc, args.argv, prefix);
> +       }
> +
> +       return 0;
> +}

[...]

> +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();
> +       strbuf_addf(&stash_index_path, "%s.stash.%" PRIuMAX, index_file, (uintmax_t)pid);
> +
> +       if (argc < 1)
> +               usage_with_options(git_stash_helper_usage, options);

usage_with_options() does an `exit(129)` ...

> +       else if (!strcmp(argv[0], "apply"))

... so we can get rid of the `else` above...

> +               result = apply_stash(argc, argv, prefix);
> +       else {
> +               error(_("unknown subcommand: %s"), argv[0]);
> +               usage_with_options(git_stash_helper_usage, options);
> +               result = 1;

... and here we don't need to set `result`.

> +       }
> +
> +       return result;
> +}

So maybe we can get rid of `result` and have something like:

       if (argc < 1) {
               error(_("at least one argument is required"));
               usage_with_options(git_stash_helper_usage, options);
       }

       if (!strcmp(argv[0], "apply"))
               return apply_stash(argc, argv, prefix);

       error(_("unknown subcommand: %s"), argv[0]);
       usage_with_options(git_stash_helper_usage, options);
}