Use more strbufs series [on top of next]
Here is a series of patches on top of next, which use strbufs in even
more places. Most notably, it uses it in the commit pretty printer (and commit message formatter), and it supersedes definitely read_fd (previously in sha1_file.c).
The latter is not strictly speaking necessary, but strbuf_read andread_fd do almost the same. The sole difference is that read_fd use custom reallocation mechanisms (which is bad now that strbufs exists) though had a different semantics when an error occurs. Though, like for strbuf_read, read_fd callers either die() or discard the buffer, and current strbuf_read semantics works with that.
As a result we once again have a nice reduction of the code lines.
$ git diff --shortstat origin/next strbuf.* 2 files changed, 24 insertions(+), 0 deletions(-)
$ git diff --shortstat origin/next ^strbuf.* 16 files changed, 270 insertions(+), 466 deletions(-)
I feel that with the current series commit.c can be simplified
further, but the patch was big enough as is already.
If this series will be rewritten for this or this reason, please note
that I found a memory leak (not a severe one though) in builtin-archive.c, where the "fmt" pointer is allocated and never freed.
Cheers,
PATCH 1/6 Add strbuf_rtrim and strbuf_insert.
* strbuf_rtrim removes trailing spaces.
* strbuf_insert insert data at a given position.
--- strbuf.c | 18 ++++++++++++++++++ strbuf.h | 6 ++++++ 2 files changed, 24 insertions(+), 0 deletions(-)
strbuf.c +18 -0
@@ -28,6 +28,24 @@ 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_insert (struct strbuf *sb,size_t pos,const void *data,size_t len) { +strbuf_grow (sb, len); +if (pos >= sb->len) { + sb->len = pos; + }else { +memmove (sb->buf + pos + len, sb->buf + pos, sb->len - pos); + } +memcpy (sb->buf + pos, data, len); +strbuf_setlen (sb, sb->len + len); +} +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 +6 -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 );
@@ -75,6 +78,9 @@ static inline void strbuf_addch(struct strbuf *sb, int c) { sb->buf[sb->len] ='\0' ; } +/* inserts after pos, or appends if pos >= sb->len */ +extern void strbuf_insert (struct strbuf *,size_t pos,const void *,size_t ); +extern void strbuf_add (struct strbuf *,const void *,size_t );static inline void strbuf_addstr (struct strbuf *sb,const char *s) {strbuf_add (sb, s,strlen (s));
Signature · 1 line
1.5.3.1
PATCH 2/6 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
@@ -923,15 +923,14 @@ long format_commit_message(const struct commit *commit, const void *format,do {char *buf = *buf_p; -unsigned long space = *space_p; +unsigned long len; - space =interpolate (buf, space, format, + len =interpolate (buf, *space_p, 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/6 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.
This patch also fixes (as a side effect) a memory leak intoruced inbuiltin-archive.c at commit df4a394f (fmt was xmalloc'ed and not free'd)
--- builtin-archive.c | 38 +++---- builtin-branch.c | 15 +-- builtin-log.c | 12 +- builtin-rev-list.c | 13 +- builtin-show-branch.c | 13 +- commit.c | 330 ++++++++++++++++++------------------------------- commit.h | 9 +- log-tree.c | 56 +++------ 8 files changed, 189 insertions(+), 297 deletions(-)
builtin-archive.c +16 -22
@@ -84,14 +84,16 @@ static int run_remote_archiver(const char *remote, int argc,static void *format_subst (const struct commit *commit,const char *format,unsigned long *sizep) { -unsigned long len = *sizep, result_len =0 ; +unsigned long len = *sizep;const char *a = format; -char *result =NULL ; +struct strbuf result; +struct strbuf fmt; + +strbuf_init (&result); +strbuf_init (&fmt);for (;;) {const char *b, *c; -char *fmt, *formatted =NULL ; -unsigned long a_len, fmt_len, formatted_len, allocated =0 ; b =memmem (a, len,"$Format:" ,8 );if (!b || a + len < b +9 )
@@ -100,31 +102,23 @@ static void *format_subst(const struct commit *commit, const char *format,if (!c)break ; - a_len = b - a; - fmt_len = c - b -8 ; - fmt =xmalloc (fmt_len +1 ); -memcpy (fmt, b +8 , fmt_len); - fmt[fmt_len] ='\0' ; - - formatted_len =format_commit_message (commit, fmt, &formatted, - &allocated); - result =xrealloc (result, result_len + a_len + formatted_len); -memcpy (result + result_len, a, a_len); -memcpy (result + result_len + a_len, formatted, formatted_len); - result_len += a_len + formatted_len; +strbuf_reset (&fmt); +strbuf_add (&fmt, b +8 , c - b -8 ); + +strbuf_add (&result, a, b - a); +format_commit_message (commit, fmt.buf, &result); len -= c +1 - a; a = c +1 ; } -if (result && len) { - result =xrealloc (result, result_len + len); -memcpy (result + result_len, a, len); - result_len += len; +if (result.len && len) { +strbuf_add (&result, a, len); } - *sizep = result_len; + *sizep = result.len; -return result; +strbuf_release (&fmt); +return strbuf_detach (&result); } static void *convert_to_archive(const char *path,
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 +7 -2
@@ -3,6 +3,7 @@#include "object.h" #include "tree.h" +#include "strbuf.h" #include "decorate.h" struct commit_list {
@@ -61,8 +62,12 @@ enum cmit_fmt { };extern enum cmit_fmt get_commit_format (const char *arg); -extern long format_commit_message (const struct commit *commit,const void *template,char **buf_p,unsigned long *space_p); -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 format_commit_message (const struct commit *commit, +const void *format,struct strbuf *sb); +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
PATCH 4/6 Use strbuf_read in builtin-fetch-tool.c.
xrealloc.use --;
--- builtin-fetch--tool.c | 23 ++++++----------------- 1 files changed, 6 insertions(+), 17 deletions(-)
builtin-fetch--tool.c +6 -17
@@ -2,27 +2,16 @@#include "cache.h" #include "refs.h" #include "commit.h" - -#define CHUNK_SIZE 1024 +#include "strbuf.h" static char *get_stdin (void ) { -size_t offset =0 ; -char *data =xmalloc (CHUNK_SIZE ); - -while (1 ) { -ssize_t cnt =xread (0 , data + offset,CHUNK_SIZE ); -if (cnt <0 ) -die ("error reading standard input: %s" , -strerror (errno)); -if (cnt ==0 ) { - data[offset] =0 ; -break ; - } - offset += cnt; - data =xrealloc (data, offset +CHUNK_SIZE ); +struct strbuf buf; +strbuf_init (&buf); +if (strbuf_read (&buf,0 ) <0 ) { +die ("error reading standard input: %s" ,strerror (errno)); } -return data; +return strbuf_detach (&buf); }static void show_new (enum object_type type,unsigned char *sha1_new)
Signature · 1 line
1.5.3.1
PATCH 5/6 Use strbufs to in read_message (imap-send.c), custom buffer--.
--- imap-send.c | 31 +++++++++++-------------------- 1 files changed, 11 insertions(+), 20 deletions(-)
imap-send.c +11 -20
@@ -23,6 +23,7 @@*/ #include "cache.h" +#include "strbuf.h" typedef struct store_conf { char *name;
@@ -1160,28 +1161,18 @@ imap_store_msg( store_t *gctx, msg_data_t *data, int *uid )static int read_message (FILE *f,msg_data_t *msg ) { -int len, r; +struct strbuf buf; -memset ( msg,0 ,sizeof *msg ); - len =CHUNKSIZE ; - msg->data =xmalloc ( len+1 ); - msg->data[0 ] =0 ; - -while (!feof ( f )) { -if (msg->len >= len) { -void *p; - len +=CHUNKSIZE ; - p =xrealloc (msg->data, len+1 ); -if (!p) -break ; - msg->data = p; - } - r =fread ( &msg->data[msg->len],1 , len - msg->len, f ); -if (r <=0 ) +memset (msg,0 ,sizeof (*msg)); +strbuf_init (&buf); + +do { +if (strbuf_fread (&buf,CHUNKSIZE , f) <=0 )break ; - msg->len += r; - } - msg->data[msg->len] =0 ; + }while (!feof (f)); + + msg->len = buf.len; + msg->data =strbuf_detach (&buf);return msg->len; }
Signature · 1 line
1.5.3.1
PATCH 6/6 Replace all read_fd use with strbuf_read, and get rid of it.
This brings builtin-stripspace, builtin-tag and mktag to use strbufs.
--- builtin-stripspace.c | 21 ++++++-------- builtin-tag.c | 78 ++++++++++++++++++++------------------------------ cache.h | 1 - mktag.c | 15 ++++----- sha1_file.c | 60 ++++++-------------------------------- 5 files changed, 56 insertions(+), 119 deletions(-)
builtin-stripspace.c +9 -12
@@ -1,5 +1,6 @@#include "builtin.h" #include "cache.h" +#include "strbuf.h" /* * Returns the length of a line, without trailing spaces.
@@ -74,26 +75,22 @@ size_t stripspace(char *buffer, size_t length, int skip_comments)int cmd_stripspace (int argc,const char **argv,const char *prefix) { -char *buffer; -unsigned long size; +struct strbuf buf;int strip_comments =0 ;if (argc >1 && (!strcmp (argv[1 ],"-s" ) || !strcmp (argv[1 ],"--strip-comments" ))) strip_comments =1 ; - size =1024 ; - buffer =xmalloc (size); -if (read_fd (0 , &buffer, &size)) { -free (buffer); +strbuf_init (&buf); +if (strbuf_read (&buf,0 ) <0 )die ("could not read the input" ); - } - size =stripspace (buffer, size, strip_comments); -write_or_die (1 , buffer, size); -if (size) -putc ('\n' , stdout); +strbuf_setlen (&buf,stripspace (buf.buf, buf.len, strip_comments)); +if (buf.len) +strbuf_addch (&buf,'\n' ); -free (buffer); +write_or_die (1 , buf.buf, buf.len); +strbuf_release (&buf);return 0 ; }
builtin-tag.c +31 -47
180 lines not on this page · raw message
cache.h +0 -1
@@ -269,7 +269,6 @@ extern int ie_match_stat(struct index_state *, struct cache_entry *, struct statextern int ie_modified (struct index_state *,struct cache_entry *,struct stat *,int );extern int ce_path_match (const struct cache_entry *ce,const char **pathspec);extern int index_fd (unsigned char *sha1,int fd,struct stat *st,int write_object,enum object_type type,const char *path); -extern int read_fd (int fd,char **return_buf,unsigned long *return_size);extern int index_pipe (unsigned char *sha1,int fd,const char *type,int write_object);extern int index_path (unsigned char *sha1,const char *path,struct stat *st,int write_object);extern void fill_stat_cache_info (struct cache_entry *ce,struct stat *st);
mktag.c +7 -8
@@ -1,4 +1,5 @@#include "cache.h" +#include "strbuf.h" #include "tag.h" /*
@@ -111,8 +112,7 @@ static int verify_tag(char *buffer, unsigned long size)int main (int argc,char **argv) { -unsigned long size =4096 ; -char *buffer =xmalloc (size); +struct strbuf buf;unsigned char result_sha1[20 ];if (argc !=1 )
@@ -120,21 +120,20 @@ int main(int argc, char **argv)setup_git_directory (); -if (read_fd (0 , &buffer, &size)) { -free (buffer); +strbuf_init (&buf); +if (strbuf_read (&buf,0 ) <0 ) {die ("could not read from stdin" ); }/* Verify it for some basic sanity: it needs to start with "object <sha1>\ntype\ntagger " */ -if (verify_tag (buffer, size) <0 ) +if (verify_tag (buf.buf, buf.len) <0 )die ("invalid tag signature file" ); -if (write_sha1_file (buffer, size, tag_type, result_sha1) <0 ) +if (write_sha1_file (buf.buf, buf.len, tag_type, result_sha1) <0 )die ("unable to write tag file" ); -free (buffer); - +strbuf_release (&buf);printf ("%s\n" ,sha1_to_hex (result_sha1));return 0 ; }
sha1_file.c +9 -51
83 lines not on this page · raw message
Signature · 1 line
1.5.3.1
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: return void expressions in C (Was: [PATCH 3/6] Rework pretty_print_commit ...)
Quoted 4 lines · Pierre Habouzit
Okay, I stand corrected, but as I'm curious, I've now the Truth about
this, for those who still care at that point (as the patch has been fixed in the next round anyways ;p)
The
void fn1(void);
void fn2(void)
{
return fn1();
}thing is a gcc extension. It happens to be one we use in the kernel too, since it makes for nicer-looking source code in some circumstances, but it's definitely nonstandard.
Linus
Quoted 4 lines · Pierre Habouzit
Here is a series of patches on top of next, which use strbufs in even
more places. Most notably, it uses it in the commit pretty printer (and commit message formatter), and it supersedes definitelyread_fd(previously insha1_file.c).
A side note that could help the patch readers: in some places I've
made hunks that basically rename variables called space or size into len. The reason is that (in my coding standards) length is the length of the object we are referring to, whereas space/size/... is the amount of memory we have allocated for it. For a string, the latter is strictly greater than the former because of the ending NUL of course.
The rename is because my brain is used to this way of thinking and
that it helps knowing if the off-by-some (off-by-one for a string) are included in the length-related number you work with or not. So please don't discard those :)
Signature · 3 lines
·O· Pierre Habouzit ··O [email protected] OOO http://www.madism.org