Re: [PATCH 02/30] merge-recursive: Fix logic ordering issue

[Elijah Newren <newren@gmail.com>]

Thanks for the reviews!

On Mon, Nov 13, 2017 at 11:48 AM, Stefan Beller <sbeller@google.com> wrote:
> On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren <newren@gmail.com> wrote:
>> merge_trees() did a variety of work, including:
>>   * Calling get_unmerged() to get unmerged entries
>>   * Calling record_df_conflict_files() with all unmerged entries to
>>     do some work to ensure we could handle D/F conflicts correctly
>>   * Calling get_renames() to check for renames.
>>
>> An easily overlooked issue is that get_renames() can create more
>> unmerged entries and add them to the list, which have the possibility of
>> being involved in D/F conflicts.
>
> I presume these are created via insert_stage_data called in
> get_renames, when the path entry is not found?

Yes.

>> So the call to
>> record_df_conflict_files() should really be moved after all the rename
>> detection.  I didn't come up with any testcases demonstrating any bugs
>> with the old ordering, but I suspect there were some for both normal
>> renames and for directory renames.  Fix the ordering.
>
> It is hard to trace this down, though looking at
> 3af244caa8 (Cumulative update of merge-recursive in C, 2006-07-27)
> may help us reason about it.

It doesn't really go back that far.  I added the
record_df_conflict_files() function (originally named
make_room_for_directories_of_df_conflicts()) in commit ef02b31721
(merge-recursive: Make room for directories in D/F conflicts
2010-09-20); the rename happened in commit 70cc3d36eb
(merge-recursive: Save D/F conflict filenames instead of unlinking
them 2011-08-11).

> How would a bug look like?

Some of these corner cases sometimes get confusing to try to reason
about and duplicate, so I was trying to avoid that....oh, well.  :-)
I mostly wanted to use the simple logic that:
record_df_conflict_files() exists to take an inventory of all unmerged
files to make sure that D/F conflicts can be handled appropriately.
get_renames() has the potential for adding more unmerged files, thus I
should have placed record_df_conflict_files() after get_renames() when
I introduced it.

But since you asked...

A bug here would essentially mean that a git merge fails to handle
files in directories under a D/F conflict; when trying to process such
files and write out their conflict state to disk, it would fail to
create the necessary directory because a file is in the way.

In order to trigger it, you'd need to have a D/F conflict where the
file involved in the D/F conflict wasn't unmerged after unpack_trees()
but only "shows up" due to the rename detection (i.e. added by the
insert_stage_data() call as you mention above).  I think reading
through Documentation/technical/trivial-merge.txt, that this actually
isn't possible with what I'm calling "normal" renames; it's actually
something newly possible only due to directory rename detection.  But
you may have to get the merge direction just right, you might have to
worry about files that sort between a file with the same name as a
directory and the files within the directory (e.g. "path.txt" in the
list "path", then "path.txt", then "path/foo").

Do you feel it's important that I come up with a demonstration case
here?  If so, I'll see if I can generate one.