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

On Sun, May 28, 2017 at 11:26 AM, Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb [off-list ref] wrote:

quoted

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

quoted

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

quoted

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

quoted

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