strbuf API
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.
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org
Quoted 33 lines · Pierre Habouzit
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 usestrbuf_endto 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 (seewrite_tar_entryinarchive-tar.ce.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".
Quoted 1 line
So my question is, do people think I raise a valid point
Yes.
Quoted 4 lines
, 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.
Quoted 9 lines
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
Quoted 10 lines
[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
Signature · 2 lines
Johan Herland, <[email protected]> www.herland.net
Quoted 6 lines · Johan Herland
Quoted 4 lines · Pierre Habouzit
, 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 ?
Quoted 8 lines
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.
Oh I completely missed that the thread was still alive.
Quoted 4 lines
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?
Well, technically, I don't mind, I wasn't really pushing this or this
implementation. I had a look at bstring, I dislike the code that looks pretty ugly, but they almost took the same decisions, so if the consensus is that bstring gives flexibility, so be it.
For the record, I dislike the fact that bstring pullutes the b*
namespace, whereas it should use bstr as a prefix everywhere, and some functions (like bstrcpy, that should definitely be named bstrdup) are really named the _wrong_ way: you can dislike legacy C string APIs all the way you want, if you name a function from your foostr library foostrbar, it better behave the way strbar does in the legacy C API. In the legacy C, strcpy clobbers the destination, here there is no destination as bstrcpy is definitely a strdup operation. I only had a quick look, and maybe it was the sole of the kind, but ... well.
I also dislike the fact that there is no inline version of the
function that appends C strings, because it means that when you append a constant string into a bstr, you'll need to write:
bstrcat(bstr, "foo", sizeof("foo") - 1) or bstrcat(bstr, "foo", 3),
both suck a lot, and with an inline function using strlen, most compilers will optimize the strlen away, and the code is way more readable. But again, this is trivial to implement as an extension as well, so I really don't mind that much either.
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org
Quoted 4 lines · Pierre Habouzit
For the record, I dislike the fact that bstring pullutes the b*
namespace, whereas it should use bstr as a prefix everywhere, and some functions (like bstrcpy, that should definitely be named bstrdup) are really named the _wrong_ way
Yeah, I got the same impression. There's an odd "accumulated haphazardly over time" feel to much of it.
[One thing I found especially weird is the way the "core" interface uses lowercase names, but the "aux" interfaces all seem to use StUDlyCaPS...]
-Miles
Signature · 1 line
Ich bin ein Virus. Mach' mit und kopiere mich in Deine .signature.
PATCH 1/1 Add strbuf_fread, use it in fast-import.c.
This is a good proof of concept that combined with strbuf_detach it often
helps improving readability a lot, while remaining as efficient as before.
--- fast-import.c | 26 ++++++++++---------------- strbuf.c | 12 ++++++++++++ strbuf.h | 3 +++ 3 files changed, 25 insertions(+), 16 deletions(-)
fast-import.c +10 -16
@@ -1638,20 +1638,15 @@ static void cmd_mark(void)static void *cmd_data (size_t *size) { -size_t length; -char *buffer; +struct strbuf buffer =STRBUF_INIT ;if (prefixcmp (command_buf.buf,"data " ))die ("Expected 'data n' command, found: %s" , command_buf.buf);if (!prefixcmp (command_buf.buf +5 ,"<<" )) {char *term =xstrdup (command_buf.buf +5 +2 ); -size_t sz =8192 , term_len = command_buf.len -5 -2 ; - length =0 ; - buffer =xmalloc (sz); +size_t term_len = command_buf.len -5 -2 ; -/* XXX possible memory leak ? */ -strbuf_detach (&command_buf);for (;;) {read_line (&command_buf, stdin,'\n' );if (command_buf.eof)
@@ -1659,19 +1654,18 @@ static void *cmd_data (size_t *size)if (term_len == command_buf.len && !strcmp (term, command_buf.buf))break ; -ALLOC_GROW (buffer, length + command_buf.len +1 , sz); -memcpy (buffer + length, command_buf.buf, command_buf.len); - length += command_buf.len; - buffer[length++] ='\n' ; +strbuf_addbuf (&buffer, &command_buf); +strbuf_addch (&buffer,'\n' ); }free (term); }else { -size_t n =0 ; +size_t n =0 , length; + length =strtoul (command_buf.buf +5 ,NULL ,10 ); - buffer =xmalloc (length); +while (n < length) { -size_t s =fread (buffer + n,1 , length - n, stdin); +size_t s =strbuf_fread (&buffer, length - n, stdin);if (!s &&feof (stdin))die ("EOF in data (%lu bytes remaining)" , (unsigned long )(length - n));
@@ -1680,8 +1674,8 @@ static void *cmd_data (size_t *size) }skip_optional_lf (); - *size = length; -return buffer; + *size = buffer.len; +return strbuf_detach (&buffer); }static int validate_raw_date (const char *src,char *result,int maxlen)
strbuf.c +12 -0
@@ -58,6 +58,18 @@ void strbuf_addvf(struct strbuf *sb, const char *fmt, va_list ap) sb->buf[sb->len] ='\0' ; } +size_t strbuf_fread (struct strbuf *sb,size_t size,FILE *f) { +size_t res; + +strbuf_ensure (sb, size); + res =fread (sb->buf + sb->len,1 , size, f); +if (res >0 ) { + sb->len += res; + sb->buf[sb->len] ='\0' ; + } +return res; +} +void read_line (struct strbuf *sb,FILE *fp,int term) {int ch;if (feof (fp)) {
strbuf.h +3 -0
@@ -42,6 +42,9 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...) { va_end(ap); } + +extern size_t strbuf_fread (struct strbuf *,size_t ,FILE *); +extern void read_line (struct strbuf *,FILE *,int );#endif /* STRBUF_H */
Signature · 1 line
1.5.3
PATCH 1/1 Rework strbuf API and semantics.
In reply to Miles Bader A strbuf can be used to store byte arrays, or as an extended string
library. The buf' member can be passed to any C legacy string function, because strbuf operations always ensure there is a terminating \0 at the end of the buffer, not accounted in the len' field of the structure.
A strbuf can be used to generate a string/buffer whose final size is not
really known, and then "strbuf_detach" can be used to get the built buffer, and keep the wrapping "strbuf" structure usable for further work again.
Other interesting feature: buffer_ensure(sb, size) ensure that there is
enough allocated space in sb' to put size' new octets of data in the buffer. It helps avoiding reallocating data for nothing when the problem the strbuf helps to solve has a known typical size.
--- archive-tar.c | 65 +++++++++++++--------------------------------------- fast-import.c | 24 +++++++++--------- mktree.c | 4 +-- strbuf.c | 71 +++++++++++++++++++++++++++++++++++++++++++++----------- strbuf.h | 34 +++++++++++++++++++++++++++ 5 files changed, 120 insertions(+), 78 deletions(-)
archive-tar.c +16 -49
@@ -78,19 +78,6 @@ static void write_trailer(void) } } -static void strbuf_append_string (struct strbuf *sb,const char *s) -{ -int slen =strlen (s); -int total = sb->len + slen; -if (total +1 > sb->alloc) { - sb->buf =xrealloc (sb->buf, total +1 ); - sb->alloc = total +1 ; - } -memcpy (sb->buf + sb->len, s, slen); - sb->len = total; - sb->buf[total] ='\0' ; -} -/* * pax extended header records have the format "%u %s=%s\n". %u contains * the size of the whole string (including the %u), the first %s is the
@@ -100,26 +87,17 @@ static void strbuf_append_string(struct strbuf *sb, const char *s)static void strbuf_append_ext_header (struct strbuf *sb,const char *keyword,const char *value,unsigned int valuelen) { -char *p; -int len, total, tmp; +int len, tmp;/* "%u %s=%s\n" */ len =1 +1 +strlen (keyword) +1 + valuelen +1 ;for (tmp = len; tmp >9 ; tmp /=10 ) len++; - total = sb->len + len; -if (total > sb->alloc) { - sb->buf =xrealloc (sb->buf, total); - sb->alloc = total; - } - - p = sb->buf; - p +=sprintf (p,"%u %s=" , len, keyword); -memcpy (p, value, valuelen); - p += valuelen; - *p ='\n' ; - sb->len = total; +strbuf_ensure (sb, len); +strbuf_addf (sb,"%u %s=" , len, keyword); +strbuf_add (sb, value, valuelen); +strbuf_addch (sb,'\n' ); }static unsigned int ustar_header_chksum (const struct ustar_header *header)
@@ -153,8 +131,7 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,struct strbuf ext_header;memset (&header,0 ,sizeof (header)); - ext_header.buf =NULL ; - ext_header.len = ext_header.alloc =0 ; +strbuf_init (&ext_header);if (!sha1) { *header.typeflag =TYPEFLAG_GLOBAL_HEADER ;
@@ -225,8 +202,8 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,if (ext_header.len >0 ) {write_entry (sha1,NULL ,0 , ext_header.buf, ext_header.len); -free (ext_header.buf); } +strbuf_wipe (&ext_header);write_blocked (&header,sizeof (header));if (S_ISREG (mode) && buffer && size >0 )write_blocked (buffer, size);
@@ -235,11 +212,11 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,static void write_global_extended_header (const unsigned char *sha1) {struct strbuf ext_header; - ext_header.buf =NULL ; - ext_header.len = ext_header.alloc =0 ; + +strbuf_init (&ext_header);strbuf_append_ext_header (&ext_header,"comment" ,sha1_to_hex (sha1),40 );write_entry (NULL ,NULL ,0 , ext_header.buf, ext_header.len); -free (ext_header.buf); +strbuf_wipe (&ext_header); }static int git_tar_config (const char *var,const char *value)
@@ -260,28 +237,18 @@ static int write_tar_entry(const unsigned char *sha1, const char *base, int baselen, const char *filename, unsigned mode, int stage) { - static struct strbuf path; + static struct strbuf path = STRBUF_INIT;int filenamelen =strlen (filename);void *buffer;enum object_type type;unsigned long size; -if (!path.alloc) { - path.buf =xmalloc (PATH_MAX ); - path.alloc =PATH_MAX ; - path.len = path.eof =0 ; - } -if (path.alloc < baselen + filenamelen +1 ) { -free (path.buf); - path.buf =xmalloc (baselen + filenamelen +1 ); - path.alloc = baselen + filenamelen +1 ; - } -memcpy (path.buf, base, baselen); -memcpy (path.buf + baselen, filename, filenamelen); - path.len = baselen + filenamelen; - path.buf[path.len] ='\0' ; +strbuf_ensure (&path,MAX (PATH_MAX , baselen + filenamelen +1 )); +strbuf_reset (&path); +strbuf_add (&path, base, baselen); +strbuf_add (&path, filename, filenamelen);if (S_ISDIR (mode) ||S_ISGITLINK (mode)) { -strbuf_append_string (&path,"/" ); +strbuf_addch (&path,'/' ); buffer =NULL ; size =0 ; }else {
fast-import.c +12 -12
@@ -340,7 +340,7 @@ static struct tag *last_tag;/* Input stream parsing */ static whenspec_type whenspec =WHENSPEC_RAW ; -static struct strbuf command_buf; +static struct strbuf command_buf =STRBUF_INIT ;static int unread_command_buf;static struct recent_command cmd_hist = {&cmd_hist, &cmd_hist,NULL };static struct recent_command *cmd_tail = &cmd_hist;
@@ -1595,7 +1595,7 @@ static void read_next_command(void) }else {struct recent_command *rc; - command_buf.buf =NULL ; +strbuf_detach (&command_buf);read_line (&command_buf, stdin,'\n' );if (command_buf.eof)return ;
@@ -1610,7 +1610,7 @@ static void read_next_command(void)free (rc->buf); } - rc->buf = command_buf.buf; + rc->buf = command_buf.buf; rc->prev = cmd_tail; rc->next = cmd_hist.prev; rc->prev->next = rc;
@@ -1649,7 +1649,9 @@ static void *cmd_data (size_t *size)size_t sz =8192 , term_len = command_buf.len -5 -2 ; length =0 ; buffer =xmalloc (sz); - command_buf.buf =NULL ; + +/* XXX possible memory leak ? */ +strbuf_detach (&command_buf);for (;;) {read_line (&command_buf, stdin,'\n' );if (command_buf.eof)
@@ -1657,11 +1659,9 @@ static void *cmd_data (size_t *size)if (term_len == command_buf.len && !strcmp (term, command_buf.buf))break ; -ALLOC_GROW (buffer, length + command_buf.len, sz); -memcpy (buffer + length, - command_buf.buf, - command_buf.len -1 ); - length += command_buf.len -1 ; +ALLOC_GROW (buffer, length + command_buf.len +1 , sz); +memcpy (buffer + length, command_buf.buf, command_buf.len); + length += command_buf.len; buffer[length++] ='\n' ; }free (term);
@@ -2101,7 +2101,7 @@ static void cmd_new_commit(void) }/* file_change* */ -while (!command_buf.eof && command_buf.len >1 ) { +while (!command_buf.eof && command_buf.len >0 ) {if (!prefixcmp (command_buf.buf,"M " ))file_change_m (b);else if (!prefixcmp (command_buf.buf,"D " ))
@@ -2256,7 +2256,7 @@ static void cmd_reset_branch(void)else b =new_branch (sp);read_next_command (); -if (!cmd_from (b) && command_buf.len >1 ) +if (!cmd_from (b) && command_buf.len >0 ) unread_command_buf =1 ; }
@@ -2273,7 +2273,7 @@ static void cmd_checkpoint(void)static void cmd_progress (void ) { -fwrite (command_buf.buf,1 , command_buf.len -1 , stdout); +fwrite (command_buf.buf,1 , command_buf.len, stdout);fputc ('\n' , stdout);fflush (stdout);skip_optional_lf ();
mktree.c +1 -3
@@ -92,7 +92,6 @@ int main(int ac, char **av)strbuf_init (&sb);while (1 ) { -int len;char *ptr, *ntr;unsigned mode;enum object_type type;
@@ -101,7 +100,6 @@ int main(int ac, char **av)read_line (&sb, stdin, line_termination);if (sb.eof)break ; - len = sb.len; ptr = sb.buf;/* Input is non-recursive ls-tree output format * mode SP type SP sha1 TAB name
@@ -111,7 +109,7 @@ int main(int ac, char **av)die ("input format error: %s" , sb.buf); ptr = ntr +1 ;/* type */ ntr =strchr (ptr,' ' ); -if (!ntr || sb.buf + len <= ntr +41 || +if (!ntr || sb.buf + sb.len <= ntr +40 || ntr[41 ] !='\t' ||get_sha1_hex (ntr +1 , sha1))die ("input format error: %s" , sb.buf);
strbuf.c +57 -14
97 lines not on this page · raw message
strbuf.h +34 -0
@@ -7,7 +7,41 @@ struct strbuf {char *buf; }; +#define STRBUF_INIT { 0, 0, 0, NULL } + +/* strbuf life cycle */ extern void strbuf_init (struct strbuf *); +extern void strbuf_wipe (struct strbuf *); +extern void strbuf_reset (struct strbuf *); +extern char *strbuf_detach (struct strbuf *); + + +extern void strbuf_ensure (struct strbuf *,int ); +extern void strbuf_add (struct strbuf *,const void *data,int len); + +static inline void strbuf_addstr (struct strbuf *sb,const char *s) { +strbuf_add (sb, s,strlen (s)); +} +static inline void strbuf_addbuf (struct strbuf *sb,struct strbuf *sb2) { +strbuf_add (sb, sb2->buf, sb2->len); +} +static inline void strbuf_addch (struct strbuf *sb,int c) { +strbuf_ensure (sb,1 ); + sb->buf[sb->len++] = c; + sb->buf[sb->len] ='\0' ; +} + +__attribute__((format (printf,2 ,0 ))) +extern void strbuf_addvf (struct strbuf *,const char *,va_list ); + +static inline __attribute__((format (printf,2 ,3 ))) +void strbuf_addf (struct strbuf *sb,const char *fmt, ...) { +va_list ap; +va_start (ap, fmt); +strbuf_addvf (sb, fmt, ap); +va_end (ap); +} +extern void read_line (struct strbuf *,FILE *,int );#endif /* STRBUF_H */
Signature · 1 line
1.5.3
Re: [PATCH] Rework strbuf API and semantics.
Hi,
Quoted 10 lines · Pierre Habouzit
A strbuf can be used to store byte arrays, or as an extended string
library. Thebuf' member can be passed to any C legacy string function, because strbuf operations always ensure there is a terminating \0 at the end of the buffer, not accounted in thelen' field of the structure.A strbuf can be used to generate a string/buffer whose final size is
not really known, and then "strbuf_detach" can be used to get the built buffer, and keep the wrapping "strbuf" structure usable for further work again.
Quoted 4 lines
Other interesting feature:
buffer_ensure(sb, size)ensure that there
is enough allocated space insb' to putsize' new octets of data in the buffer. It helps avoiding reallocating data for nothing when the problem the strbuf helps to solve has a known typical size.
I like the general idea of this!
However, some comments are due:
- IMHO strbuf_grow() would be more descriptive than buffer_ensure(),
- IMHO the same goes for strbuf_free() instead of strbuf_wipe(), and
- it would be nice to split this patch into
- the API change (with _minimal_ changes to anything outside of strbuf.[ch]), and
- the cleanups in the rest of the code.
Ciao,
Dscho
Re: [PATCH] Rework strbuf API and semantics.
Quoted 19 lines · Johannes Schindelin
Hi,
Quoted 10 lines · Pierre Habouzit
A strbuf can be used to store byte arrays, or as an extended string
library. Thebuf' member can be passed to any C legacy string function, because strbuf operations always ensure there is a terminating \0 at the end of the buffer, not accounted in thelen' field of the structure.A strbuf can be used to generate a string/buffer whose final size is
not really known, and then "strbuf_detach" can be used to get the built buffer, and keep the wrapping "strbuf" structure usable for further work again.Quoted 4 lines
Other interesting feature:
buffer_ensure(sb, size)ensure that there
is enough allocated space insb' to putsize' new octets of data in the buffer. It helps avoiding reallocating data for nothing when the problem the strbuf helps to solve has a known typical size.
Quoted 5 lines
I like the general idea of this!
However, some comments are due:
- IMHO
strbuf_grow()would be more descriptive thanbuffer_ensure(),
Yeah, strbuf_ensure (and not buffer_ensure sorry :P) isn't a clever
name. strbuf_grow looks better indeed.
Quoted 1 line
- IMHO the same goes for
strbuf_free()instead ofstrbuf_wipe(), and
Well, I don't like strbuf_free, because it's opposed to
strbuf_alloc/new whatever, that would be functions that create and release a struct strbuf * (meaning allocating the struct shell as well). Here you only free the internal buffer, not the shell, so _free() would be a very bad name.
In my own coding rules, I have _new/_delete and _init/_wipe functions,
the former acts on pointers, the latter on the structs. Hence the naming. Though, looking at git's code, it seems that the usual name for this operation is _release. So would you go for strbuf_release ?
Quoted 6 lines
- it would be nice to split this patch into
- the API change (with _minimal_ changes to anything outside of strbuf.[ch]), and
- the cleanups in the rest of the code.
I'll try to do that, but it's quite hard to achieve knowing that in
many places of the current state of master, there are embeded NUL's (accounted in ->len). I'll try to see what I can split, but it's unlikely I'll be able to have an intermediate _working_ state (I mean that would pass the testsuite) with the new API/semantics _and_ without touching a lot less of the rest of the code.
But I really understand that it helps seeing what the new API
improves, whereas right now it may not be that obvious because of the (not so) many new lines in strbuf.[hc].
I'll try to roll a new patchset ASAP.
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org
PATCH 1/1 Use proper strbuf API, and also simplify cmd_data code.
This patch features the use of strbuf_detach, and prevent the programmer
to mess with allocation directly. The code is as efficent as before, just more concise and more straightforward.
--- fast-import.c | 29 ++++++++++++----------------- 1 files changed, 12 insertions(+), 17 deletions(-)
fast-import.c +12 -17
@@ -340,7 +340,7 @@ static struct tag *last_tag;/* Input stream parsing */ static whenspec_type whenspec =WHENSPEC_RAW ; -static struct strbuf command_buf; +static struct strbuf command_buf =STRBUF_INIT ;static int unread_command_buf;static struct recent_command cmd_hist = {&cmd_hist, &cmd_hist,NULL };static struct recent_command *cmd_tail = &cmd_hist;
@@ -1638,17 +1638,15 @@ static void cmd_mark(void)static void *cmd_data (size_t *size) { -size_t length; -char *buffer; +struct strbuf buffer =STRBUF_INIT ;if (prefixcmp (command_buf.buf,"data " ))die ("Expected 'data n' command, found: %s" , command_buf.buf);if (!prefixcmp (command_buf.buf +5 ,"<<" )) {char *term =xstrdup (command_buf.buf +5 +2 ); -size_t sz =8192 , term_len = command_buf.len -5 -2 ; - length =0 ; - buffer =xmalloc (sz); +size_t term_len = command_buf.len -5 -2 ; +for (;;) {read_line (&command_buf, stdin,'\n' );if (command_buf.eof)
@@ -1656,21 +1654,18 @@ static void *cmd_data (size_t *size)if (term_len == command_buf.len && !strcmp (term, command_buf.buf))break ; -ALLOC_GROW (buffer, length + command_buf.len +1 , sz); -memcpy (buffer + length, - command_buf.buf, - command_buf.len); - length += command_buf.len; - buffer[length++] ='\n' ; +strbuf_addbuf (&buffer, &command_buf); +strbuf_addch (&buffer,'\n' ); }free (term); }else { -size_t n =0 ; +size_t n =0 , length; + length =strtoul (command_buf.buf +5 ,NULL ,10 ); - buffer =xmalloc (length); +while (n < length) { -size_t s =fread (buffer + n,1 , length - n, stdin); +size_t s =strbuf_fread (&buffer, length - n, stdin);if (!s &&feof (stdin))die ("EOF in data (%lu bytes remaining)" , (unsigned long )(length - n));
@@ -1679,8 +1674,8 @@ static void *cmd_data (size_t *size) }skip_optional_lf (); - *size = length; -return buffer; + *size = buffer.len; +return strbuf_detach (&buffer); }static int validate_raw_date (const char *src,char *result,int maxlen)
Signature · 1 line
1.5.3
PATCH 1/1 Rework strbuf API and semantics.
In reply to Pierre Habouzit A strbuf can be used to store byte arrays, or as an extended string
library. The buf' member can be passed to any C legacy string function, because strbuf operations always ensure there is a terminating \0 at the end of the buffer, not accounted in the len' field of the structure.
A strbuf can be used to generate a string/buffer whose final size is not
really known, and then "strbuf_detach" can be used to get the built buffer, and keep the wrapping "strbuf" structure usable for further work again.
Other interesting feature: strbuf_grow(sb, size) ensure that there is
enough allocated space in sb' to put size' new octets of data in the buffer. It helps avoiding reallocating data for nothing when the problem the strbuf helps to solve has a known typical size.
--- archive-tar.c | 2 +- fast-import.c | 15 +++++----- mktree.c | 4 +-- strbuf.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++--------- strbuf.h | 41 ++++++++++++++++++++++++++- 5 files changed, 119 insertions(+), 28 deletions(-)
archive-tar.c +1 -1
@@ -166,7 +166,7 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,sprintf (header.name,"%s.paxheader" ,sha1_to_hex (sha1)); }else {if (verbose) -fprintf (stderr,"%.*s\n" , path->len, path->buf); +fprintf (stderr,"%.*s\n" , (int )path->len, path->buf);if (S_ISDIR (mode) ||S_ISGITLINK (mode)) { *header.typeflag =TYPEFLAG_DIR ; mode = (mode |0777 ) & ~tar_umask;
fast-import.c +7 -8
@@ -1595,7 +1595,7 @@ static void read_next_command(void) }else {struct recent_command *rc; - command_buf.buf =NULL ; +strbuf_detach (&command_buf);read_line (&command_buf, stdin,'\n' );if (command_buf.eof)return ;
@@ -1649,7 +1649,6 @@ static void *cmd_data (size_t *size)size_t sz =8192 , term_len = command_buf.len -5 -2 ; length =0 ; buffer =xmalloc (sz); - command_buf.buf =NULL ;for (;;) {read_line (&command_buf, stdin,'\n' );if (command_buf.eof)
@@ -1657,11 +1656,11 @@ static void *cmd_data (size_t *size)if (term_len == command_buf.len && !strcmp (term, command_buf.buf))break ; -ALLOC_GROW (buffer, length + command_buf.len, sz); +ALLOC_GROW (buffer, length + command_buf.len +1 , sz);memcpy (buffer + length, command_buf.buf, - command_buf.len -1 ); - length += command_buf.len -1 ; + command_buf.len); + length += command_buf.len; buffer[length++] ='\n' ; }free (term);
@@ -2101,7 +2100,7 @@ static void cmd_new_commit(void) }/* file_change* */ -while (!command_buf.eof && command_buf.len >1 ) { +while (!command_buf.eof && command_buf.len >0 ) {if (!prefixcmp (command_buf.buf,"M " ))file_change_m (b);else if (!prefixcmp (command_buf.buf,"D " ))
@@ -2256,7 +2255,7 @@ static void cmd_reset_branch(void)else b =new_branch (sp);read_next_command (); -if (!cmd_from (b) && command_buf.len >1 ) +if (!cmd_from (b) && command_buf.len >0 ) unread_command_buf =1 ; }
@@ -2273,7 +2272,7 @@ static void cmd_checkpoint(void)static void cmd_progress (void ) { -fwrite (command_buf.buf,1 , command_buf.len -1 , stdout); +fwrite (command_buf.buf,1 , command_buf.len, stdout);fputc ('\n' , stdout);fflush (stdout);skip_optional_lf ();
mktree.c +1 -3
@@ -92,7 +92,6 @@ int main(int ac, char **av)strbuf_init (&sb);while (1 ) { -int len;char *ptr, *ntr;unsigned mode;enum object_type type;
@@ -101,7 +100,6 @@ int main(int ac, char **av)read_line (&sb, stdin, line_termination);if (sb.eof)break ; - len = sb.len; ptr = sb.buf;/* Input is non-recursive ls-tree output format * mode SP type SP sha1 TAB name
@@ -111,7 +109,7 @@ int main(int ac, char **av)die ("input format error: %s" , sb.buf); ptr = ntr +1 ;/* type */ ntr =strchr (ptr,' ' ); -if (!ntr || sb.buf + len <= ntr +41 || +if (!ntr || sb.buf + sb.len <= ntr +40 || ntr[41 ] !='\t' ||get_sha1_hex (ntr +1 , sha1))die ("input format error: %s" , sb.buf);
strbuf.c +71 -14
112 lines not on this page · raw message
strbuf.h +39 -2
52 lines not on this page · raw message
Signature · 1 line
1.5.3
Re: [PATCH] Rework strbuf API and semantics.
Hi,
strbuf.c line 32
Quoted 7 lines · Pierre Habouzit
strbuf.c +7
-0+void strbuf_grow (struct strbuf *sb,size_t extra) { +if (sb->len + extra +STRBUF_GROW_STEP < sb->len) +die ("you want to use way to much memory" ); + + sb->alloc = ((sb->len + extra) +STRBUF_GROW_STEP ) & ~(STRBUF_GROW_STEP -1 ); + sb->buf =xrealloc (sb->buf, sb->alloc); +}
Why not use ALLOC_GROW()? Seems to me more efficient than growing by 1kB blocks all the time, for big strings as for short strings.
Ciao,
Dscho
Re: [PATCH] Rework strbuf API and semantics.
Quoted 4 lines · Johannes Schindelin
Hi,
On Tue, 4 Sep 2007, Pierre Habouzit wrote:
Quoted 10 lines
strbuf.c line 32
Quoted 7 lines
strbuf.c +7
-0+void strbuf_grow (struct strbuf *sb,size_t extra) { +if (sb->len + extra +STRBUF_GROW_STEP < sb->len) +die ("you want to use way to much memory" ); + + sb->alloc = ((sb->len + extra) +STRBUF_GROW_STEP ) & ~(STRBUF_GROW_STEP -1 ); + sb->buf =xrealloc (sb->buf, sb->alloc); +}Why not use
ALLOC_GROW()? Seems to me more efficient than growing by 1kB blocks all the time, for big strings as for short strings.
ooooh, now I'm guilty of not knowing all git APIs very well yet :) Indeed,
this should just be:
void strbuf_grow(struct strbuf *sb, size_t extra) {
if (sb->len + extra + 1 < sb->len)
die("you want to use way to much memory");
ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
}This is definitely better on so many levels !
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org
Re: [PATCH] Rework strbuf API and semantics.
Quoted 9 lines · Pierre Habouzit
ooooh, now I'm guilty of not knowing all git APIs very well
yet :) Indeed,
this should just be:void strbuf_grow(struct strbuf *sb, size_t extra) { if (sb->len + extra + 1 < sb->len) die("you want to use way to much memory"); ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc); }
"too much memory", not "to much memory"
Cheers,
Wincent
PATCH 1/1 Simplify strbuf uses in fast-import.c using the proper functions.
In reply to Pierre Habouzit This is just cleaner way to deal with strbufs, using its API rather than
reinventing it in the module (e.g. strbuf_append_string is just the plain strbuf_addstr function, and it was used to perform what strbuf_addch does anyways).
--- archive-tar.c | 65 ++++++++++++++------------------------------------------- 1 files changed, 16 insertions(+), 49 deletions(-)
archive-tar.c +16 -49
@@ -78,19 +78,6 @@ static void write_trailer(void) } } -static void strbuf_append_string (struct strbuf *sb,const char *s) -{ -int slen =strlen (s); -int total = sb->len + slen; -if (total +1 > sb->alloc) { - sb->buf =xrealloc (sb->buf, total +1 ); - sb->alloc = total +1 ; - } -memcpy (sb->buf + sb->len, s, slen); - sb->len = total; - sb->buf[total] ='\0' ; -} -/* * pax extended header records have the format "%u %s=%s\n". %u contains * the size of the whole string (including the %u), the first %s is the
@@ -100,26 +87,17 @@ static void strbuf_append_string(struct strbuf *sb, const char *s)static void strbuf_append_ext_header (struct strbuf *sb,const char *keyword,const char *value,unsigned int valuelen) { -char *p; -int len, total, tmp; +int len, tmp;/* "%u %s=%s\n" */ len =1 +1 +strlen (keyword) +1 + valuelen +1 ;for (tmp = len; tmp >9 ; tmp /=10 ) len++; - total = sb->len + len; -if (total > sb->alloc) { - sb->buf =xrealloc (sb->buf, total); - sb->alloc = total; - } - - p = sb->buf; - p +=sprintf (p,"%u %s=" , len, keyword); -memcpy (p, value, valuelen); - p += valuelen; - *p ='\n' ; - sb->len = total; +strbuf_grow (sb, len); +strbuf_addf (sb,"%u %s=" , len, keyword); +strbuf_add (sb, value, valuelen); +strbuf_addch (sb,'\n' ); }static unsigned int ustar_header_chksum (const struct ustar_header *header)
@@ -153,8 +131,7 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,struct strbuf ext_header;memset (&header,0 ,sizeof (header)); - ext_header.buf =NULL ; - ext_header.len = ext_header.alloc =0 ; +strbuf_init (&ext_header);if (!sha1) { *header.typeflag =TYPEFLAG_GLOBAL_HEADER ;
@@ -225,8 +202,8 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,if (ext_header.len >0 ) {write_entry (sha1,NULL ,0 , ext_header.buf, ext_header.len); -free (ext_header.buf); } +strbuf_release (&ext_header);write_blocked (&header,sizeof (header));if (S_ISREG (mode) && buffer && size >0 )write_blocked (buffer, size);
@@ -235,11 +212,11 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,static void write_global_extended_header (const unsigned char *sha1) {struct strbuf ext_header; - ext_header.buf =NULL ; - ext_header.len = ext_header.alloc =0 ; + +strbuf_init (&ext_header);strbuf_append_ext_header (&ext_header,"comment" ,sha1_to_hex (sha1),40 );write_entry (NULL ,NULL ,0 , ext_header.buf, ext_header.len); -free (ext_header.buf); +strbuf_release (&ext_header); }static int git_tar_config (const char *var,const char *value)
@@ -260,28 +237,18 @@ static int write_tar_entry(const unsigned char *sha1, const char *base, int baselen, const char *filename, unsigned mode, int stage) { - static struct strbuf path; + static struct strbuf path = STRBUF_INIT;int filenamelen =strlen (filename);void *buffer;enum object_type type;unsigned long size; -if (!path.alloc) { - path.buf =xmalloc (PATH_MAX ); - path.alloc =PATH_MAX ; - path.len = path.eof =0 ; - } -if (path.alloc < baselen + filenamelen +1 ) { -free (path.buf); - path.buf =xmalloc (baselen + filenamelen +1 ); - path.alloc = baselen + filenamelen +1 ; - } -memcpy (path.buf, base, baselen); -memcpy (path.buf + baselen, filename, filenamelen); - path.len = baselen + filenamelen; - path.buf[path.len] ='\0' ; +strbuf_grow (&path,MAX (PATH_MAX , baselen + filenamelen +1 )); +strbuf_reset (&path); +strbuf_add (&path, base, baselen); +strbuf_add (&path, filename, filenamelen);if (S_ISDIR (mode) ||S_ISGITLINK (mode)) { -strbuf_append_string (&path,"/" ); +strbuf_addch (&path,'/' ); buffer =NULL ; size =0 ; }else {
Signature · 1 line
1.5.3
Re: [PATCH] Simplify strbuf uses in fast-import.c using the proper functions.
Quoted 7 lines · Pierre Habouzit
This is just cleaner way to deal with strbufs, using its API rather than
reinventing it in the module (e.g.strbuf_append_stringis just the plainstrbuf_addstrfunction, and it was used to perform whatstrbuf_addchdoes anyways).--- archive-tar.c | 65 ++++++++++++++------------------------------------------- 1 files changed, 16 insertions(+), 49 deletions(-)
Apart from the wrong subject I really like this patch. :-D
Thanks!
René
Re: [PATCH] Simplify strbuf uses in fast-import.c using the proper functions.
In reply to Pierre HabouzitQuoted 7 lines · Pierre Habouzit
This is just cleaner way to deal with strbufs, using its API rather than
reinventing it in the module (e.g.strbuf_append_stringis just the plainstrbuf_addstrfunction, and it was used to perform whatstrbuf_addchdoes anyways).--- archive-tar.c | 65 ++++++++++++++------------------------------------------- 1 files changed, 16 insertions(+), 49 deletions(-)
Apart from the wrong subject line I really like this patch. :-D
Thanks!
René
Re: [PATCH] Simplify strbuf uses in fast-import.c using the proper functions.
Quoted 10 lines · René Scharfe
Quoted 7 lines · Pierre Habouzit
This is just cleaner way to deal with strbufs, using its API rather than
reinventing it in the module (e.g.strbuf_append_stringis just the plainstrbuf_addstrfunction, and it was used to perform whatstrbuf_addchdoes anyways).--- archive-tar.c | 65 ++++++++++++++------------------------------------------- 1 files changed, 16 insertions(+), 49 deletions(-)Apart from the wrong subject line I really like this patch. :-D
oh boy, yes I fixed that in my local patch collection. I'm waiting for
a few hours (days ?) to see if there will be some more comments, I've integrated every single one done here already (and some I had on IRC too), and I'll repost a new clean series that I intend to be a real proposal for inclusion.
And yes, this patch is a perfect example of the gain we have to share
a common buffer API. The code looks (at least to me) way nicer, and if you look in the details, we perform as many memory allocations, copies, and so on as in the previous version.
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org
Re: [PATCH] Simplify strbuf uses in fast-import.c using the proper functions.
Quoted 5 lines · Pierre Habouzit
oh boy, yes I fixed that in my local patch collection. I'm waiting for
a few hours (days ?) to see if there will be some more comments, I've integrated every single one done here already (and some I had on IRC too), and I'll repost a new clean series that I intend to be a real proposal for inclusion.
Ah, I actually did the single trivial fix-up (ALLOC_GROW) and have been looking at it, but I'll discard it. Thanks.
Quoted 4 lines
And yes, this patch is a perfect example of the gain we have to share
a common buffer API. The code looks (at least to me) way nicer, and if you look in the details, we perform as many memory allocations, copies, and so on as in the previous version.
Wait. What is your point in saying this? Is that a good thing to do "as many"? "API is cleaned-up and it is much easier to read but we do not do more than before" is certainly a *BIG* plus, so perhaps that is what you meant, but when I first read it I thought you were saying "we are not optimizing it at all" in a negative sense.
Re: [PATCH] Simplify strbuf uses in fast-import.c using the proper functions.
Quoted 10 lines · Junio C Hamano
Quoted 5 lines · Pierre Habouzit
oh boy, yes I fixed that in my local patch collection. I'm waiting for
a few hours (days ?) to see if there will be some more comments, I've integrated every single one done here already (and some I had on IRC too), and I'll repost a new clean series that I intend to be a real proposal for inclusion.Ah, I actually did the single trivial fix-up (
ALLOC_GROW) and have been looking at it, but I'll discard it. Thanks.
Yeah I integrated that as well already. I think I'll post the patches
at the end of the Day (French Timezone so in 5 or 6 hours at least) to be sure nobody has any good remarks to do first. If you want I can share my branch somewhere if you want to look at it, just ask.
Quoted 11 lines
Quoted 4 lines
And yes, this patch is a perfect example of the gain we have to share
a common buffer API. The code looks (at least to me) way nicer, and if you look in the details, we perform as many memory allocations, copies, and so on as in the previous version.Wait. What is your point in saying this? Is that a good thing to do "as many"? "API is cleaned-up and it is much easier to read but we do not do more than before" is certainly a *BIG* plus, so perhaps that is what you meant, but when I first read it I thought you were saying "we are not optimizing it at all" in a negative sense.
Sorry if I chose wrong words to say that, it is clearly a big plus ! I
wrote it having in mind that very often abstractions and high level APIs often degrade performance. It does not, and that's a huge win. The point is that "despite the somehow higher level API we don't loose an inch of performance".
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org
PATCH 1/1 Rework strbuf API and semantics.
A strbuf can be used to store byte arrays, or as an extended string
library. The buf' member can be passed to any C legacy string function, because strbuf operations always ensure there is a terminating \0 at the end of the buffer, not accounted in the len' field of the structure.
A strbuf can be used to generate a string/buffer whose final size is not
really known, and then "strbuf_detach" can be used to get the built buffer, and keep the wrapping "strbuf" structure usable for further work again.
Other interesting feature: strbuf_grow(sb, size) ensure that there is
enough allocated space in sb' to put size' new octets of data in the buffer. It helps avoiding reallocating data for nothing when the problem the strbuf helps to solve has a known typical size.
--- archive-tar.c | 2 +- fast-import.c | 15 ++++---- mktree.c | 4 +-- strbuf.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++-------- strbuf.h | 42 ++++++++++++++++++++++- 5 files changed, 138 insertions(+), 28 deletions(-)
archive-tar.c +1 -1
@@ -166,7 +166,7 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,sprintf (header.name,"%s.paxheader" ,sha1_to_hex (sha1)); }else {if (verbose) -fprintf (stderr,"%.*s\n" , path->len, path->buf); +fprintf (stderr,"%.*s\n" , (int )path->len, path->buf);if (S_ISDIR (mode) ||S_ISGITLINK (mode)) { *header.typeflag =TYPEFLAG_DIR ; mode = (mode |0777 ) & ~tar_umask;
fast-import.c +7 -8
@@ -1595,7 +1595,7 @@ static void read_next_command(void) }else {struct recent_command *rc; - command_buf.buf =NULL ; +strbuf_detach (&command_buf);read_line (&command_buf, stdin,'\n' );if (command_buf.eof)return ;
@@ -1649,7 +1649,6 @@ static void *cmd_data (size_t *size)size_t sz =8192 , term_len = command_buf.len -5 -2 ; length =0 ; buffer =xmalloc (sz); - command_buf.buf =NULL ;for (;;) {read_line (&command_buf, stdin,'\n' );if (command_buf.eof)
@@ -1657,11 +1656,11 @@ static void *cmd_data (size_t *size)if (term_len == command_buf.len && !strcmp (term, command_buf.buf))break ; -ALLOC_GROW (buffer, length + command_buf.len, sz); +ALLOC_GROW (buffer, length + command_buf.len +1 , sz);memcpy (buffer + length, command_buf.buf, - command_buf.len -1 ); - length += command_buf.len -1 ; + command_buf.len); + length += command_buf.len; buffer[length++] ='\n' ; }free (term);
@@ -2101,7 +2100,7 @@ static void cmd_new_commit(void) }/* file_change* */ -while (!command_buf.eof && command_buf.len >1 ) { +while (!command_buf.eof && command_buf.len >0 ) {if (!prefixcmp (command_buf.buf,"M " ))file_change_m (b);else if (!prefixcmp (command_buf.buf,"D " ))
@@ -2256,7 +2255,7 @@ static void cmd_reset_branch(void)else b =new_branch (sp);read_next_command (); -if (!cmd_from (b) && command_buf.len >1 ) +if (!cmd_from (b) && command_buf.len >0 ) unread_command_buf =1 ; }
@@ -2273,7 +2272,7 @@ static void cmd_checkpoint(void)static void cmd_progress (void ) { -fwrite (command_buf.buf,1 , command_buf.len -1 , stdout); +fwrite (command_buf.buf,1 , command_buf.len, stdout);fputc ('\n' , stdout);fflush (stdout);skip_optional_lf ();
mktree.c +1 -3
@@ -92,7 +92,6 @@ int main(int ac, char **av)strbuf_init (&sb);while (1 ) { -int len;char *ptr, *ntr;unsigned mode;enum object_type type;
@@ -101,7 +100,6 @@ int main(int ac, char **av)read_line (&sb, stdin, line_termination);if (sb.eof)break ; - len = sb.len; ptr = sb.buf;/* Input is non-recursive ls-tree output format * mode SP type SP sha1 TAB name
@@ -111,7 +109,7 @@ int main(int ac, char **av)die ("input format error: %s" , sb.buf); ptr = ntr +1 ;/* type */ ntr =strchr (ptr,' ' ); -if (!ntr || sb.buf + len <= ntr +41 || +if (!ntr || sb.buf + sb.len <= ntr +40 || ntr[41 ] !='\t' ||get_sha1_hex (ntr +1 , sha1))die ("input format error: %s" , sb.buf);
strbuf.c +89 -14
129 lines not on this page · raw message
strbuf.h +40 -2
53 lines not on this page · raw message
Signature · 1 line
1.5.3
PATCH 1/1 Simplify strbuf uses in archive-tar.c using the proper functions.
This is just cleaner way to deal with strbufs, using its API rather than
reinventing it in the module (e.g. strbuf_append_string is just the plain strbuf_addstr function, and it was used to perform what strbuf_addch does anyways).
--- archive-tar.c | 65 ++++++++++++++------------------------------------------- 1 files changed, 16 insertions(+), 49 deletions(-)
archive-tar.c +16 -49
@@ -78,19 +78,6 @@ static void write_trailer(void) } } -static void strbuf_append_string (struct strbuf *sb,const char *s) -{ -int slen =strlen (s); -int total = sb->len + slen; -if (total +1 > sb->alloc) { - sb->buf =xrealloc (sb->buf, total +1 ); - sb->alloc = total +1 ; - } -memcpy (sb->buf + sb->len, s, slen); - sb->len = total; - sb->buf[total] ='\0' ; -} -/* * pax extended header records have the format "%u %s=%s\n". %u contains * the size of the whole string (including the %u), the first %s is the
@@ -100,26 +87,17 @@ static void strbuf_append_string(struct strbuf *sb, const char *s)static void strbuf_append_ext_header (struct strbuf *sb,const char *keyword,const char *value,unsigned int valuelen) { -char *p; -int len, total, tmp; +int len, tmp;/* "%u %s=%s\n" */ len =1 +1 +strlen (keyword) +1 + valuelen +1 ;for (tmp = len; tmp >9 ; tmp /=10 ) len++; - total = sb->len + len; -if (total > sb->alloc) { - sb->buf =xrealloc (sb->buf, total); - sb->alloc = total; - } - - p = sb->buf; - p +=sprintf (p,"%u %s=" , len, keyword); -memcpy (p, value, valuelen); - p += valuelen; - *p ='\n' ; - sb->len = total; +strbuf_grow (sb, len); +strbuf_addf (sb,"%u %s=" , len, keyword); +strbuf_add (sb, value, valuelen); +strbuf_addch (sb,'\n' ); }static unsigned int ustar_header_chksum (const struct ustar_header *header)
@@ -153,8 +131,7 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,struct strbuf ext_header;memset (&header,0 ,sizeof (header)); - ext_header.buf =NULL ; - ext_header.len = ext_header.alloc =0 ; +strbuf_init (&ext_header);if (!sha1) { *header.typeflag =TYPEFLAG_GLOBAL_HEADER ;
@@ -225,8 +202,8 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,if (ext_header.len >0 ) {write_entry (sha1,NULL ,0 , ext_header.buf, ext_header.len); -free (ext_header.buf); } +strbuf_release (&ext_header);write_blocked (&header,sizeof (header));if (S_ISREG (mode) && buffer && size >0 )write_blocked (buffer, size);
@@ -235,11 +212,11 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,static void write_global_extended_header (const unsigned char *sha1) {struct strbuf ext_header; - ext_header.buf =NULL ; - ext_header.len = ext_header.alloc =0 ; + +strbuf_init (&ext_header);strbuf_append_ext_header (&ext_header,"comment" ,sha1_to_hex (sha1),40 );write_entry (NULL ,NULL ,0 , ext_header.buf, ext_header.len); -free (ext_header.buf); +strbuf_release (&ext_header); }static int git_tar_config (const char *var,const char *value)
@@ -260,28 +237,18 @@ static int write_tar_entry(const unsigned char *sha1, const char *base, int baselen, const char *filename, unsigned mode, int stage) { - static struct strbuf path; + static struct strbuf path = STRBUF_INIT;int filenamelen =strlen (filename);void *buffer;enum object_type type;unsigned long size; -if (!path.alloc) { - path.buf =xmalloc (PATH_MAX ); - path.alloc =PATH_MAX ; - path.len = path.eof =0 ; - } -if (path.alloc < baselen + filenamelen +1 ) { -free (path.buf); - path.buf =xmalloc (baselen + filenamelen +1 ); - path.alloc = baselen + filenamelen +1 ; - } -memcpy (path.buf, base, baselen); -memcpy (path.buf + baselen, filename, filenamelen); - path.len = baselen + filenamelen; - path.buf[path.len] ='\0' ; +strbuf_grow (&path,MAX (PATH_MAX , baselen + filenamelen +1 )); +strbuf_reset (&path); +strbuf_add (&path, base, baselen); +strbuf_add (&path, filename, filenamelen);if (S_ISDIR (mode) ||S_ISGITLINK (mode)) { -strbuf_append_string (&path,"/" ); +strbuf_addch (&path,'/' ); buffer =NULL ; size =0 ; }else {
Signature · 1 line
1.5.3
PATCH 1/1 Simplify strbuf uses in fast-import.c using the proper functions.
This is just cleaner way to deal with strbufs, using its API rather than
reinventing it in the module (e.g. strbuf_append_string is just the plain strbuf_addstr function, and it was used to perform what strbuf_addch does anyways).
--- archive-tar.c | 65 ++++++++++++++------------------------------------------- 1 files changed, 16 insertions(+), 49 deletions(-)
archive-tar.c +16 -49
@@ -78,19 +78,6 @@ static void write_trailer(void) } } -static void strbuf_append_string (struct strbuf *sb,const char *s) -{ -int slen =strlen (s); -int total = sb->len + slen; -if (total +1 > sb->alloc) { - sb->buf =xrealloc (sb->buf, total +1 ); - sb->alloc = total +1 ; - } -memcpy (sb->buf + sb->len, s, slen); - sb->len = total; - sb->buf[total] ='\0' ; -} -/* * pax extended header records have the format "%u %s=%s\n". %u contains * the size of the whole string (including the %u), the first %s is the
@@ -100,26 +87,17 @@ static void strbuf_append_string(struct strbuf *sb, const char *s)static void strbuf_append_ext_header (struct strbuf *sb,const char *keyword,const char *value,unsigned int valuelen) { -char *p; -int len, total, tmp; +int len, tmp;/* "%u %s=%s\n" */ len =1 +1 +strlen (keyword) +1 + valuelen +1 ;for (tmp = len; tmp >9 ; tmp /=10 ) len++; - total = sb->len + len; -if (total > sb->alloc) { - sb->buf =xrealloc (sb->buf, total); - sb->alloc = total; - } - - p = sb->buf; - p +=sprintf (p,"%u %s=" , len, keyword); -memcpy (p, value, valuelen); - p += valuelen; - *p ='\n' ; - sb->len = total; +strbuf_grow (sb, len); +strbuf_addf (sb,"%u %s=" , len, keyword); +strbuf_add (sb, value, valuelen); +strbuf_addch (sb,'\n' ); }static unsigned int ustar_header_chksum (const struct ustar_header *header)
@@ -153,8 +131,7 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,struct strbuf ext_header;memset (&header,0 ,sizeof (header)); - ext_header.buf =NULL ; - ext_header.len = ext_header.alloc =0 ; +strbuf_init (&ext_header);if (!sha1) { *header.typeflag =TYPEFLAG_GLOBAL_HEADER ;
@@ -225,8 +202,8 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,if (ext_header.len >0 ) {write_entry (sha1,NULL ,0 , ext_header.buf, ext_header.len); -free (ext_header.buf); } +strbuf_release (&ext_header);write_blocked (&header,sizeof (header));if (S_ISREG (mode) && buffer && size >0 )write_blocked (buffer, size);
@@ -235,11 +212,11 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,static void write_global_extended_header (const unsigned char *sha1) {struct strbuf ext_header; - ext_header.buf =NULL ; - ext_header.len = ext_header.alloc =0 ; + +strbuf_init (&ext_header);strbuf_append_ext_header (&ext_header,"comment" ,sha1_to_hex (sha1),40 );write_entry (NULL ,NULL ,0 , ext_header.buf, ext_header.len); -free (ext_header.buf); +strbuf_release (&ext_header); }static int git_tar_config (const char *var,const char *value)
@@ -260,28 +237,18 @@ static int write_tar_entry(const unsigned char *sha1, const char *base, int baselen, const char *filename, unsigned mode, int stage) { - static struct strbuf path; + static struct strbuf path = STRBUF_INIT;int filenamelen =strlen (filename);void *buffer;enum object_type type;unsigned long size; -if (!path.alloc) { - path.buf =xmalloc (PATH_MAX ); - path.alloc =PATH_MAX ; - path.len = path.eof =0 ; - } -if (path.alloc < baselen + filenamelen +1 ) { -free (path.buf); - path.buf =xmalloc (baselen + filenamelen +1 ); - path.alloc = baselen + filenamelen +1 ; - } -memcpy (path.buf, base, baselen); -memcpy (path.buf + baselen, filename, filenamelen); - path.len = baselen + filenamelen; - path.buf[path.len] ='\0' ; +strbuf_grow (&path,MAX (PATH_MAX , baselen + filenamelen +1 )); +strbuf_reset (&path); +strbuf_add (&path, base, baselen); +strbuf_add (&path, filename, filenamelen);if (S_ISDIR (mode) ||S_ISGITLINK (mode)) { -strbuf_append_string (&path,"/" ); +strbuf_addch (&path,'/' ); buffer =NULL ; size =0 ; }else {
Signature · 1 line
1.5.3
PATCH 1/1 Use proper strbuf API, and also simplify cmd_data code.
This patch features the use of strbuf_detach, and prevent the programmer
to mess with allocation directly. The code is as efficent as before, just more concise and more straightforward.
--- fast-import.c | 29 ++++++++++++----------------- 1 files changed, 12 insertions(+), 17 deletions(-)
fast-import.c +12 -17
@@ -340,7 +340,7 @@ static struct tag *last_tag;/* Input stream parsing */ static whenspec_type whenspec =WHENSPEC_RAW ; -static struct strbuf command_buf; +static struct strbuf command_buf =STRBUF_INIT ;static int unread_command_buf;static struct recent_command cmd_hist = {&cmd_hist, &cmd_hist,NULL };static struct recent_command *cmd_tail = &cmd_hist;
@@ -1638,17 +1638,15 @@ static void cmd_mark(void)static void *cmd_data (size_t *size) { -size_t length; -char *buffer; +struct strbuf buffer =STRBUF_INIT ;if (prefixcmp (command_buf.buf,"data " ))die ("Expected 'data n' command, found: %s" , command_buf.buf);if (!prefixcmp (command_buf.buf +5 ,"<<" )) {char *term =xstrdup (command_buf.buf +5 +2 ); -size_t sz =8192 , term_len = command_buf.len -5 -2 ; - length =0 ; - buffer =xmalloc (sz); +size_t term_len = command_buf.len -5 -2 ; +for (;;) {read_line (&command_buf, stdin,'\n' );if (command_buf.eof)
@@ -1656,21 +1654,18 @@ static void *cmd_data (size_t *size)if (term_len == command_buf.len && !strcmp (term, command_buf.buf))break ; -ALLOC_GROW (buffer, length + command_buf.len +1 , sz); -memcpy (buffer + length, - command_buf.buf, - command_buf.len); - length += command_buf.len; - buffer[length++] ='\n' ; +strbuf_addbuf (&buffer, &command_buf); +strbuf_addch (&buffer,'\n' ); }free (term); }else { -size_t n =0 ; +size_t n =0 , length; + length =strtoul (command_buf.buf +5 ,NULL ,10 ); - buffer =xmalloc (length); +while (n < length) { -size_t s =fread (buffer + n,1 , length - n, stdin); +size_t s =strbuf_fread (&buffer, length - n, stdin);if (!s &&feof (stdin))die ("EOF in data (%lu bytes remaining)" , (unsigned long )(length - n));
@@ -1679,8 +1674,8 @@ static void *cmd_data (size_t *size) }skip_optional_lf (); - *size = length; -return buffer; + *size = buffer.len; +return strbuf_detach (&buffer); }static int validate_raw_date (const char *src,char *result,int maxlen)
Signature · 1 line
1.5.3
PATCH 1/1 Simplify write_tree using strbuf's.
--- mktree.c | 22 +++++++--------------- 1 files changed, 7 insertions(+), 15 deletions(-)
mktree.c +7 -15
@@ -44,30 +44,22 @@ static int ent_compare(const void *a_, const void *b_)static void write_tree (unsigned char *sha1) { -char *buffer; -unsigned long size, offset; +struct strbuf buf =STRBUF_INIT ; +size_t size;int i;qsort (entries, used,sizeof (*entries), ent_compare);for (size = i =0 ; i < used; i++) size +=32 + entries[i]->len; - buffer =xmalloc (size); - offset =0 ; +strbuf_grow (&buf, size);for (i =0 ; i < used; i++) {struct treeent *ent = entries[i]; - -if (offset + ent->len +100 < size) { - size =alloc_nr (offset + ent->len +100 ); - buffer =xrealloc (buffer, size); - } - offset +=sprintf (buffer + offset,"%o " , ent->mode); - offset +=sprintf (buffer + offset,"%s" , ent->name); - buffer[offset++] =0 ; -hashcpy ((unsigned char *)buffer + offset, ent->sha1); - offset +=20 ; +strbuf_addf (&buf,"%o %s%c" , ent->mode, ent->name,'\0' ); +strbuf_add (&buf, ent->sha1,20 ); } -write_sha1_file (buffer, offset, tree_type, sha1); + +write_sha1_file (buf.buf, buf.len, tree_type, sha1); }static const char mktree_usage[] ="git-mktree [-z]" ;
Signature · 1 line
1.5.3
PATCH 1/1 Further strbuf re-engineering.
--- builtin-apply.c | 29 ++++++----------------- builtin-blame.c | 34 +++++++++------------------- builtin-commit-tree.c | 59 ++++++++++-------------------------------------- diff.c | 25 ++++++-------------- 4 files changed, 40 insertions(+), 107 deletions(-)
builtin-apply.c +8 -21
@@ -12,6 +12,7 @@#include "blob.h" #include "delta.h" #include "builtin.h" +#include "strbuf.h" /* * --check turns on checking that the working tree matches the
@@ -181,34 +182,20 @@ static void say_patch_name(FILE *output, const char *pre, struct patch *patch, cstatic void *read_patch_file (int fd,unsigned long *sizep) { -unsigned long size =0 , alloc =CHUNKSIZE ; -void *buffer =xmalloc (alloc); +struct strbuf buf =STRBUF_INIT ; -for (;;) { -ssize_t nr = alloc - size; -if (nr <1024 ) { - alloc +=CHUNKSIZE ; - buffer =xrealloc (buffer, alloc); - nr = alloc - size; - } - nr =xread (fd, (char *) buffer + size, nr); -if (!nr) -break ; -if (nr <0 ) -die ("git-apply: read returned %s" ,strerror (errno)); - size += nr; - } - *sizep = size; +if (strbuf_read (&buf, fd) <0 ) +die ("git-apply: read returned %s" ,strerror (errno)); + *sizep = buf.len;/* * Make sure that we have some slop in the buffer * so that we can do speculative "memcmp" etc, and * see to it that it is NUL-filled. */ -if (alloc < size +SLOP ) - buffer =xrealloc (buffer, size +SLOP ); -memset ((char *) buffer + size,0 ,SLOP ); -return buffer; +strbuf_grow (&buf,SLOP ); +memset (buf.buf + buf.len,0 ,SLOP ); +return strbuf_detach (&buf); }static unsigned long linelen (const char *buffer,unsigned long size)
builtin-blame.c +11 -23
@@ -18,6 +18,7 @@#include "cache-tree.h" #include "path-list.h" #include "mailmap.h" +#include "strbuf.h" static char blame_usage[] ="git-blame [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-p] [-w] [-L n,m] [-S <revs-file>] [-M] [-C] [-C] [--contents <filename>] [--incremental] [commit] [--] file\n"
@@ -2001,11 +2002,10 @@ static struct commit *fake_working_tree_commit(const char *path, const char *construct commit *commit;struct origin *origin;unsigned char head_sha1[20 ]; -char *buf; +struct strbuf buf =STRBUF_INIT ;const char *ident;int fd;time_t now; -unsigned long fin_size;int size, len;struct cache_entry *ce;unsigned mode;
@@ -2026,6 +2026,7 @@ static struct commit *fake_working_tree_commit(const char *path, const char *conif (!contents_from ||strcmp ("-" , contents_from)) {struct stat st;const char *read_from; +unsigned long fin_size;if (contents_from) {if (stat (contents_from, &st) <0 )
@@ -2038,19 +2039,19 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con read_from = path; } fin_size =xsize_t (st.st_size); - buf =xmalloc (fin_size+1 ); mode =canon_mode (st.st_mode);switch (st.st_mode &S_IFMT ) {case S_IFREG : fd =open (read_from,O_RDONLY );if (fd <0 )die ("cannot open %s" , read_from); -if (read_in_full (fd, buf, fin_size) != fin_size) +if (strbuf_read (&buf, fd) !=xsize_t (st.st_size))die ("cannot read %s" , read_from);break ;case S_IFLNK : -if (readlink (read_from, buf, fin_size+1 ) != fin_size) +if (readlink (read_from, buf.buf, buf.alloc) != fin_size)die ("cannot readlink %s" , read_from); + buf.len = fin_size;break ;default :die ("unsupported file type %s" , read_from);
@@ -2059,26 +2060,13 @@ static struct commit *fake_working_tree_commit(const char *path, const char *conelse {/* Reading from stdin */ contents_from ="standard input" ; - buf =NULL ; - fin_size =0 ; mode =0 ; -while (1 ) { -ssize_t cnt =8192 ; - buf =xrealloc (buf, fin_size + cnt); - cnt =xread (0 , buf + fin_size, cnt); -if (cnt <0 ) -die ("read error %s from stdin" , -strerror (errno)); -if (!cnt) -break ; - fin_size += cnt; - } - buf =xrealloc (buf, fin_size +1 ); +if (strbuf_read (&buf,0 ) <0 ) +die ("read error %s from stdin" ,strerror (errno)); } - buf[fin_size] =0 ; - origin->file.ptr = buf; - origin->file.size = fin_size; -pretend_sha1_file (buf, fin_size,OBJ_BLOB , origin->blob_sha1); + origin->file.ptr = buf.buf; + origin->file.size = buf.len; +pretend_sha1_file (buf.buf, buf.len,OBJ_BLOB , origin->blob_sha1); commit->util = origin;/*
builtin-commit-tree.c +13 -46
98 lines not on this page · raw message
diff.c +8 -17
@@ -9,6 +9,7 @@#include "xdiff-interface.h" #include "color.h" #include "attr.h" +#include "strbuf.h" #ifdef NO_FAST_WORKING_DIRECTORY #define FAST_WORKING_DIRECTORY 0
@@ -1546,25 +1547,15 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, intstatic int populate_from_stdin (struct diff_filespec *s) {#define INCREMENT 1024 -char *buf; -unsigned long size; -ssize_t got; - - size =0 ; - buf =NULL ; -while (1 ) { - buf =xrealloc (buf, size +INCREMENT ); - got =xread (0 , buf + size,INCREMENT ); -if (!got) -break ;/* EOF */ -if (got <0 ) -return error ("error while reading from stdin %s" , +struct strbuf buf =STRBUF_INIT ; + +if (strbuf_read (&buf,0 ) <0 ) +return error ("error while reading from stdin %s" ,strerror (errno)); - size += got; - } + s->should_munmap =0 ; - s->data = buf; - s->size = size; + s->size = buf.len; + s->data =strbuf_detach (&buf); s->should_free =1 ;return 0 ; }
Signature · 1 line
1.5.3
PATCH 1/1 Eradicate yet-another-buffer implementation in buitin-rerere.c
--- builtin-rerere.c | 53 +++++++++++++++-------------------------------------- 1 files changed, 15 insertions(+), 38 deletions(-)
builtin-rerere.c +15 -38
@@ -1,6 +1,7 @@#include "builtin.h" #include "cache.h" #include "path-list.h" +#include "strbuf.h" #include "xdiff/xdiff.h" #include "xdiff-interface.h"
@@ -66,38 +67,14 @@ static int write_rr(struct path_list *rr, int out_fd)return commit_lock_file (&write_lock); } -struct buffer { -char *ptr; -int nr, alloc; -}; - -static void append_line (struct buffer *buffer,const char *line) -{ -int len =strlen (line); - -if (buffer->nr + len > buffer->alloc) { - buffer->alloc =alloc_nr (buffer->nr + len); - buffer->ptr =xrealloc (buffer->ptr, buffer->alloc); - } -memcpy (buffer->ptr + buffer->nr, line, len); - buffer->nr += len; -} - -static void clear_buffer (struct buffer *buffer) -{ -free (buffer->ptr); - buffer->ptr =NULL ; - buffer->nr = buffer->alloc =0 ; -} -static int handle_file (const char *path,unsigned char *sha1,const char *output) {SHA_CTX ctx;char buf[1024 ];int hunk =0 , hunk_no =0 ; -struct buffer minus = {NULL ,0 ,0 }, plus = {NULL ,0 ,0 }; -struct buffer *one = &minus, *two = + +struct strbuf minus =STRBUF_INIT , plus =STRBUF_INIT ; +struct strbuf *one = &minus, *two = +FILE *f =fopen (path,"r" );FILE *out;
@@ -122,36 +99,36 @@ static int handle_file(const char *path,else if (!prefixcmp (buf,"=======" )) hunk =2 ;else if (!prefixcmp (buf,">>>>>>> " )) { -int one_is_longer = (one->nr > two->nr); -int common_len = one_is_longer ? two->nr : one->nr; -int cmp =memcmp (one->ptr, two->ptr, common_len); +int one_is_longer = (one->len > two->len); +int common_len = one_is_longer ? two->len : one->len; +int cmp =memcmp (one->buf, two->buf, common_len); hunk_no++; hunk =0 ;if ((cmp >0 ) || ((cmp ==0 ) && one_is_longer)) { -struct buffer *swap = one; +struct strbuf *swap = one; one = two; two = swap; }if (out) {fputs ("<<<<<<<\n" , out); -fwrite (one->ptr, one->nr,1 , out); +fwrite (one->buf, one->len,1 , out);fputs ("=======\n" , out); -fwrite (two->ptr, two->nr,1 , out); +fwrite (two->buf, two->len,1 , out);fputs (">>>>>>>\n" , out); }if (sha1) { -SHA1_Update (&ctx, one->ptr, one->nr); +SHA1_Update (&ctx, one->buf, one->len);SHA1_Update (&ctx,"\0" ,1 ); -SHA1_Update (&ctx, two->ptr, two->nr); +SHA1_Update (&ctx, two->buf, two->len);SHA1_Update (&ctx,"\0" ,1 ); } -clear_buffer (one); -clear_buffer (two); +strbuf_release (one); +strbuf_release (two); }else if (hunk ==1 ) -append_line (one, buf); +strbuf_addstr (one, buf);else if (hunk ==2 ) -append_line (two, buf); +strbuf_addstr (two, buf);else if (out)fputs (buf, out); }
Signature · 1 line
1.5.3
PATCH 1/1 More strbuf uses in cache-tree.c.
Should even be marginally faster.
--- cache-tree.c | 57 ++++++++++++++++++++------------------------------------- 1 files changed, 20 insertions(+), 37 deletions(-)
cache-tree.c +20 -37
@@ -1,4 +1,5 @@#include "cache.h" +#include "strbuf.h" #include "tree.h" #include "cache-tree.h"
@@ -235,8 +236,7 @@ static int update_one(struct cache_tree *it, int missing_ok, int dryrun) { -unsigned long size, offset; -char *buffer; +struct strbuf buffer =STRBUF_INIT ;int i;if (0 <= it->entry_count &&has_sha1_file (it->sha1))
@@ -293,9 +293,7 @@ static int update_one(struct cache_tree *it,/* * Then write out the tree object for this level. */ - size =8192 ; - buffer =xmalloc (size); - offset =0 ; +strbuf_grow (&buffer,8192 );for (i =0 ; i < entries; i++) {struct cache_entry *ce = cache[i];
@@ -332,15 +330,9 @@ static int update_one(struct cache_tree *it,if (!ce->ce_mode)continue ;/* entry being removed */ -if (size < offset + entlen +100 ) { - size =alloc_nr (offset + entlen +100 ); - buffer =xrealloc (buffer, size); - } - offset +=sprintf (buffer + offset, -"%o %.*s" , mode, entlen, path + baselen); - buffer[offset++] =0 ; -hashcpy ((unsigned char *)buffer + offset, sha1); - offset +=20 ; +strbuf_grow (&buffer, entlen +100 ); +strbuf_addf (&buffer,"%o %.*s%c" , mode, entlen, path + baselen,'\0' ); +strbuf_add (&buffer, sha1,20 );#if DEBUG fprintf(stderr, "cache-tree update-one %o %.*s\n",
@@ -349,10 +341,10 @@ static int update_one(struct cache_tree *it, }if (dryrun) -hash_sha1_file (buffer, offset, tree_type, it->sha1); +hash_sha1_file (buffer.buf, buffer.len, tree_type, it->sha1);else -write_sha1_file (buffer, offset, tree_type, it->sha1); -free (buffer); +write_sha1_file (buffer.buf, buffer.len, tree_type, it->sha1); +strbuf_release (&buffer); it->entry_count = i;#if DEBUG fprintf(stderr, "cache-tree update-one (%d ent, %d subtree) %s\n",
@@ -378,12 +370,10 @@ int cache_tree_update(struct cache_tree *it,return 0 ; } -static void *write_one (struct cache_tree *it, +static void write_one (struct cache_tree *it,char *path,int pathlen, -char *buffer, -unsigned long *size, -unsigned long *offset) +struct strbuf *buffer) {int i;
@@ -393,13 +383,9 @@ static void *write_one(struct cache_tree *it,* tree-sha1 (missing if invalid) * subtree_nr "cache-tree" entries for subtrees. */ -if (*size < *offset + pathlen +100 ) { - *size =alloc_nr (*offset + pathlen +100 ); - buffer =xrealloc (buffer, *size); - } - *offset +=sprintf (buffer + *offset,"%.*s%c%d %d\n" , - pathlen, path,0 , - it->entry_count, it->subtree_nr); +strbuf_grow (buffer, pathlen +100 ); +strbuf_add (buffer, path, pathlen); +strbuf_addf (buffer,"%c%d %d\n" ,0 , it->entry_count, it->subtree_nr);#if DEBUG if (0 <= it->entry_count)
@@ -412,8 +398,7 @@ static void *write_one(struct cache_tree *it,#endif if (0 <= it->entry_count) { -hashcpy ((unsigned char *)buffer + *offset, it->sha1); - *offset +=20 ; +strbuf_add (buffer, it->sha1,20 ); }for (i =0 ; i < it->subtree_nr; i++) {struct cache_tree_sub *down = it->down[i];
@@ -423,21 +408,19 @@ static void *write_one(struct cache_tree *it, prev->name, prev->namelen) <= 0)die ("fatal - unsorted cache subtree" ); } - buffer =write_one (down->cache_tree, down->name, down->namelen, - buffer, size, offset); +write_one (down->cache_tree, down->name, down->namelen, buffer); } -return buffer; }void *cache_tree_write (struct cache_tree *root,unsigned long *size_p) {char path[PATH_MAX ]; -unsigned long size =8192 ; -char *buffer =xmalloc (size); +struct strbuf buffer =STRBUF_INIT ; - *size_p =0 ; path[0 ] =0 ; -return write_one (root, path,0 , buffer, &size, size_p); +write_one (root, path,0 , &buffer); + *size_p = buffer.len; +return strbuf_detach (&buffer); }static struct cache_tree *read_one (const char **buffer,unsigned long *size_p)
Signature · 1 line
1.5.3
Re: [PATCH] Further strbuf re-engineering.
In reply to Pierre HabouzitQuoted 13 lines · Pierre Habouzit
Signed-off-by Pierre Habouzit <[email protected]>--- builtin-apply.c | 29 ++++++----------------- builtin-blame.c | 34 +++++++++------------------- builtin-commit-tree.c | 59 ++++++++++-------------------------------------- diff.c | 25 ++++++-------------- 4 files changed, 40 insertions(+), 107 deletions(-)...
builtin-commit-tree.c +0
-0
builtin-commit-tree.c line 17
Quoted 4 lines
builtin-commit-tree.c +0
-0@@ -8,42 +8,13 @@/* * FIXME! Share the code with "write-tree.c" */
You fixed it. Remove this comment.
Re: [PATCH] Simplify strbuf uses in fast-import.c using the proper functions.
In reply to Pierre Habouzit Sorry, this one is a false positive :|
I should have been more careful.
Quoted 4 lines · Pierre Habouzit
This is just cleaner way to deal with strbufs, using its API rather than
reinventing it in the module (e.g.strbuf_append_stringis just the plainstrbuf_addstrfunction, and it was used to perform whatstrbuf_addchdoes anyways).
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org
Re: [PATCH] Simplify strbuf uses in archive-tar.c using the proper functions.
In reply to Pierre HabouzitQuoted 28 lines · Pierre Habouzit
This is just cleaner way to deal with strbufs, using its API rather than
reinventing it in the module (e.g.strbuf_append_stringis just the plainstrbuf_addstrfunction, and it was used to perform whatstrbuf_addchdoes anyways).--- archive-tar.c | 65 ++++++++++++++------------------------------------------- 1 files changed, 16 insertions(+), 49 deletions(-)archive-tar.c +1
-6@@ -260,28 +237,18 @@ static int write_tar_entry(const unsigned char *sha1, const char *base, int baselen, const char *filename, unsigned mode, int stage) { - static struct strbuf path; + static struct strbuf path = STRBUF_INIT;int filenamelen =strlen (filename);void *buffer;enum object_type type;unsigned long size; -if (!path.alloc) { - path.buf =xmalloc (PATH_MAX ); - path.alloc =PATH_MAX ; - path.len = path.eof =0 ; - }
archive-tar.c static int write_tar_entry(const unsigned char *sha1, · line 246
Quoted 10 lines
archive-tar.c +1
-9-if (path.alloc < baselen + filenamelen +1 ) { -free (path.buf); - path.buf =xmalloc (baselen + filenamelen +1 ); - path.alloc = baselen + filenamelen +1 ; - } -memcpy (path.buf, base, baselen); -memcpy (path.buf + baselen, filename, filenamelen); - path.len = baselen + filenamelen; - path.buf[path.len] ='\0' ; +strbuf_grow (&path,MAX (PATH_MAX , baselen + filenamelen +1 ));
Where are you getting the MAX() macro from? On my Linux box it appears that <sys/params.h> happens to define it but I do not think that is something we can rely upon portably.
Moreover, isn't this allocation wrong? I thought "grow" was about "we want this much more in addition to the existing length", not "reserve at least this much", and this "path" is a static that will keep the buffer and length from the previous invocation.
archive-tar.c static int write_tar_entry(const unsigned char *sha1, · line 254
Quoted 9 lines
archive-tar.c +4
-1+strbuf_reset (&path); +strbuf_add (&path, base, baselen); +strbuf_add (&path, filename, filenamelen);if (S_ISDIR (mode) ||S_ISGITLINK (mode)) { -strbuf_append_string (&path,"/" ); +strbuf_addch (&path,'/' ); buffer =NULL ; size =0 ; }else {
Having said that, I suspect that the preallocation does not really matter in practice. How about doing something like:
strbuf_reset(&path);
strbuf_grow(&path, PATH_MAX);
strbuf_add(&path, base, baselen);
strbuf_add(&path, filename, filenamelen);Re: [PATCH] Simplify strbuf uses in archive-tar.c using the proper functions.
Quoted 6 lines · Junio C Hamano
archive-tar.c static int write_tar_entry(const unsigned char *sha1, · line 246
Quoted 1 line · Pierre Habouzit
archive-tar.c +1
-0+strbuf_grow (&path,MAX (PATH_MAX , baselen + filenamelen +1 ));Where are you getting the
MAX()macro from? On my Linux box it appears that <sys/params.h> happens to define it but I do not think that is something we can rely upon portably.
indeed.
Quoted 5 lines
Moreover, isn't this allocation wrong? I thought "grow" was about "we want this much more in addition to the existing length", not "reserve at least this much", and this "path" is a static that will keep the buffer and length from the previous invocation.
I'm a nitwit, the strbuf_reset() should be done _before_ the grow
indeed.
Quoted 13 lines
archive-tar.c static int write_tar_entry(const unsigned char *sha1, · line 254
Quoted 9 lines
archive-tar.c +4
-1+strbuf_reset (&path); +strbuf_add (&path, base, baselen); +strbuf_add (&path, filename, filenamelen);if (S_ISDIR (mode) ||S_ISGITLINK (mode)) { -strbuf_append_string (&path,"/" ); +strbuf_addch (&path,'/' ); buffer =NULL ; size =0 ; }else {Having said that, I suspect that the preallocation does not really matter in practice. How about doing something like:
Quoted 4 lines
strbuf_reset(&path);
strbuf_grow(&path, PATH_MAX);
strbuf_add(&path, base, baselen);
strbuf_add(&path, filename, filenamelen); yeah, I was about to propose the same, I'll add a patch in my current
series to fix that this way.
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org
Re: [PATCH] Rework strbuf API and semantics.
In reply to Pierre HabouzitQuoted 5 lines · Pierre Habouzit
A strbuf can be used to store byte arrays, or as an extended string
library. Thebuf' member can be passed to any C legacy string function, because strbuf operations always ensure there is a terminating \0 at the end of the buffer, not accounted in thelen' field of the structure.
Quoted 8 lines
A strbuf can be used to generate a string/buffer whose final size is not
really known, and then "strbuf_detach" can be used to get the built buffer, and keep the wrapping "strbuf" structure usable for further work again.Other interesting feature:
strbuf_grow(sb, size)ensure that there is
enough allocated space insb' to putsize' new octets of data in the buffer. It helps avoiding reallocating data for nothing when the problem the strbuf helps to solve has a known typical size.
"Rework API semantics" needs to be accompanied with an API description, perhaps at the beginning of each externally visible function.
Also the commit log message needs to explain what the old semantics was and what the improved one is, to highlight the changes needed to the callers. Especially...
Quoted 5 lines
fast-import.c +0
-1@@ -1657,11 +1656,11 @@ static void *cmd_data (size_t *size)if (term_len == command_buf.len && !strcmp (term, command_buf.buf))break ; -ALLOC_GROW (buffer, length + command_buf.len, sz);
fast-import.c static void *cmd_data (size_t *size) · line 1666
Quoted 10 lines
fast-import.c +3
-2+ALLOC_GROW (buffer, length + command_buf.len +1 , sz);memcpy (buffer + length, command_buf.buf, - command_buf.len -1 ); - length += command_buf.len -1 ; + command_buf.len); + length += command_buf.len; buffer[length++] ='\n' ; }free (term);
... it is not all obvious why these off-by-one changes are needed without such a description. The other hunks in this patch to this file are all such changes.
strbuf.h line 32
Quoted 7 lines
strbuf.h +5
-1-static void inline strbuf_add (struct strbuf *sb,int ch) { +static inline void strbuf_addch (struct strbuf *sb,size_t c) { +strbuf_grow (sb,1 ); + sb->buf[sb->len++] = c; + sb->buf[sb->len] ='\0' ; +}
You certainly did not mean size_t wide characters.
Re: [PATCH] Rework strbuf API and semantics.
Quoted 35 lines · Junio C Hamano
Quoted 5 lines · Pierre Habouzit
A strbuf can be used to store byte arrays, or as an extended string
library. Thebuf' member can be passed to any C legacy string function, because strbuf operations always ensure there is a terminating \0 at the end of the buffer, not accounted in thelen' field of the structure.Quoted 8 lines
A strbuf can be used to generate a string/buffer whose final size is not
really known, and then "strbuf_detach" can be used to get the built buffer, and keep the wrapping "strbuf" structure usable for further work again.Other interesting feature:
strbuf_grow(sb, size)ensure that there is
enough allocated space insb' to putsize' new octets of data in the buffer. It helps avoiding reallocating data for nothing when the problem the strbuf helps to solve has a known typical size."Rework API semantics" needs to be accompanied with an API description, perhaps at the beginning of each externally visible function.
Also the commit log message needs to explain what the old semantics was and what the improved one is, to highlight the changes needed to the callers. Especially...
Quoted 11 lines
fast-import.c +2
-3@@ -1657,11 +1656,11 @@ static void *cmd_data (size_t *size)if (term_len == command_buf.len && !strcmp (term, command_buf.buf))break ; -ALLOC_GROW (buffer, length + command_buf.len, sz); +ALLOC_GROW (buffer, length + command_buf.len +1 , sz);memcpy (buffer + length, command_buf.buf, - command_buf.len -1 ); - length += command_buf.len -1 ; + command_buf.len);
Quoted 8 lines
fast-import.c static void *cmd_data (size_t *size) · line 1666
Quoted 4 lines
fast-import.c +1
-0+ length += command_buf.len; buffer[length++] ='\n' ; }free (term);.... it is not all obvious why these off-by-one changes are needed without such a description. The other hunks in this patch to this file are all such changes.
Yes, as I suppose you know, but I state it here again so that
everybody understands, before strbuf's were merely a byte array, not necessarily NUL-terminated. Hence many parts of the code that wanted to pass the buffer to str* functions had to manually insert a NUL, hence it was accounted in the length of the buffer.
Now, we always have a NUL after the "official" end of the buffer, so
it's not needed anymore. The off-by-ones are just that. The hunk you quote is one where git's code was messing with strbufs internals directly, so the ALLOC_GROW has to take the 1 octed needed to maintain the internal invariant. Though, the patch after this one rewrites the hunk to use strbuf's API's.
Quoted 10 lines
strbuf.h line 32
Quoted 8 lines
strbuf.h +5
-1-static void inline strbuf_add (struct strbuf *sb,int ch) { +static inline void strbuf_addch (struct strbuf *sb,size_t c) { +strbuf_grow (sb,1 ); + sb->buf[sb->len++] = c; + sb->buf[sb->len] ='\0' ; +}You certainly did not mean
size_twide characters.
Oh boy, now I've been red-handed of :%s/\<int\>/\<size_t\>/ :)
I'll repost a _clean_ patch series soon with those things fixed, and
the wrong overflow test (that should be a >= and not a >) as well, as we discussed it on IRC before.
Oh and FWIW I believe the details of the NUL always after the buffer
array has to be in strbuf.h and not in the commit comment, maybe I'll put it in both to make everybody happy.
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org
Re: [PATCH] Rework strbuf API and semantics.
In reply to Pierre HabouzitQuoted 13 lines · Pierre Habouzit
strbuf.c +11
-0+void strbuf_addvf (struct strbuf *sb,const char *fmt,va_list ap) +{ +size_t len; +va_list ap2; + +va_copy (ap2, ap); + + len =vsnprintf (sb->buf + sb->len, sb->alloc - sb->len, fmt, ap); +if (len <0 ) { + len =0 ; + }
strbuf.c line 57
Quoted 10 lines
strbuf.c +9
-0+if (len >= sb->alloc - sb->len) { +strbuf_grow (sb, len); + len =vsnprintf (sb->buf + sb->len, sb->alloc - sb->len, fmt, ap2); +if (len >= sb->alloc - sb->len) { + len = sb->alloc - sb->len -1 ; + } + } + sb->len = sb->len + len; + sb->buf[sb->len] ='\0' ; }
Hmmmmm... Didn't we recently had a patch that used va_copy() which was not available somewhere and was shot down?
Instead of that nice inline strbuf_addf(), you may have to do something like:
strbuf_addf(..., fmt, ...) {
va_list ap;
va_start(ap, fmt);
len = vsnprintf(...);
va_end(ap);
if (len >= sb_avail(sb)) {
strbuf_grow(sb, len);
va_start(ap, fmt);
len = vsnprintf(...);
va_end(ap);
if (len >= sb_avail(sb))
gaah();
}
sb->len += len;
sb->buf[sb->len] = '\0';
}strbuf.c line 81
Quoted 11 lines
strbuf.c +11
-0+ssize_t strbuf_read (struct strbuf *sb,int fd) +{ +size_t oldlen = sb->len; + +for (;;) { +ssize_t cnt; + +strbuf_grow (sb,8192 ); + cnt =xread (fd, sb->buf + sb->len, sb->alloc - sb->len -1 ); +if (cnt <0 ) { + sb->buf[sb->len = oldlen] ='\0' ;
Assignment inside array subscript is very hard to read. Besides, what's the error semantics? On error, this behaves as if no bytes are read (i.e. partial read results in the initial round is lost forever)?
Re: [PATCH] Rework strbuf API and semantics.
Quoted 41 lines · Junio C Hamano
Quoted 13 lines · Pierre Habouzit
strbuf.c +11
-0+void strbuf_addvf (struct strbuf *sb,const char *fmt,va_list ap) +{ +size_t len; +va_list ap2; + +va_copy (ap2, ap); + + len =vsnprintf (sb->buf + sb->len, sb->alloc - sb->len, fmt, ap); +if (len <0 ) { + len =0 ; + }strbuf.c line 57
Quoted 10 lines
strbuf.c +9
-0+if (len >= sb->alloc - sb->len) { +strbuf_grow (sb, len); + len =vsnprintf (sb->buf + sb->len, sb->alloc - sb->len, fmt, ap2); +if (len >= sb->alloc - sb->len) { + len = sb->alloc - sb->len -1 ; + } + } + sb->len = sb->len + len; + sb->buf[sb->len] ='\0' ; }Hmmmmm... Didn't we recently had a patch that used
va_copy()which was not available somewhere and was shot down?Instead of that nice inline
strbuf_addf(), you may have to do something like:strbuf_addf(..., fmt, ...) { va_list ap; va_start(ap, fmt); len = vsnprintf(...); va_end(ap); if (len >= sb_avail(sb)) { strbuf_grow(sb, len); va_start(ap, fmt);
Quoted 8 lines
len = vsnprintf(...);
va_end(ap);
if (len >= sb_avail(sb))
gaah();
}
sb->len += len;
sb->buf[sb->len] = '\0';
}I'll do that.
Quoted 8 lines
Quoted 8 lines
strbuf.c +8
-0+ssize_t strbuf_read (struct strbuf *sb,int fd) +{ +size_t oldlen = sb->len; + +for (;;) { +ssize_t cnt; + +strbuf_grow (sb,8192 );
Quoted 8 lines
strbuf.c line 81
Quoted 3 lines
strbuf.c +3
-0+ cnt =xread (fd, sb->buf + sb->len, sb->alloc - sb->len -1 ); +if (cnt <0 ) { + sb->buf[sb->len = oldlen] ='\0' ;Assignment inside array subscript is very hard to read. Besides, what's the error semantics? On error, this behaves as if no bytes are read (i.e. partial read results in the initial round is lost forever)?
Yes that is the semantics, because it's how it was used everywhere in
git: either we were able to load all the data from the file descriptor, or weren't able to and the next thing we do is to die().
Maybe I should call the function strbuf_xread() instead ?
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org
Re: [PATCH] Rework strbuf API and semantics.
In reply to Pierre HabouzitQuoted 13 lines · Pierre Habouzit
Quoted 5 lines · Johannes Schindelin
Hi,
On Tue, 4 Sep 2007, Pierre Habouzit wrote:
- IMHO the same goes for
strbuf_free()instead ofstrbuf_wipe(), andWell, I don't like
strbuf_free, because it's opposed to
strbuf_alloc/new whatever, that would be functions that create and release a struct strbuf * (meaning allocating the struct shell as well). Here you only free the internal buffer, not the shell, so_free()would be a very bad name.
Quoted 5 lines
In my own coding rules, I have _new/_delete and _init/_wipe functions,
the former acts on pointers, the latter on the structs. Hence the naming. Though, looking at git's code, it seems that the usual name for this operation is_release. So would you go forstrbuf_release?
release is indeed better. To me, "wipe" means zeroing out (possibly after an optional number of passes writing trash to the memory area) rather than actually freeing any memory.
Quoted 6 lines
Quoted 6 lines
- it would be nice to split this patch into
- the API change (with _minimal_ changes to anything outside of strbuf.[ch]), and
Quoted 9 lines
Quoted 1 line
- the cleanups in the rest of the code.
I'll try to do that, but it's quite hard to achieve knowing that in
many places of the current state of master, there are embeded NUL's (accounted in ->len). I'll try to see what I can split, but it's unlikely I'll be able to have an intermediate _working_ state (I mean that would pass the testsuite) with the new API/semantics _and_ without touching a lot less of the rest of the code.
Always having a compilable/operational tree is pretty nice primarily due to bisect. If the code turns from working to non-working in between, make sure to jot it down in the commit message.
It might even be better to add the embedded NUL handling code in the first patch, modify the callers in the second, and remove the embedded NUL handling in a third.
Signature · 3 lines
Andreas Ericsson [email protected] OP5 AB www.op5.se Tel: +46 8-230225 Fax: +46 8-230231
Re: [PATCH] Rework strbuf API and semantics.
In reply to Pierre HabouzitQuoted 3 lines · Pierre Habouzit
In my own coding rules, I have _new/_delete and _init/_wipe functions,
the former acts on pointers, the latter on the structs. Hence the naming.
A little off-topic, but my own personal convention for the latter case are _init/_fini -- I've always thought the "fini" name was a pretty good partner for "init", so I share it here... :-)
-Miles
Signature · 2 lines
o The existentialist, not having a pillow, goes everywhere with the book by Sullivan, _I am going to spit on your graves_.
strbuf new semantics, let's give it a try
In reply to Miles Bader Allright, here is a proposal for a new strbuf semantics like the one I
proposed before. Like said, with this proposal, strbuf's always have a '\0' at the end of the buffer, not accounted in its length, so that any C str* function can still be used.
The second patch demonstrate that strbufs makes the code smaller,
simpler to read, while keeping the exact same efficiency (as may mallocs, memcpys, and fread's in that case).
I've tried to keep the amount of inlines low. Though, I wonder ifstrbuf_add shouldn't be inlined (so that memcpys could be even more inlined/optmized by gcc). And strbuf_addstr is inlined so that the strlen call is optimized when the argument is an immediate string. Meaning that calling strbuf_addstr(sb, "foo") is not sloppy at all.
I don't really like the buffer growth scheme right now, I just stick
with the one that was used in git before. Though I really belive it's inelegant as is, and should be simplified (strbuf_extend).
Cheers,
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org
Quoted 6 lines · Pierre Habouzit
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.
No overflow, but it's still potentially dangerous: if your strbuf contains a '\0', you can't use any standard C string function.
For example, it would be very tempting to compare files with "strcmp(buf1, buf2)", but that would just fail silently when the file contains a '\0' byte.
I don't have a perfect solution, but having something explicit to say "hey, I'm going to use a legacy C function" doesn't sound like a bad idea.
Quoted 2 lines
The other shortcoming is that you cannot tell the buffer "Hey, it's
very likely that you'll end up being _that_ long.
100% ACK on this point.
Signature · 1 line
Matthieu
Quoted 9 lines · Matthieu Moy
Quoted 6 lines · Pierre Habouzit
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.
Quoted 6 lines
No overflow, but it's still potentially dangerous: if your strbuf contains a '\0', you can't use any standard C string function.
For example, it would be very tempting to compare files with "
strcmp(buf1, buf2)", but that would just fail silently when the file contains a '\0' byte.
Indeed, OTHO doing that would be pretty silly, as embending NULs in a
strbuf is wrong, it's a _str_buf, not a random-binary-buffer. It's meant to make the use of strings easier, not to use as generic purpose byte buffers. Of course they can, but well, it's not what they are designed for in the first place.
Also note that it generates bugs, but never crashes if you still do
that, bugs suck, but security bugs, buffer overflows, ... suck more.
So I'm tempted to say, it's not really a problem.
Quoted 4 lines
Quoted 2 lines
The other shortcoming is that you cannot tell the buffer "Hey, it's
very likely that you'll end up being _that_ long.100% ACK on this point.
Also note that it seems that bstring lack this, but maybe I'm wrong.
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org
Quoted 11 lines · Pierre Habouzit
Quoted 4 lines · Matthieu Moy
...
For example, it would be very tempting to compare files with "strcmp(buf1, buf2)", but that would just fail silently when the file contains a '\0' byte.Indeed, OTHO doing that would be pretty silly, as embending NULs in a
strbuf is wrong, it's a_str_buf, not a random-binary-buffer. It's meant to make the use of strings easier, not to use as generic purpose byte buffers. Of course they can, but well, it's not what they are designed for in the first place.
People, please realize strbuf "API" is not a serious API.
It wasn't even intended to be anything more than just a quick-and-dirty implementation of fgets that can grow dynamically. The other callers added by people to have it do general string manipulations were just bolted-on, not designed. I haven't taken a serious look at bstring nor any of the alternatives yet, but defending strbuf as if it was designed to be a sane API is just silly.
Quoted 16 lines · Junio C Hamano
Quoted 11 lines · Pierre Habouzit
Quoted 4 lines · Matthieu Moy
...
For example, it would be very tempting to compare files with "strcmp(buf1, buf2)", but that would just fail silently when the file contains a '\0' byte.Indeed, OTHO doing that would be pretty silly, as embending NULs in a
strbuf is wrong, it's a_str_buf, not a random-binary-buffer. It's meant to make the use of strings easier, not to use as generic purpose byte buffers. Of course they can, but well, it's not what they are designed for in the first place.People, please realize strbuf "API" is not a serious API.
Quoted 7 lines
It wasn't even intended to be anything more than just a quick-and-dirty implementation of fgets that can grow dynamically. The other callers added by people to have it do general string manipulations were just bolted-on, not designed. I haven't taken a serious look at bstring nor any of the alternatives yet, but defending strbuf as if it was designed to be a sane API is just silly.
actually I was defending the "enhanced" strbuf API I was proposing
before, sorry if that was unclear.
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org
Hi,
Quoted 16 lines · Junio C Hamano
Quoted 11 lines · Pierre Habouzit
Quoted 4 lines · Matthieu Moy
...
For example, it would be very tempting to compare files with "strcmp(buf1, buf2)", but that would just fail silently when the file contains a '\0' byte.Indeed, OTHO doing that would be pretty silly, as embending NULs in a
strbuf is wrong, it's a_str_buf, not a random-binary-buffer. It's meant to make the use of strings easier, not to use as generic purpose byte buffers. Of course they can, but well, it's not what they are designed for in the first place.People, please realize strbuf "API" is not a serious API.
Quoted 7 lines
It wasn't even intended to be anything more than just a quick-and-dirty implementation of fgets that can grow dynamically. The other callers added by people to have it do general string manipulations were just bolted-on, not designed. I haven't taken a serious look at bstring nor any of the alternatives yet, but defending strbuf as if it was designed to be a sane API is just silly.
Two points:
- it can be turned into an API (pretty easy, really, since it is _really_ small), and
- it is easy to hack on, because it is _not_ a full-blown cover-them-all library. As such, we can tweak it the way _we_ want. And if that means that we do not NUL terminate, so be it. And if that means that we turn it into a rope structure, so be it.
IOW it might be utterly incomplete, but at least it can evolve in a manner that we like. It does not need to be a *kaboom-here-we-have-a-big-dump*.
I know that we had something like that with LibXDiff. I was not particularly happy with the process, but it turned out quite well, as Davide helped us a lot.
But the diff situation is completely different from the string situation:
- we did not have _anything_ in the way of diff generation,
- it is pretty well defined what a diff generation library should do, and
- diff generation is not trivial, and it would have taken us quite a long time to write it ourselves.
All three points are not true with string manipulation.
But code talks, and as I do not have anything presentable (yet), I'll just shut up, until I have.
Ciao,
Dscho
Quoted 2 lines · Pierre Habouzit
Indeed, OTHO doing that would be pretty silly, as embending NULs in a
strbuf is wrong, it's a_str_buf, not a random-binary-buffer.
I see nothing in strbuf that prevents you from embedding '\0' in it. And the git code calls read_line on stdin, which may well contain '\0'. So, either I missed something, or strbuf can contain '\0', or the git code is not robust to null-characters on stdin, and should be fixed.
Signature · 1 line
Matthieu
strbuf new API, take 2 for inclusion
In reply to Pierre Habouzit Here is a new patchset (with no wrong patchs and numbering this time)
with Junio's remarks integrated (the va_copy stuff, better documentation and patch comments and so on).
strbuf_read remains a "I can read all the content of the file or the
caller is supposed to die()" operation for now, as it's how it's used right now, this can be changed easily in the future if needs for it exists.
I've also stripped as many STRBUF_INIT uses as possible, some people
didn't liked it. I've kept its use for "static" strbufs where it's way more convenient that a function call.
PATCH 1/7 Rework strbuf API and semantics.
The gory details are explained in strbuf.h. The change of semantics this
patch enforces is that the embeded buffer has always a '\0' character after its last byte, to always make it a C-string. The offs-by-one changes are all related to that very change.
A strbuf can be used to store byte arrays, or as an extended string
library. The buf' member can be passed to any C legacy string function, because strbuf operations always ensure there is a terminating \0 at the end of the buffer, not accounted in the len' field of the structure.
A strbuf can be used to generate a string/buffer whose final size is not
really known, and then "strbuf_detach" can be used to get the built buffer, and keep the wrapping "strbuf" structure usable for further work again.
Other interesting feature: strbuf_grow(sb, size) ensure that there is
enough allocated space in sb' to put size' new octets of data in the buffer. It helps avoiding reallocating data for nothing when the problem the strbuf helps to solve has a known typical size.
--- archive-tar.c | 2 +- fast-import.c | 15 ++++---- mktree.c | 4 +-- strbuf.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++-------- strbuf.h | 86 +++++++++++++++++++++++++++++++++++++++++++++++- 5 files changed, 181 insertions(+), 28 deletions(-)
archive-tar.c +1 -1
@@ -166,7 +166,7 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,sprintf (header.name,"%s.paxheader" ,sha1_to_hex (sha1)); }else {if (verbose) -fprintf (stderr,"%.*s\n" , path->len, path->buf); +fprintf (stderr,"%.*s\n" , (int )path->len, path->buf);if (S_ISDIR (mode) ||S_ISGITLINK (mode)) { *header.typeflag =TYPEFLAG_DIR ; mode = (mode |0777 ) & ~tar_umask;
fast-import.c +7 -8
@@ -1595,7 +1595,7 @@ static void read_next_command(void) }else {struct recent_command *rc; - command_buf.buf =NULL ; +strbuf_detach (&command_buf);read_line (&command_buf, stdin,'\n' );if (command_buf.eof)return ;
@@ -1649,7 +1649,6 @@ static void *cmd_data (size_t *size)size_t sz =8192 , term_len = command_buf.len -5 -2 ; length =0 ; buffer =xmalloc (sz); - command_buf.buf =NULL ;for (;;) {read_line (&command_buf, stdin,'\n' );if (command_buf.eof)
@@ -1657,11 +1656,11 @@ static void *cmd_data (size_t *size)if (term_len == command_buf.len && !strcmp (term, command_buf.buf))break ; -ALLOC_GROW (buffer, length + command_buf.len, sz); +ALLOC_GROW (buffer, length + command_buf.len +1 , sz);memcpy (buffer + length, command_buf.buf, - command_buf.len -1 ); - length += command_buf.len -1 ; + command_buf.len); + length += command_buf.len; buffer[length++] ='\n' ; }free (term);
@@ -2101,7 +2100,7 @@ static void cmd_new_commit(void) }/* file_change* */ -while (!command_buf.eof && command_buf.len >1 ) { +while (!command_buf.eof && command_buf.len >0 ) {if (!prefixcmp (command_buf.buf,"M " ))file_change_m (b);else if (!prefixcmp (command_buf.buf,"D " ))
@@ -2256,7 +2255,7 @@ static void cmd_reset_branch(void)else b =new_branch (sp);read_next_command (); -if (!cmd_from (b) && command_buf.len >1 ) +if (!cmd_from (b) && command_buf.len >0 ) unread_command_buf =1 ; }
@@ -2273,7 +2272,7 @@ static void cmd_checkpoint(void)static void cmd_progress (void ) { -fwrite (command_buf.buf,1 , command_buf.len -1 , stdout); +fwrite (command_buf.buf,1 , command_buf.len, stdout);fputc ('\n' , stdout);fflush (stdout);skip_optional_lf ();
mktree.c +1 -3
@@ -92,7 +92,6 @@ int main(int ac, char **av)strbuf_init (&sb);while (1 ) { -int len;char *ptr, *ntr;unsigned mode;enum object_type type;
@@ -101,7 +100,6 @@ int main(int ac, char **av)read_line (&sb, stdin, line_termination);if (sb.eof)break ; - len = sb.len; ptr = sb.buf;/* Input is non-recursive ls-tree output format * mode SP type SP sha1 TAB name
@@ -111,7 +109,7 @@ int main(int ac, char **av)die ("input format error: %s" , sb.buf); ptr = ntr +1 ;/* type */ ntr =strchr (ptr,' ' ); -if (!ntr || sb.buf + len <= ntr +41 || +if (!ntr || sb.buf + sb.len <= ntr +40 || ntr[41 ] !='\t' ||get_sha1_hex (ntr +1 , sha1))die ("input format error: %s" , sb.buf);
strbuf.c +88 -14
128 lines not on this page · raw message
strbuf.h +84 -2
97 lines not on this page · raw message
Signature · 1 line
1.5.3.1
PATCH 2/7 Simplify strbuf uses in archive-tar.c using the proper functions.
This is just cleaner way to deal with strbufs, using its API rather than
reinventing it in the module (e.g. strbuf_append_string is just the plain strbuf_addstr function, and it was used to perform what strbuf_addch does anyways).
--- archive-tar.c | 65 ++++++++++++++------------------------------------------- 1 files changed, 16 insertions(+), 49 deletions(-)
archive-tar.c +16 -49
@@ -78,19 +78,6 @@ static void write_trailer(void) } } -static void strbuf_append_string (struct strbuf *sb,const char *s) -{ -int slen =strlen (s); -int total = sb->len + slen; -if (total +1 > sb->alloc) { - sb->buf =xrealloc (sb->buf, total +1 ); - sb->alloc = total +1 ; - } -memcpy (sb->buf + sb->len, s, slen); - sb->len = total; - sb->buf[total] ='\0' ; -} -/* * pax extended header records have the format "%u %s=%s\n". %u contains * the size of the whole string (including the %u), the first %s is the
@@ -100,26 +87,17 @@ static void strbuf_append_string(struct strbuf *sb, const char *s)static void strbuf_append_ext_header (struct strbuf *sb,const char *keyword,const char *value,unsigned int valuelen) { -char *p; -int len, total, tmp; +int len, tmp;/* "%u %s=%s\n" */ len =1 +1 +strlen (keyword) +1 + valuelen +1 ;for (tmp = len; tmp >9 ; tmp /=10 ) len++; - total = sb->len + len; -if (total > sb->alloc) { - sb->buf =xrealloc (sb->buf, total); - sb->alloc = total; - } - - p = sb->buf; - p +=sprintf (p,"%u %s=" , len, keyword); -memcpy (p, value, valuelen); - p += valuelen; - *p ='\n' ; - sb->len = total; +strbuf_grow (sb, len); +strbuf_addf (sb,"%u %s=" , len, keyword); +strbuf_add (sb, value, valuelen); +strbuf_addch (sb,'\n' ); }static unsigned int ustar_header_chksum (const struct ustar_header *header)
@@ -153,8 +131,7 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,struct strbuf ext_header;memset (&header,0 ,sizeof (header)); - ext_header.buf =NULL ; - ext_header.len = ext_header.alloc =0 ; +strbuf_init (&ext_header);if (!sha1) { *header.typeflag =TYPEFLAG_GLOBAL_HEADER ;
@@ -225,8 +202,8 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,if (ext_header.len >0 ) {write_entry (sha1,NULL ,0 , ext_header.buf, ext_header.len); -free (ext_header.buf); } +strbuf_release (&ext_header);write_blocked (&header,sizeof (header));if (S_ISREG (mode) && buffer && size >0 )write_blocked (buffer, size);
@@ -235,11 +212,11 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,static void write_global_extended_header (const unsigned char *sha1) {struct strbuf ext_header; - ext_header.buf =NULL ; - ext_header.len = ext_header.alloc =0 ; + +strbuf_init (&ext_header);strbuf_append_ext_header (&ext_header,"comment" ,sha1_to_hex (sha1),40 );write_entry (NULL ,NULL ,0 , ext_header.buf, ext_header.len); -free (ext_header.buf); +strbuf_release (&ext_header); }static int git_tar_config (const char *var,const char *value)
@@ -260,28 +237,18 @@ static int write_tar_entry(const unsigned char *sha1, const char *base, int baselen, const char *filename, unsigned mode, int stage) { - static struct strbuf path; + static struct strbuf path = STRBUF_INIT;int filenamelen =strlen (filename);void *buffer;enum object_type type;unsigned long size; -if (!path.alloc) { - path.buf =xmalloc (PATH_MAX ); - path.alloc =PATH_MAX ; - path.len = path.eof =0 ; - } -if (path.alloc < baselen + filenamelen +1 ) { -free (path.buf); - path.buf =xmalloc (baselen + filenamelen +1 ); - path.alloc = baselen + filenamelen +1 ; - } -memcpy (path.buf, base, baselen); -memcpy (path.buf + baselen, filename, filenamelen); - path.len = baselen + filenamelen; - path.buf[path.len] ='\0' ; +strbuf_grow (&path,MAX (PATH_MAX , baselen + filenamelen +1 )); +strbuf_reset (&path); +strbuf_add (&path, base, baselen); +strbuf_add (&path, filename, filenamelen);if (S_ISDIR (mode) ||S_ISGITLINK (mode)) { -strbuf_append_string (&path,"/" ); +strbuf_addch (&path,'/' ); buffer =NULL ; size =0 ; }else {
Signature · 1 line
1.5.3.1
PATCH 3/7 Use proper strbuf API, and also simplify cmd_data code.
This patch features the use of strbuf_detach, and prevent the programmer
to mess with allocation directly. The code is as efficent as before, just more concise and more straightforward.
--- fast-import.c | 30 +++++++++++++----------------- 1 files changed, 13 insertions(+), 17 deletions(-)
fast-import.c +13 -17
@@ -340,7 +340,7 @@ static struct tag *last_tag;/* Input stream parsing */ static whenspec_type whenspec =WHENSPEC_RAW ; -static struct strbuf command_buf; +static struct strbuf command_buf =STRBUF_INIT ;static int unread_command_buf;static struct recent_command cmd_hist = {&cmd_hist, &cmd_hist,NULL };static struct recent_command *cmd_tail = &cmd_hist;
@@ -1638,17 +1638,16 @@ static void cmd_mark(void)static void *cmd_data (size_t *size) { -size_t length; -char *buffer; +struct strbuf buffer; +strbuf_init (&buffer);if (prefixcmp (command_buf.buf,"data " ))die ("Expected 'data n' command, found: %s" , command_buf.buf);if (!prefixcmp (command_buf.buf +5 ,"<<" )) {char *term =xstrdup (command_buf.buf +5 +2 ); -size_t sz =8192 , term_len = command_buf.len -5 -2 ; - length =0 ; - buffer =xmalloc (sz); +size_t term_len = command_buf.len -5 -2 ; +for (;;) {read_line (&command_buf, stdin,'\n' );if (command_buf.eof)
@@ -1656,21 +1655,18 @@ static void *cmd_data (size_t *size)if (term_len == command_buf.len && !strcmp (term, command_buf.buf))break ; -ALLOC_GROW (buffer, length + command_buf.len +1 , sz); -memcpy (buffer + length, - command_buf.buf, - command_buf.len); - length += command_buf.len; - buffer[length++] ='\n' ; +strbuf_addbuf (&buffer, &command_buf); +strbuf_addch (&buffer,'\n' ); }free (term); }else { -size_t n =0 ; +size_t n =0 , length; + length =strtoul (command_buf.buf +5 ,NULL ,10 ); - buffer =xmalloc (length); +while (n < length) { -size_t s =fread (buffer + n,1 , length - n, stdin); +size_t s =strbuf_fread (&buffer, length - n, stdin);if (!s &&feof (stdin))die ("EOF in data (%lu bytes remaining)" , (unsigned long )(length - n));
@@ -1679,8 +1675,8 @@ static void *cmd_data (size_t *size) }skip_optional_lf (); - *size = length; -return buffer; + *size = buffer.len; +return strbuf_detach (&buffer); }static int validate_raw_date (const char *src,char *result,int maxlen)
Signature · 1 line
1.5.3.1
PATCH 4/7 Simplify write_tree using strbuf's.
--- mktree.c | 23 ++++++++--------------- 1 files changed, 8 insertions(+), 15 deletions(-)
mktree.c +8 -15
@@ -44,30 +44,23 @@ static int ent_compare(const void *a_, const void *b_)static void write_tree (unsigned char *sha1) { -char *buffer; -unsigned long size, offset; +struct strbuf buf; +size_t size;int i;qsort (entries, used,sizeof (*entries), ent_compare);for (size = i =0 ; i < used; i++) size +=32 + entries[i]->len; - buffer =xmalloc (size); - offset =0 ; +strbuf_init (&buf); +strbuf_grow (&buf, size);for (i =0 ; i < used; i++) {struct treeent *ent = entries[i]; - -if (offset + ent->len +100 < size) { - size =alloc_nr (offset + ent->len +100 ); - buffer =xrealloc (buffer, size); - } - offset +=sprintf (buffer + offset,"%o " , ent->mode); - offset +=sprintf (buffer + offset,"%s" , ent->name); - buffer[offset++] =0 ; -hashcpy ((unsigned char *)buffer + offset, ent->sha1); - offset +=20 ; +strbuf_addf (&buf,"%o %s%c" , ent->mode, ent->name,'\0' ); +strbuf_add (&buf, ent->sha1,20 ); } -write_sha1_file (buffer, offset, tree_type, sha1); + +write_sha1_file (buf.buf, buf.len, tree_type, sha1); }static const char mktree_usage[] ="git-mktree [-z]" ;
Signature · 1 line
1.5.3.1
PATCH 5/7 Further strbuf re-engineering.
--- builtin-apply.c | 30 +++++++----------------- builtin-blame.c | 35 +++++++++------------------ builtin-commit-tree.c | 60 +++++++++++------------------------------------- diff.c | 27 +++++++-------------- 4 files changed, 44 insertions(+), 108 deletions(-)
builtin-apply.c +9 -21
@@ -12,6 +12,7 @@#include "blob.h" #include "delta.h" #include "builtin.h" +#include "strbuf.h" /* * --check turns on checking that the working tree matches the
@@ -181,34 +182,21 @@ static void say_patch_name(FILE *output, const char *pre, struct patch *patch, cstatic void *read_patch_file (int fd,unsigned long *sizep) { -unsigned long size =0 , alloc =CHUNKSIZE ; -void *buffer =xmalloc (alloc); +struct strbuf buf; -for (;;) { -ssize_t nr = alloc - size; -if (nr <1024 ) { - alloc +=CHUNKSIZE ; - buffer =xrealloc (buffer, alloc); - nr = alloc - size; - } - nr =xread (fd, (char *) buffer + size, nr); -if (!nr) -break ; -if (nr <0 ) -die ("git-apply: read returned %s" ,strerror (errno)); - size += nr; - } - *sizep = size; +strbuf_init (&buf); +if (strbuf_read (&buf, fd) <0 ) +die ("git-apply: read returned %s" ,strerror (errno)); + *sizep = buf.len;/* * Make sure that we have some slop in the buffer * so that we can do speculative "memcmp" etc, and * see to it that it is NUL-filled. */ -if (alloc < size +SLOP ) - buffer =xrealloc (buffer, size +SLOP ); -memset ((char *) buffer + size,0 ,SLOP ); -return buffer; +strbuf_grow (&buf,SLOP ); +memset (buf.buf + buf.len,0 ,SLOP ); +return strbuf_detach (&buf); }static unsigned long linelen (const char *buffer,unsigned long size)
builtin-blame.c +12 -23
@@ -18,6 +18,7 @@#include "cache-tree.h" #include "path-list.h" #include "mailmap.h" +#include "strbuf.h" static char blame_usage[] ="git-blame [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-p] [-w] [-L n,m] [-S <revs-file>] [-M] [-C] [-C] [--contents <filename>] [--incremental] [commit] [--] file\n"
@@ -2001,11 +2002,10 @@ static struct commit *fake_working_tree_commit(const char *path, const char *construct commit *commit;struct origin *origin;unsigned char head_sha1[20 ]; -char *buf; +struct strbuf buf;const char *ident;int fd;time_t now; -unsigned long fin_size;int size, len;struct cache_entry *ce;unsigned mode;
@@ -2023,9 +2023,11 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con origin =make_origin (commit, path); +strbuf_init (&buf);if (!contents_from ||strcmp ("-" , contents_from)) {struct stat st;const char *read_from; +unsigned long fin_size;if (contents_from) {if (stat (contents_from, &st) <0 )
@@ -2038,19 +2040,19 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con read_from = path; } fin_size =xsize_t (st.st_size); - buf =xmalloc (fin_size+1 ); mode =canon_mode (st.st_mode);switch (st.st_mode &S_IFMT ) {case S_IFREG : fd =open (read_from,O_RDONLY );if (fd <0 )die ("cannot open %s" , read_from); -if (read_in_full (fd, buf, fin_size) != fin_size) +if (strbuf_read (&buf, fd) !=xsize_t (st.st_size))die ("cannot read %s" , read_from);break ;case S_IFLNK : -if (readlink (read_from, buf, fin_size+1 ) != fin_size) +if (readlink (read_from, buf.buf, buf.alloc) != fin_size)die ("cannot readlink %s" , read_from); + buf.len = fin_size;break ;default :die ("unsupported file type %s" , read_from);
@@ -2059,26 +2061,13 @@ static struct commit *fake_working_tree_commit(const char *path, const char *conelse {/* Reading from stdin */ contents_from ="standard input" ; - buf =NULL ; - fin_size =0 ; mode =0 ; -while (1 ) { -ssize_t cnt =8192 ; - buf =xrealloc (buf, fin_size + cnt); - cnt =xread (0 , buf + fin_size, cnt); -if (cnt <0 ) -die ("read error %s from stdin" , -strerror (errno)); -if (!cnt) -break ; - fin_size += cnt; - } - buf =xrealloc (buf, fin_size +1 ); +if (strbuf_read (&buf,0 ) <0 ) +die ("read error %s from stdin" ,strerror (errno)); } - buf[fin_size] =0 ; - origin->file.ptr = buf; - origin->file.size = fin_size; -pretend_sha1_file (buf, fin_size,OBJ_BLOB , origin->blob_sha1); + origin->file.ptr = buf.buf; + origin->file.size = buf.len; +pretend_sha1_file (buf.buf, buf.len,OBJ_BLOB , origin->blob_sha1); commit->util = origin;/*
builtin-commit-tree.c +14 -46
99 lines not on this page · raw message
diff.c +9 -18
@@ -9,6 +9,7 @@#include "xdiff-interface.h" #include "color.h" #include "attr.h" +#include "strbuf.h" #ifdef NO_FAST_WORKING_DIRECTORY #define FAST_WORKING_DIRECTORY 0
@@ -1545,26 +1546,16 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, intstatic int populate_from_stdin (struct diff_filespec *s) { -#define INCREMENT 1024 -char *buf; -unsigned long size; -ssize_t got; - - size =0 ; - buf =NULL ; -while (1 ) { - buf =xrealloc (buf, size +INCREMENT ); - got =xread (0 , buf + size,INCREMENT ); -if (!got) -break ;/* EOF */ -if (got <0 ) -return error ("error while reading from stdin %s" , +struct strbuf buf; + +strbuf_init (&buf); +if (strbuf_read (&buf,0 ) <0 ) +return error ("error while reading from stdin %s" ,strerror (errno)); - size += got; - } + s->should_munmap =0 ; - s->data = buf; - s->size = size; + s->size = buf.len; + s->data =strbuf_detach (&buf); s->should_free =1 ;return 0 ; }
Signature · 1 line
1.5.3.1
PATCH 6/7 Eradicate yet-another-buffer implementation in buitin-rerere.c
--- builtin-rerere.c | 56 +++++++++++++++++------------------------------------ 1 files changed, 18 insertions(+), 38 deletions(-)
builtin-rerere.c +18 -38
@@ -1,6 +1,7 @@#include "builtin.h" #include "cache.h" #include "path-list.h" +#include "strbuf.h" #include "xdiff/xdiff.h" #include "xdiff-interface.h"
@@ -66,41 +67,20 @@ static int write_rr(struct path_list *rr, int out_fd)return commit_lock_file (&write_lock); } -struct buffer { -char *ptr; -int nr, alloc; -}; - -static void append_line (struct buffer *buffer,const char *line) -{ -int len =strlen (line); - -if (buffer->nr + len > buffer->alloc) { - buffer->alloc =alloc_nr (buffer->nr + len); - buffer->ptr =xrealloc (buffer->ptr, buffer->alloc); - } -memcpy (buffer->ptr + buffer->nr, line, len); - buffer->nr += len; -} - -static void clear_buffer (struct buffer *buffer) -{ -free (buffer->ptr); - buffer->ptr =NULL ; - buffer->nr = buffer->alloc =0 ; -} -static int handle_file (const char *path,unsigned char *sha1,const char *output) {SHA_CTX ctx;char buf[1024 ];int hunk =0 , hunk_no =0 ; -struct buffer minus = {NULL ,0 ,0 }, plus = {NULL ,0 ,0 }; -struct buffer *one = &minus, *two = + +struct strbuf minus, plus; +struct strbuf *one = &minus, *two = +FILE *f =fopen (path,"r" );FILE *out; +strbuf_init (&minus); +strbuf_init (&plus); +if (!f)return error ("Could not open %s" , path);
@@ -122,36 +102,36 @@ static int handle_file(const char *path,else if (!prefixcmp (buf,"=======" )) hunk =2 ;else if (!prefixcmp (buf,">>>>>>> " )) { -int one_is_longer = (one->nr > two->nr); -int common_len = one_is_longer ? two->nr : one->nr; -int cmp =memcmp (one->ptr, two->ptr, common_len); +int one_is_longer = (one->len > two->len); +int common_len = one_is_longer ? two->len : one->len; +int cmp =memcmp (one->buf, two->buf, common_len); hunk_no++; hunk =0 ;if ((cmp >0 ) || ((cmp ==0 ) && one_is_longer)) { -struct buffer *swap = one; +struct strbuf *swap = one; one = two; two = swap; }if (out) {fputs ("<<<<<<<\n" , out); -fwrite (one->ptr, one->nr,1 , out); +fwrite (one->buf, one->len,1 , out);fputs ("=======\n" , out); -fwrite (two->ptr, two->nr,1 , out); +fwrite (two->buf, two->len,1 , out);fputs (">>>>>>>\n" , out); }if (sha1) { -SHA1_Update (&ctx, one->ptr, one->nr); +SHA1_Update (&ctx, one->buf, one->len);SHA1_Update (&ctx,"\0" ,1 ); -SHA1_Update (&ctx, two->ptr, two->nr); +SHA1_Update (&ctx, two->buf, two->len);SHA1_Update (&ctx,"\0" ,1 ); } -clear_buffer (one); -clear_buffer (two); +strbuf_release (one); +strbuf_release (two); }else if (hunk ==1 ) -append_line (one, buf); +strbuf_addstr (one, buf);else if (hunk ==2 ) -append_line (two, buf); +strbuf_addstr (two, buf);else if (out)fputs (buf, out); }
Signature · 1 line
1.5.3.1
PATCH 7/7 More strbuf uses in cache-tree.c.
Should even be marginally faster.
--- cache-tree.c | 59 +++++++++++++++++++++------------------------------------ 1 files changed, 22 insertions(+), 37 deletions(-)
cache-tree.c +22 -37
@@ -1,4 +1,5 @@#include "cache.h" +#include "strbuf.h" #include "tree.h" #include "cache-tree.h"
@@ -235,8 +236,7 @@ static int update_one(struct cache_tree *it, int missing_ok, int dryrun) { -unsigned long size, offset; -char *buffer; +struct strbuf buffer;int i;if (0 <= it->entry_count &&has_sha1_file (it->sha1))
@@ -293,9 +293,8 @@ static int update_one(struct cache_tree *it,/* * Then write out the tree object for this level. */ - size =8192 ; - buffer =xmalloc (size); - offset =0 ; +strbuf_init (&buffer); +strbuf_grow (&buffer,8192 );for (i =0 ; i < entries; i++) {struct cache_entry *ce = cache[i];
@@ -332,15 +331,9 @@ static int update_one(struct cache_tree *it,if (!ce->ce_mode)continue ;/* entry being removed */ -if (size < offset + entlen +100 ) { - size =alloc_nr (offset + entlen +100 ); - buffer =xrealloc (buffer, size); - } - offset +=sprintf (buffer + offset, -"%o %.*s" , mode, entlen, path + baselen); - buffer[offset++] =0 ; -hashcpy ((unsigned char *)buffer + offset, sha1); - offset +=20 ; +strbuf_grow (&buffer, entlen +100 ); +strbuf_addf (&buffer,"%o %.*s%c" , mode, entlen, path + baselen,'\0' ); +strbuf_add (&buffer, sha1,20 );#if DEBUG fprintf(stderr, "cache-tree update-one %o %.*s\n",
@@ -349,10 +342,10 @@ static int update_one(struct cache_tree *it, }if (dryrun) -hash_sha1_file (buffer, offset, tree_type, it->sha1); +hash_sha1_file (buffer.buf, buffer.len, tree_type, it->sha1);else -write_sha1_file (buffer, offset, tree_type, it->sha1); -free (buffer); +write_sha1_file (buffer.buf, buffer.len, tree_type, it->sha1); +strbuf_release (&buffer); it->entry_count = i;#if DEBUG fprintf(stderr, "cache-tree update-one (%d ent, %d subtree) %s\n",
@@ -378,12 +371,10 @@ int cache_tree_update(struct cache_tree *it,return 0 ; } -static void *write_one (struct cache_tree *it, +static void write_one (struct cache_tree *it,char *path,int pathlen, -char *buffer, -unsigned long *size, -unsigned long *offset) +struct strbuf *buffer) {int i;
@@ -393,13 +384,9 @@ static void *write_one(struct cache_tree *it,* tree-sha1 (missing if invalid) * subtree_nr "cache-tree" entries for subtrees. */ -if (*size < *offset + pathlen +100 ) { - *size =alloc_nr (*offset + pathlen +100 ); - buffer =xrealloc (buffer, *size); - } - *offset +=sprintf (buffer + *offset,"%.*s%c%d %d\n" , - pathlen, path,0 , - it->entry_count, it->subtree_nr); +strbuf_grow (buffer, pathlen +100 ); +strbuf_add (buffer, path, pathlen); +strbuf_addf (buffer,"%c%d %d\n" ,0 , it->entry_count, it->subtree_nr);#if DEBUG if (0 <= it->entry_count)
@@ -412,8 +399,7 @@ static void *write_one(struct cache_tree *it,#endif if (0 <= it->entry_count) { -hashcpy ((unsigned char *)buffer + *offset, it->sha1); - *offset +=20 ; +strbuf_add (buffer, it->sha1,20 ); }for (i =0 ; i < it->subtree_nr; i++) {struct cache_tree_sub *down = it->down[i];
@@ -423,21 +409,20 @@ static void *write_one(struct cache_tree *it, prev->name, prev->namelen) <= 0)die ("fatal - unsorted cache subtree" ); } - buffer =write_one (down->cache_tree, down->name, down->namelen, - buffer, size, offset); +write_one (down->cache_tree, down->name, down->namelen, buffer); } -return buffer; }void *cache_tree_write (struct cache_tree *root,unsigned long *size_p) {char path[PATH_MAX ]; -unsigned long size =8192 ; -char *buffer =xmalloc (size); +struct strbuf buffer; - *size_p =0 ; path[0 ] =0 ; -return write_one (root, path,0 , buffer, &size, size_p); +strbuf_init (&buffer); +write_one (root, path,0 , &buffer); + *size_p = buffer.len; +return strbuf_detach (&buffer); }static struct cache_tree *read_one (const char **buffer,unsigned long *size_p)
Signature · 1 line
1.5.3.1
Re: [PATCH 6/7] Eradicate yet-another-buffer implementation in buitin-rerere.c
In reply to Pierre HabouzitHi,
Quoted 4 lines · Pierre Habouzit
Signed-off-by Pierre Habouzit <[email protected]>--- builtin-rerere.c | 56 +++++++++++++++++------------------------------------ 1 files changed, 18 insertions(+), 38 deletions(-)
I like that one very much, but ...
builtin-rerere.c static int write_rr(struct path_list *rr, int out_fd) · line 83
Quoted 6 lines
builtin-rerere.c +3
-0FILE *f =fopen (path,"r" );FILE *out; +strbuf_init (&minus); +strbuf_init (&plus); +
You used spaces instead of tabs here.
Ciao,
Dscho
Re: [PATCH 6/7] Eradicate yet-another-buffer implementation in buitin-rerere.c
Quoted 11 lines · Johannes Schindelin
Hi,
Quoted 4 lines · Pierre Habouzit
Signed-off-by Pierre Habouzit <[email protected]>--- builtin-rerere.c | 56 +++++++++++++++++------------------------------------ 1 files changed, 18 insertions(+), 38 deletions(-)I like that one very much, but ...
Quoted 8 lines
builtin-rerere.c static int write_rr(struct path_list *rr, int out_fd) · line 83
Quoted 6 lines
builtin-rerere.c +3
-0FILE *f =fopen (path,"r" );FILE *out; +strbuf_init (&minus); +strbuf_init (&plus); +You used spaces instead of tabs here.
crap, and I did that in the 5th patch as well. well, I'll maybe send
privately a "fixed" version of the patch to junio then, to avoid flooding the list with spacing issues.
And I'll also set my vim to use tabs when I'm hacking on git.
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org
Re: [PATCH 6/7] Eradicate yet-another-buffer implementation in buitin-rerere.c
Quoted 3 lines · Pierre Habouzit
crap, and I did that in the 5th patch as well. well, I'll maybe send
privately a "fixed" version of the patch to junio then, to avoid flooding the list with spacing issues.
Heh, no need. I can fix them up locally here.
For me personally, your use of Mail-Followup-To: header is very much more irritating. When I want to talk to you, I want my MUA to put YOUR name on To: header, not Johannes. I consider it rude and selfish to use M-F-T to avoid getting duplicates from a mailing list you subscribe to.
I have said this often enough in the past, and have been trying not to say this unless necessary. It is sad and happy at the same time that I still occasionary need to bring this up.
Sad because I have to get irritated enough before I say this, and happy because it means I found another person who is worth communicating with and expect to communicate again with on the list.
Please, don't.
Re: [PATCH 6/7] Eradicate yet-another-buffer implementation in buitin-rerere.c
Quoted 7 lines · Junio C Hamano
Quoted 3 lines · Pierre Habouzit
crap, and I did that in the 5th patch as well. well, I'll maybe send
privately a "fixed" version of the patch to junio then, to avoid flooding the list with spacing issues.Heh, no need. I can fix them up locally here.
okay, perfect.
Quoted 11 lines
For me personally, your use of Mail-Followup-To: header is very much more irritating. When I want to talk to you, I want my MUA to put YOUR name on To: header, not Johannes. I consider it rude and selfish to use M-F-T to avoid getting duplicates from a mailing list you subscribe to.
I have said this often enough in the past, and have been trying not to say this unless necessary. It is sad and happy at the same time that I still occasionary need to bring this up.
Sad because I have to get irritated enough before I say this,
I'm sorry, I'll remove it (for the next mail though, I'm too lazy to
change that right now). It's a policy on Debian lists to force M-F-T, and it's indeed not very practical on lists like the lkml or git. I could have guessed that myself.
Quoted 3 lines
and happy because it means I found another person who is worth communicating with and expect to communicate again with on the list.
heh, thanks :)
Quoted 1 line
Please, don't.
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org
Re: [PATCH 6/7] Eradicate yet-another-buffer implementation in buitin-rerere.c
In reply to Pierre HabouzitQuoted 9 lines · Pierre Habouzit
Quoted 1 line · Johannes Schindelin
You used spaces instead of tabs here.
crap, and I did that in the 5th patch as well. well, I'll maybe send
privately a "fixed" version of the patch to junio then, to avoid flooding the list with spacing issues.And I'll also set my vim to use tabs when I'm hacking on git.
Just use Emacs and
M-x c-set-style RET linux RET
Signature · 1 line
David Kastrup, Kriemhildstr. 15, 44793 Bochum
Re: [PATCH 6/7] Eradicate yet-another-buffer implementation in buitin-rerere.c
Quoted 6 lines · David Kastrup
Quoted 4 lines · Pierre Habouzit
Quoted 1 line · Johannes Schindelin
You used spaces instead of tabs here.
Quoted 8 lines
Quoted 5 lines
crap, and I did that in the 5th patch as well. well, I'll maybe send
privately a "fixed" version of the patch to junio then, to avoid flooding the list with spacing issues.And I'll also set my vim to use tabs when I'm hacking on git.
Just use Emacs and
M-x c-set-style RET linux RET
I'll give you a hunch:
apt-cache showsrc vim|grep Uplo
…, Pierre Habouzit <[email protected]>, …
And it's not like vim doesn't has this either :)
Anyways, we're going a bit OT aren't we ?
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org
Re: [PATCH 2/7] Simplify strbuf uses in archive-tar.c using the proper functions.
In reply to Pierre HabouzitQuoted 52 lines · Pierre Habouzit
This is just cleaner way to deal with strbufs, using its API rather than reinventing it in the module (e.g.
strbuf_append_stringis just the plainstrbuf_addstrfunction, and it was used to perform whatstrbuf_addchdoes anyways).--- archive-tar.c | 65 ++++++++++++++------------------------------------------- 1 files changed, 16 insertions(+), 49 deletions(-)archive-tar.c +1
-22@@ -78,19 +78,6 @@ static void write_trailer(void) } } -static void strbuf_append_string (struct strbuf *sb,const char *s) -{ -int slen =strlen (s); -int total = sb->len + slen; -if (total +1 > sb->alloc) { - sb->buf =xrealloc (sb->buf, total +1 ); - sb->alloc = total +1 ; - } -memcpy (sb->buf + sb->len, s, slen); - sb->len = total; - sb->buf[total] ='\0' ; -} -/* * pax extended header records have the format "%u %s=%s\n". %u contains * the size of the whole string (including the %u), the first %s is the @@ -100,26 +87,17 @@ static void strbuf_append_string(struct strbuf *sb, const char *s)static void strbuf_append_ext_header (struct strbuf *sb,const char *keyword,const char *value,unsigned int valuelen) { -char *p; -int len, total, tmp; +int len, tmp;/* "%u %s=%s\n" */ len =1 +1 +strlen (keyword) +1 + valuelen +1 ;for (tmp = len; tmp >9 ; tmp /=10 ) len++; - total = sb->len + len; -if (total > sb->alloc) { - sb->buf =xrealloc (sb->buf, total); - sb->alloc = total; - } - - p = sb->buf;
archive-tar.c static void strbuf_append_string(struct strbuf *sb, const char *s) · line 101
Quoted 10 lines
archive-tar.c +4
-5- p +=sprintf (p,"%u %s=" , len, keyword); -memcpy (p, value, valuelen); - p += valuelen; - *p ='\n' ; - sb->len = total; +strbuf_grow (sb, len); +strbuf_addf (sb,"%u %s=" , len, keyword); +strbuf_add (sb, value, valuelen); +strbuf_addch (sb,'\n' ); }
This entire function can be collapsed to just:
strbuf_addf(sb, "%u %s=%.*s\n", len, keyword, valuelen, value);
archive-tar.c static void write_entry(const unsigned char *sha1, struct strbuf *path, · line 134
Quoted 9 lines
archive-tar.c +1
-2static unsigned int ustar_header_chksum (const struct ustar_header *header)@@ -153,8 +131,7 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,struct strbuf ext_header;memset (&header,0 ,sizeof (header)); - ext_header.buf =NULL ; - ext_header.len = ext_header.alloc =0 ; +strbuf_init (&ext_header);
Just use your STRBUF_INIT macro?
archive-tar.c static void write_entry(const unsigned char *sha1, struct strbuf *path, · line 205
Quoted 8 lines
archive-tar.c +0
-1if (!sha1) { *header.typeflag =TYPEFLAG_GLOBAL_HEADER ;@@ -225,8 +202,8 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,if (ext_header.len >0 ) {write_entry (sha1,NULL ,0 , ext_header.buf, ext_header.len); -free (ext_header.buf); }
Remove excess braces?
archive-tar.c static void write_entry(const unsigned char *sha1, struct strbuf *path, · line 216
Quoted 12 lines
archive-tar.c +3
-2+strbuf_release (&ext_header);write_blocked (&header,sizeof (header));if (S_ISREG (mode) && buffer && size >0 )write_blocked (buffer, size);@@ -235,11 +212,11 @@ static void write_entry(const unsigned char *sha1, struct strbuf *path,static void write_global_extended_header (const unsigned char *sha1) {struct strbuf ext_header; - ext_header.buf =NULL ; - ext_header.len = ext_header.alloc =0 ; + +strbuf_init (&ext_header);
STRBUF_INIT macro?
Quoted 24 lines
archive-tar.c +2
-8strbuf_append_ext_header (&ext_header,"comment" ,sha1_to_hex (sha1),40 );write_entry (NULL ,NULL ,0 , ext_header.buf, ext_header.len); -free (ext_header.buf); +strbuf_release (&ext_header); }static int git_tar_config (const char *var,const char *value)@@ -260,28 +237,18 @@ static int write_tar_entry(const unsigned char *sha1, const char *base, int baselen, const char *filename, unsigned mode, int stage) { - static struct strbuf path; + static struct strbuf path = STRBUF_INIT;int filenamelen =strlen (filename);void *buffer;enum object_type type;unsigned long size; -if (!path.alloc) { - path.buf =xmalloc (PATH_MAX ); - path.alloc =PATH_MAX ; - path.len = path.eof =0 ; - } -if (path.alloc < baselen + filenamelen +1 ) {
archive-tar.c static int write_tar_entry(const unsigned char *sha1, · line 247
Quoted 10 lines
archive-tar.c +2
-8-free (path.buf); - path.buf =xmalloc (baselen + filenamelen +1 ); - path.alloc = baselen + filenamelen +1 ; - } -memcpy (path.buf, base, baselen); -memcpy (path.buf + baselen, filename, filenamelen); - path.len = baselen + filenamelen; - path.buf[path.len] ='\0' ; +strbuf_grow (&path,MAX (PATH_MAX , baselen + filenamelen +1 )); +strbuf_reset (&path);
Does strbuf_reset() do anything here?
Quoted 8 lines
archive-tar.c +3
-1+strbuf_add (&path, base, baselen); +strbuf_add (&path, filename, filenamelen);if (S_ISDIR (mode) ||S_ISGITLINK (mode)) { -strbuf_append_string (&path,"/" ); +strbuf_addch (&path,'/' ); buffer =NULL ; size =0 ; }else {
Re: [PATCH 2/7] Simplify strbuf uses in archive-tar.c using the proper functions.
Quoted 10 lines · Kristian Høgsberg
archive-tar.c static void strbuf_append_string(struct strbuf *sb, const char *s) · line 101
Quoted 5 lines · Pierre Habouzit
archive-tar.c +4
-0+strbuf_grow (sb, len); +strbuf_addf (sb,"%u %s=" , len, keyword); +strbuf_add (sb, value, valuelen); +strbuf_addch (sb,'\n' ); }This entire function can be collapsed to just:
strbuf_addf(sb, "%u %s=%.*s\n", len, keyword, valuelen, value);
yes, but it's less efficient, because %.*s says that sprintf must copy
at most valuelen bytes from value, but it still has to stop if it finds a \0 before. And the strbuf_grow has sense because the extend policy at snprintf time is optimistic: we try to write, and if it didn't fit, we try again. So there is a huge benefit if we have a clue of the final size.
I would not change a thing.
Quoted 3 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ strbuf_init(&ext_header);Just use your
STRBUF_INITmacro?
Many people dilike it, I'm not sure it's a good idea either, and the
performance hit should be negligible, if it's not, then we can still make the _init() function an inline.
Quoted 6 lines
archive-tar.c static void write_entry(const unsigned char *sha1, struct strbuf *path, · line 205
Quoted 4 lines
archive-tar.c +0
-1if (ext_header.len >0 ) {write_entry (sha1,NULL ,0 , ext_header.buf, ext_header.len); -free (ext_header.buf); }Remove excess braces?
bah, I don't like to strip braces so I won't do that, else you end up
with stupidities like:
if (foo)
// bar();
do_some_very_important_stuff();Call me paranoid but well, it saved me so many times ...
Quoted 10 lines
archive-tar.c static int write_tar_entry(const unsigned char *sha1, · line 247
Quoted 6 lines
archive-tar.c +2
-4-memcpy (path.buf, base, baselen); -memcpy (path.buf + baselen, filename, filenamelen); - path.len = baselen + filenamelen; - path.buf[path.len] ='\0' ; +strbuf_grow (&path,MAX (PATH_MAX , baselen + filenamelen +1 )); +strbuf_reset (&path);Does
strbuf_reset()do anything here?archive-tar.c static int write_tar_entry(const unsigned char *sha1, · line 248
Quoted 1 line
archive-tar.c +1
-0+strbuf_add (&path, base, baselen);
Yes _reset() sets length to 0. so the add here will write at the start
of the buffer again. It definitely is important !
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org
Re: [PATCH 2/7] Simplify strbuf uses in archive-tar.c using the proper functions.
Quoted 10 lines · Pierre Habouzit
Quoted 9 lines · Kristian Høgsberg
archive-tar.c static void strbuf_append_string(struct strbuf *sb, const char *s) · line 101
Quoted 5 lines · Pierre Habouzit
archive-tar.c +4
-0+strbuf_grow (sb, len); +strbuf_addf (sb,"%u %s=" , len, keyword); +strbuf_add (sb, value, valuelen); +strbuf_addch (sb,'\n' ); }This entire function can be collapsed to just:
Quoted 8 lines
Quoted 1 line
strbuf_addf(sb, "%u %s=%.*s\n", len, keyword, valuelen, value);yes, but it's less efficient, because %.*s says that sprintf must copy
at most valuelen bytes from value, but it still has to stop if it finds a \0 before. And thestrbuf_growhas sense because the extend policy at snprintf time is optimistic: we try to write, and if it didn't fit, we try again. So there is a huge benefit if we have a clue of the final size.
Yeah, my strbuf_addf() suggestion doesn't work, since the format seems to indicate the embedded NULs indeed is allowed (the %u is the length of the value), so *.% could truncate data.
Quoted 9 lines
Quoted 9 lines
archive-tar.c static int write_tar_entry(const unsigned char *sha1, · line 247
Quoted 6 lines
archive-tar.c +2
-4-memcpy (path.buf, base, baselen); -memcpy (path.buf + baselen, filename, filenamelen); - path.len = baselen + filenamelen; - path.buf[path.len] ='\0' ; +strbuf_grow (&path,MAX (PATH_MAX , baselen + filenamelen +1 )); +strbuf_reset (&path);Does
strbuf_reset()do anything here?
Quoted 4 lines
Quoted 1 line
archive-tar.c static int write_tar_entry(const unsigned char *sha1, · line 248
Quoted 1 line
archive-tar.c +1
-0+strbuf_add (&path, base, baselen);Yes
_reset()sets length to 0. so the add here will write at the start
of the buffer again. It definitely is important !
But where was length set to non-zero? path is initialized on entry to the function, and strbuf_grow() just increases the allocation, not length, right?
Kristian
Re: [PATCH 2/7] Simplify strbuf uses in archive-tar.c using the proper functions.
Quoted 12 lines · Kristian Høgsberg
Quoted 11 lines · Pierre Habouzit
Quoted 10 lines · Kristian Høgsberg
archive-tar.c static int write_tar_entry(const unsigned char *sha1, · line 247
Quoted 6 lines · Pierre Habouzit
archive-tar.c +2
-4-memcpy (path.buf, base, baselen); -memcpy (path.buf + baselen, filename, filenamelen); - path.len = baselen + filenamelen; - path.buf[path.len] ='\0' ; +strbuf_grow (&path,MAX (PATH_MAX , baselen + filenamelen +1 )); +strbuf_reset (&path);Does
strbuf_reset()do anything here?
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
archive-tar.c static int write_tar_entry(const unsigned char *sha1, · line 248
Quoted 1 line
archive-tar.c +1
-0+strbuf_add (&path, base, baselen);Yes
_reset()sets length to 0. so the add here will write at the start
of the buffer again. It definitely is important !But where was length set to non-zero? path is initialized on entry to the function, and
strbuf_grow()just increases the allocation, not length, right?
The path is static, hence when you reenter the function you have the
last value in it. The fact that it's static may be questionable, but it was like it before, I kept it, I've supposed it was for performance reasons.
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org
Re: [PATCH 2/7] Simplify strbuf uses in archive-tar.c using the proper functions.
Quoted 15 lines · Pierre Habouzit
Quoted 14 lines · Kristian Høgsberg
Quoted 13 lines · Pierre Habouzit
Quoted 10 lines · Kristian Høgsberg
archive-tar.c static int write_tar_entry(const unsigned char *sha1, · line 247
Quoted 6 lines · Pierre Habouzit
archive-tar.c +2
-4-memcpy (path.buf, base, baselen); -memcpy (path.buf + baselen, filename, filenamelen); - path.len = baselen + filenamelen; - path.buf[path.len] ='\0' ; +strbuf_grow (&path,MAX (PATH_MAX , baselen + filenamelen +1 )); +strbuf_reset (&path);Does
strbuf_reset()do anything here?archive-tar.c static int write_tar_entry(const unsigned char *sha1, · line 248
Quoted 1 line
archive-tar.c +1
-0+strbuf_add (&path, base, baselen);Yes
_reset()sets length to 0. so the add here will write at the start
of the buffer again. It definitely is important !
Quoted 8 lines
Quoted 3 lines
But where was length set to non-zero? path is initialized on entry to the function, and
strbuf_grow()just increases the allocation, not length, right?The path is static, hence when you reenter the function you have the
last value in it. The fact that it's static may be questionable, but it was like it before, I kept it, I've supposed it was for performance reasons.
You're right, I've made it static to get the number of mallocs from number-of-files-in-archive down to one. It was a ~10% speedup in the warm cache case, so I could not resist. :)
René
Re: [PATCH 1/7] Rework strbuf API and semantics.
In reply to Pierre HabouzitQuoted 10 lines · Pierre Habouzit
The gory details are explained in
strbuf.h. The change of semantics this patch enforces is that the embeded buffer has always a '\0' character after its last byte, to always make it a C-string. The offs-by-one changes are all related to that very change.A strbuf can be used to store byte arrays, or as an extended string
library. Thebuf' member can be passed to any C legacy string function, because strbuf operations always ensure there is a terminating \0 at the end of the buffer, not accounted in thelen' field of the structure.
Quoted 8 lines
A strbuf can be used to generate a string/buffer whose final size is not
really known, and then "strbuf_detach" can be used to get the built buffer, and keep the wrapping "strbuf" structure usable for further work again.Other interesting feature:
strbuf_grow(sb, size)ensure that there is
enough allocated space insb' to putsize' new octets of data in the buffer. It helps avoiding reallocating data for nothing when the problem the strbuf helps to solve has a known typical size.
This looks good, should be very useful.
Kristian
Re: [PATCH 1/7] Rework strbuf API and semantics.
In reply to Pierre HabouzitHi,
let me thank you for this very nicely done patch series. Except for 5/7, they look pretty much obvious changes to me. I'll review that in detail later.
Quoted 1 line · Pierre Habouzit
+#define
STRBUF_INIT{ 0, 0, 0, NULL }
Would not "struct strbuf sb = { 0 };" have the same effect? (I am not so standards-keen as other people, who I have no doubt will gladly answer this one.)
Ciao,
Dscho
Re: [PATCH 1/7] Rework strbuf API and semantics.
Quoted 3 lines · Johannes Schindelin
let me thank you for this very nicely done patch series. Except for 5/7, they look pretty much obvious changes to me. I'll review that in detail later.
I second that; I am glad somebody is taking an interest in this area (though I haven't closely reviewed the patches yet).
Quoted 5 lines
Quoted 1 line
+#define
STRBUF_INIT{ 0, 0, 0, NULL }Would not "struct strbuf sb = { 0 };" have the same effect? (I am not so standards-keen as other people, who I have no doubt will gladly answer this one.)
Yes, it would, according to the standard.
-Peff
Re: [PATCH 1/7] Rework strbuf API and semantics.
Quoted 9 lines · Jeff King
Quoted 3 lines · Johannes Schindelin
let me thank you for this very nicely done patch series. Except for 5/7, they look pretty much obvious changes to me. I'll review that in detail later.
I second that; I am glad somebody is taking an interest in this area (though I haven't closely reviewed the patches yet).
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
+#define
STRBUF_INIT{ 0, 0, 0, NULL }Would not "struct strbuf sb = { 0 };" have the same effect? (I am not so standards-keen as other people, who I have no doubt will gladly answer this one.)
Yes, it would, according to the standard.
Have a citation for that?
Signature · 1 line
David Kastrup, Kriemhildstr. 15, 44793 Bochum
Re: [PATCH 1/7] Rework strbuf API and semantics.
Quoted 7 lines · David Kastrup
Quoted 5 lines
Quoted 3 lines
Would not "struct strbuf sb = { 0 };" have the same effect? (I am not so standards-keen as other people, who I have no doubt will gladly answer this one.)
Yes, it would, according to the standard.
Have a citation for that?
Of course.
See ISO 9899:1999, section 6.7.8.
Paragraph 10:
If an object that has automatic storage duration is not initialized explicitly, its value is indeterminate. If an object that has static storage duration is not initialized explicitly, then:
-- if it has pointer type, it is initialized to a null pointer;
-- if it has arithmetic type, it is initialized to (positive or unsigned) zero;
-- if it is an aggregate, every member is initialized (recursively) according to these rules;
-- if it is a union, the first named member is initialized (recursively) according to these rules.
Paragraph 21:
If there are fewer initializers in a brace-enclosed list than there are elements or members of an aggregate, or fewer characters in a string literal used to initialize an array of known size than there are elements in the array, the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration.
-Peff
Re: [PATCH 1/7] Rework strbuf API and semantics.
Quoted 14 lines · Jeff King
Quoted 7 lines · David Kastrup
Quoted 5 lines
Quoted 3 lines
Would not "struct strbuf sb = { 0 };" have the same effect? (I am not so standards-keen as other people, who I have no doubt will gladly answer this one.)
Yes, it would, according to the standard.
Have a citation for that?
Of course.
See ISO 9899:1999, section 6.7.8.
Quoted 7 lines
Paragraph 10:
If an object that has automatic storage duration is not initialized explicitly, its value is indeterminate. If an object that has static storage duration is not initialized explicitly, then:
-- if it has pointer type, it is initialized to a null pointer;
That's actually a new one to me. I don't think that it has been always the case in ANSI C.
Thanks.
Signature · 1 line
David Kastrup, Kriemhildstr. 15, 44793 Bochum
Re: [PATCH 1/7] Rework strbuf API and semantics.
Quoted 8 lines · David Kastrup
Quoted 5 lines
If an object that has automatic storage duration is not initialized explicitly, its value is indeterminate. If an object that has static storage duration is not initialized explicitly, then:
-- if it has pointer type, it is initialized to a null pointer;
That's actually a new one to me. I don't think that it has been always the case in ANSI C.
I don't have the C89 standard, so it's hard to be authoritative. However, according to TCOR1 to the C89 standard, the original text of 6.5.7 contained:
If an object that has static storage duration is not initialized explicitly, it is initialized implicitly as if every member that has arithmetic type were assigned 0 and every member that has pointer type were assigned a null pointer constant.
and was changed to:
If an object that has static storage duration is not initialized explicitly, then:
- if it has pointer type, it is initialized to a null pointer;
- if it has arithmetic type, it is initialized to zero;
- if it is an aggregate, every member is initialized (recursively) according to these rules;
- if it is a union, the first named member is initialized (recursively) according to these rules.
But for the case of pointer initializations, both have the same effect. So I think it has always been the case. Pre-ANSI, who knows. :)
You can find TCOR1 here:
http://www.open-std.org/jtc1/sc22/wg14/www/docs/tc1.htm
And now I must go get some real work done instead of snooping through standards. :)
-Peff
Re: [PATCH 1/7] Rework strbuf API and semantics.
Quoted 11 lines · Jeff King
Quoted 8 lines · David Kastrup
Quoted 5 lines
If an object that has automatic storage duration is not initialized explicitly, its value is indeterminate. If an object that has static storage duration is not initialized explicitly, then:
-- if it has pointer type, it is initialized to a null pointer;
That's actually a new one to me. I don't think that it has been always the case in ANSI C.
Quoted 8 lines
I don't have the C89 standard, so it's hard to be authoritative. However, according to TCOR1 to the C89 standard, the original text of 6.5.7 contained:
If an object that has static storage duration is not initialized explicitly, it is initialized implicitly as if every member that has arithmetic type were assigned 0 and every member that has pointer type were assigned a null pointer constant.
Maybe I am confusing this with the effects of calloc or memset(...,0).
Quoted 3 lines
But for the case of pointer initializations, both have the same effect. So I think it has always been the case. Pre-ANSI, who knows. :)
In the original K&R C, a null pointer likely could have been assumed to have zero bits throughout. The non-zero-bits NULL pointer concept just reeks of standard committees... So maybe this never had been an issue, for different reasons.
Quoted 2 lines
And now I must go get some real work done instead of snooping through standards. :)
Sorry for the diversion.
Signature · 1 line
David Kastrup, Kriemhildstr. 15, 44793 Bochum
Re: [PATCH 1/7] Rework strbuf API and semantics.
In reply to David KastrupQuoted 5 lines · David Kastrup
Quoted 4 lines · Jeff King
See ISO 9899:1999, section 6.7.8.
Paragraph 10:
Quoted 8 lines
Quoted 5 lines
If an object that has automatic storage duration is not initialized explicitly, its value is indeterminate. If an object that has static storage duration is not initialized explicitly, then:
-- if it has pointer type, it is initialized to a null pointer;
That's actually a new one to me. I don't think that it has been always the case in ANSI C.
AFAIR, this has always been the case.
-- Hannes
Re: [PATCH 1/7] Rework strbuf API and semantics.
In reply to Johannes SchindelinQuoted 11 lines · Johannes Schindelin
let me thank you for this very nicely done patch series. Except for 5/7, they look pretty much obvious changes to me. I'll review that in detail later.
Quoted 1 line · Pierre Habouzit
+#define
STRBUF_INIT{ 0, 0, 0, NULL }Would not "struct strbuf sb = { 0 };" have the same effect? (I am not so standards-keen as other people, who I have no doubt will gladly answer this one.)
AFAIR, non-specified static memory areas are initialized to zero bits, and NULL resp (void *)0 is not guaranteed to be represented by zero bits.
Signature · 1 line
David Kastrup, Kriemhildstr. 15, 44793 Bochum
Re: [PATCH 1/7] Rework strbuf API and semantics.
Quoted 7 lines · David Kastrup
Quoted 3 lines
Would not "struct strbuf sb = { 0 };" have the same effect? (I am not so standards-keen as other people, who I have no doubt will gladly answer this one.)
AFAIR, non-specified static memory areas are initialized to zero bits, and NULL resp (void *)0 is not guaranteed to be represented by zero bits.
You are right that the NULL pointer is not necessarily represented as zero bits, but static pointers are explicitly initialized to the NULL pointer (not all-bits-zero).
-Peff
Re: strbuf new API, take 2 for inclusion
In reply to Pierre HabouzitQuoted 3 lines · Pierre Habouzit
I've also stripped as many
STRBUF_INITuses as possible, some people
didn't liked it. I've kept its use for "static" strbufs where it's way more convenient that a function call.
The STRBUF_INIT initializer just sets everything to '0' or NULL. Static objects already have this done automagically by the compiler, so there's no need to use STRBUF_INIT at all there.
-Peff
Re: strbuf new API, take 2 for inclusion
Quoted 9 lines · Jeff King
Quoted 3 lines · Pierre Habouzit
I've also stripped as many
STRBUF_INITuses as possible, some people
didn't liked it. I've kept its use for "static" strbufs where it's way more convenient that a function call.The
STRBUF_INITinitializer just sets everything to '0' or NULL. Static objects already have this done automagically by the compiler, so there's no need to useSTRBUF_INITat all there.
Yes, Junio already did that remark. The reason is that it's forward
compatible: if we ever change strbuf's intitial value for some reason, we would just have to rebuild the code. As junio disliked it (and I'm not sure I love it either) I've used it where using the _init() function was impractical.
And yes { 0 } would have worked the same as per C standard. It's just
that I setup my compiler to flag missing C89 initializers (because it often detects real errors).
Here are the whys'
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org
Re: strbuf new API, take 2 for inclusion
Quoted 5 lines · Pierre Habouzit
Yes, Junio already did that remark. The reason is that it's forward
compatible: if we ever change strbuf's intitial value for some reason, we would just have to rebuild the code. As junio disliked it (and I'm not sure I love it either) I've used it where using the_init()function was impractical.
OK, I missed that discussion. Thanks for the explanation.
-Peff
Re: strbuf new API, take 2 for inclusion
Quoted 9 lines · Jeff King
Quoted 5 lines · Pierre Habouzit
Yes, Junio already did that remark. The reason is that it's forward
compatible: if we ever change strbuf's intitial value for some reason, we would just have to rebuild the code. As junio disliked it (and I'm not sure I love it either) I've used it where using the_init()function was impractical.OK, I missed that discussion. Thanks for the explanation.
That was on IRC, maybe that's the why :)
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org
Use strbufs in commit.c (pretty printing)
In reply to Pierre Habouzit Here is a 3 patch series to use strbufs for the pretty printing
functions in commit.c.
* The first one adds a needed useful function to strbufs (rtrim).
* The second one changes the semantics of interpolate (as the third will need that) and fixes the rest of the code accordingly.
* Do the change.
Testsuite passes after step 2 and 3.
PATCH 1/3 Add strbuf_rtrim (to remove trailing spaces).
--- strbuf.c | 7 +++++++ strbuf.h | 3 +++ 2 files changed, 10 insertions(+), 0 deletions(-)
strbuf.c +7 -0
@@ -28,6 +28,13 @@ void strbuf_grow(struct strbuf *sb, size_t extra) {ALLOC_GROW (sb->buf, sb->len + extra +1 , sb->alloc); } +void strbuf_rtrim (struct strbuf *sb) +{ +while (sb->len >0 &&isspace ((unsigned char )sb->buf[sb->len -1 ])) + sb->len--; + sb->buf[sb->len] ='\0' ; +} +void strbuf_add (struct strbuf *sb,const void *data,size_t len) {strbuf_grow (sb, len);memcpy (sb->buf + sb->len, data, len);
strbuf.h +3 -0
@@ -68,6 +68,9 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len) {extern void strbuf_grow (struct strbuf *,size_t ); +/*----- content related -----*/ +extern void strbuf_rtrim (struct strbuf *); +/*----- add data in your buffer -----*/ static inline void strbuf_addch (struct strbuf *sb,int c) {strbuf_grow (sb,1 );
Signature · 1 line
1.5.3.1
PATCH 2/3 Change semantics of interpolate to work like snprintf.
Also fix many off-by-ones and a useless memset.
--- commit.c | 9 ++++----- interpolate.c | 20 ++++++++------------ 2 files changed, 12 insertions(+), 17 deletions(-)
commit.c +4 -5
@@ -922,15 +922,14 @@ static long format_commit_message(const struct commit *commit,do {char *buf = *buf_p; -unsigned long space = *space_p; +unsigned long len; - space =interpolate (buf, space, user_format, + len =interpolate (buf, *space_p, user_format, table,ARRAY_SIZE (table)); -if (!space) +if (len < *space_p)break ; - buf =xrealloc (buf, space); +ALLOC_GROW (buf, len +1 , *space_p); *buf_p = buf; - *space_p = space; }while (1 );interp_clear_table (table,ARRAY_SIZE (table));
interpolate.c +8 -12
@@ -44,9 +44,8 @@ void interp_clear_table(struct interp *table, int ninterps)* { "%%", "%"}, * } * -* Returns 0 on a successful substitution pass that fits in result, -* Returns a number of bytes needed to hold the full substituted -* string otherwise. +* Returns the length of the substituted string (not including the final \0). +* Like with snprintf, if the result is >= reslen, then it overflowed. */ unsigned long interpolate(char *result, unsigned long reslen,
@@ -61,8 +60,6 @@ unsigned long interpolate(char *result, unsigned long reslen,int i;char c; -memset (result,0 , reslen); -while ((c = *src)) {if (c =='%' ) {/* Try to match an interpolation string. */
@@ -78,9 +75,9 @@ unsigned long interpolate(char *result, unsigned long reslen, value = interps[i].value; valuelen =strlen (value); -if (newlen + valuelen +1 < reslen) { +if (newlen + valuelen < reslen) {/* Substitute. */ -strncpy (dest, value, valuelen); +memcpy (dest, value, valuelen); dest += valuelen; } newlen += valuelen;
@@ -89,14 +86,13 @@ unsigned long interpolate(char *result, unsigned long reslen, } }/* Straight copy one non-interpolation character. */ -if (newlen +1 < reslen) +if (newlen < reslen) *dest++ = *src; src++; newlen++; } -if (newlen +1 < reslen) -return 0 ; -else -return newlen +2 ; +if (reslen >0 ) + *dest ='\0' ; +return newlen; }
Signature · 1 line
1.5.3.1
PATCH 3/3 Rework pretty_print_commit to use strbufs instead of custom buffers.
Also remove the "len" parameter, as:
(1) it was used as a max boundary, and every caller used ~0u
(2) we check for final NUL no matter what, so it doesn't help for speed.
As a result most of the pp_* function takes 3 arguments less, and we need
a lot less local variables, this makes the code way more readable, and easier to extend if needed.
This patch also fixes some spacing and cosmetic issues.
--- builtin-branch.c | 15 +-- builtin-log.c | 12 +- builtin-rev-list.c | 13 +- builtin-show-branch.c | 13 +- commit.c | 330 ++++++++++++++++++------------------------------- commit.h | 6 +- log-tree.c | 56 +++------ 7 files changed, 171 insertions(+), 274 deletions(-)
builtin-branch.c +7 -8
@@ -268,23 +268,22 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose, }if (verbose) { -char *subject =NULL ; -unsigned long subject_len =0 ; +struct strbuf subject;const char *sub =" **** invalid ref ****" ; +strbuf_init (&subject); + commit =lookup_commit (item->sha1);if (commit && !parse_commit (commit)) { -pretty_print_commit (CMIT_FMT_ONELINE , commit, ~0 , - &subject, &subject_len,0 , -NULL ,NULL ,0 ); - sub = subject; +pretty_print_commit (CMIT_FMT_ONELINE , commit, + &subject,0 ,NULL ,NULL ,0 ); + sub = subject.buf; }printf ("%c %s%-*s%s %s %s\n" , c,branch_get_color (color), maxwidth, item->name,branch_get_color (COLOR_BRANCH_RESET ),find_unique_abbrev (item->sha1, abbrev), sub); -if (subject) -free (subject); +strbuf_release (&subject); }else {printf ("%c %s%s%s\n" , c,branch_get_color (color), item->name,branch_get_color (COLOR_BRANCH_RESET ));
builtin-log.c +6 -6
@@ -763,13 +763,13 @@ int cmd_cherry(int argc, const char **argv, const char *prefix) sign ='-' ;if (verbose) { -char *buf =NULL ; -unsigned long buflen =0 ; -pretty_print_commit (CMIT_FMT_ONELINE , commit, ~0 , - &buf, &buflen,0 ,NULL ,NULL ,0 ); +struct strbuf buf; +strbuf_init (&buf); +pretty_print_commit (CMIT_FMT_ONELINE , commit, + &buf,0 ,NULL ,NULL ,0 );printf ("%c %s %s\n" , sign, -sha1_to_hex (commit->object.sha1), buf); -free (buf); +sha1_to_hex (commit->object.sha1), buf.buf); +strbuf_release (&buf); }else { printf("%c %s\n", sign,
builtin-rev-list.c +6 -7
@@ -80,13 +80,12 @@ static void show_commit(struct commit *commit)putchar ('\n' );if (revs.verbose_header) { -char *buf =NULL ; -unsigned long buflen =0 ; -pretty_print_commit (revs.commit_format, commit, ~0 , - &buf, &buflen, - revs.abbrev,NULL ,NULL , revs.date_mode); -printf ("%s%c" , buf, hdr_termination); -free (buf); +struct strbuf buf; +strbuf_init (&buf); +pretty_print_commit (revs.commit_format, commit, + &buf, revs.abbrev,NULL ,NULL , revs.date_mode); +printf ("%s%c" , buf.buf, hdr_termination); +strbuf_release (&buf); }maybe_flush_or_die (stdout,"stdout" );if (commit->parents) {
builtin-show-branch.c +6 -7
@@ -259,16 +259,15 @@ static void join_revs(struct commit_list **list_p,static void show_one_commit (struct commit *commit,int no_name) { -char *pretty =NULL ; +struct strbuf pretty;const char *pretty_str ="(unavailable)" ; -unsigned long pretty_len =0 ;struct commit_name *name = commit->util; +strbuf_init (&pretty);if (commit->object.parsed) { -pretty_print_commit (CMIT_FMT_ONELINE , commit, ~0 , - &pretty, &pretty_len, -0 ,NULL ,NULL ,0 ); - pretty_str = pretty; +pretty_print_commit (CMIT_FMT_ONELINE , commit, + &pretty,0 ,NULL ,NULL ,0 ); + pretty_str = pretty.buf; }if (!prefixcmp (pretty_str,"[PATCH] " )) pretty_str +=8 ;
@@ -289,7 +288,7 @@ static void show_one_commit(struct commit *commit, int no_name) find_unique_abbrev(commit->object.sha1, 7)); }puts (pretty_str); -free (pretty); +strbuf_release (&pretty); }static char *ref_name[MAX_REVS +1 ];
commit.c +122 -208
629 lines not on this page · raw message
commit.h +5 -1
@@ -3,6 +3,7 @@#include "object.h" #include "tree.h" +#include "strbuf.h" #include "decorate.h" struct commit_list {
@@ -61,7 +62,10 @@ enum cmit_fmt { };extern enum cmit_fmt get_commit_format (const char *arg); -extern unsigned long pretty_print_commit (enum cmit_fmt fmt,const struct commit *,unsigned long len,char **buf_p,unsigned long *space_p,int abbrev,const char *subject,const char *after_subject,enum date_mode dmode); +extern void pretty_print_commit (enum cmit_fmt fmt,const struct commit *, +struct strbuf *, +int abbrev,const char *subject, +const char *after_subject,enum date_mode );/** Removes the first commit from a list sorted by date, and adds all * of its parents.
log-tree.c +19 -37
105 lines not on this page · raw message
Signature · 1 line
1.5.3.1
Re: [PATCH 3/3] Rework pretty_print_commit to use strbufs instead of custom buffers.
Quoted 11 lines · Pierre Habouzit
Also remove the "len" parameter, as:
(1) it was used as a max boundary, and every caller used ~0u
(2) we check for final NUL no matter what, so it doesn't help for speed.As a result most of the pp_* function takes 3 arguments less, and we need
a lot less local variables, this makes the code way more readable, and easier to extend if needed.This patch also fixes some spacing and cosmetic issues.
Signed-off-by Pierre Habouzit <[email protected]>
Quoted 9 lines
--- builtin-branch.c | 15 +-- builtin-log.c | 12 +- builtin-rev-list.c | 13 +- builtin-show-branch.c | 13 +- commit.c | 330 ++++++++++++++++++------------------------------- commit.h | 6 +- log-tree.c | 56 +++------ 7 files changed, 171 insertions(+), 274 deletions(-)
Nice! I wonder if we should #include strbuf.h from git-compat-util.h (just like e.g. string.h) instead of from commit.h, in order to have strbuf available everywhere in git.
Please be aware of the changes to commit.c already in next which your patch conflicts with: format_commit_message() has been exported and is used in builtin-archive.c there.
René
Re: [PATCH 3/3] Rework pretty_print_commit to use strbufs instead of custom buffers.
Quoted 22 lines · René Scharfe
Quoted 11 lines · Pierre Habouzit
Also remove the "len" parameter, as:
(1) it was used as a max boundary, and every caller used ~0u
(2) we check for final NUL no matter what, so it doesn't help for speed.As a result most of the pp_* function takes 3 arguments less, and we need
a lot less local variables, this makes the code way more readable, and easier to extend if needed.This patch also fixes some spacing and cosmetic issues.
Signed-off-by Pierre Habouzit <[email protected]>Quoted 9 lines
--- builtin-branch.c | 15 +-- builtin-log.c | 12 +- builtin-rev-list.c | 13 +- builtin-show-branch.c | 13 +- commit.c | 330 ++++++++++++++++++------------------------------- commit.h | 6 +- log-tree.c | 56 +++------ 7 files changed, 171 insertions(+), 274 deletions(-)
Quoted 7 lines
Nice! I wonder if we should #include
strbuf.hfromgit-compat-util.h(just like e.g.string.h) instead of fromcommit.h, in order to have strbuf available everywhere in git.Please be aware of the changes to
commit.calready in next which your patch conflicts with:format_commit_message()has been exported and is used inbuiltin-archive.cthere.
Okay, then I'll hope than Junio will merge the first series and then
rebase this one on next. If I'm going to propose more of those patches, should I write them rather on next or master (like I do now) ?
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org
Re: [PATCH 3/3] Rework pretty_print_commit to use strbufs instead of custom buffers.
In reply to Pierre HabouzitQuoted 3 lines · Pierre Habouzit
Also remove the "len" parameter, as:
(1) it was used as a max boundary, and every caller used ~0u
(2) we check for final NUL no matter what, so it doesn't help for speed.
That sounds like a change that makes improvement of callers impossible when it is found out that it leads to a performance issue. Is it only the pretty-print that is affected?
Signature · 1 line
David Kastrup, Kriemhildstr. 15, 44793 Bochum
Re: [PATCH 3/3] Rework pretty_print_commit to use strbufs instead of custom buffers.
Quoted 9 lines · David Kastrup
Quoted 3 lines · Pierre Habouzit
Also remove the "len" parameter, as:
(1) it was used as a max boundary, and every caller used ~0u
(2) we check for final NUL no matter what, so it doesn't help for speed.That sounds like a change that makes improvement of callers impossible when it is found out that it leads to a performance issue. Is it only the pretty-print that is affected?
I removed the "len" argument of pretty_print_commit and all the sub
pp_* functions it uses. This argument was supposed to tell which size the commit message you want to read to format the pretty printing.
It leads to a lot of code that works like this:
if (position < len || *msg) /* test if len is overflowed or if we are
at the end of the string */
break;
This impose us to maintain the len of the message while we make the
"msg" pointer progress, and so on. And it's a limit of what to read in the commit message.
Seeing where pretty_print_commit is used:
builtin-branch.c: pretty_print_commit(CMIT_FMT_ONELINE, commit,builtin-log.c: pretty_print_commit(CMIT_FMT_ONELINE, commit,builtin-rev-list.c: pretty_print_commit(revs.commit_format, commit,builtin-show-branch.c: pretty_print_commit(CMIT_FMT_ONELINE, commit,
commit.c:void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,log-tree.c: pretty_print_commit(opt->commit_format, commit, &msgbuf,
I assume that the user would not be very pleased if we decide to crop
his commits logs when he uses git-log. And given that git log in the linux repository on my laptop takes:
2,13s user 0,06s system 99% cpu 2,213 total
when the repo is hot ... I hardly think it can ever be a performance issue :)
But if people disagree I can refactor a patch with the "len" argument
kept.
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org
Re: [PATCH 3/3] Rework pretty_print_commit to use strbufs instead of custom buffers.
In reply to David KastrupQuoted 9 lines · David Kastrup
Quoted 3 lines · Pierre Habouzit
Also remove the "len" parameter, as:
(1) it was used as a max boundary, and every caller used ~0u
(2) we check for final NUL no matter what, so it doesn't help for speed.That sounds like a change that makes improvement of callers impossible when it is found out that it leads to a performance issue. Is it only the pretty-print that is affected?
Speaking of performance, here is a small interesting bench:
$ for i in `seq 1 10`; do time git-log >| /dev/null; done git-log >| /dev/null 2,12s user 0,08s system 99% cpu 2,205 total git-log >| /dev/null 2,14s user 0,06s system 99% cpu 2,208 total git-log >| /dev/null 2,12s user 0,08s system 99% cpu 2,205 total git-log >| /dev/null 2,10s user 0,10s system 99% cpu 2,209 total git-log >| /dev/null 2,14s user 0,07s system 99% cpu 2,211 total git-log >| /dev/null 2,11s user 0,09s system 99% cpu 2,210 total git-log >| /dev/null 2,15s user 0,05s system 99% cpu 2,213 total git-log >| /dev/null 2,12s user 0,07s system 99% cpu 2,203 total git-log >| /dev/null 2,13s user 0,06s system 99% cpu 2,204 total git-log >| /dev/null 2,17s user 0,09s system 100% cpu 2,254 total
$ for i in `seq 1 10`; do time ~/dev/scm/git/git-log >| /dev/null; done ~/dev/scm/git/git-log >| /dev/null 2,06s user 0,11s system 99% cpu 2,185 total ~/dev/scm/git/git-log >| /dev/null 2,11s user 0,07s system 99% cpu 2,192 total ~/dev/scm/git/git-log >| /dev/null 2,10s user 0,10s system 99% cpu 2,207 total ~/dev/scm/git/git-log >| /dev/null 2,10s user 0,08s system 99% cpu 2,188 total ~/dev/scm/git/git-log >| /dev/null 2,10s user 0,08s system 99% cpu 2,187 total ~/dev/scm/git/git-log >| /dev/null 2,10s user 0,10s system 100% cpu 2,196 total ~/dev/scm/git/git-log >| /dev/null 2,08s user 0,11s system 99% cpu 2,195 total ~/dev/scm/git/git-log >| /dev/null 2,11s user 0,08s system 99% cpu 2,193 total ~/dev/scm/git/git-log >| /dev/null 2,10s user 0,08s system 100% cpu 2,188 total ~/dev/scm/git/git-log >| /dev/null 2,12s user 0,07s system 98% cpu 2,213 total
$ git rev-list --all | wc -l 64271
The underling repository is a not so old linux-2.6 git repository.
git-log is the default unstable git-log, (a pristine 1.5.3.1, pre-new-strbuf).
~/dev/scm/git/git-log is obviously the new one with the whole strbuf
patch series applied. Strbufs definitely made the code more readable (at least it's my impression), here is the proof that it did not affected an inch from performance (we even seem to have a marginal 0.5% gain in performance ;p).
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org
Re: [PATCH 1/3] Add strbuf_rtrim (to remove trailing spaces).
In reply to Pierre HabouzitQuoted 5 lines · Pierre Habouzit
strbuf.c +0
-0
strbuf.c void strbuf_grow(struct strbuf *sb, size_t extra) { · line 36
Quoted 9 lines
strbuf.c +6
-0@@ -28,6 +28,13 @@ void strbuf_grow(struct strbuf *sb, size_t extra) {ALLOC_GROW (sb->buf, sb->len + extra +1 , sb->alloc); } +void strbuf_rtrim (struct strbuf *sb) +{ +while (sb->len >0 &&isspace ((unsigned char )sb->buf[sb->len -1 ])) + sb->len--; + sb->buf[sb->len] ='\0' ; +}
Please use tabs instead of spaces to indent, just like you did in your other patches. :)
René
Re: [PATCH 1/3] Add strbuf_rtrim (to remove trailing spaces).
Quoted 9 lines · René Scharfe
Quoted 8 lines · Pierre Habouzit
strbuf.c +0
-0@@ -28,6 +28,13 @@ void strbuf_grow(struct strbuf *sb, size_t extra) {ALLOC_GROW (sb->buf, sb->len + extra +1 , sb->alloc); }
Quoted 9 lines
strbuf.c void strbuf_grow(struct strbuf *sb, size_t extra) { · line 36
Quoted 6 lines
strbuf.c +6
-0+void strbuf_rtrim (struct strbuf *sb) +{ +while (sb->len >0 &&isspace ((unsigned char )sb->buf[sb->len -1 ])) + sb->len--; + sb->buf[sb->len] ='\0' ; +}Please use tabs instead of spaces to indent, just like you did in your other patches. :)
yeah, good catch.
I've added some more patches tonight, and have added another function
in strbufs, and thanks to the awsome git rebase -i, this patch integrates this new function as well, so this patch series is obsolete already. I wont repost it until I know if I should rebase it on next rather than master though.
Cheers,
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org
Re: [PATCH 1/3] Add strbuf_rtrim (to remove trailing spaces).
Quoted 20 lines · Pierre Habouzit
Quoted 9 lines · René Scharfe
Quoted 8 lines · Pierre Habouzit
strbuf.c +0
-0@@ -28,6 +28,13 @@ void strbuf_grow(struct strbuf *sb, size_t extra) {ALLOC_GROW (sb->buf, sb->len + extra +1 , sb->alloc); }Quoted 9 lines
strbuf.c void strbuf_grow(struct strbuf *sb, size_t extra) { · line 36
Quoted 6 lines
strbuf.c +6
-0+void strbuf_rtrim (struct strbuf *sb) +{ +while (sb->len >0 &&isspace ((unsigned char )sb->buf[sb->len -1 ])) + sb->len--; + sb->buf[sb->len] ='\0' ; +}Please use tabs instead of spaces to indent, just like you did in your other patches. :)
Quoted 6 lines
yeah, good catch.
I've added some more patches tonight, and have added another function
in strbufs, and thanks to the awsome git rebase -i, this patch integrates this new function as well, so this patch series is obsolete already. I wont repost it until I know if I should rebase it on next rather than master though.
Okay I'm stupid, Junio already merged my patches in next, doh.
I've seen the conflict btw, and there is a memory leak (on the format)
in builtin-archive.c now... but I'm going to fix it (hopefuly) :)
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org