Re: [PATCH 0/5] Sparse index: integrate with commit and checkout

[Derrick Stolee <stolee@gmail.com>]

On 7/12/2021 2:46 PM, Derrick Stolee wrote:
> On 7/9/2021 5:26 PM, Elijah Newren wrote:
>> On Mon, Jun 28, 2021 at 7:13 PM Derrick Stolee via GitGitGadget
>> <gitgitgadget@gmail.com> wrote:
>>>
...
> This is a strange case in that we have a staged tree that is
> outside of the sparse-checkout cone. When running the 'git
> checkout df-conflict' command, the twoway_merge() method
> receives the following values:
> 
>  current: "folder1/" (tree OID)
>  oldtree: "" (NULL OID)
>  newtree: "folder1" (blob OID)
> 
> Is this value for 'oldtree' correct? It seems strange to me,
> so I'll look further into it.

This is correct. This 'oldtree' entry is actually the o->df_conflict
placeholder and is set to NULL inside the method.

> Further, I expect it to be simpler to modify the behavior
> here to match the full checkout case than to make the
> sparse-index case match the normal sparse-checkout case.
> The "natural" thing would be to keep the staged "folder1/"
> directory, but that would present as adding all contained
> content, not just the single staged entry.
Taking a closer look at the full checkout case, I discovered that the
'git checkout df-conflict' command succeeds in the full checkout case if I
apply it directly to the 'master' branch. In that situation, it completely
removes the staged change to folder1/edited-content! This seems like
incorrect behavior, and has nothing to do with the sparse-checkout feature.

It just happens that a sparse-checkout will have a _different_ kind of
incorrect behavior!

However, when adding the test on top of the ds/status-with-sparse-index
branch, the full checkout case matches the sparse-checkout! I bisected
this to the additions of files adjacent to folder1/ (folder1. folder1-,
etc) in e669ffb (t1092: expand repository data shape, 2021-07-14). If I
switch the test to conflict on folder2, then I get the strange behavior
that I was noticing on 'master'.

Some very subtle things are going on here, and they don't necessarily
involve the sparse index. Adding the sparse index to the mix creates a
third incorrect behavior to this already-broken case.

If we agree that the correct thing to do here is to reject the merge and
fail the command, then I can start working on making that change in
isolation (because _none_ of the existing behaviors are correct).

That leaves a question as to whether we should hold up this series for
that reason, or if I should pursue a fix to this kind of conflict as a
forward fix on top of it. What do you think, Elijah and Junio?

Thanks,
-Stolee