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@xxxxxxxxxx> wrote:
> >
> > Signed-off-by: Brandon Williams <bmwill@xxxxxxxxxx>
> > ---
> > 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