Re: [PATCH v3 4/4] stash: implement builtin stash

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 28, 2017, 18:26 UTC
Message-ID: <CACBZZX5cxkV4C42OwJ077b1XMHCk+uTCbdHV9Ez5EVStR4DWxg@mail.gmail.com>
In-Reply-To: <20170528165642.14699-5-joel@teichroeb.net>

On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb <joel@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

Show 7 quoted lines

> +       info->has_u = get_sha1_with_context(u_commit_rev.buf, 0, info->u_commit.hash, &unused) == 0 &&
> +               get_sha1_with_context(u_tree_rev.buf, 0, info->u_tree.hash, &unused) == 0;
> +
> +
> +       /* TODO: Improve this logic */
> +       strbuf_addf(&symbolic, "%s", REV);
> +       str = strstr(symbolic.buf, "@");

Could you elaborate on how this should be improved?

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

Show 11 quoted lines

> +       const char *message = NULL;
> +       const char *commit = NULL;
> +       struct object_id obj;
> +       struct option options[] = {
> +               OPT_STRING('m', "message", &message, N_("message"),
> +                        N_("stash commit message")),
> +               OPT__QUIET(&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).