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

On 7/8/2021 7:51 PM, Elijah Newren wrote:

On Thu, Jul 8, 2021 at 4:10 PM Elijah Newren [off-list ref] wrote:

quoted

On Mon, Jun 28, 2021 at 7:05 PM Derrick Stolee via GitGitGadget

...

quoted

quoted

+       /*
+        * 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