On Fri, Oct 05, 2007 at 08:26:44AM -0700, Linus Torvalds wrote:
quoted text
> 
> 
> 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.

git did not build with this change....

	Sam


diff --git a/strbuf.c b/strbuf.c
index e33d06b..0d2d578 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -1,6 +1,14 @@
 #include "cache.h"
 #include "strbuf.h"
 
+struct strbuf {
+       int alloc;
+       int len;
+       int eof;
+       char *buf;
+};
+
+
 void strbuf_init(struct strbuf *sb) {
        sb->buf = NULL;
        sb->eof = sb->alloc = sb->len = 0;
diff --git a/strbuf.h b/strbuf.h
index 74cc012..c057be3 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -1,11 +1,6 @@
 #ifndef STRBUF_H
 #define STRBUF_H
-struct strbuf {
-       int alloc;
-       int len;
-       int eof;
-       char *buf;
-};
+struct strbuf;
 
 extern void strbuf_init(struct strbuf *);
 extern void read_line(struct strbuf *, FILE *, int);

-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html