[v2,7/7] unpack-trees: resolve sparse-directory/file conflicts

Commit Message

Derrick Stolee July 20, 2021, 8:14 p.m. UTC

From: Derrick Stolee <[e-mail]>

When running unpack_trees() with a sparse index, we attempt to operate
on the index without expanding the sparse directory entries. Thus, we
operate by manipulating entire directories and passing them to the
unpack function. In the case of the 'git checkout' command, this is the
twoway_merge() function.

There are several cases in twoway_merge() that handle different
situations. One new one to add is the case of a directory/file conflict
where the directory is sparse. Before the sparse index, such a conflict
would appear as a list of file additions and deletions. Now,
twoway_merge() initializes 'current', 'oldtree', and 'newtree' from
src[0], src[1], and src[2], then sets 'oldtree' to NULL because it is
equal to the df_conflict_entry. The way to determine that we have a
directory/file conflict is to test that 'current' and 'newtree' disagree
on being sparse directory entries.

When we are in this case, we want to resolve the situation by calling
merged_entry(). This allows replacing the 'current' entry with the
'newtree' entry. This is important for cases where we want to run 'git
checkout' across the conflict and have the new HEAD represent the new
file type at that path. The first NEEDSWORK comment dropped in t1092
demonstrates this necessary behavior.

However, we still are in a confusing state when 'current' corresponds to
a staged change within a sparse directory that is not present at HEAD.
This should be atypical, because it requires adding a change outside of
the sparse-checkout cone, but it is possible. Since we are unable to
determine that this is a staged change within twoway_merge(), we cannot
add a case to reject the merge at this point. I believe this is due to
the use of df_conflict_entry in the place of 'oldtree' instead of using
the valud at HEAD, which would provide some perspective to this
decision. Any change that would allow this differentiation for staged
entries would need to involve information further up in unpack_trees().

That work should be done, sometime, because we are further confusing the
behavior of a directory/file conflict when staging a change in the
directory. The two cases 'checkout behaves oddly with df-conflict-?' in
t1092 demonstrate that even without a sparse-checkout, Git is not
consistent in its behavior. Neither of the two options seems correct,
either. This change makes the sparse-index behave differently than the
typcial sparse-checkout case, but it does match the full checkout
behavior in the df-conflict-2 case.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 t/t1092-sparse-checkout-compatibility.sh | 24 ++++++++++++------------
 unpack-trees.c                           | 11 +++++++++++
 2 files changed, 23 insertions(+), 12 deletions(-)

Comments

Elijah Newren July 22, 2021, 4:19 a.m. UTC | #1

On Tue, Jul 20, 2021 at 1:14 PM Derrick Stolee via GitGitGadget
<[e-mail]> wrote:
>
> From: Derrick Stolee <[e-mail]>
>
> When running unpack_trees() with a sparse index, we attempt to operate
> on the index without expanding the sparse directory entries. Thus, we
> operate by manipulating entire directories and passing them to the
> unpack function. In the case of the 'git checkout' command, this is the
> twoway_merge() function.
>
> There are several cases in twoway_merge() that handle different
> situations. One new one to add is the case of a directory/file conflict
> where the directory is sparse. Before the sparse index, such a conflict
> would appear as a list of file additions and deletions. Now,
> twoway_merge() initializes 'current', 'oldtree', and 'newtree' from
> src[0], src[1], and src[2], then sets 'oldtree' to NULL because it is
> equal to the df_conflict_entry. The way to determine that we have a
> directory/file conflict is to test that 'current' and 'newtree' disagree
> on being sparse directory entries.
>
> When we are in this case, we want to resolve the situation by calling
> merged_entry(). This allows replacing the 'current' entry with the
> 'newtree' entry. This is important for cases where we want to run 'git
> checkout' across the conflict and have the new HEAD represent the new
> file type at that path. The first NEEDSWORK comment dropped in t1092
> demonstrates this necessary behavior.
>
> However, we still are in a confusing state when 'current' corresponds to
> a staged change within a sparse directory that is not present at HEAD.
> This should be atypical, because it requires adding a change outside of
> the sparse-checkout cone, but it is possible. Since we are unable to
> determine that this is a staged change within twoway_merge(), we cannot
> add a case to reject the merge at this point. I believe this is due to
> the use of df_conflict_entry in the place of 'oldtree' instead of using
> the valud at HEAD, which would provide some perspective to this
> decision. Any change that would allow this differentiation for staged
> entries would need to involve information further up in unpack_trees().
>
> That work should be done, sometime, because we are further confusing the
> behavior of a directory/file conflict when staging a change in the
> directory. The two cases 'checkout behaves oddly with df-conflict-?' in
> t1092 demonstrate that even without a sparse-checkout, Git is not
> consistent in its behavior. Neither of the two options seems correct,
> either. This change makes the sparse-index behave differently than the
> typcial sparse-checkout case, but it does match the full checkout
> behavior in the df-conflict-2 case.
>
> Signed-off-by: Derrick Stolee <[e-mail]>
> ---
>  t/t1092-sparse-checkout-compatibility.sh | 24 ++++++++++++------------
>  unpack-trees.c                           | 11 +++++++++++
>  2 files changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 79b4a8ce199..91e30d6ec22 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -396,14 +396,14 @@ test_expect_success 'diff with renames and conflicts' '
>         done
>  '
>
> -# NEEDSWORK: the sparse-index fails to move HEAD across a directory/file
> -# conflict such as when checking out df-conflict-1 and df-conflict2.
>  test_expect_success 'diff with directory/file conflicts' '
>         init_repos &&
>
>         for branch in rename-out-to-out \
>                       rename-out-to-in \
>                       rename-in-to-out \
> +                     df-conflict-1 \
> +                     df-conflict-2 \
>                       fd-conflict
>         do
>                 git -C full-checkout reset --hard &&
> @@ -667,10 +667,7 @@ test_expect_success 'checkout behaves oddly with df-conflict-1' '
>         git -C sparse-checkout checkout df-conflict-1 \
>                 1>sparse-checkout-out \
>                 2>sparse-checkout-err &&
> -
> -       # NEEDSWORK: the sparse-index case refuses to change HEAD here,
> -       # but for the wrong reason.
> -       test_must_fail git -C sparse-index checkout df-conflict-1 \
> +       git -C sparse-index checkout df-conflict-1 \
>                 1>sparse-index-out \
>                 2>sparse-index-err &&
>
> @@ -684,7 +681,11 @@ test_expect_success 'checkout behaves oddly with df-conflict-1' '
>         test_cmp expect full-checkout-out &&
>         test_cmp expect sparse-checkout-out &&
>
> +       # The sparse-index reports no output
> +       test_must_be_empty sparse-index-out &&
> +
>         # stderr: Switched to branch df-conflict-1
> +       test_cmp full-checkout-err sparse-checkout-err &&
>         test_cmp full-checkout-err sparse-checkout-err
>  '
>
> @@ -719,17 +720,15 @@ test_expect_success 'checkout behaves oddly with df-conflict-2' '
>         git -C sparse-checkout checkout df-conflict-2 \
>                 1>sparse-checkout-out \
>                 2>sparse-checkout-err &&
> -
> -       # NEEDSWORK: the sparse-index case refuses to change HEAD
> -       # here, but for the wrong reason.
> -       test_must_fail git -C sparse-index checkout df-conflict-2 \
> +       git -C sparse-index checkout df-conflict-2 \
>                 1>sparse-index-out \
>                 2>sparse-index-err &&
>
>         # The full checkout deviates from the df-conflict-1 case here!
>         # It drops the change to folder1/larger-content and leaves the
> -       # folder1 path as-is on disk.
> +       # folder1 path as-is on disk. The sparse-index behaves the same.
>         test_must_be_empty full-checkout-out &&
> +       test_must_be_empty sparse-index-out &&
>
>         # In the sparse-checkout case, the checkout deletes the folder1
>         # file and adds the folder1/larger-content file, leaving all other
> @@ -741,7 +740,8 @@ test_expect_success 'checkout behaves oddly with df-conflict-2' '
>         test_cmp expect sparse-checkout-out &&
>
>         # Switched to branch df-conflict-1
> -       test_cmp full-checkout-err sparse-checkout-err
> +       test_cmp full-checkout-err sparse-checkout-err &&
> +       test_cmp full-checkout-err sparse-index-err
>  '
>
>  test_done
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 0a5135ab397..309c1352f5d 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -2619,6 +2619,17 @@ int twoway_merge(const struct cache_entry * const *src,
>                          same(current, oldtree) && !same(current, newtree)) {
>                         /* 20 or 21 */
>                         return merged_entry(newtree, current, o);
> +               } else if (current && !oldtree && newtree &&
> +                          S_ISSPARSEDIR(current->ce_mode) != S_ISSPARSEDIR(newtree->ce_mode) &&
> +                          ce_stage(current) == 0) {
> +                       /*
> +                        * This case is a directory/file conflict across the sparse-index
> +                        * boundary. When we are changing from one path to another via
> +                        * 'git checkout', then we want to replace one entry with another
> +                        * via merged_entry(). If there are staged changes, then we should
> +                        * reject the merge instead.
> +                        */
> +                       return merged_entry(newtree, current, o);
>                 } else
>                         return reject_merge(current, o);
>         }
> --

I'm still a bit unhappy with the unpack-trees.c change (I wonder if
having "path/" vs "path" is going to make D/F conflicts hard to handle
and whether we need to make unpack_trees do something special to make
both paths be considered at the same time with one call to
twoway_merge() instead of two in order to fix this), BUT I think
you've done a really good job of documenting it and pointing out that
unpack_trees() messes up even without sparse checkouts on D/F
conflicts (though in a different way).  I think you've documented it
well enough, and argued about the likelihood of issues well enough,
that it makes sense to proceed and circle back and fix this up later.

Patch

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 79b4a8ce199..91e30d6ec22 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -396,14 +396,14 @@  test_expect_success 'diff with renames and conflicts' '
 	done
 '
 
-# NEEDSWORK: the sparse-index fails to move HEAD across a directory/file
-# conflict such as when checking out df-conflict-1 and df-conflict2.
 test_expect_success 'diff with directory/file conflicts' '
 	init_repos &&
 
 	for branch in rename-out-to-out \
 		      rename-out-to-in \
 		      rename-in-to-out \
+		      df-conflict-1 \
+		      df-conflict-2 \
 		      fd-conflict
 	do
 		git -C full-checkout reset --hard &&
@@ -667,10 +667,7 @@  test_expect_success 'checkout behaves oddly with df-conflict-1' '
 	git -C sparse-checkout checkout df-conflict-1 \
 		1>sparse-checkout-out \
 		2>sparse-checkout-err &&
-
-	# NEEDSWORK: the sparse-index case refuses to change HEAD here,
-	# but for the wrong reason.
-	test_must_fail git -C sparse-index checkout df-conflict-1 \
+	git -C sparse-index checkout df-conflict-1 \
 		1>sparse-index-out \
 		2>sparse-index-err &&
 
@@ -684,7 +681,11 @@  test_expect_success 'checkout behaves oddly with df-conflict-1' '
 	test_cmp expect full-checkout-out &&
 	test_cmp expect sparse-checkout-out &&
 
+	# The sparse-index reports no output
+	test_must_be_empty sparse-index-out &&
+
 	# stderr: Switched to branch df-conflict-1
+	test_cmp full-checkout-err sparse-checkout-err &&
 	test_cmp full-checkout-err sparse-checkout-err
 '
 
@@ -719,17 +720,15 @@  test_expect_success 'checkout behaves oddly with df-conflict-2' '
 	git -C sparse-checkout checkout df-conflict-2 \
 		1>sparse-checkout-out \
 		2>sparse-checkout-err &&
-
-	# NEEDSWORK: the sparse-index case refuses to change HEAD
-	# here, but for the wrong reason.
-	test_must_fail git -C sparse-index checkout df-conflict-2 \
+	git -C sparse-index checkout df-conflict-2 \
 		1>sparse-index-out \
 		2>sparse-index-err &&
 
 	# The full checkout deviates from the df-conflict-1 case here!
 	# It drops the change to folder1/larger-content and leaves the
-	# folder1 path as-is on disk.
+	# folder1 path as-is on disk. The sparse-index behaves the same.
 	test_must_be_empty full-checkout-out &&
+	test_must_be_empty sparse-index-out &&
 
 	# In the sparse-checkout case, the checkout deletes the folder1
 	# file and adds the folder1/larger-content file, leaving all other
@@ -741,7 +740,8 @@  test_expect_success 'checkout behaves oddly with df-conflict-2' '
 	test_cmp expect sparse-checkout-out &&
 
 	# Switched to branch df-conflict-1
-	test_cmp full-checkout-err sparse-checkout-err
+	test_cmp full-checkout-err sparse-checkout-err &&
+	test_cmp full-checkout-err sparse-index-err
 '
 
 test_done
diff --git a/unpack-trees.c b/unpack-trees.c
index 0a5135ab397..309c1352f5d 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2619,6 +2619,17 @@  int twoway_merge(const struct cache_entry * const *src,
 			 same(current, oldtree) && !same(current, newtree)) {
 			/* 20 or 21 */
 			return merged_entry(newtree, current, o);
+		} else if (current && !oldtree && newtree &&
+			   S_ISSPARSEDIR(current->ce_mode) != S_ISSPARSEDIR(newtree->ce_mode) &&
+			   ce_stage(current) == 0) {
+			/*
+			 * This case is a directory/file conflict across the sparse-index
+			 * boundary. When we are changing from one path to another via
+			 * 'git checkout', then we want to replace one entry with another
+			 * via merged_entry(). If there are staged changes, then we should
+			 * reject the merge instead.
+			 */
+			return merged_entry(newtree, current, o);
 		} else
 			return reject_merge(current, o);
 	}