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

["Martin Ågren" <martin.agren@gmail.com>]

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.

It turns out that the only user of `unpack_opts` outside this function
can indeed end up wanting to use the error messages that `clear_...()`
would set out to free. So yes, the call to `clear_...()` will need to go
elsewhere.

It does sort of make me wonder if we should memset `unpack_opts` to zero
somewhere early, so that we can then `clear_...()` it early here before
zeroizing it. So yes, we'd be constantly allocating and freeing those
strings. Am I right to assume that the code after your series would do
(roughly) the same number of calls to `setup_unpack_trees_porcelain()`,
i.e., `git_merge_trees()` as it did before?

All of this is arguably irrelevant for this series. It might be better
if I clarify this memory ownership and do any adjustments as part of my
patch (series), rather than you shuffling things around at this time.

Mostly thinking out loud. If you have any thoughts, feel free to share.

Martin

[1] https://public-inbox.org/git/CAN0heSquJboMMgay+5XomqXCGoHtXxf1mJBmY_L7y+AA4eG0KA@mail.gmail.com/