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@xxxxxxxxx> 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
