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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jun 28, 2021 at 7:05 PM Derrick Stolee via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
>
> From: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
>
> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
> ---
>  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...

> 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) {

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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux