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

[Derrick Stolee <stolee@gmail.com>]

On 7/17/2021 11:37 AM, Elijah Newren wrote:
> On Fri, Jul 16, 2021 at 6:59 AM Derrick Stolee <stolee@gmail.com> 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@gmail.com> 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