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

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

From
Junio C Hamano <gitster@pobox.com>
Date
May 29, 2017, 07:16 UTC
Message-ID
<xmqqinkk6skg.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<20170528165642.14699-5-joel@teichroeb.net>
Joel Teichroeb <joel@teichroeb.net> writes:
> +int untracked_files(struct strbuf *out, int include_untracked,
> +		const char **argv)
Does this need to be public?

For a caller that wants to learn if there is _any_ untracked file, having a strbuf that holds all output is overkill. For a caller that wants to learn what are the untracked paths, having a strbuf that it needs to parse is not all that helpful. Only for a caller that does an unusual thing, namely, just grab the output and throw it at another command as input, without checking what the output was itself at all, would benefit.

So the interface to this function doesn't look like a very good one if this wants to be a public helper. Perhaps mark it as "static"?

Show 8 quoted lines
> +{
> +	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");
> +	if (include_untracked != 2)
> +		argv_array_push(&cp.args, "--exclude-standard");
This magic number "2" will be hard to grok by future readers.
Show 29 quoted lines
> +	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, "--");
> +	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);
Show 6 quoted lines
> +	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);
> +}
Possible leak of out.buf here.
> +
> +static int get_stash_info(struct stash_info *info, const char *commit)
> +{

Judging from the callers of get_stash_info(), nobody passes a "commit" as parameter; as far as they are concerned, they are dealing with the name of one item in the stash (stash-id?). They do not care the fact that the item happens to be implemented as a commit object.

Perhaps rename "commit" (parameter) to "stash_id" or something.
Show 15 quoted lines
> +	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;

Variables and field names that are all caps become misleading. Avoid them.

Show 29 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;
> +
> +	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);
> +
> +
> +	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);

Hmph. What's the reason to use get_sha1_with_context() if you declare the context is unused? Wouldn't plain-vanilla get_sha1() work better?

Having to take both commit and tree of these sounds somewhat cumbersome. Do we have to know all of them and do we use all of them (not a complaint, just asking because I find it somewhat surprising)?

Show 9 quoted lines
> +
> +	if (!ret) {
> +		fprintf_ln(stderr, _("%s is not a valid reference"), REV);
> +		return 1;
> +	}
> +
> +
> +	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;
Ditto.
Show 12 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;
> +	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()
> +	};

OPT_BOOL(), unlike its now deprecated and removed predecessor OPT_BOOLEAN(), does not count up, so the value of include_untracked is lmited to either 0 or 1. So this is not the source of mysterious magic number "2" I noticed above.

I'll stop here for now.  
Thanks for working on this.
Previous: Jeff KingNext: Ævar Arnfjörð Bjarmason
Message 22 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.