| Message ID | 20200505104849.13602-4-alban.gruin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | stash: drop usage of a second index | expand |
On Tue, May 5, 2020 at 12:56 PM Alban Gruin <alban.gruin@gmail.com> wrote: > > This removes the second index used in stash_patch(). > > This function starts by resetting the index (which is set at `i_tree') > to HEAD, which has been stored in `b_commit' by do_create_stash(), and > the call to `read-tree' is replaced by reset_tree(). The index is > discarded after run_add_interactive(), but not `diff-tree' as this > command should not change it. > > Since the index has been changed, and subsequent code might be sensitive > to this, it is reset to `i_tree' at the end of the function. [...] > /* State of the working tree. */ > - if (write_index_as_tree(&info->w_tree, &istate, stash_index_path.buf, 0, > - NULL)) { > - ret = -1; > - goto done; > - } > + discard_cache(); > + if (write_cache_as_tree(&info->w_tree, 0, NULL)) > + return -1; In the previous patch you use the following: + if (write_cache_as_tree(&info->w_tree, 0, NULL) || + reset_tree(&info->i_tree, 0, 1)) Here the reset_tree(&info->i_tree, 0, 1) call is in the "done:" part of the code. Is there a reason for that? Can't reset_tree(&info->i_tree, 0, 1) be tried here before returning an error? Or was the previous patch wrong? I guess the reason why the reset_tree(&info->i_tree, 0, 1) call here is in the "done:" part of the call is because it should be after the diff tree command is launched. I see that the commit message tries to explain this a bit, but it is still not very clear to me.
On Sat, Jun 13, 2020 at 11:38 AM Christian Couder <christian.couder@gmail.com> wrote: > > On Tue, May 5, 2020 at 12:56 PM Alban Gruin <alban.gruin@gmail.com> wrote: > > > > This removes the second index used in stash_patch(). > > > > This function starts by resetting the index (which is set at `i_tree') > > to HEAD, which has been stored in `b_commit' by do_create_stash(), and > > the call to `read-tree' is replaced by reset_tree(). The index is > > discarded after run_add_interactive(), but not `diff-tree' as this > > command should not change it. > > > > Since the index has been changed, and subsequent code might be sensitive > > to this, it is reset to `i_tree' at the end of the function. > > [...] > > > /* State of the working tree. */ > > - if (write_index_as_tree(&info->w_tree, &istate, stash_index_path.buf, 0, > > - NULL)) { > > - ret = -1; > > - goto done; > > - } > > + discard_cache(); > > + if (write_cache_as_tree(&info->w_tree, 0, NULL)) > > + return -1; > > In the previous patch you use the following: > > + if (write_cache_as_tree(&info->w_tree, 0, NULL) || > + reset_tree(&info->i_tree, 0, 1)) > > Here the reset_tree(&info->i_tree, 0, 1) call is in the "done:" part > of the code. Is there a reason for that? Can't > reset_tree(&info->i_tree, 0, 1) be tried here before returning an > error? Or was the previous patch wrong? Sorry I think I misunderstood the code in the previous patch. It actually doesn't try reset_tree(&info->i_tree, 0, 1) when write_cache_as_tree(&info->w_tree, 0, NULL) fails. > I guess the reason why the > reset_tree(&info->i_tree, 0, 1) call here is in the "done:" part of > the call is because it should be after the diff tree command is > launched. I see that the commit message tries to explain this a bit, > but it is still not very clear to me.
diff --git a/builtin/stash.c b/builtin/stash.c index 2535335275..eaeb7bc8c4 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -995,51 +995,24 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps, struct strbuf *out_patch, int quiet) { int ret = 0; - struct child_process cp_read_tree = CHILD_PROCESS_INIT; struct child_process cp_diff_tree = CHILD_PROCESS_INIT; - struct index_state istate = { NULL }; - char *old_index_env = NULL, *old_repo_index_file; - - remove_path(stash_index_path.buf); - cp_read_tree.git_cmd = 1; - argv_array_pushl(&cp_read_tree.args, "read-tree", "HEAD", NULL); - argv_array_pushf(&cp_read_tree.env_array, "GIT_INDEX_FILE=%s", - stash_index_path.buf); - if (run_command(&cp_read_tree)) { - ret = -1; - goto done; - } + if (reset_tree(&info->b_commit, 0, 1)) + return -1; /* Find out what the user wants. */ - old_repo_index_file = the_repository->index_file; - the_repository->index_file = stash_index_path.buf; - old_index_env = xstrdup_or_null(getenv(INDEX_ENVIRONMENT)); - setenv(INDEX_ENVIRONMENT, the_repository->index_file, 1); - ret = run_add_interactive(NULL, "--patch=stash", ps); - the_repository->index_file = old_repo_index_file; - if (old_index_env && *old_index_env) - setenv(INDEX_ENVIRONMENT, old_index_env, 1); - else - unsetenv(INDEX_ENVIRONMENT); - FREE_AND_NULL(old_index_env); - /* State of the working tree. */ - if (write_index_as_tree(&info->w_tree, &istate, stash_index_path.buf, 0, - NULL)) { - ret = -1; - goto done; - } + discard_cache(); + if (write_cache_as_tree(&info->w_tree, 0, NULL)) + return -1; cp_diff_tree.git_cmd = 1; argv_array_pushl(&cp_diff_tree.args, "diff-tree", "-p", "-U1", "HEAD", oid_to_hex(&info->w_tree), "--", NULL); - if (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) { - ret = -1; - goto done; - } + if (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) + return -1; if (!out_patch->len) { if (!quiet) @@ -1047,9 +1020,9 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps, ret = 1; } -done: - discard_index(&istate); - remove_path(stash_index_path.buf); + if (reset_tree(&info->i_tree, 0, 1)) + return -1; + return ret; }
This removes the second index used in stash_patch(). This function starts by resetting the index (which is set at `i_tree') to HEAD, which has been stored in `b_commit' by do_create_stash(), and the call to `read-tree' is replaced by reset_tree(). The index is discarded after run_add_interactive(), but not `diff-tree' as this command should not change it. Since the index has been changed, and subsequent code might be sensitive to this, it is reset to `i_tree' at the end of the function. Signed-off-by: Alban Gruin <alban.gruin@gmail.com> --- builtin/stash.c | 47 ++++++++++------------------------------------- 1 file changed, 10 insertions(+), 37 deletions(-)