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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 16, 2017, 22:47 UTC
Message-ID: <xmqqvanvv9be.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170608005535.13080-6-joel@teichroeb.net>

Joel Teichroeb <joel@teichroeb.net> writes:

Show 9 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;

The order is somewhat ugly. Move "struct stat st;" that does not have any initialization at the beginning.

Show 5 quoted lines

> +		remove_file_from_index(&the_index, path);
> +		if (!lstat(path, &st))
> +			add_to_index(&the_index, path, &st, 0);
> +	}
> +}

So this will be called with list of paths that are different from the working tree and the index, and adds all the paths the index knows about to the index from the working tree? Sounds OK, but I am not sure if that is "stash_create_callback()". Surely it is _part_ of creating a stash, but it would be better to name it to reflect which part of creating a stash this helper is about. I think this is about recording the working tree state, so I would have expected "record" and/or "working_tree" in its name.

Show 26 quoted lines

> +/*
> + * Untracked files are stored by themselves in a parentless commit, for
> + * ease of unpacking later.
> + */
> +static int save_untracked(struct stash_info *info, const char *message,
> +		int include_untracked, int include_ignored, const char **argv)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	struct strbuf out = STRBUF_INIT;
> +	struct object_id orig_tree;
> +	int ret;
> +	const char *index_file = get_index_file();
> +
> +	set_alternate_index_output(stash_index_path);
> +	untracked_files(&out, include_untracked, include_ignored, argv);
> +
> +	cp.git_cmd = 1;
> +	argv_array_pushl(&cp.args, "update-index", "-z", "--add", "--remove",
> +		"--stdin", NULL);
> +	argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", stash_index_path);
> +
> +	if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0)) {
> +		strbuf_release(&out);
> +		return 1;
> +	}
> +

OK, that's a very straight-forward way of doing this, and as we do not care too much about performance in this initial conversion to C, it is even sensible. In a later update after this patch lands, you may want to use dir.c's fill_directory() API to find the untracked files and add them yourself internally, without running ls-files (in untracked_files()) or update-index (here) as subprocesses, but that is in the future. Let's get this round finished.

Show 6 quoted lines

> +	strbuf_reset(&out);
> +
> +	discard_cache();
> +	read_cache_from(stash_index_path);
> +
> +	write_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0,NULL);

SP before "NULL".

> +	discard_cache();
> +
> +	read_cache_from(stash_index_path);

Hmph, what did anybody change in the on-disk stash_index (or contents in the_index) since you read_cache_from()?

> +	write_cache_as_tree(info->u_tree.hash, 0, NULL);

Then you write exactly the same index contents again, this time to info->u_tree here. I am not sure why you need to do this twice, and I do not see how orig_tree.hash you wrote earlier is used?

Show 14 quoted lines

> +	strbuf_addf(&out, "untracked files on %s", message);
> +
> +	ret = commit_tree(out.buf, out.len, info->u_tree.hash, NULL,
> +			info->u_commit.hash, NULL, NULL);
> +	strbuf_release(&out);
> +	if (ret)
> +		return 1;
> +
> +	set_alternate_index_output(index_file);
> +	discard_cache();
> +	read_cache();
> +
> +	return 0;
> +}

OK, except for minor nits, this seems to correctly replicate what u_commit=$(...) does in create_stash shell function in the original.

Show 16 quoted lines

> +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);
> +	prime_cache_tree(&the_index, tree);
> +	write_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0, NULL);
> +	discard_cache();

Hmph, the caller of this function did read_cache(), refresh_index(), and write_cache_as_tree(), and the result is in info->i_tree. The above sequence discards, reads that tree into the index and writes the same tree again. Which seems like a huge no-op. IIUC, the write_cache_as_tree() the caller already did should have already primed the cache-tree structure, too. These five lines are puzzling.

> +	read_cache_from(stash_index_path);

Hmph, it is unclear who wrote what state to this $TMPindex from this codeflow. Do you really want to read from there? I am guessing that this part corresponds to w_tree=$( ... ) in create_stash shell function, which does "read-tree --index-output=$TMPindex -m $i_tree" starting from the real $GIT_DIR/index and the call to unpack_tree() that follows here is that "read-tree".

A one-way "read-tree -m" is purely a performance measure and the resulting index will have the entries in $i_tree no matter what index contents you start from, so you may not have seen an incorrect result per-se, but I suspect that you do not want to be reading from $TMPindex here. Puzzled...

Show 38 quoted lines

> +
> +	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;
> +
> +	init_revisions(&rev, prefix);
> +	setup_revisions(0, NULL, &rev, NULL);
> +	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
> +	rev.diffopt.format_callback = stash_create_callback;
> +	DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
> +
> +	parse_pathspec(&rev.prune_data, 0, 0, prefix, argv);
> +
> +	diff_setup_done(&rev.diffopt);
> +	obj = parse_object(&info->b_commit);
> +	add_pending_object(&rev, obj, "");
> +	if (run_diff_index(&rev, 0))
> +		return 1;
> +
> +	if (write_cache_as_tree(info->w_tree.hash, 0, NULL))
> +		return 1;
> +
> +	discard_cache();
> +	read_cache();
> +
> +	return 0;
> +}

This part otherwise looks like a correct way to grab changes to the working tree into w_tree.

Again, I need to stop here for now.  Will continue later.