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

From: Alban Gruin <alban.gruin@gmail.com>
Date: Jun 13, 2020, 18:00 UTC
Message-ID: <2158eec0-9332-fe10-3636-95550a66f05d@gmail.com>
In-Reply-To: <CAP8UFD1aT4dmuNkEz95eDFTE7sY+4eK_TwbTD-Vw8U7KyyZ-DA@mail.gmail.com>

Hi Christian,

Le 13/06/2020 à 10:52, Christian Couder a écrit :

Show 34 quoted lines

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

Show 10 quoted lines

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