Re: [PATCH v5 10/14] diff-lib: handle index diffs with sparse dirs

From: Derrick Stolee <stolee@gmail.com>
Date: Jun 11, 2021, 17:27 UTC
Message-ID: <2784d29b-b22a-2bf6-2450-7b4a0a72df54@gmail.com>
In-Reply-To: <7d6ca59b-2713-5c10-88de-963efc7779fc@gmail.com>

On 6/11/2021 8:57 AM, Derrick Stolee wrote:

Show 7 quoted lines

> On 6/10/2021 5:31 PM, Elijah Newren wrote:
>> On Thu, Jun 10, 2021 at 10:45 AM Derrick Stolee <stolee@gmail.com> wrote:
>>>
>>> I'll send a new version with this patch tomorrow, as well as the
>>> other recommended edits.
> 
> ...still planning on this today, but...

So optimistic!

Show 21 quoted lines

>>> +       /*
>>> +        * If both are sparse directory entries, then expand the
>>> +        * modifications to the file level.
>>> +        */
>>> +       if (old_entry && new_entry &&
>>> +           S_ISSPARSEDIR(old_entry->ce_mode) &&
>>> +           S_ISSPARSEDIR(new_entry->ce_mode)) {
>>> +               show_modified_sparse_directory(revs, old_entry, new_entry, report_missing, cached, match_missing);
>>> +               return 0;
>>> +       }
>>
>> What if S_ISSPARSEDIR(old_entry->ce_mode) != S_ISSPARSEDIR(new_entry->ce_mode) ?
> 
> You make a good point that something different would happen
> in the case of a directory/file conflict on the sparse checkout
> boundary. This can be as simple as the trivial "only files at
> root" cone-mode sparse-checkout definition, with "folder/" (tree)
> changing to "folder" (blob).
> 
> I'll see what I can do to create a test scenario for
> this and add the correct cases.

Creating a directory/file conflict in this way exposes a bug in a different codepath in unpack_trees(), although it isn't visible until 'git checkout' allows the index to stay sparse. It's due to the code in unpack_callback() that handles blobs and trees differently, and hence the blob/tree conflict isn't handled appropriately there. The changes from Patch 8 are to blame for these first errors.

At least, those are the first errors I have discovered with these conflicts. There might be other scenarios that care about this section of diff-lib.c, but I have not gotten to a point where such behavior would be exposed.

I don't expect to succeed in squashing this bug today, so I'll try again next week.

Thanks, -Stolee