On 7/17/2021 11:37 AM, Elijah Newren wrote: > 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 is fragile to the data shape in my test, so I'll be sure to include one in the next series version that demonstrates the change. >> 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. That's a good point. I reverted the patch and re-ran the test and found that actually the patch is necessary in order to match the _incorrect_ behavior. Without the patch, the sparse-index case (correctly) refuses to complete the checkout. I'll replace this patch with a test change that demonstrates these subtleties and marks them as NEEDSWORK. Thanks, -Stolee
