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

From: Derrick Stolee <stolee@gmail.com>
Date: Jul 16, 2021, 13:59 UTC
Message-ID: <c127ceed-10fd-9ad0-e858-db79bec0cf8d@gmail.com>
In-Reply-To: <b362c428-eec9-39e3-55a0-0738431e1d98@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:
>>>

...

Show 11 quoted lines

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

Show 6 quoted lines

> 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