On 7/8/2021 7:51 PM, Elijah Newren wrote:
> On Thu, Jul 8, 2021 at 4:10 PM Elijah Newren <newren@xxxxxxxxx> 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
