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

From: Martin Ågren <martin.agren@gmail.com>
Date: Apr 19, 2018, 20:54 UTC
Message-ID: <CAN0heSpvjPoXESsyBnua2NsRPMv8OqCqUdddZYxYa+87+tERMQ@mail.gmail.com>
In-Reply-To: <CAN0heSrSn1wYGoRD6+c_Jk+rAUTbY94JLspwfiki6z4h=mnPSQ@mail.gmail.com>

On 19 April 2018 at 22:48, Martin Ågren <martin.agren@gmail.com> wrote:

Show 7 quoted lines

> On 19 April 2018 at 19:58, Elijah Newren <newren@gmail.com> 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");

[...]

Show 19 quoted lines

>>  }
>
> 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.