Re: [RFC 16/17] object_array_entry: copy name before storing in name field

On 05/20/2013 12:33 PM, Johan Herland wrote:

On Sun, May 19, 2013 at 10:27 PM, Michael Haggerty [off-list ref] wrote:

quoted

This is the culmination of the last few commits.  Since some callers
want to store refnames in the name field of object_array elements, but
we don't want those callers to assume that the refnames that they got
from for_each_ref() have infinite lifetime, the easiest thing to do is
have object_array make a copy of the names before writing them in the
entries, and to free the names for entries that are no longer in use.
This change fixes the problem, but has some disadvantages:

* It requires extra copies to be made of strings that are already
  copies, for example when the results of path_name(path, name) are
  used as a name in revision.c:add_object().  This might be rare
  enough that it can be ignored (though the original result of
  path_name() would have to be freed, which this patch doesn't do so
  there is a memory leak).

* Many callers store the empty string ("") as the name; for example,
  most of the entries created during a run of rev-list have "" as
  their name.  This means that lots of needless copies of "" are being
  made.  I think that the best solution to this problem would be to
  store NULL rather than "" for such entries, but I haven't figured
  out all of the places where the name is used.

Use strbufs?

No allocation (except for the strbuf object itself) is needed for
empty strings, and string ownership and be transferred to and from it
to prevent extra copies.

That would cost two extra size_t per object_array_entry.  I have the
feeling that this structure is used often enough that the extra overhead
would be a disadvantage, but I'm not sure.

The obvious alternative would be to teach users to deal with NULL and
either add another constructor alternative that transfers string
ownership or *always* transfer string ownership and change the callers
to call xstrdup() if they don't already own the name string.  I think I
will try that approach first.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/