Re: strbuf API

On Monday 03 September 2007, Pierre Habouzit wrote:

  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

Yes.

  , 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 ?

I don't know. Keep in mind that there is a parallel process (cf. the 
continuation of the "Buffer overflows" thread) to evaluate the Bstring 
library ( http://bstring.sourceforge.net/ ), and possibly substitute that 
for the strbuf "module".

I wouldn't want the work done by you or others to be wasted (depending on 
which solution wins out in the end), so I suggest you take a look at 
bstring and offer up arguments why your solution would be better for git.

  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.

All the points you raise are valid, but please have in mind that all 
programmers write their own string library at some point. (Writing a string 
library is right up there with "discovering why 'goto' is bad" and "2nd 
system syndrome" on the top list of rites of passage all programmers (need 
to) go through. I'm guessing 99% of the people on this list have written 
their own string library. Why not 100%? Well, Linus have obviously not, 
else we'd be using his already... ;)

I'd like to know how your solution compare with the alternatives (in this 
case bstring): What's the performance compared to bstring? How much thought 
has been given to the usability of the API? What functionality is covered 
(compared to bstring)? In short: Why is your alternative the best for git?


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net