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

[Joel Teichroeb <joel@teichroeb.net>]

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

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

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