Re: [PATCH v3 2/4] unpack-trees: optimize walking same trees with cache-tree

> > diff --git a/unpack-trees.c b/unpack-trees.c
> > index a32ddee159..ba3d2e947e 100644
> > --- a/unpack-trees.c
> > +++ b/unpack-trees.c
> > @@ -644,6 +644,102 @@ static inline int are_same_oid(struct name_entry *name_j, struct name_entry *nam
> >         return name_j->oid && name_k->oid && !oidcmp(name_j->oid, name_k->oid);
> >  }
> >
> > +static int all_trees_same_as_cache_tree(int n, unsigned long dirmask,
> > +                                       struct name_entry *names,
> > +                                       struct traverse_info *info)
> > +{
> > +       struct unpack_trees_options *o = info->data;
> > +       int i;
> > +
> > +       if (!o->merge || dirmask != ((1 << n) - 1))
> > +               return 0;
> > +
> > +       for (i = 1; i < n; i++)
> > +               if (!are_same_oid(names, names + i))
> > +                       return 0;
> > +
> > +       return cache_tree_matches_traversal(o->src_index->cache_tree, names, info);
> > +}
>
> I was curious whether this could also be extended in the case of a
> merge; as long as HEAD and MERGE have the same tree, even if the base
> commit doesn't match, we can still just use the tree from HEAD which
> should be in the current index/cache_tree.  However, it'd be a
> somewhat odd history for HEAD and MERGE to match on some significantly
> sized tree when the base commit doesn't also match.

> > +       /*
> > +        * Do what unpack_callback() and unpack_nondirectories() normally
> > +        * do. But we walk all paths recursively in just one loop instead.
> > +        *
> > +        * D/F conflicts and staged entries are not a concern because
>
> "staged entries"?  Do you mean "higher stage entries"?  I'm not sure
> the correct terminology here, but the former makes me think of changes
> the user has staged but not committed (i.e. stuff found at stage #0 in
> the index, but which isn't found in any tree yet) vs. the latter which
> I'd use to refer to entries at stages 1 or higher.

> > +        * cache-tree would be invalidated and we would never get here
> > +        * in the first place.
> > +        */
> > +       for (i = 0; i < nr_entries; i++) {
> > +               struct cache_entry *tree_ce;
> > +               int len, rc;
> > +
> > +               src[0] = o->src_index->cache[pos + i];
> > +
> > +               len = ce_namelen(src[0]);
> > +               tree_ce = xcalloc(1, cache_entry_size(len));
> > +
> > +               tree_ce->ce_mode = src[0]->ce_mode;
> > +               tree_ce->ce_flags = create_ce_flags(0);
> > +               tree_ce->ce_namelen = len;
> > +               oidcpy(&tree_ce->oid, &src[0]->oid);
> > +               memcpy(tree_ce->name, src[0]->name, len + 1);
>
> We do a bunch of work to setup tree_ce...
>
> > +               for (d = 1; d <= nr_names; d++)
> > +                       src[d] = tree_ce;
>
> ...then we make nr_names copies of tree_ce (so that *way_merge or
> bind_merge or oneway_diff or whatever will have the expected number of
> entries).
>
> > +               rc = call_unpack_fn((const struct cache_entry * const *)src, o);
>
> ...then we call o->fn (via call_unpack_fn) to do various complicated
> logic to figure out which tree_ce to use??  Isn't that just an
> expensive way to recompute that what we currently have in the index is
> what we want to keep there?
>
> Granted, a caller of this may have set o->fn to something other than
> {one,two,three}way_merge (or bind_merge), and that function might have
> important side effects...but it just seems annoying to have to do so
> much work when for most uses we already know the entry in the index is
> the one we already want.

> In fact, the only other thing in the
> codebase that o->fn is now set to is oneway_diff, which I think is a
> no-op when the two trees match.
>
> Would be nice if we could avoid all this, at least in the common cases
> where o->fn is a function known to not have side effects.  Or did I
> not read those functions closely enough and they do have important
> side effects?