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?
> Signed-off-by: Elijah Newren <newren@xxxxxxxxx>
> ---
> read-cache.c | 13 ++++++++-----
> t/t1015-read-index-unmerged.sh | 8 ++++----
> t/t6020-merge-df.sh | 3 ---
> t/t6042-merge-rename-corner-cases.sh | 1 -
> 4 files changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 372588260..666d295a5 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2632,10 +2632,13 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
>
> /*
> * Read the index file that is potentially unmerged into given
> - * index_state, dropping any unmerged entries. Returns true if
> - * the index is unmerged. Callers who want to refuse to work
> - * from an unmerged state can call this and check its return value,
> - * instead of calling read_cache().
> + * index_state, dropping any unmerged entries to stage #0 (potentially
> + * resulting in a path appearing as both a file and a directory in the
> + * index; the caller is responsible to clear out the extra entries
> + * before writing the index to a tree). Returns true if the index is
> + * unmerged. Callers who want to refuse to work from an unmerged
> + * state can call this and check its return value, instead of calling
> + * read_cache().
> */
> int read_index_unmerged(struct index_state *istate)
> {
> @@ -2658,7 +2661,7 @@ int read_index_unmerged(struct index_state *istate)
> new_ce->ce_flags = create_ce_flags(0) | CE_CONFLICTED;
> new_ce->ce_namelen = len;
> new_ce->ce_mode = ce->ce_mode;
> - if (add_index_entry(istate, new_ce, 0))
> + if (add_index_entry(istate, new_ce, ADD_CACHE_SKIP_DFCHECK))
> return error("%s: cannot drop to stage #0",
> new_ce->name);
> }
> diff --git a/t/t1015-read-index-unmerged.sh b/t/t1015-read-index-unmerged.sh
> index bbd64587c..5034ed931 100755
> --- a/t/t1015-read-index-unmerged.sh
> +++ b/t/t1015-read-index-unmerged.sh
> @@ -30,7 +30,7 @@ test_expect_success 'setup modify/delete + directory/file conflict' '
> )
> '
>
> -test_expect_failure 'read-tree --reset cleans unmerged entries' '
> +test_expect_success 'read-tree --reset cleans unmerged entries' '
> test_when_finished "git -C df_plus_modify_delete clean -f" &&
> test_when_finished "git -C df_plus_modify_delete reset --hard" &&
> (
> @@ -45,7 +45,7 @@ test_expect_failure 'read-tree --reset cleans unmerged entries' '
> )
> '
>
> -test_expect_failure 'One reset --hard cleans unmerged entries' '
> +test_expect_success 'One reset --hard cleans unmerged entries' '
> test_when_finished "git -C df_plus_modify_delete clean -f" &&
> test_when_finished "git -C df_plus_modify_delete reset --hard" &&
> (
> @@ -87,7 +87,7 @@ test_expect_success 'setup directory/file conflict + simple edit/edit' '
> )
> '
>
> -test_expect_failure 'git merge --abort succeeds despite D/F conflict' '
> +test_expect_success 'git merge --abort succeeds despite D/F conflict' '
> test_when_finished "git -C df_plus_edit_edit clean -f" &&
> test_when_finished "git -C df_plus_edit_edit reset --hard" &&
> (
> @@ -103,7 +103,7 @@ test_expect_failure 'git merge --abort succeeds despite D/F conflict' '
> )
> '
>
> -test_expect_failure 'git am --skip succeeds despite D/F conflict' '
> +test_expect_success 'git am --skip succeeds despite D/F conflict' '
> test_when_finished "git -C df_plus_edit_edit clean -f" &&
> test_when_finished "git -C df_plus_edit_edit reset --hard" &&
> (
> diff --git a/t/t6020-merge-df.sh b/t/t6020-merge-df.sh
> index 2af1beec5..46b506b3b 100755
> --- a/t/t6020-merge-df.sh
> +++ b/t/t6020-merge-df.sh
> @@ -89,9 +89,6 @@ test_expect_success 'modify/delete + directory/file conflict' '
> '
>
> test_expect_success 'modify/delete + directory/file conflict; other way' '
> - # Yes, we really need the double reset since "letters" appears as
> - # both a file and a directory.
> - git reset --hard &&
> git reset --hard &&
> git clean -f &&
> git checkout modify^0 &&
> diff --git a/t/t6042-merge-rename-corner-cases.sh b/t/t6042-merge-rename-corner-cases.sh
> index 1cbd946fc..583e68997 100755
> --- a/t/t6042-merge-rename-corner-cases.sh
> +++ b/t/t6042-merge-rename-corner-cases.sh
> @@ -323,7 +323,6 @@ test_expect_success 'rename/directory conflict + content merge conflict' '
> (
> cd rename-directory-1 &&
>
> - git reset --hard &&
> git reset --hard &&
> git clean -fdqx &&
