Subject: Re: [PATCH v7 12/16] diff-lib: handle index diffs with sparse dirs

From: Derrick Stolee <[email-masked]>
Date: Mon, 12 Jul 2021 09:52:41 -0400

On 7/8/2021 7:51 PM, Elijah Newren wrote:
> On Thu, Jul 8, 2021 at 4:10 PM Elijah Newren <[email-masked]> wrote:
>>
>> On Mon, Jun 28, 2021 at 7:05 PM Derrick Stolee via GitGitGadget
...
>>> +       /*
>>> +        * 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)) {
>>> +               diff_tree_oid(&old_entry->oid, &new_entry->oid, new_entry->name, &revs->diffopt);
>>> +               return 0;
>>> +       }
>>> +
>>>         if (get_stat_data(istate, new_entry, &oid, &mode, cached, match_missing,
>>>                           &dirty_submodule, &revs->diffopt) < 0) {
>>>                 if (report_missing)
>>
>> Love the simpler patch.
>>
>> I'm curious about the case where S_ISSPARSEDIR(old_entry->ce_mode) !=
>> S_ISSPARSEDIR(new_entry->ce_mode), though; how is that handled?
> 
> Digging a little deeper, it appears that we could add this just before
> your new if-block:
> 
>     assert(S_ISSPARSEDIR(old_entry->ce_mode) ==
>            S_ISSPARSEDIR(new_entry->ce_mode));
> 
> And the code still functions, while that also removes some of the
> surprise factor.  I'm guessing that the difference between "folder1"
> and "folder1/" cause us to never try to directly compare a file to a
> directory...but if that's accurate, a comment of some effect might
> help make this code be a little clearer and make readers less likely
> to wonder why you need to check that both old and new are sparse
> directories.

I was surprised that this worked, because my patch conditioned on
old_entry and new_entry being non-NULL. But of course show_modified()
requires them to be non-NULL. That can be further simplified.

Adding the assert helps demonstrate this expectation, but also I will
expand upon the comment.

Thanks,
-Stolee