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

[Christian Couder]

On Thu, Aug 11, 2016 at 10:17 PM, Junio C Hamano  wrote:
> Christian Couder  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 
>> ---
>>  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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Christian Couder  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 
> ---
>  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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html