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: >> ... > I've read over these patches and didn't find any problems in them in > my reading; however, since it builds upon ds/status-with-sparse-index > (v7)... > > I decided to retry some of my ideas and testing on Patch 10/16 of v7, > over at https://lore.kernel.org/git/CABPp-BHwTAKwFiWQ0-2P=_g+7HLK5FfOAz-uujRjLou1fXT3zw@xxxxxxxxxxxxxx/ > > It turns out that the block you added there is now triggered by t1092 > after this series, and the testcase won't pass without that block. It > might be clearer to move that code fragment, or perhaps the whole > patch, into this series...though the code fragment as is has > introduced a bug. If you take t1092 test 12 ("diff with > directory/file conflicts") and modify it so that before the > > git checkout df-conflict > > invocation from sparse-index, you first run: > > $ git sparse-checkout disable > $ echo more stuff >>folder1/edited-content > $ git add -u > $ git diff HEAD # note the changes > $ git sparse-checkout init --cone --sparse-index > $ git diff HEAD # note the changes are still there > $ git checkout df-conflict # no error?? What about the > conflicting changes? > $ git diff HEAD > > then the last command will show that the staged changes from before > the commit have simply been discarded. In short, this makes the > series behave like --force was passed with sparse directory entries, > when --force wasn't passed. > > So we've still got some directory/file conflict issues. You are absolutely right that this seems strange. In fact, there is a behavior change during 'git checkout' for sparse-checkouts in general, but also my sparse-index change creates an additional change in this case. Here is a test that demonstrates the issue: test_expect_success 'staged directory/file conflict' ' init_repos && test_sparse_match git sparse-checkout disable && write_script edit-contents <<-\EOF && echo text >>folder1/edited-content EOF run_on_all ../edit-contents && test_all_match git add folder1/edited-content && test_all_match git diff HEAD && git -C sparse-checkout sparse-checkout init --cone --no-sparse-index && git -C sparse-index sparse-checkout init --cone --sparse-index && test_all_match git diff HEAD && # Sparse-checkouts handle this conflict differently than # full checkouts, as they consider the file "folder1" to # be deleted in favor of the staged file # "folder1/edited-content". test_sparse_match git checkout df-conflict && test_sparse_match git diff HEAD ' The sparse-index case drops all staged changes during the 'git checkout df-conflict' command, so the test fails on that line. That final diff looks like this in the sparse-checkout repo (no sparse index): diff --git a/folder1 b/folder1 deleted file mode 100644 index d95f3ad..0000000 --- a/folder1 +++ /dev/null @@ -1 +0,0 @@ -content diff --git a/folder1/edited-content b/folder1/edited-content new file mode 100644 index 0000000..8e27be7 --- /dev/null +++ b/folder1/edited-content @@ -0,0 +1 @@ +text 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. Clearly, the resolution that was presented in the previous patch was incorrect so I will try to understand this situation better. 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. Thanks, -Stolee
