Re: [PATCH 2/2] read-cache: fix directory/file conflict handling in read_index_unmerged()

> Elijah Newren <newren@gmail.com> writes:
>
>>     The _only_ reason we want to keep a previously unmerged entry in the
>>     index at stage #0 is so that we don't forget the fact that we have
>>     corresponding file in the work tree in order to be able to remove it
>>     when the tree we are resetting to does not have the path.
>> ...
>> So, that's the intended purpose of this function.  The problem is that
>> when directory/files conflicts are present, trying to add the file to the
>> index at stage 0 fails (because there is still a directory in the way),
>> and the function returns early with a -1 return code to signify the error.
>> As noted above, none of the callers who want the drop-to-stage-0 behavior
>> check the return status, though, so this means all remaining unmerged
>> entries remain in the index and the callers proceed assuming otherwise.
>
> Nicely analysed and explained so far.
>
>> ...  The temporary simultaneous appearance of the
>> directory and file entries in the index will be removed by the callers
>> before they attempt to write the index anywhere.
>
> But this part makes me feel a bit uneasy, as I find this "will be
> removed" a bit hand-wavy.  There are two such callers.  "am --skip"
> and "reset".
>
> The former uses am.c::fast_forward_to that calls unpack_trees() to
> two-way merge (aka "switch to the other branch") and these entries
> with CE_CONFLICTED flag get removed in merged/deleted_entry().
>
> "reset" (all variants) call unpack_trees() on the index prepared
> with read_cache_unmerged(), and the unmerged entries that are marked
> with CE_CONFLICTED bit get removed the same way.
>
> So perhaps before "before they attempt to", saying "by calling
> unpack_trees(), which excludes these unmerged entries marked with
> CE_CONFLICTED flag from the resulting index," or something like that
> would help uneasy readers?