unpack-trees: resolve sparse-directory/file conflicts

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 <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Derrick Stolee committed Jul 20, 2021 at 20:14 UTC e05cdb17e89a2d3257533d47350b3138bfce8737
2 files changed +23 -12
t/t1092-sparse-checkout-compatibility.sh
+12 -12
@@ -396,14 +396,14 @@ test_expect_success 'diff with renames and conflicts' '
396 done
397 '
398
399 -# NEEDSWORK: the sparse-index fails to move HEAD across a directory/file
400 -# conflict such as when checking out df-conflict-1 and df-conflict2.
399 test_expect_success 'diff with directory/file conflicts' '
400 init_repos &&
401
402 for branch in rename-out-to-out \
403 rename-out-to-in \
404 rename-in-to-out \
405 + df-conflict-1 \
406 + df-conflict-2 \
407 fd-conflict
408 do
409 git -C full-checkout reset --hard &&
@@ -667,10 +667,7 @@ test_expect_success 'checkout behaves oddly with df-conflict-1' '
667 git -C sparse-checkout checkout df-conflict-1 \
668 1>sparse-checkout-out \
669 2>sparse-checkout-err &&
670 -
671 - # NEEDSWORK: the sparse-index case refuses to change HEAD here,
672 - # but for the wrong reason.
673 - test_must_fail git -C sparse-index checkout df-conflict-1 \
670 + git -C sparse-index checkout df-conflict-1 \
671 1>sparse-index-out \
672 2>sparse-index-err &&
673
@@ -684,7 +681,11 @@ test_expect_success 'checkout behaves oddly with df-conflict-1' '
681 test_cmp expect full-checkout-out &&
682 test_cmp expect sparse-checkout-out &&
683
684 + # The sparse-index reports no output
685 + test_must_be_empty sparse-index-out &&
686 +
687 # stderr: Switched to branch df-conflict-1
688 + test_cmp full-checkout-err sparse-checkout-err &&
689 test_cmp full-checkout-err sparse-checkout-err
690 '
691
@@ -719,17 +720,15 @@ test_expect_success 'checkout behaves oddly with df-conflict-2' '
720 git -C sparse-checkout checkout df-conflict-2 \
721 1>sparse-checkout-out \
722 2>sparse-checkout-err &&
722 -
723 - # NEEDSWORK: the sparse-index case refuses to change HEAD
724 - # here, but for the wrong reason.
725 - test_must_fail git -C sparse-index checkout df-conflict-2 \
723 + git -C sparse-index checkout df-conflict-2 \
724 1>sparse-index-out \
725 2>sparse-index-err &&
726
727 # The full checkout deviates from the df-conflict-1 case here!
728 # It drops the change to folder1/larger-content and leaves the
731 - # folder1 path as-is on disk.
729 + # folder1 path as-is on disk. The sparse-index behaves the same.
730 test_must_be_empty full-checkout-out &&
731 + test_must_be_empty sparse-index-out &&
732
733 # In the sparse-checkout case, the checkout deletes the folder1
734 # 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' '
740 test_cmp expect sparse-checkout-out &&
741
742 # Switched to branch df-conflict-1
744 - test_cmp full-checkout-err sparse-checkout-err
743 + test_cmp full-checkout-err sparse-checkout-err &&
744 + test_cmp full-checkout-err sparse-index-err
745 '
746
747 test_done
unpack-trees.c
+11
@@ -2619,6 +2619,17 @@ int twoway_merge(const struct cache_entry * const *src,
2619 same(current, oldtree) && !same(current, newtree)) {
2620 /* 20 or 21 */
2621 return merged_entry(newtree, current, o);
2622 + } else if (current && !oldtree && newtree &&
2623 + S_ISSPARSEDIR(current->ce_mode) != S_ISSPARSEDIR(newtree->ce_mode) &&
2624 + ce_stage(current) == 0) {
2625 + /*
2626 + * This case is a directory/file conflict across the sparse-index
2627 + * boundary. When we are changing from one path to another via
2628 + * 'git checkout', then we want to replace one entry with another
2629 + * via merged_entry(). If there are staged changes, then we should
2630 + * reject the merge instead.
2631 + */
2632 + return merged_entry(newtree, current, o);
2633 } else
2634 return reject_merge(current, o);
2635 }