Re: [PATCH 30/33] tree-diff: convert diff_tree_paths to struct object_id

[Jeff King <peff@peff.net>]

On Wed, May 31, 2017 at 11:24:33AM -0700, Stefan Beller wrote:

> On Tue, May 30, 2017 at 10:31 AM, Brandon Williams <bmwill@google.com> wrote:
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> >  combine-diff.c | 10 +++++-----
> >  diff.h         |  4 ++--
> >  tree-diff.c    | 63 +++++++++++++++++++++++++++++-----------------------------
> >  3 files changed, 39 insertions(+), 38 deletions(-)
> >
> > diff --git a/combine-diff.c b/combine-diff.c
> > index 04c4ae856..ec9d93044 100644
> > --- a/combine-diff.c
> > +++ b/combine-diff.c
> > @@ -1364,22 +1364,22 @@ static struct combine_diff_path *find_paths_multitree(
> >         struct diff_options *opt)
> >  {
> >         int i, nparent = parents->nr;
> > -       const unsigned char **parents_sha1;
> > +       const struct object_id **parents_oid;
> >         struct combine_diff_path paths_head;
> >         struct strbuf base;
> >
> > -       ALLOC_ARRAY(parents_sha1, nparent);
> > +       ALLOC_ARRAY(parents_oid, nparent);
> >         for (i = 0; i < nparent; i++)
> > -               parents_sha1[i] = parents->oid[i].hash;
> > +               parents_oid[i] = &parents->oid[i];
> 
> I have the impression that we could get away with one layer less
> of indirection. Previously we had a heap allocated array (*) of (char*),
> now we'd have a an array (*) of pointers(*) of the oid struct, that
> is a (char[]) essentially. Maybe I am just confused?

I don't think so. We always could have allocated the original as an
array of 20-byte chunks. It's syntactically less awkward in C when that
20-byte chunk is wrapped in a struct like object_id. But fundamentally
I think we don't need to be making a copy of the oid. We're pointing to
the existing copy in the "parents" array.

Or did you mean that diff_tree_paths() could now take an actual
array-of-struct rather than an array-of-pointer-to-struct? That would
drop the "parents_oid" array entirely. I think that's actually
orthogonal to this change (the same could have been done with the
original sha1 array), but would be a nice cleanup on top.

-Peff