On Thu, Aug 11, 2016 at 10:17 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Christian Couder <christian.couder@xxxxxxxxx> 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@xxxxxxxxxxxxx>
>> ---
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html