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

From: Elijah Newren <newren@gmail.com>
Date: Nov 13, 2017, 22:04 UTC
Message-ID: <CABPp-BFX-xZf962vuCjJSaB0=sPMT_zNF8+mGPG04dFi5PE6SQ@mail.gmail.com>
In-Reply-To: <CAGZ79kYXbvDL_SoEENf60DsDFA-yEWfvgv8bNv_v+mw042ZH=w@mail.gmail.com>

Thanks for the reviews!

On Mon, Nov 13, 2017 at 11:48 AM, Stefan Beller <sbeller@google.com> wrote:

Show 13 quoted lines

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

Show 9 quoted lines

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