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

[Derrick Stolee <stolee@gmail.com>]

On 7/8/2021 8:58 PM, Elijah Newren wrote:
> On Wed, Jul 7, 2021 at 4:19 PM Elijah Newren <newren@gmail.com> wrote:
>>
>> On Mon, Jun 28, 2021 at 7:05 PM Derrick Stolee via GitGitGadget
>> <gitgitgadget@gmail.com> wrote:
...
>>> +               } 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.

I have been working with the whole stack of patches (including the
ones that update 'git add') and trying to make the necessary changes
in this series, especially when updating the test data shape or
modifying methods that were necessary for 'git status'.

It is likely I was just overcautious thinking this was necessary so
early. It might also have been necessary due to some strange case
that is only exposed in a Scalar functional test. I'll try moving it
to the next series and double check those tests.

>> 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?

The most important point is that 'current' and 'newtree' are both
present but have different types (blob and tree) and the tree is
necessarily at the edge of the sparse-checkout cone. In the cases
where I was able to trigger this logic in the debugger, 'oldtree'
was NULL, so I added as a condition to be extra cautious around
unexpected initialization. It is possible that the '!oldtree'
condition is unnecessary. I will investigate.

As for verify_uptodate(), it ignores the sparse directory unless
o->skip_sparse_checkout is set. I wonder if it is possible to
have that set and hit this case.

>> 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:
>>
...
>> # Run a command that aborts with a fatal error
>> git checkout -m master

Thank you for finding an interesting test!

> 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.
 
I have some upcoming thoughts on changing how sparse-checkout works
for other complicated cases, so I will add this to that pile.

Thanks,
-Stolee