Re: [AGGREGATED PATCH] Fix in-place editing functions in convert.c

On Fri, Oct 05, 2007 at 03:26:44PM +0000, Linus Torvalds wrote:

  I agree.

  the point here is that it's in a "filter" that is called like this:

some_filter(buf->buf, buf->len, buf);
            src       len       dst

  You can call the filter with src/len being data from anywere,
including the current content of the destination buffer.

  Then there is two cases, either the filter is known to be done in
place, either we can't know or we know it wont.

  In the latter case, we have a bit of code like that:

      char *to_free = NULL;

      if (buf->buf == src)
	  to_free = strbuf_detach(&buf);

      .. hack ..
      free(to_free);

  In the former case, then there is a small glitch, being that if we are
doing in place editing, we should not touch buffer at all (or it would
invalidate "src"). If we are not in the in-place editing code though,
then we have to make the resulting buffer be big enough...

  Right, that would do the same btw ;)

  Well, the above code is used in filters to spare reallocations. So if
we want to "blackbox" such a think, strbuf_make_room isn't the proper
API. We should rather use
  void *strbuf_begin_filter(struct strbuf *sb, const char *src, size_t reslen);
  strbuf_end_filter(void *);

  `strbuf_begin_filter` would decide upon the hint `reslen` argument if
we know if we can work in place or not (has a meaning iff src points
into the strbuf buffer). If not, it could stash the strbuf buffer in the
returned void * to be freed at the end of the filter. It seems like a
better alternative than a strbuf_make_room.

  Of course, strbuf_begin_filter() would really be simple and basically
be:

    char *tmp;
    if (src points into sb->buf && reslen > sb->alloc - 1) {
        // in place editing is OK
        return NULL;
    }
    tmp = strbuf_release(&sb);
    strbuf_grow(&sb, len);
    return tmp;

  and strbuf_end_filter would just be "free" :)

  We could even make "reslen" be a ssize_t so that -1 would mean "I've
absolutely no idea how much space I'll need (or just in place editing is
not supported). This way, both hacks I described in this mail could be
hidden in the strbuf module, and be properly documented _and_ safe _and_
efficient.

What do you think ?

[Though if we do that, I still think it's more important to fix the bug in master, and have a new patch implementing this approach]