On Thu, Apr 19, 2018 at 1:48 PM, Martin Ågren <martin.agren@xxxxxxxxx> wrote:
> On 19 April 2018 at 19:58, Elijah Newren <newren@xxxxxxxxx> 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@xxxxxxxxxx>
>> Signed-off-by: Elijah Newren <newren@xxxxxxxxx>
>> Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
>> ---
>> 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.
