diff --git a/include/git2/errors.h b/include/git2/errors.h index e189e55f1..4698366d8 100644 --- a/include/git2/errors.h +++ b/include/git2/errors.h @@ -113,18 +113,6 @@ GIT_EXTERN(const git_error *) giterr_last(void); */ GIT_EXTERN(void) giterr_clear(void); -/** - * Get the last error data and clear it. - * - * This copies the last error into the given `git_error` struct - * and returns 0 if the copy was successful, leaving the error - * cleared as if `giterr_clear` had been called. - * - * If there was no existing error in the library, -1 will be returned - * and the contents of `cpy` will be left unmodified. - */ -GIT_EXTERN(int) giterr_detach(git_error *cpy); - /** * Set the error message string for this thread. * diff --git a/src/clone.c b/src/clone.c index 070daf94d..6b4b7ae53 100644 --- a/src/clone.c +++ b/src/clone.c @@ -440,14 +440,14 @@ int git_clone( if (error != 0) { git_error_state last_error = {0}; - giterr_capture(&last_error, error); + giterr_state_capture(&last_error, error); git_repository_free(repo); repo = NULL; (void)git_futils_rmdir_r(local_path, NULL, rmdir_flags); - giterr_restore(&last_error); + giterr_state_restore(&last_error); } *out = repo; diff --git a/src/common.h b/src/common.h index 2b1dedc01..6dca36fbd 100644 --- a/src/common.h +++ b/src/common.h @@ -141,20 +141,25 @@ void giterr_system_set(int code); * Structure to preserve libgit2 error state */ typedef struct { - int error_code; + int error_code; + unsigned int oom : 1; git_error error_msg; } git_error_state; /** * Capture current error state to restore later, returning error code. - * If `error_code` is zero, this does nothing and returns zero. + * If `error_code` is zero, this does not clear the current error state. + * You must either restore this error state, or free it. */ -int giterr_capture(git_error_state *state, int error_code); +extern int giterr_state_capture(git_error_state *state, int error_code); /** * Restore error state to a previous value, returning saved error code. */ -int giterr_restore(git_error_state *state); +extern int giterr_state_restore(git_error_state *state); + +/** Free an error state. */ +extern void giterr_state_free(git_error_state *state); /** * Check a versioned structure for validity diff --git a/src/errors.c b/src/errors.c index 7a2600586..91acc3541 100644 --- a/src/errors.c +++ b/src/errors.c @@ -18,19 +18,30 @@ static git_error g_git_oom_error = { GITERR_NOMEMORY }; -static void set_error(int error_class, char *string) +static void set_error_from_buffer(int error_class) { git_error *error = &GIT_GLOBAL->error_t; + git_buf *buf = &GIT_GLOBAL->error_buf; - if (error->message != string) - git__free(error->message); - - error->message = string; + error->message = buf->ptr; error->klass = error_class; GIT_GLOBAL->last_error = error; } +static void set_error(int error_class, char *string) +{ + git_buf *buf = &GIT_GLOBAL->error_buf; + + git_buf_clear(buf); + if (string) { + git_buf_puts(buf, string); + git__free(string); + } + + set_error_from_buffer(error_class); +} + void giterr_set_oom(void) { GIT_GLOBAL->last_error = &g_git_oom_error; @@ -38,27 +49,28 @@ void giterr_set_oom(void) void giterr_set(int error_class, const char *string, ...) { - git_buf buf = GIT_BUF_INIT; va_list arglist; #ifdef GIT_WIN32 DWORD win32_error_code = (error_class == GITERR_OS) ? GetLastError() : 0; #endif int error_code = (error_class == GITERR_OS) ? errno : 0; + git_buf *buf = &GIT_GLOBAL->error_buf; + git_buf_clear(buf); if (string) { va_start(arglist, string); - git_buf_vprintf(&buf, string, arglist); + git_buf_vprintf(buf, string, arglist); va_end(arglist); if (error_class == GITERR_OS) - git_buf_PUTS(&buf, ": "); + git_buf_PUTS(buf, ": "); } if (error_class == GITERR_OS) { #ifdef GIT_WIN32 char * win32_error = git_win32_get_error_message(win32_error_code); if (win32_error) { - git_buf_puts(&buf, win32_error); + git_buf_puts(buf, win32_error); git__free(win32_error); SetLastError(0); @@ -66,26 +78,29 @@ void giterr_set(int error_class, const char *string, ...) else #endif if (error_code) - git_buf_puts(&buf, strerror(error_code)); + git_buf_puts(buf, strerror(error_code)); if (error_code) errno = 0; } - if (!git_buf_oom(&buf)) - set_error(error_class, git_buf_detach(&buf)); + if (!git_buf_oom(buf)) + set_error_from_buffer(error_class); } void giterr_set_str(int error_class, const char *string) { - char *message; + git_buf *buf = &GIT_GLOBAL->error_buf; assert(string); - message = git__strdup(string); + if (!string) + return; - if (message) - set_error(error_class, message); + git_buf_clear(buf); + git_buf_puts(buf, string); + if (!git_buf_oom(buf)) + set_error_from_buffer(error_class); } int giterr_set_regex(const regex_t *regex, int error_code) @@ -116,45 +131,65 @@ void giterr_clear(void) #endif } -int giterr_detach(git_error *cpy) -{ - git_error *error = GIT_GLOBAL->last_error; - - assert(cpy); - - if (!error) - return -1; - - cpy->message = error->message; - cpy->klass = error->klass; - - error->message = NULL; - giterr_clear(); - - return 0; -} - const git_error *giterr_last(void) { return GIT_GLOBAL->last_error; } -int giterr_capture(git_error_state *state, int error_code) +int giterr_state_capture(git_error_state *state, int error_code) { + git_error *error = GIT_GLOBAL->last_error; + git_buf *error_buf = &GIT_GLOBAL->error_buf; + + memset(state, 0, sizeof(git_error_state)); + + if (!error_code) + return 0; + state->error_code = error_code; - if (error_code) - giterr_detach(&state->error_msg); + state->oom = (error == &g_git_oom_error); + + if (error) { + state->error_msg.klass = error->klass; + + if (state->oom) + state->error_msg.message = g_git_oom_error.message; + else + state->error_msg.message = git_buf_detach(error_buf); + } + + giterr_clear(); return error_code; } -int giterr_restore(git_error_state *state) +int giterr_state_restore(git_error_state *state) { - if (state && state->error_code && state->error_msg.message) - set_error(state->error_msg.klass, state->error_msg.message); - else - giterr_clear(); + int ret = 0; - return state ? state->error_code : 0; + giterr_clear(); + + if (state && state->error_msg.message) { + if (state->oom) + giterr_set_oom(); + else + set_error(state->error_msg.klass, state->error_msg.message); + + ret = state->error_code; + memset(state, 0, sizeof(git_error_state)); + } + + return ret; +} + +void giterr_state_free(git_error_state *state) +{ + if (!state) + return; + + if (!state->oom) + git__free(state->error_msg.message); + + memset(state, 0, sizeof(git_error_state)); } int giterr_system_last(void) diff --git a/src/global.c b/src/global.c index 37a47bd27..3d37ee4de 100644 --- a/src/global.c +++ b/src/global.c @@ -279,18 +279,19 @@ int git_libgit2_shutdown(void) git_global_st *git__global_state(void) { - void *ptr; + git_global_st *ptr; assert(git_atomic_get(&git__n_inits) > 0); if ((ptr = TlsGetValue(_tls_index)) != NULL) return ptr; - ptr = git__malloc(sizeof(git_global_st)); + ptr = git__calloc(1, sizeof(git_global_st)); if (!ptr) return NULL; - memset(ptr, 0x0, sizeof(git_global_st)); + git_buf_init(&ptr->error_buf, 0); + TlsSetValue(_tls_index, ptr); return ptr; } @@ -378,18 +379,18 @@ int git_libgit2_shutdown(void) git_global_st *git__global_state(void) { - void *ptr; + git_global_st *ptr; assert(git_atomic_get(&git__n_inits) > 0); if ((ptr = pthread_getspecific(_tls_key)) != NULL) return ptr; - ptr = git__malloc(sizeof(git_global_st)); + ptr = git__calloc(1, sizeof(git_global_st)); if (!ptr) return NULL; - memset(ptr, 0x0, sizeof(git_global_st)); + git_buf_init(&ptr->error_buf, 0); pthread_setspecific(_tls_key, ptr); return ptr; } @@ -407,6 +408,7 @@ int git_libgit2_init(void) ssl_inited = 1; } + git_buf_init(&__state.error_buf, 0); return git_atomic_inc(&git__n_inits); } diff --git a/src/global.h b/src/global.h index fdad6ba89..37e909ac6 100644 --- a/src/global.h +++ b/src/global.h @@ -14,6 +14,7 @@ typedef struct { git_error *last_error; git_error error_t; + git_buf error_buf; char oid_fmt[GIT_OID_HEXSZ+1]; } git_global_st; diff --git a/src/index.c b/src/index.c index 73f0b3d26..e424698bb 100644 --- a/src/index.c +++ b/src/index.c @@ -1286,13 +1286,13 @@ int git_index_add_bypath(git_index *index, const char *path) git_submodule *sm; git_error_state err; - giterr_capture(&err, ret); + giterr_state_capture(&err, ret); ret = git_submodule_lookup(&sm, INDEX_OWNER(index), path); if (ret == GIT_ENOTFOUND) - return giterr_restore(&err); + return giterr_state_restore(&err); - git__free(err.error_msg.message); + giterr_state_free(&err); /* * EEXISTS means that there is a repository at that path, but it's not known diff --git a/src/iterator.c b/src/iterator.c index cf51a340d..900ffdcaa 100644 --- a/src/iterator.c +++ b/src/iterator.c @@ -1120,7 +1120,7 @@ static int fs_iterator__expand_dir(fs_iterator *fi) if (error < 0) { git_error_state last_error = { 0 }; - giterr_capture(&last_error, error); + giterr_state_capture(&last_error, error); /* these callbacks may clear the error message */ fs_iterator__free_frame(ff); @@ -1128,7 +1128,7 @@ static int fs_iterator__expand_dir(fs_iterator *fi) /* next time return value we skipped to */ fi->base.flags &= ~GIT_ITERATOR_FIRST_ACCESS; - return giterr_restore(&last_error); + return giterr_state_restore(&last_error); } if (ff->entries.length == 0) { diff --git a/tests/core/errors.c b/tests/core/errors.c index a06ec4abc..ab18951a6 100644 --- a/tests/core/errors.c +++ b/tests/core/errors.c @@ -93,23 +93,69 @@ void test_core_errors__restore(void) giterr_clear(); cl_assert(giterr_last() == NULL); - cl_assert_equal_i(0, giterr_capture(&err_state, 0)); + cl_assert_equal_i(0, giterr_state_capture(&err_state, 0)); memset(&err_state, 0x0, sizeof(git_error_state)); giterr_set(42, "Foo: %s", "bar"); - cl_assert_equal_i(-1, giterr_capture(&err_state, -1)); + cl_assert_equal_i(-1, giterr_state_capture(&err_state, -1)); cl_assert(giterr_last() == NULL); giterr_set(99, "Bar: %s", "foo"); - giterr_restore(&err_state); + giterr_state_restore(&err_state); cl_assert_equal_i(42, giterr_last()->klass); cl_assert_equal_s("Foo: bar", giterr_last()->message); } +void test_core_errors__free_state(void) +{ + git_error_state err_state = {0}; + + giterr_clear(); + + giterr_set(42, "Foo: %s", "bar"); + cl_assert_equal_i(-1, giterr_state_capture(&err_state, -1)); + + giterr_set(99, "Bar: %s", "foo"); + + giterr_state_free(&err_state); + + cl_assert_equal_i(99, giterr_last()->klass); + cl_assert_equal_s("Bar: foo", giterr_last()->message); + + giterr_state_restore(&err_state); + + cl_assert(giterr_last() == NULL); +} + +void test_core_errors__restore_oom(void) +{ + git_error_state err_state = {0}; + const git_error *oom_error = NULL; + + giterr_clear(); + + giterr_set_oom(); /* internal fn */ + oom_error = giterr_last(); + cl_assert(oom_error); + + cl_assert_equal_i(-1, giterr_state_capture(&err_state, -1)); + + cl_assert(giterr_last() == NULL); + cl_assert_equal_i(GITERR_NOMEMORY, err_state.error_msg.klass); + cl_assert_equal_s("Out of memory", err_state.error_msg.message); + + giterr_state_restore(&err_state); + + cl_assert(giterr_last()->klass == GITERR_NOMEMORY); + cl_assert_(giterr_last() == oom_error, "static oom error not restored"); + + giterr_clear(); +} + static int test_arraysize_multiply(size_t nelem, size_t size) { size_t out;