From 8c74d22ebfae33323b5561d9bd988f272ff61a01 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Sun, 27 Nov 2011 21:47:58 -0800 Subject: [PATCH 1/3] Extend git_buf with new utility functions and unit tests. Add new functions to git_buf for: * initializing a buffer from a string * joining one or more strings onto a buffer with separators * swapping two buffers in place * extracting data from a git_buf (leaving it empty) Also, make git_buf_free leave a git_buf back in its initted state, and slightly tweak buffer allocation sizes and thresholds. Finally, port unit tests to clay and extend with lots of new tests for the various git_buf functions. --- src/buffer.c | 144 ++++++++++++- src/buffer.h | 19 +- tests-clay/clay_libgit2.h | 16 ++ tests-clay/core/buffer.c | 435 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 604 insertions(+), 10 deletions(-) create mode 100644 tests-clay/core/buffer.c diff --git a/src/buffer.c b/src/buffer.c index 1fb848e46..3fd04211c 100644 --- a/src/buffer.c +++ b/src/buffer.c @@ -9,7 +9,7 @@ #include #define ENSURE_SIZE(b, d) \ - if ((ssize_t)(d) >= buf->asize && git_buf_grow(b, (d)) < GIT_SUCCESS)\ + if ((ssize_t)(d) > buf->asize && git_buf_grow(b, (d)) < GIT_SUCCESS)\ return; int git_buf_grow(git_buf *buf, size_t target_size) @@ -19,6 +19,9 @@ int git_buf_grow(git_buf *buf, size_t target_size) if (buf->asize < 0) return GIT_ENOMEM; + if (target_size <= (size_t)buf->asize) + return GIT_SUCCESS; + if (buf->asize == 0) buf->asize = target_size; @@ -27,6 +30,9 @@ int git_buf_grow(git_buf *buf, size_t target_size) while (buf->asize < (int)target_size) buf->asize = (buf->asize << 1) - (buf->asize >> 1); + /* round allocation up to multiple of 8 */ + buf->asize = (buf->asize + 7) & ~7; + new_ptr = git__realloc(buf->ptr, buf->asize); if (!new_ptr) { buf->asize = -1; @@ -42,6 +48,22 @@ int git_buf_oom(const git_buf *buf) return (buf->asize < 0); } +void git_buf_set(git_buf *buf, const char *data, size_t len) +{ + if (len == 0 || data == NULL) { + git_buf_clear(buf); + } else { + ENSURE_SIZE(buf, len); + memmove(buf->ptr, data, len); + buf->size = len; + } +} + +void git_buf_sets(git_buf *buf, const char *string) +{ + git_buf_set(buf, string, string ? strlen(string) : 0); +} + void git_buf_putc(git_buf *buf, char c) { ENSURE_SIZE(buf, buf->size + 1); @@ -51,13 +73,14 @@ void git_buf_putc(git_buf *buf, char c) void git_buf_put(git_buf *buf, const char *data, size_t len) { ENSURE_SIZE(buf, buf->size + len); - memcpy(buf->ptr + buf->size, data, len); + memmove(buf->ptr + buf->size, data, len); buf->size += len; } void git_buf_puts(git_buf *buf, const char *string) { - git_buf_put(buf, string, strlen(string)); + if (string != NULL) + git_buf_put(buf, string, strlen(string)); } void git_buf_printf(git_buf *buf, const char *format, ...) @@ -88,7 +111,8 @@ void git_buf_printf(git_buf *buf, const char *format, ...) const char *git_buf_cstr(git_buf *buf) { - if (buf->size + 1 >= buf->asize && git_buf_grow(buf, buf->size + 1) < GIT_SUCCESS) + if (buf->size + 1 > buf->asize && + git_buf_grow(buf, buf->size + 1) < GIT_SUCCESS) return NULL; buf->ptr[buf->size] = '\0'; @@ -97,7 +121,14 @@ const char *git_buf_cstr(git_buf *buf) void git_buf_free(git_buf *buf) { - git__free(buf->ptr); + if (buf) { + if (buf->ptr) { + git__free(buf->ptr); + buf->ptr = NULL; + } + buf->asize = 0; + buf->size = 0; + } } void git_buf_clear(git_buf *buf) @@ -107,7 +138,104 @@ void git_buf_clear(git_buf *buf) void git_buf_consume(git_buf *buf, const char *end) { - size_t consumed = end - buf->ptr; - memmove(buf->ptr, end, buf->size - consumed); - buf->size -= consumed; + if (end > buf->ptr && end <= buf->ptr + buf->size) { + size_t consumed = end - buf->ptr; + memmove(buf->ptr, end, buf->size - consumed); + buf->size -= consumed; + } } + +void git_buf_swap(git_buf *buf_a, git_buf *buf_b) +{ + git_buf t = *buf_a; + *buf_a = *buf_b; + *buf_b = t; +} + +char *git_buf_take_cstr(git_buf *buf) +{ + char *data = NULL; + + if (buf->ptr == NULL) + return NULL; + + if (buf->size + 1 > buf->asize && + git_buf_grow(buf, buf->size + 1) < GIT_SUCCESS) + return NULL; + + data = buf->ptr; + data[buf->size] = '\0'; + + buf->ptr = NULL; + buf->asize = 0; + buf->size = 0; + + return data; +} + +void git_buf_join(git_buf *buf, char separator, int nbuf, ...) +{ + /* Make two passes to avoid multiple reallocation */ + + va_list ap; + int i; + int total_size = 0; + char *out; + + if (buf->size > 0 && buf->ptr[buf->size - 1] != separator) + ++total_size; /* space for initial separator */ + + va_start(ap, nbuf); + for (i = 0; i < nbuf; ++i) { + const char* segment; + int segment_len; + + segment = va_arg(ap, const char *); + if (!segment) + continue; + + segment_len = strlen(segment); + total_size += segment_len; + if (segment_len == 0 || segment[segment_len - 1] != separator) + ++total_size; /* space for separator */ + } + va_end(ap); + + ENSURE_SIZE(buf, buf->size + total_size); + + out = buf->ptr + buf->size; + + /* append separator to existing buf if needed */ + if (buf->size > 0 && out[-1] != separator) + *out++ = separator; + + va_start(ap, nbuf); + for (i = 0; i < nbuf; ++i) { + const char* segment; + int segment_len; + + segment = va_arg(ap, const char *); + if (!segment) + continue; + + /* skip leading separators */ + if (out > buf->ptr && out[-1] == separator) + while (*segment == separator) segment++; + + /* copy over next buffer */ + segment_len = strlen(segment); + if (segment_len > 0) { + memmove(out, segment, segment_len); + out += segment_len; + } + + /* append trailing separator (except for last item) */ + if (i < nbuf - 1 && out > buf->ptr && out[-1] != separator) + *out++ = separator; + } + va_end(ap); + + /* set size based on num characters actually written */ + buf->size = out - buf->ptr; +} + diff --git a/src/buffer.h b/src/buffer.h index ad3b8930f..baa8f4f4d 100644 --- a/src/buffer.h +++ b/src/buffer.h @@ -17,15 +17,30 @@ typedef struct { #define GIT_BUF_INIT {NULL, 0, 0} int git_buf_grow(git_buf *buf, size_t target_size); +void git_buf_free(git_buf *buf); +void git_buf_swap(git_buf *buf_a, git_buf *buf_b); + +/** + * Any function that writes to a git_buf can fail due to memory allocation + * issues. If one fails, the git_buf will be marked with an OOM error and + * further calls to modify the buffer will fail. You just check + * git_buf_oom() at the end of your sequence and it will be true if you ran + * out of memory at any point with that buffer. + */ int git_buf_oom(const git_buf *buf); + +void git_buf_set(git_buf *buf, const char *data, size_t len); +void git_buf_sets(git_buf *buf, const char *string); void git_buf_putc(git_buf *buf, char c); void git_buf_put(git_buf *buf, const char *data, size_t len); void git_buf_puts(git_buf *buf, const char *string); void git_buf_printf(git_buf *buf, const char *format, ...) GIT_FORMAT_PRINTF(2, 3); -const char *git_buf_cstr(git_buf *buf); -void git_buf_free(git_buf *buf); void git_buf_clear(git_buf *buf); void git_buf_consume(git_buf *buf, const char *end); +void git_buf_join(git_buf *buf, char separator, int nbuf, ...); + +const char *git_buf_cstr(git_buf *buf); +char *git_buf_take_cstr(git_buf *buf); #define git_buf_PUTS(buf, str) git_buf_put(buf, str, sizeof(str) - 1) diff --git a/tests-clay/clay_libgit2.h b/tests-clay/clay_libgit2.h index 8784b7e61..2eedc61d2 100644 --- a/tests-clay/clay_libgit2.h +++ b/tests-clay/clay_libgit2.h @@ -25,4 +25,20 @@ */ #define cl_git_fail(expr) cl_must_fail(expr) +/** + * Wrapper for string comparison that knows about nulls. + */ +#define cl_assert_strequal(a,b) \ + cl_assert_strequal_internal(a,b,__FILE__,__LINE__) + +GIT_INLINE(void) cl_assert_strequal_internal(const char *a, const char *b, const char *file, int line) +{ + int match = (a == NULL || b == NULL) ? (a == b) : (strcmp(a, b) == 0); + if (!match) { + char buf[4096]; + snprintf(buf, 4096, "'%s' != '%s'", a, b); + clay__assert(0, file, line, buf, "Strings do not match", 1); + } +} + #endif diff --git a/tests-clay/core/buffer.c b/tests-clay/core/buffer.c new file mode 100644 index 000000000..411c5e84c --- /dev/null +++ b/tests-clay/core/buffer.c @@ -0,0 +1,435 @@ +#include "clay_libgit2.h" +#include "buffer.h" + +#define TESTSTR "Have you seen that? Have you seeeen that??" +const char *test_string = TESTSTR; +const char *test_string_x2 = TESTSTR TESTSTR; + +#define REP4(STR) STR STR STR STR +#define REP16(STR) REP4(REP4(STR)) +#define REP1024(STR) REP16(REP16(REP4(STR))) +#define TESTSTR_4096 REP1024("1234") +#define TESTSTR_8192 REP1024("12341234") +const char *test_4096 = TESTSTR_4096; +const char *test_8192 = TESTSTR_8192; + +/* test basic data concatenation */ +void test_core_buffer__0(void) +{ + git_buf buf = GIT_BUF_INIT; + + cl_assert(buf.size == 0); + + git_buf_puts(&buf, test_string); + cl_assert(git_buf_oom(&buf) == 0); + cl_assert_strequal(test_string, git_buf_cstr(&buf)); + + git_buf_puts(&buf, test_string); + cl_assert(git_buf_oom(&buf) == 0); + cl_assert_strequal(test_string_x2, git_buf_cstr(&buf)); + + git_buf_free(&buf); +} + +/* test git_buf_printf */ +void test_core_buffer__1(void) +{ + git_buf buf = GIT_BUF_INIT; + + git_buf_printf(&buf, "%s %s %d ", "shoop", "da", 23); + cl_assert(git_buf_oom(&buf) == 0); + cl_assert_strequal("shoop da 23 ", git_buf_cstr(&buf)); + + git_buf_printf(&buf, "%s %d", "woop", 42); + cl_assert(git_buf_oom(&buf) == 0); + cl_assert_strequal("shoop da 23 woop 42", git_buf_cstr(&buf)); + + git_buf_free(&buf); +} + +/* more thorough test of concatenation options */ +void test_core_buffer__2(void) +{ + git_buf buf = GIT_BUF_INIT; + int i; + + cl_assert(buf.size == 0); + + /* this must be safe to do */ + git_buf_free(&buf); + + cl_assert(buf.size == 0); + cl_assert(buf.asize == 0); + + /* empty buffer should be empty string */ + cl_assert_strequal("", git_buf_cstr(&buf)); + cl_assert(buf.size == 0); + cl_assert(buf.asize > 0); + + /* free should set us back to the beginning */ + git_buf_free(&buf); + cl_assert(buf.size == 0); + cl_assert(buf.asize == 0); + + /* add letter */ + git_buf_putc(&buf, '+'); + cl_assert(git_buf_oom(&buf) == 0); + cl_assert_strequal("+", git_buf_cstr(&buf)); + + /* add letter again */ + git_buf_putc(&buf, '+'); + cl_assert(git_buf_oom(&buf) == 0); + cl_assert_strequal("++", git_buf_cstr(&buf)); + + /* let's try that a few times */ + for (i = 0; i < 16; ++i) { + git_buf_putc(&buf, '+'); + cl_assert(git_buf_oom(&buf) == 0); + } + cl_assert_strequal("++++++++++++++++++", git_buf_cstr(&buf)); + + git_buf_free(&buf); + + /* add data */ + git_buf_put(&buf, "xo", 2); + cl_assert(git_buf_oom(&buf) == 0); + cl_assert_strequal("xo", git_buf_cstr(&buf)); + + /* add letter again */ + git_buf_put(&buf, "xo", 2); + cl_assert(git_buf_oom(&buf) == 0); + cl_assert_strequal("xoxo", git_buf_cstr(&buf)); + + /* let's try that a few times */ + for (i = 0; i < 16; ++i) { + git_buf_put(&buf, "xo", 2); + cl_assert(git_buf_oom(&buf) == 0); + } + cl_assert_strequal("xoxoxoxoxoxoxoxoxoxoxoxoxoxoxoxoxoxo", + git_buf_cstr(&buf)); + + git_buf_free(&buf); + + /* set to string */ + git_buf_sets(&buf, test_string); + cl_assert(git_buf_oom(&buf) == 0); + cl_assert_strequal(test_string, git_buf_cstr(&buf)); + + /* append string */ + git_buf_puts(&buf, test_string); + cl_assert(git_buf_oom(&buf) == 0); + cl_assert_strequal(test_string_x2, git_buf_cstr(&buf)); + + /* set to string again (should overwrite - not append) */ + git_buf_sets(&buf, test_string); + cl_assert(git_buf_oom(&buf) == 0); + cl_assert_strequal(test_string, git_buf_cstr(&buf)); + + /* test clear */ + git_buf_clear(&buf); + cl_assert_strequal("", git_buf_cstr(&buf)); + + git_buf_free(&buf); +} + +/* let's do some tests with larger buffers to push our limits */ +void test_core_buffer__3(void) +{ + git_buf buf = GIT_BUF_INIT; + + /* set to string */ + git_buf_set(&buf, test_4096, 4096); + cl_assert(git_buf_oom(&buf) == 0); + cl_assert_strequal(test_4096, git_buf_cstr(&buf)); + + /* append string */ + git_buf_puts(&buf, test_4096); + cl_assert(git_buf_oom(&buf) == 0); + cl_assert_strequal(test_8192, git_buf_cstr(&buf)); + + /* set to string again (should overwrite - not append) */ + git_buf_set(&buf, test_4096, 4096); + cl_assert(git_buf_oom(&buf) == 0); + cl_assert_strequal(test_4096, git_buf_cstr(&buf)); + + git_buf_free(&buf); +} + +/* let's try some producer/consumer tests */ +void test_core_buffer__4(void) +{ + git_buf buf = GIT_BUF_INIT; + int i; + + for (i = 0; i < 10; ++i) { + git_buf_puts(&buf, "1234"); /* add 4 */ + cl_assert(git_buf_oom(&buf) == 0); + git_buf_consume(&buf, buf.ptr + 2); /* eat the first two */ + cl_assert(strlen(git_buf_cstr(&buf)) == (size_t)((i + 1) * 2)); + } + /* we have appended 1234 10x and removed the first 20 letters */ + cl_assert_strequal("12341234123412341234", git_buf_cstr(&buf)); + + git_buf_consume(&buf, NULL); + cl_assert_strequal("12341234123412341234", git_buf_cstr(&buf)); + + git_buf_consume(&buf, "invalid pointer"); + cl_assert_strequal("12341234123412341234", git_buf_cstr(&buf)); + + git_buf_consume(&buf, buf.ptr); + cl_assert_strequal("12341234123412341234", git_buf_cstr(&buf)); + + git_buf_consume(&buf, buf.ptr + 1); + cl_assert_strequal("2341234123412341234", git_buf_cstr(&buf)); + + git_buf_consume(&buf, buf.ptr + buf.size); + cl_assert_strequal("", git_buf_cstr(&buf)); + + git_buf_free(&buf); +} + + +static void +check_buf_append( + const char* data_a, + const char* data_b, + const char* expected_data, + ssize_t expected_size, + ssize_t expected_asize) +{ + git_buf tgt = GIT_BUF_INIT; + + git_buf_sets(&tgt, data_a); + cl_assert(git_buf_oom(&tgt) == 0); + git_buf_puts(&tgt, data_b); + cl_assert(git_buf_oom(&tgt) == 0); + if (expected_data == NULL) + cl_assert(tgt.ptr == NULL); + else + cl_assert_strequal(expected_data, git_buf_cstr(&tgt)); + cl_assert(tgt.size == expected_size); + if (expected_asize > 0) + cl_assert(tgt.asize == expected_asize); + + git_buf_free(&tgt); +} + +static void +check_buf_append_abc( + const char* buf_a, + const char* buf_b, + const char* buf_c, + const char* expected_ab, + const char* expected_abc, + const char* expected_abca, + const char* expected_abcab, + const char* expected_abcabc) +{ + git_buf buf = GIT_BUF_INIT; + + git_buf_sets(&buf, buf_a); + cl_assert(git_buf_oom(&buf) == 0); + cl_assert_strequal(buf_a, git_buf_cstr(&buf)); + + git_buf_puts(&buf, buf_b); + cl_assert(git_buf_oom(&buf) == 0); + cl_assert_strequal(expected_ab, git_buf_cstr(&buf)); + + git_buf_puts(&buf, buf_c); + cl_assert(git_buf_oom(&buf) == 0); + cl_assert_strequal(expected_abc, git_buf_cstr(&buf)); + + git_buf_puts(&buf, buf_a); + cl_assert(git_buf_oom(&buf) == 0); + cl_assert_strequal(expected_abca, git_buf_cstr(&buf)); + + git_buf_puts(&buf, buf_b); + cl_assert(git_buf_oom(&buf) == 0); + cl_assert_strequal(expected_abcab, git_buf_cstr(&buf)); + + git_buf_puts(&buf, buf_c); + cl_assert(git_buf_oom(&buf) == 0); + cl_assert_strequal(expected_abcabc, git_buf_cstr(&buf)); + + git_buf_free(&buf); +} + +/* more variations on append tests */ +void test_core_buffer__5(void) +{ + check_buf_append(NULL, NULL, NULL, 0, 0); + check_buf_append(NULL, "", "", 0, 8); + check_buf_append("", NULL, "", 0, 8); + check_buf_append("", "", "", 0, 8); + check_buf_append("a", NULL, "a", 1, 8); + check_buf_append(NULL, "a", "a", 1, 8); + check_buf_append("", "a", "a", 1, 8); + check_buf_append("a", "", "a", 1, 8); + check_buf_append("a", "b", "ab", 2, 8); + check_buf_append("", "abcdefgh", "abcdefgh", 8, 16); + check_buf_append("abcdefgh", "", "abcdefgh", 8, 16); + + /* buffer with starting asize will grow to: + * 1 -> 2, 2 -> 3, 3 -> 5, 4 -> 6, 5 -> 8, 6 -> 9, + * 7 -> 11, 8 -> 12, 9 -> 14, 10 -> 15, 11 -> 17, 12 -> 18, + * 13 -> 20, 14 -> 21, 15 -> 23, 16 -> 24, 17 -> 26, 18 -> 27, + * 19 -> 29, 20 -> 30, 21 -> 32, 22 -> 33, 23 -> 35, 24 -> 36, + * ... + * follow sequence until value > target size, + * then round up to nearest multiple of 8. + */ + + check_buf_append("abcdefgh", "/", "abcdefgh/", 9, 16); + check_buf_append("abcdefgh", "ijklmno", "abcdefghijklmno", 15, 24); + check_buf_append("abcdefgh", "ijklmnop", "abcdefghijklmnop", 16, 24); + check_buf_append("0123456789", "0123456789", + "01234567890123456789", 20, 24); + check_buf_append(REP16("x"), REP16("o"), + REP16("x") REP16("o"), 32, 40); + + check_buf_append(test_4096, NULL, test_4096, 4096, 6144); + check_buf_append(test_4096, "", test_4096, 4096, 6144); + check_buf_append(test_4096, test_4096, test_8192, 8192, 9216); + + /* check sequences of appends */ + check_buf_append_abc("a", "b", "c", + "ab", "abc", "abca", "abcab", "abcabc"); + check_buf_append_abc("a1", "b2", "c3", + "a1b2", "a1b2c3", "a1b2c3a1", + "a1b2c3a1b2", "a1b2c3a1b2c3"); + check_buf_append_abc("a1/", "b2/", "c3/", + "a1/b2/", "a1/b2/c3/", "a1/b2/c3/a1/", + "a1/b2/c3/a1/b2/", "a1/b2/c3/a1/b2/c3/"); +} + +/* test swap */ +void test_core_buffer__6(void) +{ + git_buf a = GIT_BUF_INIT; + git_buf b = GIT_BUF_INIT; + + git_buf_sets(&a, "foo"); + cl_assert(git_buf_oom(&a) == 0); + git_buf_sets(&b, "bar"); + cl_assert(git_buf_oom(&b) == 0); + + cl_assert_strequal("foo", git_buf_cstr(&a)); + cl_assert_strequal("bar", git_buf_cstr(&b)); + + git_buf_swap(&a, &b); + + cl_assert_strequal("bar", git_buf_cstr(&a)); + cl_assert_strequal("foo", git_buf_cstr(&b)); + + git_buf_free(&a); + git_buf_free(&b); +} + + +/* test take cstr data */ +void test_core_buffer__7(void) +{ + git_buf a = GIT_BUF_INIT; + char *b = NULL; + + git_buf_sets(&a, "foo"); + cl_assert(git_buf_oom(&a) == 0); + cl_assert_strequal("foo", git_buf_cstr(&a)); + + b = git_buf_take_cstr(&a); + + cl_assert_strequal("foo", b); + cl_assert_strequal(NULL, a.ptr); + git__free(b); + + b = git_buf_take_cstr(&a); + + cl_assert_strequal(NULL, b); + cl_assert_strequal(NULL, a.ptr); + + git_buf_free(&a); +} + + +static void +check_joinbuf( + const char *a, + const char *b, + const char *expected) +{ + char sep = '/'; + git_buf buf = GIT_BUF_INIT; + + git_buf_sets(&buf, a); + cl_assert(git_buf_oom(&buf) == 0); + + git_buf_join(&buf, sep, 1, b); + cl_assert(git_buf_oom(&buf) == 0); + cl_assert_strequal(expected, git_buf_cstr(&buf)); + + git_buf_free(&buf); +} + +static void +check_joinbuf_n( + const char *a, + const char *b, + const char *c, + const char *d, + const char *expected) +{ + char sep = ';'; + git_buf buf = GIT_BUF_INIT; + git_buf_join(&buf, sep, 4, a, b, c, d); + cl_assert(git_buf_oom(&buf) == 0); + cl_assert_strequal(expected, git_buf_cstr(&buf)); + git_buf_free(&buf); +} + +/* test join */ +void test_core_buffer__8(void) +{ + git_buf a = GIT_BUF_INIT; + + git_buf_join(&a, '/', 1, "foo"); + cl_assert(git_buf_oom(&a) == 0); + cl_assert_strequal("foo", git_buf_cstr(&a)); + + git_buf_join(&a, '/', 1, "bar"); + cl_assert(git_buf_oom(&a) == 0); + cl_assert_strequal("foo/bar", git_buf_cstr(&a)); + + git_buf_join(&a, '/', 1, "baz"); + cl_assert(git_buf_oom(&a) == 0); + cl_assert_strequal("foo/bar/baz", git_buf_cstr(&a)); + + git_buf_free(&a); + + check_joinbuf("", "", ""); + check_joinbuf("", "a", "a"); + check_joinbuf("", "/a", "/a"); + check_joinbuf("a", "", "a/"); + check_joinbuf("a", "/", "a/"); + check_joinbuf("a", "b", "a/b"); + check_joinbuf("/", "a", "/a"); + check_joinbuf("/", "", "/"); + check_joinbuf("/a", "/b", "/a/b"); + check_joinbuf("/a", "/b/", "/a/b/"); + check_joinbuf("/a/", "b/", "/a/b/"); + check_joinbuf("/a/", "/b/", "/a/b/"); + check_joinbuf("/abcd", "/defg", "/abcd/defg"); + check_joinbuf("/abcd", "/defg/", "/abcd/defg/"); + check_joinbuf("/abcd/", "defg/", "/abcd/defg/"); + check_joinbuf("/abcd/", "/defg/", "/abcd/defg/"); + + check_joinbuf_n("", "", "", "", ""); + check_joinbuf_n("", "a", "", "", "a;"); + check_joinbuf_n("a", "", "", "", "a;"); + check_joinbuf_n("", "", "", "a", "a"); + check_joinbuf_n("a", "b", "", ";c;d;", "a;b;c;d;"); + check_joinbuf_n("a", "b", "", ";c;d", "a;b;c;d"); + check_joinbuf_n("abcd", "efgh", "ijkl", "mnop", "abcd;efgh;ijkl;mnop"); + check_joinbuf_n("abcd;", "efgh;", "ijkl;", "mnop;", "abcd;efgh;ijkl;mnop;"); + check_joinbuf_n(";abcd;", ";efgh;", ";ijkl;", ";mnop;", ";abcd;efgh;ijkl;mnop;"); +} + From 3aa294fd450873eaef85ecadaab086b414c4e07e Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Mon, 28 Nov 2011 10:42:57 -0800 Subject: [PATCH 2/3] Add two string git_buf_join and tweak input error checking. This commit addresses two of the comments: * renamed existing n-input git_buf_join to git_buf_join_n * added new git_buf_join that always takes two inputs * moved some parameter error checking to asserts * extended unit tests to cover new version of git_buf_join --- src/buffer.c | 76 ++++++++++++++++++++++++--- src/buffer.h | 3 +- tests-clay/core/buffer.c | 107 +++++++++++++++++++++++++++------------ 3 files changed, 145 insertions(+), 41 deletions(-) diff --git a/src/buffer.c b/src/buffer.c index 3fd04211c..50014fc11 100644 --- a/src/buffer.c +++ b/src/buffer.c @@ -121,14 +121,14 @@ const char *git_buf_cstr(git_buf *buf) void git_buf_free(git_buf *buf) { - if (buf) { - if (buf->ptr) { - git__free(buf->ptr); - buf->ptr = NULL; - } - buf->asize = 0; - buf->size = 0; + assert(buf); + + if (buf->ptr) { + git__free(buf->ptr); + buf->ptr = NULL; } + buf->asize = 0; + buf->size = 0; } void git_buf_clear(git_buf *buf) @@ -173,7 +173,7 @@ char *git_buf_take_cstr(git_buf *buf) return data; } -void git_buf_join(git_buf *buf, char separator, int nbuf, ...) +void git_buf_join_n(git_buf *buf, char separator, int nbuf, ...) { /* Make two passes to avoid multiple reallocation */ @@ -239,3 +239,63 @@ void git_buf_join(git_buf *buf, char separator, int nbuf, ...) buf->size = out - buf->ptr; } +void git_buf_join( + git_buf *buf, + char separator, + const char *str_a, + const char *str_b) +{ + int add_size = 0; + int sep_a = 0; + int strlen_a = 0; + int sep_b = 0; + int strlen_b = 0; + char *ptr; + + /* calculate string lengths and need for added separators */ + if (str_a) { + while (*str_a == separator) { sep_a = 1; str_a++; } + strlen_a = strlen(str_a); + } + if (str_b) { + while (*str_b == separator) { sep_b = 1; str_b++; } + strlen_b = strlen(str_b); + } + if (buf->size > 0) { + if (buf->ptr[buf->size - 1] == separator) { + sep_a = 0; + if (!strlen_a) sep_b = 0; + } else if (!strlen_a) + sep_b = (str_b != NULL); + } + if (strlen_a > 0) { + if (str_a[strlen_a - 1] == separator) + sep_b = 0; + else if (str_b) + sep_b = 1; + } + + add_size = sep_a + strlen_a + sep_b + strlen_b; + + if (!add_size) return; + + ENSURE_SIZE(buf, buf->size + add_size); + + /* concatenate strings */ + ptr = buf->ptr + buf->size; + if (sep_a) + *ptr++ = separator; + if (strlen_a) { + memmove(ptr, str_a, strlen_a); + ptr += strlen_a; + } + if (sep_b) + *ptr++ = separator; + if (strlen_b) { + memmove(ptr, str_b, strlen_b); + ptr += strlen_b; + } + + /* set size based on num characters actually written */ + buf->size = ptr - buf->ptr; +} diff --git a/src/buffer.h b/src/buffer.h index baa8f4f4d..2ed9047ca 100644 --- a/src/buffer.h +++ b/src/buffer.h @@ -37,7 +37,8 @@ void git_buf_puts(git_buf *buf, const char *string); void git_buf_printf(git_buf *buf, const char *format, ...) GIT_FORMAT_PRINTF(2, 3); void git_buf_clear(git_buf *buf); void git_buf_consume(git_buf *buf, const char *end); -void git_buf_join(git_buf *buf, char separator, int nbuf, ...); +void git_buf_join_n(git_buf *buf, char separator, int nbuf, ...); +void git_buf_join(git_buf *buf, char separator, const char *str_a, const char *str_b); const char *git_buf_cstr(git_buf *buf); char *git_buf_take_cstr(git_buf *buf); diff --git a/tests-clay/core/buffer.c b/tests-clay/core/buffer.c index 411c5e84c..5adc106dd 100644 --- a/tests-clay/core/buffer.c +++ b/tests-clay/core/buffer.c @@ -352,7 +352,32 @@ void test_core_buffer__7(void) static void -check_joinbuf( +check_joinbuf_2( + const char *a, + const char *b, + const char *expected) +{ + char sep = '/'; + git_buf buf = GIT_BUF_INIT; + + /* first validate join from nothing */ + git_buf_join(&buf, sep, a, b); + cl_assert(git_buf_oom(&buf) == 0); + cl_assert_strequal(expected, git_buf_cstr(&buf)); + git_buf_free(&buf); + + /* next validate join-append */ + git_buf_sets(&buf, a); + cl_assert(git_buf_oom(&buf) == 0); + git_buf_join(&buf, sep, NULL, b); + cl_assert(git_buf_oom(&buf) == 0); + cl_assert_strequal(expected, git_buf_cstr(&buf)); + + git_buf_free(&buf); +} + +static void +check_joinbuf_n_2( const char *a, const char *b, const char *expected) @@ -363,7 +388,7 @@ check_joinbuf( git_buf_sets(&buf, a); cl_assert(git_buf_oom(&buf) == 0); - git_buf_join(&buf, sep, 1, b); + git_buf_join_n(&buf, sep, 1, b); cl_assert(git_buf_oom(&buf) == 0); cl_assert_strequal(expected, git_buf_cstr(&buf)); @@ -371,7 +396,7 @@ check_joinbuf( } static void -check_joinbuf_n( +check_joinbuf_n_4( const char *a, const char *b, const char *c, @@ -380,7 +405,7 @@ check_joinbuf_n( { char sep = ';'; git_buf buf = GIT_BUF_INIT; - git_buf_join(&buf, sep, 4, a, b, c, d); + git_buf_join_n(&buf, sep, 4, a, b, c, d); cl_assert(git_buf_oom(&buf) == 0); cl_assert_strequal(expected, git_buf_cstr(&buf)); git_buf_free(&buf); @@ -391,45 +416,63 @@ void test_core_buffer__8(void) { git_buf a = GIT_BUF_INIT; - git_buf_join(&a, '/', 1, "foo"); + git_buf_join_n(&a, '/', 1, "foo"); cl_assert(git_buf_oom(&a) == 0); cl_assert_strequal("foo", git_buf_cstr(&a)); - git_buf_join(&a, '/', 1, "bar"); + git_buf_join_n(&a, '/', 1, "bar"); cl_assert(git_buf_oom(&a) == 0); cl_assert_strequal("foo/bar", git_buf_cstr(&a)); - git_buf_join(&a, '/', 1, "baz"); + git_buf_join_n(&a, '/', 1, "baz"); cl_assert(git_buf_oom(&a) == 0); cl_assert_strequal("foo/bar/baz", git_buf_cstr(&a)); git_buf_free(&a); - check_joinbuf("", "", ""); - check_joinbuf("", "a", "a"); - check_joinbuf("", "/a", "/a"); - check_joinbuf("a", "", "a/"); - check_joinbuf("a", "/", "a/"); - check_joinbuf("a", "b", "a/b"); - check_joinbuf("/", "a", "/a"); - check_joinbuf("/", "", "/"); - check_joinbuf("/a", "/b", "/a/b"); - check_joinbuf("/a", "/b/", "/a/b/"); - check_joinbuf("/a/", "b/", "/a/b/"); - check_joinbuf("/a/", "/b/", "/a/b/"); - check_joinbuf("/abcd", "/defg", "/abcd/defg"); - check_joinbuf("/abcd", "/defg/", "/abcd/defg/"); - check_joinbuf("/abcd/", "defg/", "/abcd/defg/"); - check_joinbuf("/abcd/", "/defg/", "/abcd/defg/"); + check_joinbuf_2("", "", ""); + check_joinbuf_2("", "a", "a"); + check_joinbuf_2("", "/a", "/a"); + check_joinbuf_2("a", "", "a/"); + check_joinbuf_2("a", "/", "a/"); + check_joinbuf_2("a", "b", "a/b"); + check_joinbuf_2("/", "a", "/a"); + check_joinbuf_2("/", "", "/"); + check_joinbuf_2("/a", "/b", "/a/b"); + check_joinbuf_2("/a", "/b/", "/a/b/"); + check_joinbuf_2("/a/", "b/", "/a/b/"); + check_joinbuf_2("/a/", "/b/", "/a/b/"); + check_joinbuf_2("/a/", "//b/", "/a/b/"); + check_joinbuf_2("/abcd", "/defg", "/abcd/defg"); + check_joinbuf_2("/abcd", "/defg/", "/abcd/defg/"); + check_joinbuf_2("/abcd/", "defg/", "/abcd/defg/"); + check_joinbuf_2("/abcd/", "/defg/", "/abcd/defg/"); - check_joinbuf_n("", "", "", "", ""); - check_joinbuf_n("", "a", "", "", "a;"); - check_joinbuf_n("a", "", "", "", "a;"); - check_joinbuf_n("", "", "", "a", "a"); - check_joinbuf_n("a", "b", "", ";c;d;", "a;b;c;d;"); - check_joinbuf_n("a", "b", "", ";c;d", "a;b;c;d"); - check_joinbuf_n("abcd", "efgh", "ijkl", "mnop", "abcd;efgh;ijkl;mnop"); - check_joinbuf_n("abcd;", "efgh;", "ijkl;", "mnop;", "abcd;efgh;ijkl;mnop;"); - check_joinbuf_n(";abcd;", ";efgh;", ";ijkl;", ";mnop;", ";abcd;efgh;ijkl;mnop;"); + check_joinbuf_n_2("", "", ""); + check_joinbuf_n_2("", "a", "a"); + check_joinbuf_n_2("", "/a", "/a"); + check_joinbuf_n_2("a", "", "a/"); + check_joinbuf_n_2("a", "/", "a/"); + check_joinbuf_n_2("a", "b", "a/b"); + check_joinbuf_n_2("/", "a", "/a"); + check_joinbuf_n_2("/", "", "/"); + check_joinbuf_n_2("/a", "/b", "/a/b"); + check_joinbuf_n_2("/a", "/b/", "/a/b/"); + check_joinbuf_n_2("/a/", "b/", "/a/b/"); + check_joinbuf_n_2("/a/", "/b/", "/a/b/"); + check_joinbuf_n_2("/abcd", "/defg", "/abcd/defg"); + check_joinbuf_n_2("/abcd", "/defg/", "/abcd/defg/"); + check_joinbuf_n_2("/abcd/", "defg/", "/abcd/defg/"); + check_joinbuf_n_2("/abcd/", "/defg/", "/abcd/defg/"); + + check_joinbuf_n_4("", "", "", "", ""); + check_joinbuf_n_4("", "a", "", "", "a;"); + check_joinbuf_n_4("a", "", "", "", "a;"); + check_joinbuf_n_4("", "", "", "a", "a"); + check_joinbuf_n_4("a", "b", "", ";c;d;", "a;b;c;d;"); + check_joinbuf_n_4("a", "b", "", ";c;d", "a;b;c;d"); + check_joinbuf_n_4("abcd", "efgh", "ijkl", "mnop", "abcd;efgh;ijkl;mnop"); + check_joinbuf_n_4("abcd;", "efgh;", "ijkl;", "mnop;", "abcd;efgh;ijkl;mnop;"); + check_joinbuf_n_4(";abcd;", ";efgh;", ";ijkl;", ";mnop;", ";abcd;efgh;ijkl;mnop;"); } From 679b69c49df2d8c1b9d0d99f4f3a82a0c3975e4c Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Mon, 28 Nov 2011 13:05:25 -0800 Subject: [PATCH 3/3] Resolve remaining feedback * replace some ints with size_ts * update NULL checks in various places --- src/buffer.c | 28 +++++++++++++--------------- tests-clay/core/buffer.c | 8 ++------ 2 files changed, 15 insertions(+), 21 deletions(-) diff --git a/src/buffer.c b/src/buffer.c index 50014fc11..c56a75598 100644 --- a/src/buffer.c +++ b/src/buffer.c @@ -79,8 +79,8 @@ void git_buf_put(git_buf *buf, const char *data, size_t len) void git_buf_puts(git_buf *buf, const char *string) { - if (string != NULL) - git_buf_put(buf, string, strlen(string)); + assert(string); + git_buf_put(buf, string, strlen(string)); } void git_buf_printf(git_buf *buf, const char *format, ...) @@ -121,12 +121,10 @@ const char *git_buf_cstr(git_buf *buf) void git_buf_free(git_buf *buf) { - assert(buf); + if (!buf) return; - if (buf->ptr) { - git__free(buf->ptr); - buf->ptr = NULL; - } + git__free(buf->ptr); + buf->ptr = NULL; buf->asize = 0; buf->size = 0; } @@ -179,7 +177,7 @@ void git_buf_join_n(git_buf *buf, char separator, int nbuf, ...) va_list ap; int i; - int total_size = 0; + size_t total_size = 0; char *out; if (buf->size > 0 && buf->ptr[buf->size - 1] != separator) @@ -188,7 +186,7 @@ void git_buf_join_n(git_buf *buf, char separator, int nbuf, ...) va_start(ap, nbuf); for (i = 0; i < nbuf; ++i) { const char* segment; - int segment_len; + size_t segment_len; segment = va_arg(ap, const char *); if (!segment) @@ -212,7 +210,7 @@ void git_buf_join_n(git_buf *buf, char separator, int nbuf, ...) va_start(ap, nbuf); for (i = 0; i < nbuf; ++i) { const char* segment; - int segment_len; + size_t segment_len; segment = va_arg(ap, const char *); if (!segment) @@ -245,11 +243,11 @@ void git_buf_join( const char *str_a, const char *str_b) { - int add_size = 0; - int sep_a = 0; - int strlen_a = 0; - int sep_b = 0; - int strlen_b = 0; + size_t add_size = 0; + size_t sep_a = 0; + size_t strlen_a = 0; + size_t sep_b = 0; + size_t strlen_b = 0; char *ptr; /* calculate string lengths and need for added separators */ diff --git a/tests-clay/core/buffer.c b/tests-clay/core/buffer.c index 5adc106dd..bf88a3073 100644 --- a/tests-clay/core/buffer.c +++ b/tests-clay/core/buffer.c @@ -257,12 +257,9 @@ check_buf_append_abc( /* more variations on append tests */ void test_core_buffer__5(void) { - check_buf_append(NULL, NULL, NULL, 0, 0); - check_buf_append(NULL, "", "", 0, 8); - check_buf_append("", NULL, "", 0, 8); check_buf_append("", "", "", 0, 8); - check_buf_append("a", NULL, "a", 1, 8); - check_buf_append(NULL, "a", "a", 1, 8); + check_buf_append("a", "", "a", 1, 8); + check_buf_append("", "a", "a", 1, 8); check_buf_append("", "a", "a", 1, 8); check_buf_append("a", "", "a", 1, 8); check_buf_append("a", "b", "ab", 2, 8); @@ -287,7 +284,6 @@ void test_core_buffer__5(void) check_buf_append(REP16("x"), REP16("o"), REP16("x") REP16("o"), 32, 40); - check_buf_append(test_4096, NULL, test_4096, 4096, 6144); check_buf_append(test_4096, "", test_4096, 4096, 6144); check_buf_append(test_4096, test_4096, test_8192, 8192, 9216);