From d6c6016966cff46d874a8d85b38704a6ef2150e5 Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Mon, 4 Nov 2013 15:45:31 -0800 Subject: [PATCH 1/3] Add giterr_detach API to get and clear error There are a number of cases where it is convenient to be able to fetch and "claim" the current error string, clearing the error. This is helpful when you need to call some code that may alter the error and you want to restore it later on and/or report it via some other mechanism. --- include/git2/errors.h | 17 +++++++++++++++++ src/errors.c | 23 +++++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/include/git2/errors.h b/include/git2/errors.h index a454ac956..5f5d0ab1f 100644 --- a/include/git2/errors.h +++ b/include/git2/errors.h @@ -8,6 +8,7 @@ #define INCLUDE_git_errors_h__ #include "common.h" +#include "buffer.h" /** * @file git2/errors.h @@ -45,6 +46,7 @@ typedef struct { /** Error classes */ typedef enum { + GITERR_NONE = 0, GITERR_NOMEMORY, GITERR_OS, GITERR_INVALID, @@ -84,6 +86,21 @@ 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 message into the given `git_buf` and returns + * the associated `git_error_t`, leaving the error cleared as if + * `giterr_clear` had been called. You must call `git_buf_free` on the + * message to release the memory. + * + * Note: it is possible that this will return `GITERR_NONE` and set the + * buffer to NULL, so be prepared for that condition. Also, if the last + * error was an out-of-memory error, this will return `GITERR_NOMEMORY` + * but also leave the buffer set to NULL (to avoid allocation). + */ +GIT_EXTERN(git_error_t) giterr_detach(git_buf *message); + /** * Set the error message string for this thread. * diff --git a/src/errors.c b/src/errors.c index c9d9e4e37..70b5f2668 100644 --- a/src/errors.c +++ b/src/errors.c @@ -112,6 +112,29 @@ void giterr_clear(void) #endif } +git_error_t giterr_detach(git_buf *message) +{ + git_error_t rval; + git_error *error = GIT_GLOBAL->last_error; + + assert(message); + + git_buf_free(message); + + if (!error) + return GITERR_NONE; + + rval = error->klass; + + if (error != &g_git_oom_error) + git_buf_attach(message, error->message, 0); + + error->message = NULL; + giterr_clear(); + + return rval; +} + const git_error *giterr_last(void) { return GIT_GLOBAL->last_error; From 3b259cbd1afdc96a3c3eb7af5895b310c1ac2a7d Mon Sep 17 00:00:00 2001 From: Russell Belfer Date: Mon, 4 Nov 2013 15:47:35 -0800 Subject: [PATCH 2/3] Preserve file error in iterator When the filesystem iterator encounters an error with a file, it returns the error but because of the cleanup code, it was in some cases erasing the error message. This uses the giterr_detach API to make sure that the actual error message is restored after the cleanup code has been run. --- src/iterator.c | 12 ++++++++++++ tests-clar/repo/iterator.c | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/src/iterator.c b/src/iterator.c index c0d7862ff..369a079bc 100644 --- a/src/iterator.c +++ b/src/iterator.c @@ -991,8 +991,20 @@ static int fs_iterator__expand_dir(fs_iterator *fi) fi->base.start, fi->base.end, &ff->entries); if (error < 0) { + git_buf msg = GIT_BUF_INIT; + git_error_t errt = giterr_detach(&msg); + + /* these callbacks may clear the error message */ fs_iterator__free_frame(ff); fs_iterator__advance_over(NULL, (git_iterator *)fi); + /* next time return value we skipped to */ + fi->base.flags &= ~GIT_ITERATOR_FIRST_ACCESS; + + if (msg.ptr) { + giterr_set_str(errt, msg.ptr); + git_buf_free(&msg); + } + return error; } diff --git a/tests-clar/repo/iterator.c b/tests-clar/repo/iterator.c index 1c513e9e7..56b51852c 100644 --- a/tests-clar/repo/iterator.c +++ b/tests-clar/repo/iterator.c @@ -926,3 +926,37 @@ void test_repo_iterator__fs2(void) expect_iterator_items(i, 12, expect_base, 12, expect_base); git_iterator_free(i); } + +void test_repo_iterator__fs_preserves_error(void) +{ + git_iterator *i; + const git_index_entry *e; + + if (!cl_is_chmod_supported()) + return; + + g_repo = cl_git_sandbox_init("empty_standard_repo"); + + cl_must_pass(p_mkdir("empty_standard_repo/r", 0777)); + cl_git_mkfile("empty_standard_repo/r/a", "hello"); + cl_must_pass(p_mkdir("empty_standard_repo/r/b", 0777)); + cl_git_mkfile("empty_standard_repo/r/b/problem", "not me"); + cl_must_pass(p_chmod("empty_standard_repo/r/b", 0000)); + cl_must_pass(p_mkdir("empty_standard_repo/r/c", 0777)); + cl_git_mkfile("empty_standard_repo/r/d", "final"); + + cl_git_pass(git_iterator_for_filesystem( + &i, "empty_standard_repo/r", 0, NULL, NULL)); + + cl_git_pass(git_iterator_advance(&e, i)); /* a */ + cl_git_fail(git_iterator_advance(&e, i)); /* b */ + cl_assert(giterr_last()); + cl_assert(giterr_last()->message != NULL); + /* skip 'c/' empty directory */ + cl_git_pass(git_iterator_advance(&e, i)); /* d */ + cl_assert_equal_i(GIT_ITEROVER, git_iterator_advance(&e, i)); + + cl_must_pass(p_chmod("empty_standard_repo/r/b", 0777)); + + git_iterator_free(i); +} From 1eab9f0e32178a9aac941583c69e1b9cf9849f77 Mon Sep 17 00:00:00 2001 From: Vicent Marti Date: Tue, 5 Nov 2013 14:56:10 +0100 Subject: [PATCH 3/3] error: Simplify giterr_detach --- include/git2/errors.h | 15 ++++++--------- src/errors.c | 17 ++++++----------- src/iterator.c | 11 ++++++----- 3 files changed, 18 insertions(+), 25 deletions(-) diff --git a/include/git2/errors.h b/include/git2/errors.h index 5f5d0ab1f..be7a31d8e 100644 --- a/include/git2/errors.h +++ b/include/git2/errors.h @@ -89,17 +89,14 @@ GIT_EXTERN(void) giterr_clear(void); /** * Get the last error data and clear it. * - * This copies the last error message into the given `git_buf` and returns - * the associated `git_error_t`, leaving the error cleared as if - * `giterr_clear` had been called. You must call `git_buf_free` on the - * message to release the memory. + * 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. * - * Note: it is possible that this will return `GITERR_NONE` and set the - * buffer to NULL, so be prepared for that condition. Also, if the last - * error was an out-of-memory error, this will return `GITERR_NOMEMORY` - * but also leave the buffer set to NULL (to avoid allocation). + * If there was no existing error in the library, -1 will be returned + * and the contents of `cpy` will be left unmodified. */ -GIT_EXTERN(git_error_t) giterr_detach(git_buf *message); +GIT_EXTERN(int) giterr_detach(git_error *cpy); /** * Set the error message string for this thread. diff --git a/src/errors.c b/src/errors.c index 70b5f2668..d04da4ca9 100644 --- a/src/errors.c +++ b/src/errors.c @@ -112,27 +112,22 @@ void giterr_clear(void) #endif } -git_error_t giterr_detach(git_buf *message) +int giterr_detach(git_error *cpy) { - git_error_t rval; git_error *error = GIT_GLOBAL->last_error; - assert(message); - - git_buf_free(message); + assert(cpy); if (!error) - return GITERR_NONE; + return -1; - rval = error->klass; - - if (error != &g_git_oom_error) - git_buf_attach(message, error->message, 0); + cpy->message = error->message; + cpy->klass = error->klass; error->message = NULL; giterr_clear(); - return rval; + return 0; } const git_error *giterr_last(void) diff --git a/src/iterator.c b/src/iterator.c index 369a079bc..8646399ab 100644 --- a/src/iterator.c +++ b/src/iterator.c @@ -991,8 +991,9 @@ static int fs_iterator__expand_dir(fs_iterator *fi) fi->base.start, fi->base.end, &ff->entries); if (error < 0) { - git_buf msg = GIT_BUF_INIT; - git_error_t errt = giterr_detach(&msg); + git_error last_error = {0}; + + giterr_detach(&last_error); /* these callbacks may clear the error message */ fs_iterator__free_frame(ff); @@ -1000,9 +1001,9 @@ static int fs_iterator__expand_dir(fs_iterator *fi) /* next time return value we skipped to */ fi->base.flags &= ~GIT_ITERATOR_FIRST_ACCESS; - if (msg.ptr) { - giterr_set_str(errt, msg.ptr); - git_buf_free(&msg); + if (last_error.message) { + giterr_set_str(last_error.klass, last_error.message); + free(last_error.message); } return error;