Re: [GSoC][PATCH v7 17/26] stash: avoid spawning a "diff-index" process
From: Thomas Gummerer <hidden>
Date: 2018-08-18 22:06:49
On 08/08, Paul-Sebastian Ungureanu wrote:
This commits replaces spawning `diff-index` child process by using the already existing `diff` API
I think this should be squashed into the previous commit. It's easier to review a commit that replaces all the 'run_command'/'pipe_command' calls in one function, rather than doing it call by call, especially if they interact with eachother. E.g. I was going to suggest replacing the 'write_tree' call as well, but reading ahead in the series I see that that's already being done :) While replacing all the calls of one type with the internal API call is probably easiest for writing the patches, at least I would find it easier to review replacing the run-command API calls in one codepath at a time.
quoted hunk ↗ jump to hunk
--- builtin/stash--helper.c | 56 ++++++++++++++++++++++++++++++----------- 1 file changed, 42 insertions(+), 14 deletions(-)diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c index 887b78d05..f905d3908 100644 --- a/builtin/stash--helper.c +++ b/builtin/stash--helper.c@@ -12,6 +12,7 @@ #include "rerere.h" #include "revision.h" #include "log-tree.h" +#include "diffcore.h" static const char * const git_stash_helper_usage[] = { N_("git stash--helper list [<options>]"),@@ -297,6 +298,18 @@ static int reset_head(const char *prefix) return run_command(&cp); } +static void add_diff_to_buf(struct diff_queue_struct *q, + struct diff_options *options, + void *data) +{ + int i; + for (i = 0; i < q->nr; i++) { + struct diff_filepair *p = q->queue[i]; + strbuf_addstr(data, p->one->path); + strbuf_addch(data, '\n');
What about filenames that include a '\n'? I think this in combination with removing the '-z' flag from the 'update-index' call will break with filenames that have a LF in them. This should be a '\0' instead of a '\n', and we should still be using the '-z' flag in 'update-index'.
quoted hunk ↗ jump to hunk
+ } +} + static int get_newly_staged(struct strbuf *out, struct object_id *c_tree) { struct child_process cp = CHILD_PROCESS_INIT;@@ -981,14 +994,16 @@ static int stash_patch(struct stash_info *info, const char **argv) return ret; } -static int stash_working_tree(struct stash_info *info, const char **argv) +static int stash_working_tree(struct stash_info *info, + const char **argv, const char *prefix) { int ret = 0; - struct child_process cp1 = CHILD_PROCESS_INIT; struct child_process cp2 = CHILD_PROCESS_INIT; struct child_process cp3 = CHILD_PROCESS_INIT; - struct strbuf out1 = STRBUF_INIT; struct strbuf out3 = STRBUF_INIT;
We're left with cp{2,3} and out3 here, which is a bit weird. Then
again renaming them in this patch adds more churn, making it harder to
review. Maybe instead of numbering them it would be better to name
them after the child process they are calling? e.g. 'cp1' could
become 'cp_di', and so on?
quoted hunk ↗ jump to hunk
+ struct argv_array args = ARGV_ARRAY_INIT; + struct strbuf diff_output = STRBUF_INIT; + struct rev_info rev; set_alternate_index_output(stash_index_path.buf); if (reset_tree(&info->i_tree, 0, 0)) {@@ -997,26 +1012,36 @@ static int stash_working_tree(struct stash_info *info, const char **argv) } set_alternate_index_output(".git/index"); - cp1.git_cmd = 1; - argv_array_pushl(&cp1.args, "diff-index", "--name-only", "-z", - "HEAD", "--", NULL); + argv_array_push(&args, "dummy");
Not being familiar with the setup_revisions code, I had to dig a bit to figure out why this makes sense. This is a dummy replacement for argv[0] in normal operation. In retrospect it's kind of obvious, but maybe call "dummy" "fake_argv0" instead, to help nudge future readers in the right direction?
quoted hunk ↗ jump to hunk
if (argv) - argv_array_pushv(&cp1.args, argv); - argv_array_pushf(&cp1.env_array, "GIT_INDEX_FILE=%s", - stash_index_path.buf); + argv_array_pushv(&args, argv); + git_config(git_diff_basic_config, NULL); + init_revisions(&rev, prefix); + args.argc = setup_revisions(args.argc, args.argv, &rev, NULL); + + rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK; + rev.diffopt.format_callback = add_diff_to_buf; + rev.diffopt.format_callback_data = &diff_output; + + if (read_cache_preload(&rev.diffopt.pathspec) < 0) { + ret = -1; + goto done; + } - if (pipe_command(&cp1, NULL, 0, &out1, 0, NULL, 0)) { + add_pending_object(&rev, parse_object(the_repository, &info->b_commit), ""); + if (run_diff_index(&rev, 0)) { ret = -1; goto done; } cp2.git_cmd = 1; - argv_array_pushl(&cp2.args, "update-index", "-z", "--add", + argv_array_pushl(&cp2.args, "update-index", "--add", "--remove", "--stdin", NULL); argv_array_pushf(&cp2.env_array, "GIT_INDEX_FILE=%s", stash_index_path.buf); - if (pipe_command(&cp2, out1.buf, out1.len, NULL, 0, NULL, 0)) { + if (pipe_command(&cp2, diff_output.buf, diff_output.len, + NULL, 0, NULL, 0)) { ret = -1; goto done; }@@ -1033,8 +1058,11 @@ static int stash_working_tree(struct stash_info *info, const char **argv) get_oid_hex(out3.buf, &info->w_tree); done: - strbuf_release(&out1); + UNLEAK(rev); strbuf_release(&out3); + argv_array_clear(&args); + object_array_clear(&rev.pending); + strbuf_release(&diff_output); remove_path(stash_index_path.buf); return ret; }@@ -1112,7 +1140,7 @@ static int do_create_stash(int argc, const char **argv, const char *prefix, goto done; } } else { - if (stash_working_tree(info, argv)) { + if (stash_working_tree(info, argv, prefix)) { printf_ln("Cannot save the current worktree state"); ret = -1; goto done;-- 2.18.0.573.g56500d98f