Re: [PATCH v3 1/4] replace: add --graft option

From: Jeff King <peff@peff.net>
Date: Jun 8, 2014, 11:23 UTC
Message-ID: <20140608112333.GA9691@sigill.intra.peff.net>
In-Reply-To: <CAP8UFD344NFECqtO-uK-2wHA7XEko3XMJWuhW9+KGBdaiqGq+w@mail.gmail.com>

On Sun, Jun 08, 2014 at 08:49:45AM +0200, Christian Couder wrote:

Show 8 quoted lines

> On Fri, Jun 6, 2014 at 5:44 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
> >
> >         /* find existing parents */
> >         strbuf_addstr(&buf, commit->buffer);
> 
> Unfortunately, it looks like the above will not work if the commit->buffer
> contains an embedded NUL. I wonder if it is a real problem or not.

I ran into a similar problem recently[1] and have been pondering solutions to know the size of commit->buffer. What I've been come up with is:

  1. Look up the object size via sha1_object_info. Besides being
     inefficient (which probably does not matter for you here, but might
     for using commit->buffer in a traversal), it strikes me as
     inelegant; is it possible for commit->buffer to ever disagree in
     size with the results of sha1_object_info, and if so, what happens?

  2. Add an extra member "len" to "struct commit". This is simple, but
     bloats "struct commit", which may have a performance impact for
     things like rev-list, where the field will be unused.

  3. Store the length of objects as a size_t, exactly sizeof(size_t)
     bytes before the object buffer. Provide a macro:

       #define OBJECT_SIZE(buf) (((size_t *)(buf))[-1])

     to access it. Most callers can just use the buffer as-is, but
     anybody who calls free() would need to be adjusted to use a special
     "object_free".

  4. Keep a static commit_slab that points to the length for each parsed
     commit. We pay the same memory cost as (2), but as it's not part of
     the struct, the cache effects are minimized.

-Peff

[1] http://article.gmane.org/gmane.comp.version-control.git/250480