| Message ID | 20200630151558.20975-1-alban.gruin@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | stash: drop usage of a second index | expand |
On Tue, Jun 30, 2020 at 5:16 PM Alban Gruin <alban.gruin@gmail.com> wrote: > > The old scripted `git stash' used to create a second index to save > modified and untracked files, and restore untracked files, without > affecting the main index. This behaviour was carried on when it was > rewritten in C, and here, most operations performed on the second index > are done by forked commands (ie. `read-tree' instead of reset_tree(), > etc.). > > The goal of this series is to modernise (a bit) builtin/stash.c. This patch series looks good to me. I found only a small nit or typo in the commit message of patch 5/6. > Originally, this series was also meant to fix a bug reported by Son > Luong Ngoc [0], but as emphasized by Gábor [1], the issue is not limited > to `git stash', so this series is not a good fix for this particular > issue. > > This series is based on a08a83db2b (The sixth batch, 2020-06-29). It seems to apply without conflicts on top of current master. > Changes since v1: > > - Lots of rewording, following comments from Christian Couder and Son > Luong Ngoc. > > - Removed a useless function call. [...] > builtin/stash.c | 156 +++++++++++++++--------------------------------- > 1 file changed, 48 insertions(+), 108 deletions(-) I like very much how it simplifies a lot of things. Thanks, Christian.
The old scripted `git stash' used to create a second index to save modified and untracked files, and restore untracked files, without affecting the main index. This behaviour was carried on when it was rewritten in C, and here, most operations performed on the second index are done by forked commands (ie. `read-tree' instead of reset_tree(), etc.). The goal of this series is to modernise (a bit) builtin/stash.c. Originally, this series was also meant to fix a bug reported by Son Luong Ngoc [0], but as emphasized by Gábor [1], the issue is not limited to `git stash', so this series is not a good fix for this particular issue. This series is based on a08a83db2b (The sixth batch, 2020-06-29). The tip of this series is tagged as "stash-remove-second-index-v2" at https://github.com/agrn/git. [0] https://lore.kernel.org/git/EED2CFF1-5BEF-429D-AB99-AD148A867614@gmail.com/ [1] https://lore.kernel.org/git/20200615152715.GD2898@szeder.dev/ Changes since v1: - Lots of rewording, following comments from Christian Couder and Son Luong Ngoc. - Removed a useless function call. Alban Gruin (6): stash: mark `i_tree' in reset_tree() const stash: remove the second index in stash_working_tree() stash: remove the second index in stash_patch() stash: remove the second index in save_untracked_files() stash: remove the second index in restore_untracked() stash: remove `stash_index_path' builtin/stash.c | 156 +++++++++++++++--------------------------------- 1 file changed, 48 insertions(+), 108 deletions(-) Range-diff against v1: 1: 6101e8ce64 ! 1: 598f03e76e stash: mark `i_tree' in reset_tree() const @@ -2,9 +2,9 @@ stash: mark `i_tree' in reset_tree() const - As reset_tree() does not change the value pointed by `i_tree', and that - it will be provided with `the_hash_algo->empty_tree' which is a - constant, it is changed to be a pointer to a constant. + In reset_tree(), the value pointed by `i_tree' is not modified. In a + latter commit, it will be provided with `the_hash_algo->empty_tree', + which is a constant. Hence, this changes `i_tree' to be a constant. Signed-off-by: Alban Gruin <alban.gruin@gmail.com> 2: 26dceead41 ! 2: 7265cfe65c stash: remove the second index in stash_working_tree() @@ -3,26 +3,19 @@ stash: remove the second index in stash_working_tree() 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 code. 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(). + 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 in a later commit, when save_untracked_file() will + be rewritten to use the "main" index, so I did not remove it. At the end of the function, the tree is reset to `i_tree'. 3: 1ae236daf4 = 3: b5587c0d56 stash: remove the second index in stash_patch() 4: 8c901ddf9d ! 4: 5fbcddf899 stash: remove the second index in save_untracked_files() @@ -8,7 +8,7 @@ containing only untracked files, so the tree is reset to the empty tree at the beginning (hence the need for reset_tree() to accept constant trees). The environment of `update-index' is no longer updated, and the - index is discarded after this command exited. + index is discarded after this command exits. As the only caller of this function is do_create_stash(), and the next user of the index is either save_untracked_files() or stash_patch(), 5: 413be5b5f9 ! 5: 0338986339 stash: remove the second index in restore_untracked() @@ -42,7 +42,7 @@ return -1; - } - child_process_init(&cp); +- child_process_init(&cp); cp.git_cmd = 1; argv_array_pushl(&cp.args, "checkout-index", "--all", NULL); - argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s", 6: b2c08c189e = 6: 4f151f4600 stash: remove `stash_index_path'