Resolve comments from pull request

This converts the map validation function into a macro, tweaks
the GITERR_OS system error automatic appending, and adds a
tentative new error access API and some quick unit tests for
both the old and new error APIs.

include/git2/errors.h

@@ -134,6 +134,7 @@ typedef enum {
 /**
  * Return a detailed error string with the latest error
  * that occurred in the library.
+ * @deprecated This will be replaced in the new error handling
  * @return a string explaining the error
  */
 GIT_EXTERN(const char *) git_lasterror(void);
@@ -145,6 +146,7 @@ GIT_EXTERN(const char *) git_lasterror(void);
  * NOTE: This method will be eventually deprecated in favor
  * of the new `git_lasterror`.
  *
+ * @deprecated This will be replaced in the new error handling
  * @param num The error code to explain
  * @return a string explaining the error code
  */
@@ -152,9 +154,23 @@ GIT_EXTERN(const char *) git_strerror(int num);
 
 /**
  * Clear the latest library error
+ * @deprecated This will be replaced in the new error handling
  */
 GIT_EXTERN(void) git_clearerror(void);
 
+/**
+ * Return the last `git_error` object that was generated for the
+ * current thread or NULL if no error has occurred.
+ *
+ * @return A git_error object.
+ */
+GIT_EXTERN(const git_error *) git_error_last(void);
+
+/**
+ * Clear the last library error that occurred for this thread.
+ */
+GIT_EXTERN(void) git_error_clear(void);
+
 /** @} */
 GIT_END_DECL
 #endif

src/errors.c

@@ -123,33 +123,41 @@ void giterr_set(int error_class, const char *string, ...)
 	char error_str[1024];
 	va_list arglist;
 
+	/* Grab errno before calling vsnprintf() so it won't be overwritten */
+	const char *os_error_msg =
+		(error_class == GITERR_OS && errno != 0) ? strerror(errno) : NULL;
+#ifdef GIT_WIN32
+	DWORD dwLastError = GetLastError();
+#endif
+
 	va_start(arglist, string);
 	p_vsnprintf(error_str, sizeof(error_str), string, arglist);
 	va_end(arglist);
 
 	/* automatically suffix strerror(errno) for GITERR_OS errors */
 	if (error_class == GITERR_OS) {
-		if (errno != 0) {
+		if (os_error_msg != NULL) {
 			strncat(error_str, ": ", sizeof(error_str));
-			strncat(error_str, strerror(errno), sizeof(error_str));
-			errno = 0;
+			strncat(error_str, os_error_msg, sizeof(error_str));
+			errno = 0; /* reset so same error won't be reported twice */
 		}
 #ifdef GIT_WIN32
-		else {
-			LPVOID lpMsgBuf;
-			DWORD dw = GetLastError();
+		else if (dwLastError != 0) {
+			LPVOID lpMsgBuf = NULL;
 
 			FormatMessage(
 				FORMAT_MESSAGE_ALLOCATE_BUFFER | 
 				FORMAT_MESSAGE_FROM_SYSTEM |
 				FORMAT_MESSAGE_IGNORE_INSERTS,
-				NULL, dw, 0, (LPTSTR) &lpMsgBuf, 0, NULL);
+				NULL, dwLastError, 0, (LPTSTR) &lpMsgBuf, 0, NULL);
 
 			if (lpMsgBuf) {
 				strncat(error_str, ": ", sizeof(error_str));
 				strncat(error_str, (const char *)lpMsgBuf, sizeof(error_str));
 				LocalFree(lpMsgBuf);
 			}
+
+			SetLastError(0);
 		}
 #endif
 	}
@@ -185,3 +193,14 @@ void giterr_clear(void)
 {
 	GIT_GLOBAL->last_error = NULL;
 }
+
+const git_error *git_error_last(void)
+{
+	return GIT_GLOBAL->last_error;
+}
+
+void git_error_clear(void)
+{
+	giterr_clear();
+}
+

src/map.c

@@ -1,36 +0,0 @@
-/*
- * Copyright (C) 2009-2012 the libgit2 contributors
- *
- * This file is part of libgit2, distributed under the GNU GPL v2 with
- * a Linking Exception. For full terms see the included COPYING file.
- */
-#include <git2/common.h>
-#include "map.h"
-
-int validate_map_args(
-	git_map *out, size_t len, int prot, int flags, int fd, git_off_t offset)
-{
-	GIT_UNUSED(fd);
-	GIT_UNUSED(offset);
-
-	if (out == NULL || len == 0) {
-		errno = EINVAL;
-		giterr_set(GITERR_OS, "Failed to mmap. No map or zero length");
-		return -1;
-	}
-
-	if (!(prot & GIT_PROT_WRITE) && !(prot & GIT_PROT_READ)) {
-		errno = EINVAL;
-		giterr_set(GITERR_OS, "Failed to mmap. Invalid protection parameters");
-		return -1;
-	}
-
-	if (flags & GIT_MAP_FIXED) {
-		errno = EINVAL;
-		giterr_set(GITERR_OS, "Failed to mmap. FIXED not set");
-		return -1;
-	}
-
-	return 0;
-}
-

src/map.h

@@ -31,7 +31,10 @@ typedef struct { /* memory mapped buffer	*/
 #endif
 } git_map;
 
-extern int validate_map_args(git_map *out, size_t len, int prot, int flags, int fd, git_off_t offset);
+#define GIT_MMAP_VALIDATE(out, len, prot, flags) do { \
+	assert(out != NULL && len > 0); \
+	assert((prot & GIT_PROT_WRITE) || (prot & GIT_PROT_READ)); \
+	assert((flags & GIT_MAP_FIXED) == 0); } while (0)
 
 extern int p_mmap(git_map *out, size_t len, int prot, int flags, int fd, git_off_t offset);
 extern int p_munmap(git_map *map);

src/unix/map.c

@@ -17,8 +17,7 @@ int p_mmap(git_map *out, size_t len, int prot, int flags, int fd, git_off_t offs
 	int mprot = 0;
 	int mflag = 0;
 
-	if (validate_map_args(out, len, prot, flags, fd, offset) < 0)
-		return -1;
+	GIT_MMAP_VALIDATE(out, len, prot, flags);
 
 	out->data = NULL;
 	out->len = 0;

src/win32/map.c

@@ -33,8 +33,7 @@ int p_mmap(git_map *out, size_t len, int prot, int flags, int fd, git_off_t offs
 	git_off_t page_start;
 	git_off_t page_offset;
 
-	if (validate_map_args(out, len, prot, flags, fd, offset) < 0)
-		return -1;
+	GIT_MMAP_VALIDATE(out, len, prot, flags);
 
 	out->data = NULL;
 	out->len = 0;

tests-clar/core/errors.c

@@ -0,0 +1,74 @@
+#include "clar_libgit2.h"
+#include "common.h"
+#include "util.h"
+#include "posix.h"
+
+#ifdef git__throw
+void test_core_errors__old_school(void)
+{
+	git_clearerror();
+	cl_assert(git_lasterror() == NULL);
+
+	cl_assert(git_strerror(GIT_ENOTFOUND) != NULL);
+
+	git__throw(GIT_ENOTFOUND, "My Message");
+	cl_assert(git_lasterror() != NULL);
+	cl_assert(git__prefixcmp(git_lasterror(), "My Message") == 0);
+	git_clearerror();
+}
+#endif
+
+#ifdef GITERR_CHECK_ALLOC
+void test_core_errors__new_school(void)
+{
+	char *str_in_error;
+
+	git_error_clear();
+	cl_assert(git_error_last() == NULL);
+
+	giterr_set_oom(); /* internal fn */
+
+	cl_assert(git_error_last() != NULL);
+	cl_assert(git_error_last()->klass == GITERR_NOMEMORY);
+	str_in_error = strstr(git_error_last()->message, "memory");
+	cl_assert(str_in_error != NULL);
+
+	git_error_clear();
+
+	giterr_set(GITERR_REPOSITORY, "This is a test"); /* internal fn */
+
+	cl_assert(git_error_last() != NULL);
+	str_in_error = strstr(git_error_last()->message, "This is a test");
+	cl_assert(str_in_error != NULL);
+
+	git_error_clear();
+
+	{
+		struct stat st;
+		assert(p_lstat("this_file_does_not_exist", &st) < 0);
+	}
+	giterr_set(GITERR_OS, "stat failed"); /* internal fn */
+
+	cl_assert(git_error_last() != NULL);
+	str_in_error = strstr(git_error_last()->message, "stat failed");
+	cl_assert(str_in_error != NULL);
+	cl_assert(git__prefixcmp(str_in_error, "stat failed: ") == 0);
+	cl_assert(strlen(str_in_error) > strlen("stat failed: "));
+
+#ifdef GIT_WIN32
+	git_error_clear();
+
+	/* The MSDN docs use this to generate a sample error */
+	cl_assert(GetProcessId(NULL) == 0);
+	giterr_set(GITERR_OS, "GetProcessId failed"); /* internal fn */
+
+	cl_assert(git_error_last() != NULL);
+	str_in_error = strstr(git_error_last()->message, "GetProcessId failed");
+	cl_assert(str_in_error != NULL);
+	cl_assert(git__prefixcmp(str_in_error, "GetProcessId failed: ") == 0);
+	cl_assert(strlen(str_in_error) > strlen("GetProcessId failed: "));
+#endif
+
+	git_error_clear();
+}
+#endif