Re: [PATCH] unpack-trees: do not fail reset because of unmerged skipped entry

[Duy Nguyen <pclouds@gmail.com>]

On Fri, Jun 15, 2018 at 07:42:51AM +0300, Max Kirillov wrote:
> After modify/delete merge conflict happens in a file skipped by sparse
> checkout, "git reset --merge", which implements the "--abort" actions, and
> "git reset --hard" fail with message "Entry * not uptodate. Cannot update
> sparse checkout." The reason is that the entry is verified in
> apply_sparse_checkout() for being up-to-date even when it has a conflict.

Conflicted entries should not be skipped by design. Even if you
specify sparse patterns to ignore them, they must be checked out. When
a conflicted entry appears in apply_sparse_checkout() something else
is already wrong.

I think this is a better fix along that line. As you can see we
already un-skip staged entries. But I think I forgot (or did not know)
about CE_CONFLICTED. This change passes your new tests, but I didn't
try to run the whole test suite to see if I broke anything else.

-- 8< --
diff --git a/unpack-trees.c b/unpack-trees.c
index 3a85a02a77..eb544ee1b3 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1246,7 +1246,7 @@ static void mark_new_skip_worktree(struct exclude_list *el,
 		if (select_flag && !(ce->ce_flags & select_flag))
 			continue;
 
-		if (!ce_stage(ce))
+		if (!ce_stage(ce) && !(ce->ce_flags & CE_CONFLICTED))
 			ce->ce_flags |= skip_wt_flag;
 		else
 			ce->ce_flags &= ~skip_wt_flag;
-- 8< --

> Checking conflicted entry for being up-to-date is not performed in other
> cases. One obvious reason to not check it is that it is already modified
> by inserting conflict marks.
> 
> Fix by not checking conflicted entries before performing reset.
> Also, add test case which verifies the issue is fixed.
> 
> Signed-off-by: Max Kirillov <max@max630.net>
> ---
> I have tried to use sparse-checkout for merging and cherrypicking, to save on IO
> and disk space. It works, mostly, but there are issues here and there.
> This one was low hanging, and also pretty annoying.
> 
>  t/t3035-merge-sparse.sh | 46 +++++++++++++++++++++++++++++++++++++++++
>  unpack-trees.c          |  2 +-
>  2 files changed, 47 insertions(+), 1 deletion(-)
>  create mode 100755 t/t3035-merge-sparse.sh
> 
> diff --git a/t/t3035-merge-sparse.sh b/t/t3035-merge-sparse.sh
> new file mode 100755
> index 0000000000..c6b2b0b82a
> --- /dev/null
> +++ b/t/t3035-merge-sparse.sh
> @@ -0,0 +1,46 @@
> +#!/bin/sh
> +
> +test_description='merge with sparse files'
> +
> +. ./test-lib.sh
> +
> +# test_file $filename $content
> +test_file () {
> +	echo "$2" > "$1" &&
> +	git add "$1"
> +}
> +
> +# test_commit_this $message_and_tag
> +test_commit_this () {
> +	git commit -m "$1" &&
> +	git tag "$1"
> +}
> +
> +test_expect_success 'setup' '
> +	test_file checked-out init &&
> +	test_file modify_delete modify_delete_init &&
> +	test_commit_this init &&
> +	test_file modify_delete modify_delete_theirs &&
> +	test_commit_this theirs &&
> +	git reset --hard init &&
> +	git rm modify_delete &&
> +	test_commit_this ours &&
> +	git config core.sparseCheckout true &&
> +	echo "/checked-out" >.git/info/sparse-checkout &&
> +	git reset --hard &&
> +	! git merge theirs
> +'
> +
> +test_expect_success 'reset --hard works after the conflict' '
> +	git reset --hard
> +'
> +
> +test_expect_success 'setup: conflict back' '
> +	! git merge theirs
> +'
> +
> +test_expect_success 'Merge abort works after the conflict' '
> +	git merge --abort
> +'
> +
> +test_done
> diff --git a/unpack-trees.c b/unpack-trees.c
> index e73745051e..65ae0721a6 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -468,7 +468,7 @@ static int apply_sparse_checkout(struct index_state *istate,
>  		 * also stat info may have lost after merged_entry() so calling
>  		 * verify_uptodate() again may fail
>  		 */
> -		if (!(ce->ce_flags & CE_UPDATE) && verify_uptodate_sparse(ce, o))
> +		if (!(ce->ce_flags & CE_UPDATE) && !(ce->ce_flags & CE_CONFLICTED) && verify_uptodate_sparse(ce, o))
>  			return -1;
>  		ce->ce_flags |= CE_WT_REMOVE;
>  		ce->ce_flags &= ~CE_UPDATE;
> -- 
> 2.17.0.1185.g782057d875
>