On Thu, Apr 13, 2017 at 04:11:31PM -0700, Junio C Hamano wrote: > [email protected] writes: > > > + /* > > + * Fetch the tree from the ODB for each peer directory in the > > + * n commits. > > + * > > + * For 2- and 3-way traversals, we try to avoid hitting the > > + * ODB twice for the same OID. This should yield a nice speed > > + * up in checkouts and merges when the commits are similar. > > + * > > + * We don't bother doing the full O(n^2) search for larger n, > > + * because wider traversals don't happen that often and we > > + * avoid the search setup. > > + * > > + * When 2 peer OIDs are the same, we just copy the tree > > + * descriptor data. This implicitly borrows the buffer > > + * data from the earlier cell. > > This "borrowing" made me worried, but it turns out that this is > perfectly OK. > > fill_tree_descriptor() uses read_sha1_file() to give a tree_desc its > own copy of the tree object data, so the code that calls into the > tree traversal API is responsible for freeing the buffer returned > from fill_tree_descriptor(). The updated code avoids double freeing > by setting buf[i] to NULL for borrowing [i].
Yeah, I think that logic is fine. We could also keep a separate counter for the size of buf, like: buf[nr_buf++] = fill_tree_descriptor(t+i, sha1); and then just count from zero to nr_buf in the freeing loop. I don't think it matters much either way (the free(NULL) calls are presumably cheap). -Peff
