Re: [PATCH 2/2] read-cache: fix directory/file conflict handling in read_index_unmerged()
- Date: Wed, 11 Jul 2018 11:24:55 -0700
- From: Elijah Newren <newren@xxxxxxxxx>
- Subject: Re: [PATCH 2/2] read-cache: fix directory/file conflict handling in read_index_unmerged()
On Wed, Jul 11, 2018 at 10:03 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Elijah Newren <newren@xxxxxxxxx> 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?
Makes sense, I'll include that in my re-roll after waiting a little
bit for any further reviews.