Commit fa59f18d0ddbbb98d45e33934fb0efc3e2bf1557

Vicent Marti 2011-05-09T20:54:04

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");

diff --git a/include/git2/common.h b/include/git2/common.h
index 1d2d3a3..2aae648 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 351d669..e5b9f15 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 73df2e2..40b0feb 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 ea96819..c4d3d6a 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 f5f0b86..6724e8d 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;
 }