On 19 April 2018 at 22:48, Martin Ågren <martin.agren@xxxxxxxxx> wrote:
> On 19 April 2018 at 19:58, Elijah Newren <newren@xxxxxxxxx> wrote:
>> -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)
>> {
[...]
>> + memset(&o->unpack_opts, 0, sizeof(o->unpack_opts));
[...]
>> + setup_unpack_trees_porcelain(&o->unpack_opts, "merge");
[...]
>> }
>
> 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.
