I read the recent thread Timo Sirainen raised about string APIs in
git. ANd I went read the strbuf.[hc] module.

  I believe that the choice made in that module are wrong and could be
made better. I actually use to work with a string buffer API (that
interested readers can look at on [0]), that work almost the same
(except for the eof flag, but it's trivial to keep), but have
two significant differences:

  First, and that's the most important one: the buffer is always NUL
terminated, after its official "len". That means, in terms of strbuf
API, that "alloc" is always greater or equal to len+1 to be able to
store the ending NUL[1]. The advantages are obvious: you can pass the
buffer to any legacy C string function without any fear of read
overflow.  strtol comes to mind, and atm, git has to explicitely use
strbuf_end to put that ending nul to be able to call legacy
applications. But once done, the NUL is accounted into the string (aka
it's in "len") which makes it a non C-string (I mean you cannot append
any more data in it anymore). So current implementations tries to
workaround an issue (the non systematical NUL-termination) but IMHO the
wrong way.

  The other shortcoming is that you cannot tell the buffer "Hey, it's
very likely that you'll end up being _that_ long. That's why, in some
parts of the code (see write_tar_entry in archive-tar.c e.g.) the
programmer actually messes with the buffer allocation, outside from the
strbuf module, which makes it well, useless. In my API, I have a
"buffer_ensure" call, that is supposed to do that: "please ensure that
this buffer still has _this_ amount of free and allocated space to put
more data".


  So my question is, do people think I raise a valid point, and would
patches that would refactor the strbuf module to have those functions,
and would fix the code that uses strbuf's to interact properly, be
accepted ?

  Also, the efficiency of the buffer module API I use has a lot to do
with the fact that copying functions, and length tests are inlined in
the .h, so that the compiler can optimize the ones it already tested 10
calls before. I'm not sure if this is frowned upon or if it makes sense.


  [0] http://git.madism.org/?p=pfixtools.git;a=blob;f=buffer.h;hb=HEAD
      http://git.madism.org/?p=pfixtools.git;a=blob;f=buffer.c;hb=HEAD

  [1] Of course, ensuring the NUL-termination has a cost, though it's
      often benign, and for performance-critical places where characters
      are copied one by one, it's always possible to use an "unsafe"
      addch (that would not maintain the invariant), and then call an
      equivalent of strbuf_end (that would not append a \0 like it does
      now, but just would fix the invariant that for any strbuf,
      buf->buf[buf->len] == '\0') explicitely. For places where the
      invariant generate negligible cost (like concatenating two paths
      parts with a middle '/' e.g.) then we gain safety without even
      having to think about it.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org