Re: [RFC PATCH v1 2/6] stash: remove the second index in stash_working_tree()

[Christian Couder <christian.couder@gmail.com>]

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