On Fri, Jul 16, 2021 at 6:59 AM Derrick Stolee <stolee@xxxxxxxxx> wrote: > > 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: > >>> > ... > > 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. I was not able to reproduce. Do you have other modifications to git, or is there some other special setup required to trigger the bug that I am missing in reading the paragraph above? Here's what I see: <Add an "exit 1 &&" right after "init_repos &&" in the 'diff with directory/file conflicts' test, run until first failure, then: $ cd trash directory.t1092-sparse-checkout-compatibility/full-checkout $ git reset --hard $ git checkout rename-in-to-out $ echo more stuff >>folder1/edited-content $ git add -u $ git checkout df-conflict error: Your local changes to the following files would be overwritten by checkout: folder1/edited-content Please commit your changes or stash them before you switch branches. Aborting This looks like the expected behavior to me, and is what I'd also expect from the sparse-checkout and sparse-index cases. > 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). Yes, rejecting the merge is the correct behavior. This is implied by the existing documentation for both the --merge and --force options to checkout. > 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? I only dug in and found the sparse-checkout/sparse-index bugs because the D/F changes you made to twoway_merge() looked clearly wrong to me and I was trying to find a case that would demonstrate it and make it easier for you to fix up. I still think the patch is wrong and that it adds a bug. If you can drop that patch, and still get correct behavior in your tests, then I think we can ignore other bugs in this area, but I'm not happy with that particular patch. If you need that patch, then it needs to be corrected, which probably means figuring out all these bugs.
