On Sun, May 28, 2017 at 11:26 AM, Ævar Arnfjörð Bjarmason
<avarab@xxxxxxxxx> wrote:
> On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb <joel@xxxxxxxxxxxxx> 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.