errors: simplify the saved-state handling

Instead of having a separate type for saving state, we can re-use the
`git_error` structure. In practice, saving the error code along with the
`git_error` information has not proven necessary or useful, so it can be
removed to simplify down to re-using a single structure.

src/libgit2/clone.c

@@ -542,15 +542,15 @@ static int git__clone(
 	}
 
 	if (error != 0) {
-		git_error_state last_error = {0};
-		git_error_state_capture(&last_error, error);
+		git_error *last_error;
+		git_error_save(&last_error);
 
 		git_repository_free(repo);
 		repo = NULL;
 
 		(void)git_futils_rmdir_r(local_path, NULL, rmdir_flags);
 
-		git_error_state_restore(&last_error);
+		git_error_restore(last_error);
 	}
 
 	*out = repo;

src/libgit2/errors.c

@@ -36,6 +36,10 @@ static git_error no_error = {
 	GIT_ERROR_NONE
 };
 
+#define IS_STATIC_ERROR(err) \
+	((err) == &oom_error || (err) == &uninitialized_error || \
+	 (err) == &tlsdata_error || (err) == &no_error)
+
 static void set_error_from_buffer(int error_class)
 {
 	git_threadstate *threadstate = git_threadstate_get();
@@ -66,12 +70,11 @@ static void set_error(int error_class, char *string)
 
 	git_str_clear(buf);
 
-	if (string) {
+	if (string)
 		git_str_puts(buf, string);
-		git__free(string);
-	}
 
-	set_error_from_buffer(error_class);
+	if (!git_str_oom(buf))
+		set_error_from_buffer(error_class);
 }
 
 void git_error_set_oom(void)
@@ -178,6 +181,16 @@ void git_error_clear(void)
 #endif
 }
 
+bool git_error_exists(void)
+{
+	git_threadstate *threadstate;
+
+	if ((threadstate = git_threadstate_get()) == NULL)
+		return true;
+
+	return threadstate->last_error != NULL;
+}
+
 const git_error *git_error_last(void)
 {
 	git_threadstate *threadstate;
@@ -195,67 +208,68 @@ const git_error *git_error_last(void)
 	return threadstate->last_error;
 }
 
-int git_error_state_capture(git_error_state *state, int error_code)
+int git_error_save(git_error **out)
 {
 	git_threadstate *threadstate = git_threadstate_get();
-	git_error *error;
-	git_str *error_buf;
+	git_error *error, *dup;
 
-	if (!threadstate)
+	if (!threadstate) {
+		*out = &tlsdata_error;
 		return -1;
+	}
 
 	error = threadstate->last_error;
-	error_buf = &threadstate->error_buf;
 
-	memset(state, 0, sizeof(git_error_state));
-
-	if (!error_code)
+	if (!error || error == &no_error) {
+		*out = &no_error;
+		return 0;
+	} else if (IS_STATIC_ERROR(error)) {
+		*out = error;
 		return 0;
-
-	state->error_code = error_code;
-	state->oom = (error == &oom_error);
-
-	if (error) {
-		state->error_msg.klass = error->klass;
-
-		if (state->oom)
-			state->error_msg.message = oom_error.message;
-		else
-			state->error_msg.message = git_str_detach(error_buf);
 	}
 
-	git_error_clear();
-	return error_code;
-}
-
-int git_error_state_restore(git_error_state *state)
-{
-	int ret = 0;
-
-	git_error_clear();
-
-	if (state && state->error_msg.message) {
-		if (state->oom)
-			git_error_set_oom();
-		else
-			set_error(state->error_msg.klass, state->error_msg.message);
-
-		ret = state->error_code;
-		memset(state, 0, sizeof(git_error_state));
+	if ((dup = git__malloc(sizeof(git_error))) == NULL) {
+		*out = &oom_error;
+		return -1;
 	}
 
-	return ret;
+	dup->klass = error->klass;
+	dup->message = git__strdup(error->message);
+
+	if (!dup->message) {
+		*out = &oom_error;
+		return -1;
+	}
+
+	*out = dup;
+	return 0;
 }
 
-void git_error_state_free(git_error_state *state)
+int git_error_restore(git_error *error)
 {
-	if (!state)
+	git_threadstate *threadstate = git_threadstate_get();
+
+	GIT_ASSERT_ARG(error);
+
+	if (IS_STATIC_ERROR(error) && threadstate)
+		threadstate->last_error = error;
+	else
+		set_error(error->klass, error->message);
+
+	git_error_free(error);
+	return 0;
+}
+
+void git_error_free(git_error *error)
+{
+	if (!error)
 		return;
 
-	if (!state->oom)
-		git__free(state->error_msg.message);
+	if (IS_STATIC_ERROR(error))
+		return;
 
-	memset(state, 0, sizeof(git_error_state));
+	git__free(error->message);
+	git__free(error);
 }
 
 int git_error_system_last(void)

src/libgit2/errors.h

@@ -16,6 +16,11 @@
  */
 void git_error_vset(int error_class, const char *fmt, va_list ap);
 
+/**
+ * Determines whether an error exists.
+ */
+bool git_error_exists(void);
+
 /**
  * Set error message for user callback if needed.
  *
@@ -28,9 +33,8 @@ GIT_INLINE(int) git_error_set_after_callback_function(
 	int error_code, const char *action)
 {
 	if (error_code) {
-		const git_error *e = git_error_last();
-		if (!e || !e->message)
-			git_error_set(e ? e->klass : GIT_ERROR_CALLBACK,
+		if (!git_error_exists())
+			git_error_set(GIT_ERROR_CALLBACK,
 				"%s callback returned %d", action, error_code);
 	}
 	return error_code;
@@ -54,28 +58,24 @@ int git_error_system_last(void);
  */
 void git_error_system_set(int code);
 
-/**
- * Structure to preserve libgit2 error state
- */
-typedef struct {
-	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 not clear the current error state.
  * You must either restore this error state, or free it.
+ *
+ * This function returns 0 on success, or -1 on failure. If the function
+ * fails, the `out` structure is set to the failure error message and
+ * the normal system error message is not updated.
  */
-extern int git_error_state_capture(git_error_state *state, int error_code);
+extern int git_error_save(git_error **out);
 
 /**
- * Restore error state to a previous value, returning saved error code.
+ * Restore thread error state to the given value. The given value is
+ * freed and `git_error_free` need not be called on it.
  */
-extern int git_error_state_restore(git_error_state *state);
+extern int git_error_restore(git_error *error);
 
 /** Free an error state. */
-extern void git_error_state_free(git_error_state *state);
+extern void git_error_free(git_error *error);
 
 #endif

src/libgit2/filter.c

@@ -908,7 +908,7 @@ static int buffered_stream_close(git_writestream *s)
 {
 	struct buffered_stream *buffered_stream = (struct buffered_stream *)s;
 	git_str *writebuf;
-	git_error_state error_state = {0};
+	git_error *last_error;
 	int error;
 
 	GIT_ASSERT_ARG(buffered_stream);
@@ -946,9 +946,9 @@ static int buffered_stream_close(git_writestream *s)
 	} else {
 		/* close stream before erroring out taking care
 		 * to preserve the original error */
-		git_error_state_capture(&error_state, error);
+		git_error_save(&last_error);
 		buffered_stream->target->close(buffered_stream->target);
-		git_error_state_restore(&error_state);
+		git_error_restore(last_error);
 		return error;
 	}
 

src/libgit2/index.c

@@ -1609,15 +1609,17 @@ int git_index_add_bypath(git_index *index, const char *path)
 
 	if (ret == GIT_EDIRECTORY) {
 		git_submodule *sm;
-		git_error_state err;
+		git_error *last_error;
 
-		git_error_state_capture(&err, ret);
+		git_error_save(&last_error);
 
 		ret = git_submodule_lookup(&sm, INDEX_OWNER(index), path);
-		if (ret == GIT_ENOTFOUND)
-			return git_error_state_restore(&err);
+		if (ret == GIT_ENOTFOUND) {
+			git_error_restore(last_error);
+			return GIT_EDIRECTORY;
+		}
 
-		git_error_state_free(&err);
+		git_error_free(last_error);
 
 		/*
 		 * EEXISTS means that there is a repository at that path, but it's not known

src/libgit2/transports/httpclient.c

@@ -768,25 +768,37 @@ static int check_certificate(
 	void *cert_cb_payload)
 {
 	git_cert *cert;
-	git_error_state last_error = {0};
+	git_error *last_error;
 	int error;
 
 	if ((error = git_stream_certificate(&cert, stream)) < 0)
 		return error;
 
-	git_error_state_capture(&last_error, GIT_ECERTIFICATE);
+	/*
+	 * Allow callers to set an error - but save ours and clear
+	 * it, so that we can detect if they set one and restore it
+	 * if we need to.
+	 */
+	git_error_save(&last_error);
+	git_error_clear();
 
 	error = cert_cb(cert, is_valid, url->host, cert_cb_payload);
 
-	if (error == GIT_PASSTHROUGH && !is_valid)
-		return git_error_state_restore(&last_error);
-	else if (error == GIT_PASSTHROUGH)
-		error = 0;
-	else if (error && !git_error_last())
-		git_error_set(GIT_ERROR_HTTP,
-		              "user rejected certificate for %s", url->host);
+	if (error == GIT_PASSTHROUGH) {
+		error = is_valid ? 0 : -1;
 
-	git_error_state_free(&last_error);
+		if (error) {
+			git_error_restore(last_error);
+			last_error = NULL;
+		}
+	} else if (error) {
+		if (!git_error_exists())
+			git_error_set(GIT_ERROR_HTTP,
+		              "user rejected certificate for %s",
+			      url->host);
+	}
+
+	git_error_free(last_error);
 	return error;
 }
 

tests/util/errors.c

@@ -97,58 +97,32 @@ void test_errors__new_school(void)
 
 void test_errors__restore(void)
 {
-	git_error_state err_state = {0};
+	git_error *last_error;
 
 	git_error_clear();
 	cl_assert(git_error_last() != NULL);
 	cl_assert(git_error_last()->klass == GIT_ERROR_NONE);
-	cl_assert(strcmp(git_error_last()->message, "no error") == 0);
-
-	cl_assert_equal_i(0, git_error_state_capture(&err_state, 0));
-
-	memset(&err_state, 0x0, sizeof(git_error_state));
+	cl_assert(strcmp("no error", git_error_last()->message) == 0);
 
 	git_error_set(42, "Foo: %s", "bar");
-	cl_assert_equal_i(-1, git_error_state_capture(&err_state, -1));
-
-	cl_assert(git_error_last() != NULL);
-	cl_assert(git_error_last()->klass == GIT_ERROR_NONE);
-	cl_assert(strcmp(git_error_last()->message, "no error") == 0);
-
-	git_error_set(99, "Bar: %s", "foo");
-
-	git_error_state_restore(&err_state);
-
-	cl_assert_equal_i(42, git_error_last()->klass);
-	cl_assert_equal_s("Foo: bar", git_error_last()->message);
-}
-
-void test_errors__free_state(void)
-{
-	git_error_state err_state = {0};
+	cl_assert(git_error_save(&last_error) == 0);
 
 	git_error_clear();
-
-	git_error_set(42, "Foo: %s", "bar");
-	cl_assert_equal_i(-1, git_error_state_capture(&err_state, -1));
+	cl_assert(git_error_last() != NULL);
+	cl_assert(git_error_last()->klass == GIT_ERROR_NONE);
+	cl_assert(strcmp("no error", git_error_last()->message) == 0);
 
 	git_error_set(99, "Bar: %s", "foo");
 
-	git_error_state_free(&err_state);
+	git_error_restore(last_error);
 
-	cl_assert_equal_i(99, git_error_last()->klass);
-	cl_assert_equal_s("Bar: foo", git_error_last()->message);
-
-	git_error_state_restore(&err_state);
-
-	cl_assert(git_error_last() != NULL);
-	cl_assert(git_error_last()->klass == GIT_ERROR_NONE);
-	cl_assert(strcmp(git_error_last()->message, "no error") == 0);
+	cl_assert(git_error_last()->klass == 42);
+	cl_assert(strcmp("Foo: bar", git_error_last()->message) == 0);
 }
 
 void test_errors__restore_oom(void)
 {
-	git_error_state err_state = {0};
+	git_error *last_error;
 	const git_error *oom_error = NULL;
 
 	git_error_clear();
@@ -156,17 +130,18 @@ void test_errors__restore_oom(void)
 	git_error_set_oom(); /* internal fn */
 	oom_error = git_error_last();
 	cl_assert(oom_error);
+	cl_assert(oom_error->klass == GIT_ERROR_NOMEMORY);
 
-	cl_assert_equal_i(-1, git_error_state_capture(&err_state, -1));
+	cl_assert(git_error_save(&last_error) == 0);
+	cl_assert(last_error->klass == GIT_ERROR_NOMEMORY);
+	cl_assert(strcmp("Out of memory", last_error->message) == 0);
 
+	git_error_clear();
 	cl_assert(git_error_last() != NULL);
 	cl_assert(git_error_last()->klass == GIT_ERROR_NONE);
-	cl_assert(strcmp(git_error_last()->message, "no error") == 0);
-	cl_assert_equal_i(GIT_ERROR_NOMEMORY, err_state.error_msg.klass);
-	cl_assert_equal_s("Out of memory", err_state.error_msg.message);
-
-	git_error_state_restore(&err_state);
+	cl_assert(strcmp("no error", git_error_last()->message) == 0);
 
+	git_error_restore(last_error);
 	cl_assert(git_error_last()->klass == GIT_ERROR_NOMEMORY);
 	cl_assert_(git_error_last() == oom_error, "static oom error not restored");