On 19 April 2018 at 22:48, Martin Ågren [off-list ref] wrote:

On 19 April 2018 at 19:58, Elijah Newren [off-list ref] wrote:

quoted

-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)
 {

[...]

quoted

+       memset(&o->unpack_opts, 0, sizeof(o->unpack_opts));

[...]

quoted

+       setup_unpack_trees_porcelain(&o->unpack_opts, "merge");

[...]

quoted

 }

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?

Or, of course, both `setup_...` and `clear_...` would go outside this
function to churn less memory... Anyway, this still holds:

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.