Re: [PATCH v12 12/13] apply: learn to use a different index file

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 11, 2016, 20:17 UTC
Message-ID: <xmqqvaz7ys9u.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160811184501.384-13-chriscool@tuxfamily.org>

Christian Couder <christian.couder@gmail.com> writes:

Show 35 quoted lines

> Sometimes we want to apply in a different index file.
>
> Before the apply functionality was libified it was possible to
> use the GIT_INDEX_FILE environment variable, for this purpose.
>
> But now, as the apply functionality has been libified, it should
> be possible to do that in a libified way.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  apply.c | 10 ++++++++--
>  apply.h |  1 +
>  2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/apply.c b/apply.c
> index 2ec2a8a..7e561a4 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -4674,8 +4674,14 @@ static int apply_patch(struct apply_state *state,
>  		state->apply = 0;
>  
>  	state->update_index = state->check_index && state->apply;
> -	if (state->update_index && state->newfd < 0)
> -		state->newfd = hold_locked_index(state->lock_file, 1);
> +	if (state->update_index && state->newfd < 0) {
> +		if (state->index_file)
> +			state->newfd = hold_lock_file_for_update(state->lock_file,
> +								 state->index_file,
> +								 LOCK_DIE_ON_ERROR);
> +		else
> +			state->newfd = hold_locked_index(state->lock_file, 1);
> +	}
>  
>  	if (state->check_index && read_cache() < 0) {
>  		error(_("unable to read index file"));

Here is a call to read_cache() that reads the default index file on the filesystem into the default in-core index "the_index".

Shouldn't it be reading from state->index_file instead?

If we limit the review only to the context of your series, I think

    fall_back_threeway()
     -> build_fake_ancestor() -- uses index_path to use custom index
     -> discard_cache()
     -> read_cache_from(index_path) -- reads back the fake ancestor
     -> write_index_as_tree() -- writes the fake_ancestor
     -> run_apply(index_path)
        -> apply_all_patches()
           -> apply_patch()

is the only codepath that uses a custom index file, so when the control reaches this function with a custom index file, the in-core index is already populated, making read_cache() a no-op, and that is the only thing that makes the resulting code avoid triggering this bug, but as part of a general "libified" codepath, I think it should be made to read from state->index_file using read_cache_from().

I only noticed this call to read_cache(), but there may be others lurking nearby.