git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Jul 11, 2018, 17:03 UTC
Message-ID
<xmqqsh4p3dtk.fsf@gitster-ct.c.googlers.com>
In-Reply-To
<20180711051834.28181-3-newren@gmail.com>
Elijah Newren <newren@gmail.com> writes:
Show 12 quoted lines
>     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?

Show 104 quoted lines
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  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 &&
Previous: Elijah NewrenNext: Elijah Newren
Message 6 of 14 in “Address recovery failures with directory/file conflicts”
  1. 0/2 Address recovery failures with directory/file conflictsElijah Newren, Jul 11, 2018
  2. 1/2 t1015: demonstrate directory/file conflict recovery failuresElijah Newren, Jul 11, 2018
  3. Eric SunshineJul 11, 2018
  4. Elijah NewrenJul 11, 2018
  5. 2/2 read-cache: fix directory/file conflict handling in read_index_unmerged()Elijah Newren, Jul 11, 2018
  6. Junio C HamanoJul 11, 2018
  7. Elijah NewrenJul 11, 2018
  8. 0/2 Address recovery failures with directory/file conflictsElijah Newren, Jul 13, 2018
  9. 2/2 read-cache: fix directory/file conflict handling in read_index_unmerged()Elijah Newren, Jul 13, 2018
  10. 1/2 t1015: demonstrate directory/file conflict recovery failuresElijah Newren, Jul 13, 2018
  11. Elijah NewrenJul 13, 2018
  12. 0/2 Address recovery failures with directory/file conflictsElijah Newren, Jul 31, 2018
  13. 2/2 read-cache: fix directory/file conflict handling in read_index_unmerged()Elijah Newren, Jul 31, 2018
  14. 1/2 t1015: demonstrate directory/file conflict recovery failuresElijah Newren, Jul 31, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.