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.
