Re: [PATCH 2/2] unpack_trees_options: free messages when done

[Elijah Newren]

On Mon, Apr 23, 2018 at 10:13 PM, Martin Ågren <martin.ag...@gmail.com> wrote:
> The strings allocated in `setup_unpack_trees_porcelain()` are never
> freed. Provide a function `clear_unpack_trees_porcelain()` to do so and
> call it in the functions which use `setup_unpack_trees_porcelain()`.

This is awesome; thanks.

> diff --git a/merge-recursive.c b/merge-recursive.c
> index 0c0d48624d..8229b91e2f 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -301,6 +301,7 @@ static int git_merge_trees(int index_only,
>         init_tree_desc_from_tree(t+2, merge);
>
>         rc = unpack_trees(3, t, &opts);
> +       clear_unpack_trees_porcelain(&opts);
>         cache_tree_free(&active_cache_tree);
>         return rc;

Yeah, this could result in an evil merge.  In my series, I want to
continue to be able to call verify_uptodate() from unpack_trees.c in order
to check if files affected by renames are dirty and we need to avoid
overwriting them.  That can trigger error messages, so they need to not be
freed until later.  So, instead, I'd like to see something like the below
(built on top of my series):

-- >8 --

---
 merge-recursive.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index f2cbad4f10..3491a27bf2 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -337,10 +337,10 @@ 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(struct merge_options *o,
-                          struct tree *common,
-                          struct tree *head,
-                          struct tree *merge)
+static int unpack_trees_start(struct merge_options *o,
+                             struct tree *common,
+                             struct tree *head,
+                             struct tree *merge)
 {
        int rc;
        struct tree_desc t[3];
@@ -378,6 +378,12 @@ static int git_merge_trees(struct merge_options *o,
        return rc;
 }
 
+static void unpack_trees_finish(struct merge_options *o)
+{
+       discard_index(&o->orig_index);
+       clear_unpack_trees_porcelain(&o->unpack_opts);
+}
+
 struct tree *write_tree_from_memory(struct merge_options *o)
 {
        struct tree *result = NULL;
@@ -3079,7 +3085,7 @@ int merge_trees(struct merge_options *o,
                return 1;
        }
 
-       code = git_merge_trees(o, common, head, merge);
+       code = unpack_trees_start(o, common, head, merge);
 
        if (code != 0) {
                if (show(o, 4) || o->call_depth)
@@ -3144,14 +3150,7 @@ int merge_trees(struct merge_options *o,
        else
                clean = 1;
 
-       /* Free the extra index left from git_merge_trees() */
-       /*
-        * FIXME: Need to also free data allocated by
-        * setup_unpack_trees_porcelain() tucked away in o->unpack_opts.msgs,
-        * but the problem is that only half of it refers to dynamically
-        * allocated data, while the other half points at static strings.
-        */
-       discard_index(&o->orig_index);
+       unpack_trees_finish(o);
 
        if (o->call_depth && !(*result = write_tree_from_memory(o)))
                return -1;
-- 
2.17.0.295.g791b7256b2.dirty