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

Re: [PATCH v4 5/5] stash: implement builtin stash

From
Junio C Hamano <gitster@pobox.com>
Date
Jun 22, 2017, 17:07 UTC
Message-ID
<xmqqk244kl3c.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<20170608005535.13080-6-joel@teichroeb.net>
Joel Teichroeb <joel@teichroeb.net> writes:
Show 12 quoted lines
> +static int patch_working_tree(struct stash_info *info, const char *prefix,
> +		const char **argv)
> +{
> +	struct argv_array args = ARGV_ARRAY_INIT;
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	struct strbuf out = STRBUF_INIT;
> +	size_t unused;
> +	const char *index_file = get_index_file();
> +
> +	argv_array_pushl(&args, "read-tree", "HEAD", NULL);
> +	argv_array_pushf(&args, "--index-output=%s", stash_index_path);
> +	cmd_read_tree(args.argc, args.argv, prefix);

I do not think if cmd_read_tree() is prepared to be called like this, and even if it happens to be OK, I do not think we should rely on it.

In general, cmd_foo() that implements subcommand "foo" expects only to be called from main(), and expects that the calling main() will exit with its return status. This has implications that you, who abuse a cmd_foo() function as if it is a reusable helper function, need to watch out for. For example, cmd_foo() may use static global variables in builtin/foo.c that are initialized in a certain way before it starts, so calling cmd_foo() twice may not work correctly (the first invocation may change these variables and they won't be reset when it returns). cmd_foo() can and do leave resources unreclaimed, because it expects the calling main() to exit immediately, leaving descriptors it creates open or chunks of memory it allocates unfreed. cmd_foo() also can and do die() without returning the control to the caller (this last item does not make much difference to this particular codepath, as you'd end up dying soon if this read-tree fails anyway, but in general you'd want to give a more specific error message when it happens, i.e. instead of an error message cmd_read_tree() internally gives, you want to say "Cannot save the current worktree state").

Show 6 quoted lines
> +
> +	cp.git_cmd = 1;
> +	argv_array_pushl(&cp.args, "add--interactive", "--patch=stash", "--", NULL);
> +	argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", stash_index_path);
> +	if (run_command(&cp))
> +		return 1;

Unlike the above direct call to cmd_read_tree(), the way this invokes "git add--interactive" is kosher. By returning non-zero (by the way, the prevailing convention in this codebase is to return negative for an error), you give a chance to the caller of this helper function to say "Cannot save the current worktree state".

Show 5 quoted lines
> +	discard_cache();
> +	read_cache_from(stash_index_path);
> +
> +	if (write_cache_as_tree(info->w_tree.hash, 0, NULL))
> +		return 1;
OK.
Show 7 quoted lines
> +	child_process_init(&cp);
> +	cp.git_cmd = 1;
> +	argv_array_pushl(&cp.args, "diff-tree", "-p", "HEAD", NULL);
> +	argv_array_push(&cp.args, sha1_to_hex(info->w_tree.hash));
> +	argv_array_push(&cp.args, "--");
> +	if (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0) || out.len == 0)
> +		return 1;

This "diff-tree" call is also reasonable. Instead of getting the patch text into a temporary file (which is what the original did), we slurp it into a strbuf "out", and pass it out to the caller by storing in info->patch. OK.

> +	info->patch = strbuf_detach(&out, &unused);
You can pass NULL instead of a throw-away variable &unused.
Show 7 quoted lines
> +
> +	set_alternate_index_output(index_file);
> +	discard_cache();
> +	read_cache();
> +
> +	return 0;
> +}

So this looks fairly faithful rewrite to C of a half of the create_stash shell function (we already reviewed the other half done in your save_working_tree() function).

Show 19 quoted lines
> +static int do_create_stash(struct stash_info *info, const char *prefix,
> +		const char *message, int include_untracked, int include_ignored,
> +		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_message = STRBUF_INIT;
> +	struct strbuf out = STRBUF_INIT;
> +	struct pretty_print_context ctx = {0};
> +
> +	struct commit *c = NULL;
> +	const char *hash;
> +
> +	read_cache_preload(NULL);
> +	refresh_index(&the_index, REFRESH_QUIET, NULL, NULL, NULL);
> +	if (check_no_changes(prefix, include_untracked, include_ignored, argv))
> +		return 1;

We find there is nothing to stash, so we tell the caller that fact by returning 1. The caller can tell that this is a "different kind of success" and is not an error by checking the sign of the return value.

> +	if (get_sha1_tree("HEAD", info->b_commit.hash))
> +		return error(_("You do not have the initial commit yet"));

And this is an error from the caller's point of view (error() returns -1).

Show 19 quoted lines
> +	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);
> +
> +	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_message, "%s: %s ", branch_name, hash);
> +
> +	pretty_print_commit(&ctx, c, &out_message);
> +
> +	strbuf_addf(&out, "index on %s\n", out_message.buf);

OK, the above roughly correspond to "# state of the base commit" part of the original. This message is created with

	msg=$(printf '%s: %s' "$branch" "$head")

and your "branch_name" is computed to be the same as $branch, and $head in the original is head=$(git rev-list --oneline -n 1 HEAD--) which is your "hash" with the oneline. The whole $msg corresponds to your "out_message.buf". Looks correct.

> +	commit_list_insert(lookup_commit(&info->b_commit), &parents);
> +
> +	if (write_cache_as_tree(info->i_tree.hash, 0, NULL))
> +		return error(_("git write-tree failed to write a tree"));

Shouldn't the user also see "Cannot save the current index state" message in this case? A failure by write-tree is an implementation detail and the latter is what matters more to the end user.

Show 18 quoted lines
> +	if (commit_tree(out.buf, out.len, info->i_tree.hash, parents, info->i_commit.hash, NULL, NULL))
> +		return error(_("Cannot save the current index state"));
> +
> +	strbuf_reset(&out);
> +
> +	if (include_untracked) {
> +		if (save_untracked(info, out_message.buf, include_untracked, include_ignored, argv))
> +			return error(_("Cannot save the untracked files"));
> +	}
> +
> +	if (patch) {
> +		if (patch_working_tree(info, prefix, argv))
> +			return error(_("Cannot save the current worktree state"));
> +	} else {
> +		if (save_working_tree(info, prefix, argv))
> +			return error(_("Cannot save the current worktree state"));
> +	}
> +	parents = NULL;

The elements on the parents list here are leaked here, by early returns we see above and also at the end of this function.

Show 21 quoted lines
> +	if (include_untracked)
> +		commit_list_insert(lookup_commit(&info->u_commit), &parents);
> +
> +	commit_list_insert(lookup_commit(&info->i_commit), &parents);
> +	commit_list_insert(lookup_commit(&info->b_commit), &parents);
> +
> +	if (message != NULL && strlen(message) != 0)
> +		strbuf_addf(&out, "On %s: %s\n", branch_name, message);
> +	else
> +		strbuf_addf(&out, "WIP on %s\n", out_message.buf);
> +
> +	if (commit_tree(out.buf, out.len, info->w_tree.hash, parents, info->w_commit.hash, NULL, NULL))
> +		return error(_("Cannot record working tree state"));
> +
> +	info->message = out.buf;
> +
> +	strbuf_release(&out_message);
> +	free(branch_path);
> +
> +	return 0;
> +}
Show 24 quoted lines
> +static int create_stash(int argc, const char **argv, const char *prefix)
> +{
> +	int include_untracked = 0;
> +	const char *message = NULL;
> +	struct stash_info info;
> +	int ret;
> +	struct strbuf out = STRBUF_INIT;
> +	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) {
> +		int i;
> +		for (i = 0; i < argc; ++i) {
> +			if (i != 0) {
> +				strbuf_addf(&out, " ");
> +			}
Style tips:
    Do not enclose a single statement inside a block.  
    We prefer "if (i)" over "if (i != 0)".
    We prefer "if (!i)" over "if (i == 0)".
    We prefer "i++" over "++i", UNLESS you use the value before increment.
Show 8 quoted lines
> +			strbuf_addf(&out, "%s", argv[i]);
> +		}
> +		message = out.buf;
> +	}
> +
> +	ret = do_create_stash(&info, prefix, message, include_untracked, 0, 0, NULL);
> +
> +	strbuf_release(&out);

I do not see a need for "message" variable; you can just pass out.buf to do_create_stash().

Show 7 quoted lines
> +
> +	if (ret)
> +		return 0;
> +
> +	printf("%s\n", sha1_to_hex(info.w_commit.hash));
> +	return 0;
> +}
Previous: Junio C HamanoNext: Joel Teichroeb
Message 25 of 26 in “Implement git stash as a builtin command”
  1. 0/5 Implement git stash as a builtin commandJoel Teichroeb, Jun 8, 2017
  2. 1/5 stash: add test for stash create with no filesJoel Teichroeb, Jun 8, 2017
  3. Junio C HamanoJun 13, 2017
  4. 2/5 stash: Add a test for when apply fails during stash branchJoel Teichroeb, Jun 8, 2017
  5. Junio C HamanoJun 13, 2017
  6. Joel TeichroebJun 13, 2017
  7. 3/5 stash: add test for stashing in a detached stateJoel Teichroeb, Jun 8, 2017
  8. Junio C HamanoJun 13, 2017
  9. Joel TeichroebJun 13, 2017
  10. Junio C HamanoJun 13, 2017
  11. 4/5 merge: close the index lock when not writing the new indexJoel Teichroeb, Jun 8, 2017
  12. Junio C HamanoJun 13, 2017
  13. 5/5 stash: implement builtin stashJoel Teichroeb, Jun 8, 2017
  14. Thomas GummererJun 11, 2017
  15. Joel TeichroebJun 20, 2017
  16. Thomas GummererJun 25, 2017
  17. Matthieu MoyJun 26, 2017
  18. Thomas GummererJun 27, 2017
  19. Junio C HamanoJun 16, 2017
  20. Junio C HamanoJun 16, 2017
  21. Johannes SchindelinJun 19, 2017
  22. Jeff KingJun 19, 2017
  23. Joel TeichroebJun 20, 2017
  24. Junio C HamanoJun 22, 2017
  25. Junio C HamanoJun 22, 2017
  26. Joel TeichroebJun 11, 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.