From 02f9e637a1d282d03ce3362e7c3467e73c87bf2e Mon Sep 17 00:00:00 2001 From: Vicent Marti Date: Thu, 5 May 2011 01:12:17 +0300 Subject: [PATCH 1/4] errors: Add error handling function --- include/git2/thread-utils.h | 1 + src/common.h | 2 ++ src/errors.c | 40 +++++++++++++++++++++++++++++++++++++ 3 files changed, 43 insertions(+) diff --git a/include/git2/thread-utils.h b/include/git2/thread-utils.h index fb8644b93..e26876bea 100644 --- a/include/git2/thread-utils.h +++ b/include/git2/thread-utils.h @@ -35,6 +35,7 @@ #if defined(__APPLE__) && defined(__MACH__) # undef GIT_TLS +# define GIT_TLS #elif defined(__GNUC__) || \ defined(__SUNPRO_C) || \ diff --git a/src/common.h b/src/common.h index 5ad878e26..b3f5253a9 100644 --- a/src/common.h +++ b/src/common.h @@ -56,4 +56,6 @@ typedef SSIZE_T ssize_t; #define GIT_PATH_MAX 4096 +extern int git__error(int error, const char *, ...) GIT_FORMAT_PRINTF(2, 3); + #endif /* INCLUDE_common_h__ */ diff --git a/src/errors.c b/src/errors.c index c3a495cc9..b0c1c1519 100644 --- a/src/errors.c +++ b/src/errors.c @@ -1,6 +1,33 @@ +/* + * This file is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License, version 2, + * as published by the Free Software Foundation. + * + * In addition to the permissions in the GNU General Public License, + * the authors give you unlimited permission to link the compiled + * version of this file into combinations with other programs, + * and to distribute those combinations without any restriction + * coming from the use of this file. (The General Public License + * restrictions do apply in other respects; for example, they cover + * modification of the file, and distribution when not linked into + * a combined executable.) + * + * This file is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; see the file COPYING. If not, write to + * the Free Software Foundation, 51 Franklin Street, Fifth Floor, + * Boston, MA 02110-1301, USA. + */ #include "common.h" +#include "git2/thread-utils.h" /* for GIT_TLS */ #include "thread-utils.h" /* for GIT_TLS */ +#include + static struct { int num; const char *str; @@ -46,3 +73,16 @@ const char *git_strerror(int num) return "Unknown error"; } + +static GIT_TLS char g_last_error[1024]; + +int git__error(int error, const char *msg, ...) +{ + va_list va; + + va_start(va, msg); + vsnprintf(g_last_error, sizeof(g_last_error), msg, va); + va_end(va); + + return error; +} From 3f53c97113deb986b58aac8bb89f76b47e76507b Mon Sep 17 00:00:00 2001 From: Vicent Marti Date: Thu, 5 May 2011 01:20:27 +0300 Subject: [PATCH 2/4] errors: Set error messages on memory allocation --- src/common.h | 4 ++-- src/util.h | 43 ++++++++++++++++++++++++++++++++++--------- 2 files changed, 36 insertions(+), 11 deletions(-) diff --git a/src/common.h b/src/common.h index b3f5253a9..351d6696f 100644 --- a/src/common.h +++ b/src/common.h @@ -50,12 +50,12 @@ typedef SSIZE_T ssize_t; #include "git2/common.h" #include "git2/types.h" -#include "util.h" #include "thread-utils.h" #include "bswap.h" #define GIT_PATH_MAX 4096 - extern int git__error(int error, const char *, ...) GIT_FORMAT_PRINTF(2, 3); +#include "util.h" + #endif /* INCLUDE_common_h__ */ diff --git a/src/util.h b/src/util.h index 3c606493f..f5f0b8662 100644 --- a/src/util.h +++ b/src/util.h @@ -6,16 +6,41 @@ #define MSB(x, bits) ((x) & (~0ULL << (bitsizeof(x) - (bits)))) /* - * Don't wrap malloc/calloc. - * Use the default versions in glibc, and make - * sure that any methods that allocate memory - * return a GIT_ENOMEM error when allocation - * fails. + * Custom memory allocation wrappers + * that set error code and error message + * on allocation failure */ -#define git__malloc malloc -#define git__calloc calloc -#define git__realloc realloc -#define git__strdup strdup +GIT_INLINE(void *) git__malloc(size_t len) +{ + void *ptr = malloc(len); + if (!ptr) + git__error(GIT_ENOMEM, "Out of memory. Failed to allocate %d bytes.", (int)len); + return ptr; +} + +GIT_INLINE(void *) git__calloc(size_t nelem, size_t elsize) +{ + void *ptr = calloc(nelem, elsize); + if (!ptr) + git__error(GIT_ENOMEM, "Out of memory. Failed to allocate %d bytes.", (int)elsize); + return ptr; +} + +GIT_INLINE(char *) git__strdup(const char *str) +{ + char *ptr = strdup(str); + if (!ptr) + git__error(GIT_ENOMEM, "Out of memory. Failed to duplicate string"); + return ptr; +} + +GIT_INLINE(void *) git__realloc(void *ptr, size_t size) +{ + void *new_ptr = realloc(ptr, size); + if (!new_ptr) + git__error(GIT_ENOMEM, "Out of memory. Failed to allocate %d bytes.", (int)size); + return new_ptr; +} extern int git__fmt(char *, size_t, const char *, ...) GIT_FORMAT_PRINTF(3, 4); From 5eb0fab846d6b2f8bcf3caf7a9e33afa36753850 Mon Sep 17 00:00:00 2001 From: Vicent Marti Date: Thu, 5 May 2011 01:49:27 +0300 Subject: [PATCH 3/4] errors: Update external API with new `git_lasterror` --- include/git2/common.h | 3 +++ include/git2/errors.h | 8 +++---- src/errors.c | 52 +++++-------------------------------------- 3 files changed, 13 insertions(+), 50 deletions(-) diff --git a/include/git2/common.h b/include/git2/common.h index 22c7cc466..1d2d3a3e9 100644 --- a/include/git2/common.h +++ b/include/git2/common.h @@ -170,6 +170,9 @@ /** The given literal is not a valid number */ #define GIT_ENOTNUM (GIT_ERROR - 25) +/** Streaming error */ +#define GIT_ESTREAM (GIT_ERROR - 26) + GIT_BEGIN_DECL typedef struct { diff --git a/include/git2/errors.h b/include/git2/errors.h index 627e67c70..fde0dc73d 100644 --- a/include/git2/errors.h +++ b/include/git2/errors.h @@ -34,11 +34,11 @@ GIT_BEGIN_DECL /** - * strerror() for the Git library - * @param num The error code to explain - * @return a string explaining the error code + * Return a detailed error string with the latest error + * that occurred in the library. + * @return a string explaining the error */ -GIT_EXTERN(const char *) git_strerror(int num); +GIT_EXTERN(const char *) git_lasterror(void); /** @} */ GIT_END_DECL diff --git a/src/errors.c b/src/errors.c index b0c1c1519..73df2e209 100644 --- a/src/errors.c +++ b/src/errors.c @@ -28,52 +28,6 @@ #include -static struct { - int num; - const char *str; -} error_codes[] = { - {GIT_ERROR, "Unspecified error"}, - {GIT_ENOTOID, "Input was not a properly formatted Git object id."}, - {GIT_ENOTFOUND, "Object does not exist in the scope searched."}, - {GIT_ENOMEM, "Not enough space available."}, - {GIT_EOSERR, "Consult the OS error information."}, - {GIT_EOBJTYPE, "The specified object is of invalid type"}, - {GIT_EOBJCORRUPTED, "The specified object has its data corrupted"}, - {GIT_ENOTAREPO, "The specified repository is invalid"}, - {GIT_EINVALIDTYPE, "The object type is invalid or doesn't match"}, - {GIT_EMISSINGOBJDATA, "The object cannot be written that because it's missing internal data"}, - {GIT_EPACKCORRUPTED, "The packfile for the ODB is corrupted"}, - {GIT_EFLOCKFAIL, "Failed to adquire or release a file lock"}, - {GIT_EZLIB, "The Z library failed to inflate/deflate an object's data"}, - {GIT_EBUSY, "The queried object is currently busy"}, - {GIT_EINVALIDPATH, "The path is invalid"}, - {GIT_EBAREINDEX, "The index file is not backed up by an existing repository"}, - {GIT_EINVALIDREFNAME, "The name of the reference is not valid"}, - {GIT_EREFCORRUPTED, "The specified reference has its data corrupted"}, - {GIT_ETOONESTEDSYMREF, "The specified symbolic reference is too deeply nested"}, - {GIT_EPACKEDREFSCORRUPTED, "The pack-refs file is either corrupted of its format is not currently supported"}, - {GIT_EINVALIDPATH, "The path is invalid" }, - {GIT_EREVWALKOVER, "The revision walker is empty; there are no more commits left to iterate"}, - {GIT_EINVALIDREFSTATE, "The state of the reference is not valid"}, - {GIT_ENOTIMPLEMENTED, "This feature has not been implemented yet"}, - {GIT_EEXISTS, "A reference with this name already exists"}, - {GIT_EOVERFLOW, "The given integer literal is too large to be parsed"}, - {GIT_ENOTNUM, "The given literal is not a valid number"}, -}; - -const char *git_strerror(int num) -{ - size_t i; - - if (num == GIT_EOSERR) - return strerror(errno); - for (i = 0; i < ARRAY_SIZE(error_codes); i++) - if (num == error_codes[i].num) - return error_codes[i].str; - - return "Unknown error"; -} - static GIT_TLS char g_last_error[1024]; int git__error(int error, const char *msg, ...) @@ -86,3 +40,9 @@ int git__error(int error, const char *msg, ...) return error; } + +const char *git_lasterror(void) +{ + return g_last_error; +} + From fa59f18d0ddbbb98d45e33934fb0efc3e2bf1557 Mon Sep 17 00:00:00 2001 From: Vicent Marti Date: Mon, 9 May 2011 20:54:04 +0300 Subject: [PATCH 4/4] Change error handling mechanism once again Ok, this is the real deal. Hopefully. Here's how it's going to work: - One main method, called `git__throw`, that sets the error code and error message when an error happens. This method must be called in every single place where an error code was being returned previously, setting an error message instead. Example, instead of: return GIT_EOBJCORRUPTED; Use: return git__throw(GIT_EOBJCORRUPTED, "The object is missing a finalizing line feed"); And instead of: [...] { error = GIT_EOBJCORRUPTED; goto cleanup; } Use: [...] { error = git__throw(GIT_EOBJCORRUPTED, "What an error!"); goto cleanup; } The **only** exception to this are the allocation methods, which return NULL on failure but already set the message manually. /* only place where an error code can be returned directly, because the error message has already been set by the wrapper */ if (foo == NULL) return GIT_ENOMEM; - One secondary method, called `git__rethrow`, which can be used to fine-grain an error message and build an error stack. Example, instead of: if ((error = foobar(baz)) < GIT_SUCCESS) return error; You can now do: if ((error = foobar(baz)) < GIT_SUCCESS) return git__rethrow(error, "Failed to do a major operation"); The return of the `git_lasterror` method will be a string in the shape of: "Failed to do a major operation. (Failed to do an internal operation)" E.g. "Failed to open the index. (Not enough permissions to access '/path/to/index')." NOTE: do not abuse this method. Try to write all `git__throw` messages in a descriptive manner, to avoid having to rethrow them to clarify their meaning. This method should only be used in the places where the original error message set by a subroutine is not specific enough. It is encouraged to continue using this style as much possible to enforce error propagation: if ((error = foobar(baz)) < GIT_SUCCESS) return error; /* `foobar` has set an error message, and we are just propagating it */ The error handling revamp will take place in two phases: - Phase 1: Replace all pieces of code that return direct error codes with calls to `git__throw`. This can be done semi-automatically using `ack` to locate all the error codes that must be replaced. - Phase 2: Add some `git__rethrow` calls in those cases where the original error messages are not specific enough. Phase 1 is the main goal. A minor libgit2 release will be shipped once Phase 1 is ready, and the work will start on gradually improving the error handling mechanism by refining specific error messages. OTHER NOTES: - When writing error messages, please refrain from using weasel words. They add verbosity to the message without giving any real information. (<3 Emeric) E.g. "The reference file appears to be missing a carriage return" Nope. "The reference file is missing a carriage return" Yes. - When calling `git__throw`, please try to use more generic error codes so we can eventually reduce the list of error codes to something more reasonable. Feel free to add new, more generic error codes if these are going to replace several of the old ones. E.g. return GIT_EREFCORRUPTED; Can be turned into: return git__throw(GIT_EOBJCORRUPTED, "The reference is corrupted"); --- include/git2/common.h | 3 +++ src/common.h | 3 ++- src/errors.c | 20 +++++++++++++++++++- src/refs.c | 33 +++++++++++++++++++-------------- src/util.h | 8 ++++---- 5 files changed, 47 insertions(+), 20 deletions(-) diff --git a/include/git2/common.h b/include/git2/common.h index 1d2d3a3e9..2aae648fb 100644 --- a/include/git2/common.h +++ b/include/git2/common.h @@ -173,6 +173,9 @@ /** Streaming error */ #define GIT_ESTREAM (GIT_ERROR - 26) +/** invalid arguments to function */ +#define GIT_EINVALIDARGS (GIT_ERROR - 27) + GIT_BEGIN_DECL typedef struct { diff --git a/src/common.h b/src/common.h index 351d6696f..e5b9f15ed 100644 --- a/src/common.h +++ b/src/common.h @@ -54,7 +54,8 @@ typedef SSIZE_T ssize_t; #include "bswap.h" #define GIT_PATH_MAX 4096 -extern int git__error(int error, const char *, ...) GIT_FORMAT_PRINTF(2, 3); +extern int git__throw(int error, const char *, ...) GIT_FORMAT_PRINTF(2, 3); +extern int git__rethrow(int error, const char *, ...) GIT_FORMAT_PRINTF(2, 3); #include "util.h" diff --git a/src/errors.c b/src/errors.c index 73df2e209..40b0feb91 100644 --- a/src/errors.c +++ b/src/errors.c @@ -30,7 +30,25 @@ static GIT_TLS char g_last_error[1024]; -int git__error(int error, const char *msg, ...) +int git__rethrow(int error, const char *msg, ...) +{ + char new_error[1024]; + char *old_error = NULL; + + va_list va; + + va_start(va, msg); + vsnprintf(new_error, sizeof(new_error), msg, va); + va_end(va); + + old_error = strdup(g_last_error); + snprintf(g_last_error, sizeof(g_last_error), "%s \n - %s", new_error, old_error); + free(old_error); + + return error; +} + +int git__throw(int error, const char *msg, ...) { va_list va; diff --git a/src/refs.c b/src/refs.c index ea968196f..c4d3d6ae6 100644 --- a/src/refs.c +++ b/src/refs.c @@ -122,7 +122,8 @@ static int reference_create( else if (type == GIT_REF_OID) size = sizeof(reference_oid); else - return GIT_EINVALIDREFSTATE; + return git__throw(GIT_EINVALIDARGS, + "Invalid reference type. Use either GIT_REF_OID or GIT_REF_SYMBOLIC as type specifier"); reference = git__malloc(size); if (reference == NULL) @@ -159,11 +160,9 @@ static int reference_read(gitfo_buf *file_content, time_t *mtime, const char *re /* Determine the full path of the file */ git__joinpath(path, repo_path, ref_name); - if (gitfo_stat(path, &st) < 0) - return GIT_ENOTFOUND; - - if (S_ISDIR(st.st_mode)) - return GIT_EOBJCORRUPTED; + if (gitfo_stat(path, &st) < 0 || S_ISDIR(st.st_mode)) + return git__throw(GIT_ENOTFOUND, + "Cannot read reference file '%s'", ref_name); if (mtime) *mtime = st.st_mtime; @@ -205,7 +204,8 @@ static int loose_update(git_reference *ref) else if (ref->type == GIT_REF_OID) error = loose_parse_oid(ref, &ref_file); else - error = GIT_EINVALIDREFSTATE; + error = git__throw(GIT_EOBJCORRUPTED, + "Invalid reference type (%d) for loose reference", ref->type); gitfo_free_buf(&ref_file); @@ -229,7 +229,8 @@ static int loose_parse_symbolic(git_reference *ref, gitfo_buf *file_content) ref_sym = (reference_symbolic *)ref; if (file_content->len < (header_len + 1)) - return GIT_EREFCORRUPTED; + return git__throw(GIT_EOBJCORRUPTED, + "Failed to parse loose reference. Object too short"); /* * Assume we have already checked for the header @@ -246,7 +247,8 @@ static int loose_parse_symbolic(git_reference *ref, gitfo_buf *file_content) /* remove newline at the end of file */ eol = strchr(ref_sym->target, '\n'); if (eol == NULL) - return GIT_EREFCORRUPTED; + return git__throw(GIT_EOBJCORRUPTED, + "Failed to parse loose reference. Missing EOL"); *eol = '\0'; if (eol[-1] == '\r') @@ -257,6 +259,7 @@ static int loose_parse_symbolic(git_reference *ref, gitfo_buf *file_content) static int loose_parse_oid(git_reference *ref, gitfo_buf *file_content) { + int error; reference_oid *ref_oid; char *buffer; @@ -265,17 +268,19 @@ static int loose_parse_oid(git_reference *ref, gitfo_buf *file_content) /* File format: 40 chars (OID) + newline */ if (file_content->len < GIT_OID_HEXSZ + 1) - return GIT_EREFCORRUPTED; + return git__throw(GIT_EOBJCORRUPTED, + "Failed to parse loose reference. Reference too short"); - if (git_oid_mkstr(&ref_oid->oid, buffer) < GIT_SUCCESS) - return GIT_EREFCORRUPTED; + if ((error = git_oid_mkstr(&ref_oid->oid, buffer)) < GIT_SUCCESS) + return git__rethrow(GIT_EOBJCORRUPTED, "Failed to parse loose reference."); buffer = buffer + GIT_OID_HEXSZ; if (*buffer == '\r') buffer++; if (*buffer != '\n') - return GIT_EREFCORRUPTED; + return git__throw(GIT_EOBJCORRUPTED, + "Failed to parse loose reference. Missing EOL"); return GIT_SUCCESS; } @@ -387,7 +392,7 @@ static int loose_write(git_reference *ref) strcpy(ref_contents, GIT_SYMREF); strcat(ref_contents, ref_sym->target); } else { - error = GIT_EINVALIDREFSTATE; + error = git__throw(GIT_EOBJCORRUPTED, "Failed to write reference. Invalid reference type"); goto unlock; } diff --git a/src/util.h b/src/util.h index f5f0b8662..6724e8d41 100644 --- a/src/util.h +++ b/src/util.h @@ -14,7 +14,7 @@ GIT_INLINE(void *) git__malloc(size_t len) { void *ptr = malloc(len); if (!ptr) - git__error(GIT_ENOMEM, "Out of memory. Failed to allocate %d bytes.", (int)len); + git__throw(GIT_ENOMEM, "Out of memory. Failed to allocate %d bytes.", (int)len); return ptr; } @@ -22,7 +22,7 @@ GIT_INLINE(void *) git__calloc(size_t nelem, size_t elsize) { void *ptr = calloc(nelem, elsize); if (!ptr) - git__error(GIT_ENOMEM, "Out of memory. Failed to allocate %d bytes.", (int)elsize); + git__throw(GIT_ENOMEM, "Out of memory. Failed to allocate %d bytes.", (int)elsize); return ptr; } @@ -30,7 +30,7 @@ GIT_INLINE(char *) git__strdup(const char *str) { char *ptr = strdup(str); if (!ptr) - git__error(GIT_ENOMEM, "Out of memory. Failed to duplicate string"); + git__throw(GIT_ENOMEM, "Out of memory. Failed to duplicate string"); return ptr; } @@ -38,7 +38,7 @@ GIT_INLINE(void *) git__realloc(void *ptr, size_t size) { void *new_ptr = realloc(ptr, size); if (!new_ptr) - git__error(GIT_ENOMEM, "Out of memory. Failed to allocate %d bytes.", (int)size); + git__throw(GIT_ENOMEM, "Out of memory. Failed to allocate %d bytes.", (int)size); return new_ptr; }