Re: [RFC PATCH v1 0/6] stash: drop usage of a second index

From: Alban Gruin <alban.gruin@gmail.com>
To: git@vger.kernel.org
Cc: Thomas Gummerer <t.gummerer@gmail.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [RFC PATCH v1 0/6] stash: drop usage of a second index
Date: Thu, 4 Jun 2020 14:07:22 +0200	[thread overview]
Message-ID: <c0a31a63-824b-d256-4811-c9307e87092c@gmail.com> (raw)
In-Reply-To: <20200505104849.13602-1-alban.gruin@gmail.com>

Hi,

Le 05/05/2020 à 12:48, Alban Gruin a écrit :
> 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.).  This works most of the time, except in some edge case with the
> split-index when the split file has expired and is deleted by a forked
> command: the main index may still contain a reference to the now-deleted
> file, and subsequent operations on the index will fail [0].
> 
> The goal of this series is to modernise (a bit) builtin/stash.c, and to
> fix the aforementionned edge case.
> 
> I have to admit that I don't really know how to test this.
> GIT_TEST_SPLIT_INDEX failed on me (gdb showed me that it does not enable
> the split-index at all, at least in `git stash' and its forks), and I'm
> reluctant to add explicits tests on `git stash' about the split-index,
> when nothing in its code explicitly does unusual things with the index
> once this series is applied.  If anyone wants to share opinions about
> this, I would be happy to read them.
> 
> This series is based on b34789c0b0 ("The sixth batch", 2020-05-01).
> 
> The tip of this series is tagged as "stash-remove-second-index-v1" at
> https://github.com/agrn/git.
> 
> [0] https://lore.kernel.org/git/EED2CFF1-5BEF-429D-AB99-AD148A867614@gmail.com/
> 
> 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 | 151 +++++++++++++++---------------------------------
>  1 file changed, 46 insertions(+), 105 deletions(-)
> 

I’m afraid this series fell through the cracks.

Alban