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
> <gitgitgadget@xxxxxxxxx> wrote:
> >
> > From: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
> >
> > While comparing an index to a tree, we may see a sparse directory entry.
> > In this case, we should compare that portion of the tree to the tree
> > represented by that entry. This could include a new tree which needs to
> > be expanded to a full list of added files. It could also include an
> > existing tree, in which case all of the changes inside are important to
> > describe, including the modifications, additions, and deletions. Note
> > that the case where the tree has a path and the index does not remains
> > identical to before: the lack of a cache entry is the same with a sparse
> > index.
> >
> > Use diff_tree_oid() appropriately to compute the diff.
> >
> > Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
> > ---
> > diff-lib.c | 16 ++++++++++++++++
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/diff-lib.c b/diff-lib.c
> > index c2ac9250fe9..3f32f038371 100644
> > --- a/diff-lib.c
> > +++ b/diff-lib.c
> > @@ -325,6 +325,11 @@ static void show_new_file(struct rev_info *revs,
> > unsigned dirty_submodule = 0;
> > struct index_state *istate = revs->diffopt.repo->index;
> >
> > + if (new_file && S_ISSPARSEDIR(new_file->ce_mode)) {
> > + diff_tree_oid(NULL, &new_file->oid, new_file->name, &revs->diffopt);
> > + return;
> > + }
> > +
> > /*
> > * New file in the index: it might actually be different in
> > * the working tree.
> > @@ -347,6 +352,17 @@ static int show_modified(struct rev_info *revs,
> > unsigned dirty_submodule = 0;
> > struct index_state *istate = revs->diffopt.repo->index;
> >
> > + /*
> > + * 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.
