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

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

On Thu, Aug 11, 2016 at 10:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> 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?

Yes, it should.

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

Yeah, I agree with this reasoning.

> I think it should
> be made to read from state->index_file using read_cache_from().

Yeah, I will change it.

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

Yeah, there is another one in get_current_sha1() which is only called
by build_fake_ancestor() (from apply.c not from builtin/am.c as there
is a function with this name in both files), but this function is
currently called only when --build-fake-ancestor is passed which is
not the case in run_apply() in am.c. I will change it too.

Thanks,
Christian.