Re: [PATCH v7 10/16] unpack-trees: handle dir/file conflict of sparse entries

On Wed, Jul 7, 2021 at 4:19 PM Elijah Newren [off-list ref] wrote:

On Mon, Jun 28, 2021 at 7:05 PM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

From: Derrick Stolee <redacted>

Signed-off-by: Derrick Stolee <redacted>
---
 t/t1092-sparse-checkout-compatibility.sh | 25 ++++++++++++++++++++++--
 unpack-trees.c                           |  5 ++++-
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 3f61e5686b5..4e6446e7545 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh

@@ -95,6 +95,19 @@ test_expect_success 'setup' '
                git add . &&
                git commit -m "rename deep/deeper1/... to folder1/..." &&

+               git checkout -b df-conflict base &&
+               rm -rf folder1 &&
+               echo content >folder1 &&
+               git add . &&
+               git commit -m df &&
+
+               git checkout -b fd-conflict base &&
+               rm a &&
+               mkdir a &&
+               echo content >a/a &&
+               git add . &&
+               git commit -m fd &&
+
                git checkout -b deepest base &&
                echo "updated deepest" >deep/deeper1/deepest/a &&
                git commit -a -m "update deepest" &&

@@ -325,7 +338,11 @@ test_expect_success 'diff --staged' '
 test_expect_success 'diff with renames and conflicts' '
        init_repos &&

-       for branch in rename-out-to-out rename-out-to-in rename-in-to-out
+       for branch in rename-out-to-out \
+                     rename-out-to-in \
+                     rename-in-to-out \
+                     df-conflict \
+                     fd-conflict
        do
                test_all_match git checkout rename-base &&
                test_all_match git checkout $branch -- .&&

@@ -338,7 +355,11 @@ test_expect_success 'diff with renames and conflicts' '
 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
+       for branch in rename-out-to-out \
+                     rename-out-to-in \
+                     rename-in-to-out \
+                     df-conflict \
+                     fd-conflict
        do
                git -C full-checkout reset --hard &&
                test_sparse_match git reset --hard &&

Tests look good...

quoted

diff --git a/unpack-trees.c b/unpack-trees.c
index d141dffbd94..e63b2dcacbc 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c

@@ -2617,7 +2617,10 @@ 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
+               } else if (current && !oldtree && newtree &&
+                          S_ISSPARSEDIR(current->ce_mode) != S_ISSPARSEDIR(newtree->ce_mode))
+                       return merged_entry(newtree, current, o);
+               else
                        return reject_merge(current, o);
        }
        else if (newtree) {

t1092 still passes if you replace the
    return merged_entry(newtree, current, o);
line with
    die("This line is never hit.");

Is it possible that you thought you needed this block but further
refactoring removed the need?  Or that it is only needed by the later
ds/commit-and-checkout-with-sparse-index topic (which I haven't yet
reviewed, because I was reviewing this topic first)?  It seems this
code change should either be dropped, or moved out to the relevant
series that uses it.

This seems wrong to me but I'm having a hard time nailing down a
testcase to prove it.  The logic looks to me like "if the old tree as
nothing in the index at the given path, and the newtree has something,
and the index had something staged, but the newtree and staged index
entry disagree on the type of the object, do some weird merged_entry()
logic on both types of trees that tends to just take the newer I
thought but who knows what functions like verify_uptodate(entry) do
when entry is a sparse directory...".

So, I'm not so sure about this.  Could you explain this a bit more?

However, I did find a testcase that aborts with a fatal error...though
I can't tell if it's even triggering the above logic; I think it isn't
because I have an "ignoreme" on both sides of the history.  Here's the
testcase:

# Make a little test repo
git init dumb
cd dumb

# Setup old commit
touch tracked
echo foo >ignoreme
git add .
git commit -m "Initial"
git branch orig

# Setup new commit
git rm ignoreme
mkdir ignoreme
touch ignoreme/file
git add ignoreme/file
git commit -m "whatever"

# Switch to old commit
git checkout orig

# Make index != new (and index != old)
git rm ignoreme
mkdir ignoreme
echo user-data >ignoreme/file
git add ignoreme/file

# Sparsify
GIT_TEST_SPARSE_INDEX=0 # GIT_TEST_SPARSE_INDEX is documented as a boolean;
                        # but the traditional boolean value is ignored and it
                        # really only cares about set/unset.  Confusing.
git sparse-checkout init --cone --sparse-index
git sparse-checkout set tracked

# Check status and dirs/paths in index
git status --porcelain
test-tool read-cache --table
test-tool read-cache --table --expand

# Run a command that aborts with a fatal error
git checkout -m master

It turns out that this testcase I provided still triggers the same
fatal error if you omit the --sparse-index flag, so it's not a
sparse-index-specific bug.

So, perhaps it shouldn't hold up this series, but given that a lot of
your correctness verification in t1092 relies on comparisons between
sparse checkouts and sparse indexes, it might be worth trying to get
to the root of this.