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

[Sam Ravnborg <sam@ravnborg.org>]

On Fri, Oct 05, 2007 at 06:35:17PM +0200, Pierre Habouzit wrote:
> On Fri, Oct 05, 2007 at 04:21:39PM +0000, Sam Ravnborg wrote:
> > On Fri, Oct 05, 2007 at 08:26:44AM -0700, Linus Torvalds wrote:
> > > 
> > > 
> > > On Fri, 5 Oct 2007, Pierre Habouzit wrote:
> > > >  
> > > > -	strbuf_grow(buf, len);
> > > > +	/* only grow if not in place */
> > > > +	if (strbuf_avail(buf) + buf->len < len)
> > > > +		strbuf_grow(buf, len - buf->len);
> > > 
> > > Umm. This is really ugly.
> > > 
> > > The whole point of strbuf's was that you shouldn't be doing your own 
> > > allocation decisions etc. So why do it?
> > > 
> > > Wouldn't it be much better to have a strbuf_make_room() interface that 
> > > just guarantees that there is enough room fo "len"? 
> > > 
> > > Otherwise, code like the above would seem to make the whole point of a 
> > > safer string interface rather pointless. The above code only makes sense 
> > > if you know how the strbuf's are internally done, so it should not exists 
> > > except as internal strbuf code. No?
> > 
> > Took a short look at strbuf.h after seeing the above code.
> > And I was suprised to see that all strbuf users were exposed to
> > the strbuf structure.
> > Following patch would at least make sure noone fiddle with strbuf internals.
> > Cut'n'paste - only for the example of it.
> > It simply moves strbuf declaration to the .c file where it rightfully belongs.
> 
>   you're looking at an antiquated version, please look at the one in
> current master on current next. In this one, what you can do or not do
> with the struct is explained
> 
> > git did not build with this change....
> 
>   Of course it doesn't! people want to have direct access to ->buf and
> ->len, and it's definitely OK.

Understood now - thanks for the clarification.

	Sam