git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Christian Couder <christian.couder@gmail.com>
Date
May 28, 2017, 17:56 UTC
Message-ID
<CAP8UFD26oR2Y4hTYmZLBeKA8MTVe8ZTG3pJxfS9Xv8JUExbcKA@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:
[...]
Show 8 quoted lines
> +int untracked_files(struct strbuf *out, int include_untracked,
> +               const char **argv)
> +{
> +       struct child_process cp = CHILD_PROCESS_INIT;
> +       cp.git_cmd = 1;
> +       argv_array_push(&cp.args, "ls-files");
> +       argv_array_push(&cp.args, "-o");
> +       argv_array_push(&cp.args, "-z");
You might want to use argv_array_pushl(), for example:
       argv_array_pushl(&cp.args, "ls-files", "-o", "-z", NULL);
Show 22 quoted lines
> +       if (include_untracked != 2)
> +               argv_array_push(&cp.args, "--exclude-standard");
> +       argv_array_push(&cp.args, "--");
> +       if (argv)
> +               argv_array_pushv(&cp.args, argv);
> +       return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
> +}
> +
> +static int check_no_changes(const char *prefix, int include_untracked,
> +               const char **argv)
> +{
> +       struct argv_array args1;
> +       struct argv_array args2;
> +       struct strbuf out = STRBUF_INIT;
> +
> +       argv_array_init(&args1);
> +       argv_array_push(&args1, "diff-index");
> +       argv_array_push(&args1, "--quiet");
> +       argv_array_push(&args1, "--cached");
> +       argv_array_push(&args1, "HEAD");
> +       argv_array_push(&args1, "--ignore-submodules");
> +       argv_array_push(&args1, "--");
Here and in other places also you could use argv_array_pushl().
Show 33 quoted lines
> +       if (argv)
> +               argv_array_pushv(&args1, argv);
> +       argv_array_init(&args2);
> +       argv_array_push(&args2, "diff-files");
> +       argv_array_push(&args2, "--quiet");
> +       argv_array_push(&args2, "--ignore-submodules");
> +       argv_array_push(&args2, "--");
> +       if (argv)
> +               argv_array_pushv(&args2, argv);
> +       if (include_untracked)
> +               untracked_files(&out, include_untracked, argv);
> +       return cmd_diff_index(args1.argc, args1.argv, prefix) == 0 &&
> +                       cmd_diff_files(args2.argc, args2.argv, prefix) == 0 &&
> +                       (!include_untracked || out.len == 0);
> +}
> +
> +static int get_stash_info(struct stash_info *info, const char *commit)
> +{
> +       struct strbuf w_commit_rev = STRBUF_INIT;
> +       struct strbuf b_commit_rev = STRBUF_INIT;
> +       struct strbuf i_commit_rev = STRBUF_INIT;
> +       struct strbuf u_commit_rev = STRBUF_INIT;
> +       struct strbuf w_tree_rev = STRBUF_INIT;
> +       struct strbuf b_tree_rev = STRBUF_INIT;
> +       struct strbuf i_tree_rev = STRBUF_INIT;
> +       struct strbuf u_tree_rev = STRBUF_INIT;
> +       struct strbuf commit_buf = STRBUF_INIT;
> +       struct strbuf symbolic = STRBUF_INIT;
> +       struct strbuf out = STRBUF_INIT;
> +       struct object_context unused;
> +       char *str;
> +       int ret;
> +       const char *REV = commit;
We use lower case variable names.
Show 11 quoted lines
> +       struct child_process cp = CHILD_PROCESS_INIT;
> +       info->is_stash_ref = 0;
> +
> +       if (commit == NULL) {
> +               strbuf_addf(&commit_buf, "%s@{0}", ref_stash);
> +               REV = commit_buf.buf;
> +       } else if (strlen(commit) < 3) {
> +               strbuf_addf(&commit_buf, "%s@{%s}", ref_stash, commit);
> +               REV = commit_buf.buf;
> +       }
> +       info->REV = REV;
Also the "REV" member of struct stash_info could be lower cased.
Show 10 quoted lines
> +       strbuf_addf(&w_commit_rev, "%s", REV);
> +       strbuf_addf(&b_commit_rev, "%s^1", REV);
> +       strbuf_addf(&i_commit_rev, "%s^2", REV);
> +       strbuf_addf(&u_commit_rev, "%s^3", REV);
> +       strbuf_addf(&w_tree_rev, "%s:", REV);
> +       strbuf_addf(&b_tree_rev, "%s^1:", REV);
> +       strbuf_addf(&i_tree_rev, "%s^2:", REV);
> +       strbuf_addf(&u_tree_rev, "%s^3:", REV);
> +
> +
Spurious new line above.
Show 11 quoted lines
> +       ret = (
> +               get_sha1_with_context(w_commit_rev.buf, 0, info->w_commit.hash, &unused) == 0 &&
> +               get_sha1_with_context(b_commit_rev.buf, 0, info->b_commit.hash, &unused) == 0 &&
> +               get_sha1_with_context(i_commit_rev.buf, 0, info->i_commit.hash, &unused) == 0 &&
> +               get_sha1_with_context(w_tree_rev.buf, 0, info->w_tree.hash, &unused) == 0 &&
> +               get_sha1_with_context(b_tree_rev.buf, 0, info->b_tree.hash, &unused) == 0 &&
> +               get_sha1_with_context(i_tree_rev.buf, 0, info->i_tree.hash, &unused) == 0);
> +
> +       if (!ret) {
> +               fprintf_ln(stderr, _("%s is not a valid reference"), REV);
> +               return 1;
Maybe use "return error(_("%s is not a valid reference"), REV);"
> +       }
> +
> +
Spurious new lines above.
Show 14 quoted lines
> +
> +static void stash_create_callback(struct diff_queue_struct *q,
> +                               struct diff_options *opt, void *cbdata)
> +{
> +       int i;
> +
> +       for (i = 0; i < q->nr; i++) {
> +               struct diff_filepair *p = q->queue[i];
> +               const char *path = p->one->path;
> +               struct stat st;
> +               remove_file_from_index(&the_index, path);
> +               if (!lstat(path, &st))
> +                       add_to_index(&the_index, path, &st, 0);
> +
Spurious new line above.
Show 6 quoted lines
> +       }
> +}
> +
> +/* Untracked files are stored by themselves in a parentless commit, for
> + * ease of unpacking later.
> + */
This comment should rather be like:
/*
 * Untracked files are stored by themselves in a parentless commit, for
 * ease of unpacking later.
 */
Show 6 quoted lines
> +static int save_untracked(struct stash_info *info, struct strbuf *out,
> +               int include_untracked, const char **argv)
> +{
> +       struct child_process cp2 = CHILD_PROCESS_INIT;
> +       struct strbuf out3 = STRBUF_INIT;
> +       struct strbuf out4 = STRBUF_INIT;
Please try to find more meaningful names for such variables.
Show 31 quoted lines
> +       struct object_id orig_tree;
> +
> +       set_alternate_index_output(stash_index_path);
> +       untracked_files(&out4, include_untracked, argv);
> +
> +       cp2.git_cmd = 1;
> +       argv_array_push(&cp2.args, "update-index");
> +       argv_array_push(&cp2.args, "-z");
> +       argv_array_push(&cp2.args, "--add");
> +       argv_array_push(&cp2.args, "--remove");
> +       argv_array_push(&cp2.args, "--stdin");
> +       argv_array_pushf(&cp2.env_array, "GIT_INDEX_FILE=%s", stash_index_path);
> +
> +       if (pipe_command(&cp2, out4.buf, out4.len, NULL, 0, NULL, 0))
> +               return 1;
> +
> +       discard_cache();
> +       read_cache_from(stash_index_path);
> +
> +       write_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0, NULL);
> +       discard_cache();
> +
> +       read_cache_from(stash_index_path);
> +
> +       write_cache_as_tree(info->u_tree.hash, 0, NULL);
> +       strbuf_addf(&out3, "untracked files on %s", out->buf);
> +
> +       if (commit_tree(out3.buf, out3.len, info->u_tree.hash, NULL, info->u_commit.hash, NULL, NULL))
> +               return 1;
> +
> +       set_alternate_index_output(".git/index");

Isn't there a variable, a function or a constant that could be used instead of the hard coded ".git/index"?

Show 39 quoted lines
> +       discard_cache();
> +       read_cache();
> +
> +       return 0;
> +}
> +
> +static int save_working_tree(struct stash_info *info, const char *prefix,
> +               const char **argv)
> +{
> +       struct object_id orig_tree;
> +       struct rev_info rev;
> +       int nr_trees = 1;
> +       struct tree_desc t[MAX_UNPACK_TREES];
> +       struct tree *tree;
> +       struct unpack_trees_options opts;
> +       struct object *obj;
> +
> +       discard_cache();
> +       tree = parse_tree_indirect(info->i_tree.hash);
> +       prime_cache_tree(&the_index, tree);
> +       write_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0, NULL);
> +       discard_cache();
> +
> +       read_cache_from(stash_index_path);
> +
> +       memset(&opts, 0, sizeof(opts));
> +
> +       parse_tree(tree);
> +
> +       opts.head_idx = 1;
> +       opts.src_index = &the_index;
> +       opts.dst_index = &the_index;
> +       opts.merge = 1;
> +       opts.fn = oneway_merge;
> +
> +       init_tree_desc(t, tree->buffer, tree->size);
> +
> +       if (unpack_trees(nr_trees, t, &opts))
> +               return 1;
In general I think we tend to return -1 instead of 1 in case of errors in C.
[...]
Show 58 quoted lines
> +static int do_create_stash(struct stash_info *info, const char *prefix,
> +       const char *message, int include_untracked, int patch, const char **argv)
> +{
> +       struct object_id curr_head;
> +       char *branch_path = NULL;
> +       const char *branch_name = NULL;
> +       struct commit_list *parents = NULL;
> +       struct strbuf out = STRBUF_INIT;
> +       struct strbuf out3 = STRBUF_INIT;
> +       struct pretty_print_context ctx = {0};
> +
> +       struct commit *c = NULL;
> +       const char *hash;
> +       struct strbuf out2 = STRBUF_INIT;
> +
> +       read_cache_preload(NULL);
> +       refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL);
> +       if (check_no_changes(prefix, include_untracked, argv))
> +               return 1;
> +
> +       if (get_sha1_tree("HEAD", info->b_commit.hash))
> +               die(_("You do not have the initial commit yet"));
> +
> +       branch_path = resolve_refdup("HEAD", 0, curr_head.hash, NULL);
> +
> +       if (branch_path == NULL || strcmp(branch_path, "HEAD") == 0)
> +               branch_name = "(no branch)";
> +       else
> +               skip_prefix(branch_path, "refs/heads/", &branch_name);
> +
> +       c = lookup_commit(info->b_commit.hash);
> +
> +       ctx.output_encoding = get_log_output_encoding();
> +       ctx.abbrev = 1;
> +       ctx.fmt = CMIT_FMT_ONELINE;
> +       hash = find_unique_abbrev(c->object.oid.hash, DEFAULT_ABBREV);
> +
> +       strbuf_addf(&out, "%s: %s ", branch_name, hash);
> +
> +       pretty_print_commit(&ctx, c, &out);
> +
> +       strbuf_addf(&out3, "index on %s\n", out.buf);
> +
> +       commit_list_insert(lookup_commit(info->b_commit.hash), &parents);
> +
> +       if (write_cache_as_tree(info->i_tree.hash, 0, NULL))
> +               die(_("git write-tree failed to write a tree"));
> +
> +       if (commit_tree(out3.buf, out3.len, info->i_tree.hash, parents, info->i_commit.hash, NULL, NULL))
> +               die(_("Cannot save the current index state"));
> +
> +
> +       if (include_untracked) {
> +               if (save_untracked(info, &out, include_untracked, argv))
> +                       die(_("Cannot save the untracked files"));
> +       }
> +
> +
Spurious new line above.
Show 12 quoted lines
> +       if (patch) {
> +               if (patch_working_tree(info, prefix, argv))
> +                       die(_("Cannot save the current worktree state"));
> +       } else {
> +               if (save_working_tree(info, prefix, argv))
> +                       die(_("Cannot save the current worktree state"));
> +       }
> +       parents = NULL;
> +
> +       if (include_untracked) {
> +               commit_list_insert(lookup_commit(info->u_commit.hash), &parents);
> +       }
Braces can be removed above.
Show 41 quoted lines
> +       commit_list_insert(lookup_commit(info->i_commit.hash), &parents);
> +       commit_list_insert(lookup_commit(info->b_commit.hash), &parents);
> +
> +       if (message != NULL && strlen(message) != 0)
> +               strbuf_addf(&out2, "On %s: %s\n", branch_name, message);
> +       else
> +               strbuf_addf(&out2, "WIP on %s\n", out.buf);
> +
> +       if (commit_tree(out2.buf, out2.len, info->w_tree.hash, parents, info->w_commit.hash, NULL, NULL))
> +               die(_("Cannot record working tree state"));
> +
> +       info->message = out2.buf;
> +
> +       free(branch_path);
> +
> +       return 0;
> +}
> +
> +static int create_stash(int argc, const char **argv, const char *prefix)
> +{
> +       int include_untracked = 0;
> +       const char *message = NULL;
> +       struct stash_info info;
> +       struct option options[] = {
> +               OPT_BOOL('u', "include-untracked", &include_untracked,
> +                        N_("stash untracked filed")),
> +               OPT_STRING('m', "message", &message, N_("message"),
> +                        N_("stash commit message")),
> +               OPT_END()
> +       };
> +
> +       argc = parse_options(argc, argv, prefix, options,
> +                                git_stash_create_usage, 0);
> +
> +       if (argc != 0) {
> +               struct strbuf out = STRBUF_INIT;
> +               int i;
> +               for (i = 0; i < argc; ++i) {
> +                       if (i != 0) {
> +                               strbuf_addf(&out, " ");
> +                       }
Braces can be removed.
[...]
Show 16 quoted lines
> +static int store_stash(int argc, const char **argv, const char *prefix)
> +{
> +       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);
> +
> +       if (message == NULL)
> +               message = "Create via \"git stash store\".";
Maybe you could have initialized "message" when you declared the variable above.
Show 9 quoted lines
> +       if (argc != 1)
> +               die(_("\"git stash store\" requires one <commit> argument"));
> +
> +       commit = argv[0];
> +
> +       if (get_sha1(commit, obj.hash)) {
> +               fprintf_ln(stderr, _("fatal: %s: not a valid SHA1"), commit);
> +               fprintf_ln(stderr, _("cannot update %s with %s"), ref_stash, commit);
> +               return 1;
Maybe use error() or warning().
> +       }
[...]
Show 9 quoted lines
> +static int do_push_stash(const char *prefix, const char *message,
> +               int keep_index, int include_untracked, int patch, const char **argv)
> +{
> +       int result;
> +       struct stash_info info;
> +
> +       if (patch && include_untracked) {
> +               fprintf_ln(stderr, _("can't use --patch and --include-untracked or --all at the same time"));
> +               return 1;
error()?
Show 30 quoted lines
> +       }
> +
> +       if (!include_untracked) {
> +               struct child_process cp = CHILD_PROCESS_INIT;
> +               struct strbuf out = STRBUF_INIT;
> +
> +               cp.git_cmd = 1;
> +               argv_array_push(&cp.args, "ls-files");
> +               argv_array_push(&cp.args, "--error-unmatch");
> +               argv_array_push(&cp.args, "--");
> +               if (argv)
> +                       argv_array_pushv(&cp.args, argv);
> +               result = pipe_command(&cp, NULL, 0, &out, 0, NULL, 0);
> +               if (result)
> +                       return 1;
> +       }
> +
> +       read_cache_preload(NULL);
> +       refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL);
> +       if (check_no_changes(prefix, include_untracked, argv)) {
> +               printf(_("No local changes to save\n"));
> +               return 0;
> +       }
> +
> +       if (!reflog_exists(ref_stash)) {
> +               if (do_clear_stash())
> +                       die(_("Cannot initialize stash"));
> +       }
> +
> +
Spurious new line.
Show 6 quoted lines
> +       do_create_stash(&info, prefix, message, include_untracked, patch, argv);
> +       result = do_store_stash(prefix, 1, info.message, info.w_commit);
> +
> +       if (result == 0 && !quiet) {
> +               printf(_("Saved working directory and index state %s"), info.message);
> +       }
Braces can be removed.
I stopped skimming here.

Thanks, Christian.

Previous: Joel TeichroebNext: Ævar Arnfjörð Bjarmason
Message 12 of 25 in “Implement git stash as a builtin command”
  1. 0/4 Implement git stash as a builtin commandJoel Teichroeb, May 28, 2017
  2. 1/4 stash: add test for stash create with no filesJoel Teichroeb, May 28, 2017
  3. Ævar Arnfjörð BjarmasonMay 28, 2017
  4. Junio C HamanoMay 29, 2017
  5. 2/4 stash: add test for stashing in a detached stateJoel Teichroeb, May 28, 2017
  6. Ævar Arnfjörð BjarmasonMay 28, 2017
  7. Junio C HamanoMay 29, 2017
  8. 3/4 close the index lock when not writing the new indexJoel Teichroeb, May 28, 2017
  9. Ævar Arnfjörð BjarmasonMay 28, 2017
  10. Junio C HamanoMay 29, 2017
  11. 4/4 stash: implement builtin stashJoel Teichroeb, May 28, 2017
  12. Christian CouderMay 28, 2017
  13. Ævar Arnfjörð BjarmasonMay 28, 2017
  14. Joel TeichroebMay 28, 2017
  15. Jeff KingMay 28, 2017
  16. Ævar Arnfjörð BjarmasonMay 28, 2017
  17. Jeff KingMay 28, 2017
  18. Joel TeichroebMay 29, 2017
  19. Ævar Arnfjörð BjarmasonMay 29, 2017
  20. Joel TeichroebJun 1, 2017
  21. Jeff KingJun 1, 2017
  22. Junio C HamanoMay 29, 2017
  23. Ævar Arnfjörð BjarmasonMay 28, 2017
  24. Johannes SchindelinOct 23, 2017
  25. Joel TeichroebOct 23, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.