Re: [PATCH 16/30] merge-recursive: Introduce new functions to handle rename logic

[Elijah Newren <newren@gmail.com>]

On Mon, Nov 13, 2017 at 9:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Elijah Newren <newren@gmail.com> writes:
>>
>>> +struct rename_info {
>>> +    struct string_list *head_renames;
>>> +    struct string_list *merge_renames;
>>> +};
>>
>> This type is added in order to allow the caller and the helper to
>> communicate the findings in a single logical structure, instead of
>> having to pass them as separate parameters, etc.  If we anticipate
>> that the information that needs to be passed will grow richer in
>> later steps (or a follow-up series), such encapsulation makes a lot
>
> Hmph, I actually am quite confused with the existing code.
>
> The caller (originally in merge_trees(), now in handle_renames())
> calls get_renames() twice and have the list of renamed paths in
> these two string lists.  get_renames() mostly works with the
> elements in the "entries" list and adds the "struct rename" to the
> string list that is to be returned.  And the caller uses these two
> string lists get_renames() returns when calling process_renames(),
> but once process_renames() is done with them, these two string lists
> are never looked at by anybody.

Actually, if I remember correctly, my first stab was to do all the
cleanup at the end of handle_renames(), but then I ran into
use-after-free errors.  I'm not sure if I remember all the details,
but I'll try to lay out the path:

process_renames() can't handle conflicts immediately because of D/F
concerns (if all entries in the competing directory resolve away, then
there's no more D/F conflict, but we have to wait until each of those
entries is processed to find out if that happens or if a D/F conflict
remains).  Because of that, process_renames() needs to store
information into a rename_conflict_info struct for process_entry() to
look at later.  Included in rename_conflict_info are things like
diff_filepair and stage_data entries, both taken from the rename
lists.  If the rename lists are freed at the end of handle_renames(),
then this information is freed before process_entry() runs and thus we
get a use-after-free error.

Since both you and I thought to push this cleanup to the end of
handle_renames(), though, I should probably add that explanation to
the commit message.  Granted, it isn't actually needed for this
particular commit, because up to this point all the information used
in rename_conflict_info was leaked anyway.  But it becomes an issue
with patch 17 when we start freeing that info.