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

[Junio C Hamano <gitster@pobox.com>]

Joel Teichroeb <joel@teichroeb.net> writes:

> +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.

> +		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.

> +/*
> + * 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.

> +	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?

> +	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.

> +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...

> +
> +	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.