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

[Elijah Newren]

Hi Martin,

On Sat, Apr 28, 2018 at 4:32 AM, Martin Ågren  wrote:
> As you noted elsewhere [1], Ben is also working in this area. I'd be
> perfectly happy to sit on these patches until both of your contributions
> come through to master.
>
> [1] 
> https://public-inbox.org/git/CABPp-BFh=gl6rnbst2bgtynkij1z5tmgar1via5_vytef5e...@mail.gmail.com/

Instead of waiting for these to come through to master, could you just
submit based on the top of bp/merge-rename-config?  I've got several
other merge-recursive changes, some about ready to send and others in
the works.  I don't think any conflict yet, but I would rather avoid
causing you any more waiting or conflicts and would rather just have
both your and Ben's changes in pu and then I can just build mine on
top of those.  Besides, I want to see that FIXME go away and have
fewer leaks.  :-)

Thanks,
Elijah

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

[Elijah Newren]

Hi Martin,

On Sat, Apr 28, 2018 at 4:32 AM, Martin Ågren  wrote:
> From: Elijah Newren 
>
> Hi Elijah,
>
> [Since this is leaving the topic of rename-detection in favour of
> leak-plugging, I'm shortening the cc-list a bit.]
>
>> So, instead, I'd like to see something like the below
>> (built on top of my series):
>
> Thanks a lot. I now have the below patch in my tree as a preparatory
> part of a three-patch series on top of your series. Since the gist of
> this patch is entirely your creation, is it ok if I place your Author:
> and Signed-off-by: on it? Credit where credit is due.

Sure, I'm fine with either that or an Original-patch-by attribution.

Anyway, thanks for fleshing it out with the commit message and
handling the early return cases.  And for tackling the
setup_unpack_trees_porcelain() memory leak.

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

[Johannes Schindelin]

Hi Martin,

On Sat, 28 Apr 2018, Martin Ågren wrote:

> -->8--
> Subject: merge-recursive: provide pair of `unpack_trees_{start,finish}()`
> 
> Rename `git_merge_trees()` to `unpack_trees_start()` and extract the
> call to `discard_index()` into a new function `unpack_trees_finish()`.
> As a result, these are called early resp. late in `merge_trees()`,
> making the resource handling clearer. The next commit will expand on
> that, teaching `..._finish()` to free more memory. (So rather than
> moving the TODO-comment, just drop it, since it will be addressed soon
> enough.)
> 
> Also call `..._finish()` when `merge_trees()` returns early.

Looks good! It is missing a Signed-off-by: line, and you probably want to
start a new thread that also includes the "next commit", but other than
that it is pretty nice and ready for contributing, methinks.

Ciao,
Dscho

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

[Martin Ågren]

From: Elijah Newren 

Hi Elijah,

[Since this is leaving the topic of rename-detection in favour of 
leak-plugging, I'm shortening the cc-list a bit.]

> So, instead, I'd like to see something like the below
> (built on top of my series):

Thanks a lot. I now have the below patch in my tree as a preparatory
part of a three-patch series on top of your series. Since the gist of
this patch is entirely your creation, is it ok if I place your Author:
and Signed-off-by: on it? Credit where credit is due.

As you noted elsewhere [1], Ben is also working in this area. I'd be
perfectly happy to sit on these patches until both of your contributions
come through to master.

[1] 
https://public-inbox.org/git/CABPp-BFh=gl6rnbst2bgtynkij1z5tmgar1via5_vytef5e...@mail.gmail.com/

Martin

-->8--
Subject: merge-recursive: provide pair of `unpack_trees_{start,finish}()`

Rename `git_merge_trees()` to `unpack_trees_start()` and extract the
call to `discard_index()` into a new function `unpack_trees_finish()`.
As a result, these are called early resp. late in `merge_trees()`,
making the resource handling clearer. The next commit will expand on
that, teaching `..._finish()` to free more memory. (So rather than
moving the TODO-comment, just drop it, since it will be addressed soon
enough.)

Also call `..._finish()` when `merge_trees()` returns early.
---
 merge-recursive.c | 29 +++--
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 1de8dc1c53..e64102004a 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,11 @@ static int git_merge_trees(struct merge_options *o,
return rc;
 }
 
+static void unpack_trees_finish(struct merge_options *o)
+{
+   discard_index(>orig_index);
+}
+
 struct tree *write_tree_from_memory(struct merge_options *o)
 {
struct tree *result = NULL;
@@ -3079,13 +3084,14 @@ 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)
err(o, _("merging of trees %s and %s failed"),
oid_to_hex(>object.oid),
oid_to_hex(>object.oid));
+   unpack_trees_finish(o);
return -1;
}
 
@@ -3138,20 +3144,15 @@ int merge_trees(struct merge_options *o,
 
hashmap_free(>current_file_dir_set, 1);
 
-   if (clean < 0)
+   if (clean < 0) {
+   unpack_trees_finish(o);
return clean;
+   }
}
else
clean = 1;
 
-   /* Free the extra index left from git_merge_trees() */
-   /*
-* FIXME: Need to also 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(>orig_index);
+   unpack_trees_finish(o);
 
if (o->call_depth && !(*result = write_tree_from_memory(o)))
return -1;
-- 
2.17.0

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

[Elijah Newren]

On Mon, Apr 23, 2018 at 10:13 PM, Martin Ågren  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, );
> +   clear_unpack_trees_porcelain();
> cache_tree_free(_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(>orig_index);
+   clear_unpack_trees_porcelain(>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(>orig_index);
+   unpack_trees_finish(o);
 
if (o->call_depth && !(*result = write_tree_from_memory(o)))
return -1;
-- 
2.17.0.295.g791b7256b2.dirty

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

[Martin Ågren]

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()`.

In all current callers, the pointers are about to go out of scope, so we
do not need to set them to NULL. Let's do so anyway so that a future
caller or restructured code doesn't suddenly start accessing dangling
pointers.

Note that we only take responsibility for the memory allocated in
`setup_unpack_trees_porcelain()` and not any other members of the
`struct unpack_trees_options`.

Signed-off-by: Martin Ågren 
---
 unpack-trees.h |  5 +
 builtin/checkout.c |  1 +
 merge-recursive.c  |  1 +
 merge.c|  3 +++
 unpack-trees.c | 11 +++
 5 files changed, 21 insertions(+)

diff --git a/unpack-trees.h b/unpack-trees.h
index 6c48117b84..8c56cf0150 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -32,6 +32,11 @@ enum unpack_trees_error_types {
 void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
  const char *cmd);
 
+/*
+ * Frees resources allocated by setup_unpack_trees_porcelain().
+ */
+extern void clear_unpack_trees_porcelain(struct unpack_trees_options *opts);
+
 struct unpack_trees_options {
unsigned int reset,
 merge,
diff --git a/builtin/checkout.c b/builtin/checkout.c
index b49b582071..5cebe170fc 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -526,6 +526,7 @@ static int merge_working_tree(const struct checkout_opts 
*opts,
init_tree_desc([1], tree->buffer, tree->size);
 
ret = unpack_trees(2, trees, );
+   clear_unpack_trees_porcelain();
if (ret == -1) {
/*
 * Unpack couldn't do a trivial merge; either
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, );
+   clear_unpack_trees_porcelain();
cache_tree_free(_cache_tree);
return rc;
 }
diff --git a/merge.c b/merge.c
index f123658e58..b433291d0c 100644
--- a/merge.c
+++ b/merge.c
@@ -130,8 +130,11 @@ int checkout_fast_forward(const struct object_id *head,
 
if (unpack_trees(nr_trees, t, )) {
rollback_lock_file(_file);
+   clear_unpack_trees_porcelain();
return -1;
}
+   clear_unpack_trees_porcelain();
+
if (write_locked_index(_index, _file, COMMIT_LOCK))
return error(_("unable to write new index file"));
return 0;
diff --git a/unpack-trees.c b/unpack-trees.c
index e73745051e..4c76a29241 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -179,6 +179,17 @@ void setup_unpack_trees_porcelain(struct 
unpack_trees_options *opts,
opts->unpack_rejects[i].strdup_strings = 1;
 }
 
+void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)
+{
+   char **msgs = (char **)opts->msgs;
+
+   free(msgs[ERROR_WOULD_OVERWRITE]);
+   free(msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED]);
+   free(msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN]);
+
+   memset(opts->msgs, 0, sizeof(opts->msgs));
+}
+
 static int do_add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
 unsigned int set, unsigned int clear)
 {
-- 
2.17.0