On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb <joel@xxxxxxxxxxxxx> wrote:
[...]
> +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);
> + 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().
> + 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.
> + 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.
> + 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.
> + 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.
> +
> +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.
> + }
> +}
> +
> +/* 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.
*/
> +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.
> + 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"?
> + 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.
[...]
> +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.
> + 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.
> + 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.
[...]
> +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.
> + 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().
> + }
[...]
> +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()?
> + }
> +
> + 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.
> + 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.