| Message ID | 20200505104849.13602-3-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_working_tree() to simplify > the code. It also help to avoid issues with the split-index: when s/help/helps/ > stash_working_tree() is called, the index is at `i_tree', and this tree > is extracted in a second index for use in a subcommand. This is not a > problem in the non-split-index case, but in the split-index case, if the > shared index file has expired and is removed by a subcommand, the main > index contains a reference to a file that no longer exists. As this is fixing a bug and there is no test, it might help if you can at least give an example of something that used to fail before this patch and doesn't after it. You are talking about stash subcommands but it is not very clear which one for example can trigger the bug. > The calls to set_alternative_index_output() are dropped to extract > `i_tree' to the main index, and `GIT_INDEX_FILE' is no longer set before > starting `update-index'. When it exits, the index has changed, and must > be discarded. That makes sense. > The call to reset_tree() becomes useless: Your patch doesn't remove any call to reset_tree(), but actually adds one. So the above is difficult to understand. Do you want to say that in a later patch it will be possible to remove the call to reset_tree()? Or do you want to say that the call to write_index_as_tree() becomes useless? > the only caller of > stash_working_tree() is do_create_stash(), which creates `i_tree' from > its index, calls save_untracked_files() if requested (but as it also > works on a second index, it is unaffected), then calls > stash_working_tree(). But when save_untracked_files() will be modified > to stop using another index, it won't reset the tree, because > stash_patch() wants to work on a different tree (`b_tree') than > stash_working_tree(). > > At the end of the function, the tree is reset to `i_tree'.
Hi Christian, Le 13/06/2020 à 10:52, Christian Couder a écrit : > On Tue, May 5, 2020 at 12:56 PM Alban Gruin <alban.gruin@gmail.com> wrote: >> >> This removes the second index used in stash_working_tree() to simplify >> the code. It also help to avoid issues with the split-index: when > > s/help/helps/ > >> stash_working_tree() is called, the index is at `i_tree', and this tree >> is extracted in a second index for use in a subcommand. This is not a >> problem in the non-split-index case, but in the split-index case, if the >> shared index file has expired and is removed by a subcommand, the main >> index contains a reference to a file that no longer exists. > > As this is fixing a bug and there is no test, it might help if you can > at least give an example of something that used to fail before this > patch and doesn't after it. You are talking about stash subcommands > but it is not very clear which one for example can trigger the bug. > >> The calls to set_alternative_index_output() are dropped to extract >> `i_tree' to the main index, and `GIT_INDEX_FILE' is no longer set before >> starting `update-index'. When it exits, the index has changed, and must >> be discarded. > > That makes sense. > >> The call to reset_tree() becomes useless: > > Your patch doesn't remove any call to reset_tree(), but actually adds > one. So the above is difficult to understand. > > Do you want to say that in a later patch it will be possible to remove > the call to reset_tree()? Or do you want to say that the call to > write_index_as_tree() becomes useless? > No, I meant that with this commit, reset_tree() does not need to be called at the beginning of stash_working_tree(), because it is only called by do_create_stash(), which sets the index at `i_tree', and save_untracked_files() does not change the main index. But it will become useful again down the line, when save_untracked_file() will be rewritten to use the "main" index, so I did not remove it. I hope it makes more sense now. >> the only caller of >> stash_working_tree() is do_create_stash(), which creates `i_tree' from >> its index, calls save_untracked_files() if requested (but as it also >> works on a second index, it is unaffected), then calls >> stash_working_tree(). But when save_untracked_files() will be modified >> to stop using another index, it won't reset the tree, because >> stash_patch() wants to work on a different tree (`b_tree') than >> stash_working_tree(). >> >> At the end of the function, the tree is reset to `i_tree'. Alban
On Sat, Jun 13, 2020 at 8:00 PM Alban Gruin <alban.gruin@gmail.com> wrote: > > Le 13/06/2020 à 10:52, Christian Couder a écrit : > > On Tue, May 5, 2020 at 12:56 PM Alban Gruin <alban.gruin@gmail.com> wrote: > >> The call to reset_tree() becomes useless: > > > > Your patch doesn't remove any call to reset_tree(), but actually adds > > one. So the above is difficult to understand. > > > > Do you want to say that in a later patch it will be possible to remove > > the call to reset_tree()? Or do you want to say that the call to > > write_index_as_tree() becomes useless? > > > > No, I meant that with this commit, reset_tree() does not need to be > called at the beginning of stash_working_tree(), because it is only > called by do_create_stash(), which sets the index at `i_tree', and > save_untracked_files() does not change the main index. But it will > become useful again down the line, when save_untracked_file() will be > rewritten to use the "main" index, so I did not remove it. > > I hope it makes more sense now. Yeah, it makes more sense with an explanation like the above. Instead of "down the line" though, you might want to say something like "in a later commit", as I think it will make it clearer for reviewers that the commit when it will become useful again should be part of the same series. Thanks, Christian.
diff --git a/builtin/stash.c b/builtin/stash.c index 9baa8b379e..2535335275 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -1059,17 +1059,14 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps struct rev_info rev; struct child_process cp_upd_index = CHILD_PROCESS_INIT; struct strbuf diff_output = STRBUF_INIT; - struct index_state istate = { NULL }; init_revisions(&rev, NULL); copy_pathspec(&rev.prune_data, ps); - set_alternate_index_output(stash_index_path.buf); if (reset_tree(&info->i_tree, 0, 0)) { ret = -1; goto done; } - set_alternate_index_output(NULL); rev.diffopt.output_format = DIFF_FORMAT_CALLBACK; rev.diffopt.format_callback = add_diff_to_buf; @@ -1091,8 +1088,6 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps argv_array_pushl(&cp_upd_index.args, "update-index", "--ignore-skip-worktree-entries", "-z", "--add", "--remove", "--stdin", NULL); - argv_array_pushf(&cp_upd_index.env_array, "GIT_INDEX_FILE=%s", - stash_index_path.buf); if (pipe_command(&cp_upd_index, diff_output.buf, diff_output.len, NULL, 0, NULL, 0)) { @@ -1100,19 +1095,16 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps goto done; } - if (write_index_as_tree(&info->w_tree, &istate, stash_index_path.buf, 0, - NULL)) { + discard_cache(); + if (write_cache_as_tree(&info->w_tree, 0, NULL) || + reset_tree(&info->i_tree, 0, 1)) ret = -1; - goto done; - } done: - discard_index(&istate); UNLEAK(rev); object_array_clear(&rev.pending); clear_pathspec(&rev.prune_data); strbuf_release(&diff_output); - remove_path(stash_index_path.buf); return ret; }
This removes the second index used in stash_working_tree() to simplify the code. It also help to avoid issues with the split-index: when stash_working_tree() is called, the index is at `i_tree', and this tree is extracted in a second index for use in a subcommand. This is not a problem in the non-split-index case, but in the split-index case, if the shared index file has expired and is removed by a subcommand, the main index contains a reference to a file that no longer exists. The calls to set_alternative_index_output() are dropped to extract `i_tree' to the main index, and `GIT_INDEX_FILE' is no longer set before starting `update-index'. When it exits, the index has changed, and must be discarded. The call to reset_tree() becomes useless: the only caller of stash_working_tree() is do_create_stash(), which creates `i_tree' from its index, calls save_untracked_files() if requested (but as it also works on a second index, it is unaffected), then calls stash_working_tree(). But when save_untracked_files() will be modified to stop using another index, it won't reset the tree, because stash_patch() wants to work on a different tree (`b_tree') than stash_working_tree(). At the end of the function, the tree is reset to `i_tree'. Signed-off-by: Alban Gruin <alban.gruin@gmail.com> --- builtin/stash.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)