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

From: Joel Teichroeb <joel@teichroeb.net>
Date: May 28, 2017, 18:31 UTC
Message-ID: <CA+CzEk8Xx4nh=Bh533DR=Xo_BjQsxAoYNbx3=O=rtWr8+v4xsQ@mail.gmail.com>
In-Reply-To: <CACBZZX5cxkV4C42OwJ077b1XMHCk+uTCbdHV9Ez5EVStR4DWxg@mail.gmail.com>

On Sun, May 28, 2017 at 11:26 AM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 16 quoted lines

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

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.

Show 11 quoted lines

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

Show 15 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).

I'll fix my tab width. Forgot that long lines would change, haha.