Re: [PATCH v10 25/36] merge-recursive: fix overwriting dirty files involved in renames

[Elijah Newren <newren@gmail.com>]

On Thu, Apr 19, 2018 at 1:48 PM, Martin Ågren <martin.agren@gmail.com> wrote:
> On 19 April 2018 at 19:58, Elijah Newren <newren@gmail.com> wrote:
>> This fixes an issue that existed before my directory rename detection
>> patches that affects both normal renames and renames implied by
>> directory rename detection.  Additional codepaths that only affect
>> overwriting of dirty files that are involved in directory rename
>> detection will be added in a subsequent commit.
>>
>> Reviewed-by: Stefan Beller <sbeller@google.com>
>> Signed-off-by: Elijah Newren <newren@gmail.com>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>  merge-recursive.c                   | 85 ++++++++++++++++++++++-------
>>  merge-recursive.h                   |  2 +
>>  t/t3501-revert-cherry-pick.sh       |  2 +-
>>  t/t6043-merge-rename-directories.sh |  2 +-
>>  t/t7607-merge-overwrite.sh          |  2 +-
>>  unpack-trees.c                      |  4 +-
>>  unpack-trees.h                      |  4 ++
>>  7 files changed, 77 insertions(+), 24 deletions(-)
>>
>> diff --git a/merge-recursive.c b/merge-recursive.c
>> index c1c4faf61e..7fdcba4f22 100644
>> --- a/merge-recursive.c
>> +++ b/merge-recursive.c
>> @@ -337,32 +337,37 @@ static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree)
>>         init_tree_desc(desc, tree->buffer, tree->size);
>>  }
>>
>> -static int git_merge_trees(int index_only,
>> +static int git_merge_trees(struct merge_options *o,
>>                            struct tree *common,
>>                            struct tree *head,
>>                            struct tree *merge)
>>  {
>>         int rc;
>>         struct tree_desc t[3];
>> -       struct unpack_trees_options opts;
>>
>> -       memset(&opts, 0, sizeof(opts));
>> -       if (index_only)
>> -               opts.index_only = 1;
>> +       memset(&o->unpack_opts, 0, sizeof(o->unpack_opts));
>> +       if (o->call_depth)
>> +               o->unpack_opts.index_only = 1;
>>         else
>> -               opts.update = 1;
>> -       opts.merge = 1;
>> -       opts.head_idx = 2;
>> -       opts.fn = threeway_merge;
>> -       opts.src_index = &the_index;
>> -       opts.dst_index = &the_index;
>> -       setup_unpack_trees_porcelain(&opts, "merge");
>> +               o->unpack_opts.update = 1;
>> +       o->unpack_opts.merge = 1;
>> +       o->unpack_opts.head_idx = 2;
>> +       o->unpack_opts.fn = threeway_merge;
>> +       o->unpack_opts.src_index = &the_index;
>> +       o->unpack_opts.dst_index = &the_index;
>> +       setup_unpack_trees_porcelain(&o->unpack_opts, "merge");
>>
>>         init_tree_desc_from_tree(t+0, common);
>>         init_tree_desc_from_tree(t+1, head);
>>         init_tree_desc_from_tree(t+2, merge);
>>
>> -       rc = unpack_trees(3, t, &opts);
>> +       rc = unpack_trees(3, t, &o->unpack_opts);
>> +       /*
>> +        * unpack_trees NULLifies src_index, but it's used in verify_uptodate,
>> +        * so set to the new index which will usually have modification
>> +        * timestamp info copied over.
>> +        */
>> +       o->unpack_opts.src_index = &the_index;
>>         cache_tree_free(&active_cache_tree);
>>         return rc;
>>  }
>
> As mentioned in a reply to patch 33/36 [1], I've got a patch to add
> `clear_unpack_trees_porcelain()` which frees the resources allocated by
> `setup_unpack_trees_porcelain()`. Before this patch, I could easily call
> it at the end of this function. After this, the ownership is less
> obvious to me.

I wouldn't put the call to clear_unpack_trees_porcelain() at the end
of this function, but rather at the end of merge_trees().
merge_trees() is the only caller of git_merge_trees() and it continues
using o->unpack_opts until the end of that function.  At the end of
that function, there is no further need for o->unpack_opts.
Basically, put it right where I put the "FIXME: Need to also free data
allocated by setup_unpack_trees_porcelain()" comment.