Simplify warning API pending further discussion

12 files changed, 75 insertions, 81 deletions

diff --git a/include/git2/common.h b/include/git2/common.h
index c6f3e2620..1dca8e837 100644
--- a/include/git2/common.h
+++ b/include/git2/common.h
@@ -136,8 +136,7 @@ typedef enum {
 	GIT_OPT_ENABLE_CACHING,
 	GIT_OPT_GET_CACHED_MEMORY,
 	GIT_OPT_GET_TEMPLATE_PATH,
-	GIT_OPT_SET_TEMPLATE_PATH,
-	GIT_OPT_SET_WARNING_CALLBACK,
+	GIT_OPT_SET_TEMPLATE_PATH
 } git_libgit2_opt_t;
 
 /**
@@ -223,10 +222,6 @@ typedef enum {
  *		>
  *		> - `path` directory of template.
  *
- *	* opts(GIT_OPT_SET_WARNING_CALLBACK, git_warning_callback cb, void *payload)
- *
- *		> Set callback for warnings (i.e. recoverable data problems)
- *
  * @param option Option key
  * @param ... value to set the option
  * @return 0 on success, <0 on failure
diff --git a/include/git2/sys/warning.h b/include/git2/sys/warning.h
index 5cea3035c..1b833ef4b 100644
--- a/include/git2/sys/warning.h
+++ b/include/git2/sys/warning.h
@@ -9,32 +9,40 @@
 
 GIT_BEGIN_DECL
 
+typedef enum {
+	GIT_WARNING_NONE = 0,
+	GIT_WARNING_INVALID_SIGNATURE_TIMESTAMP,
+	GIT_WARNING_INVALID_SIGNATURE_TIMEZONE,
+} git_warning_t;
+
 /**
  * Type for warning callbacks.
  *
- * Using `git_libgit2_opts(GIT_OPT_SET_WARNING_CALLBACK, ...)` you can set
- * a warning callback function (and payload) that will be used to issue
- * various warnings when recoverable data problems are encountered inside
- * libgit2.  It will be passed several parameters describing the problem.
+ * Using `git_warning_set_callback(cb, payload)` you can set a warning
+ * callback function (and payload) that will be used to issue various
+ * warnings when recoverable data problems are encountered inside libgit2.
+ * It will be passed several parameters describing the problem.
  *
+ * @param warning A git_warning_t value for the specific situation
+ * @param message A message explaining the details of the warning
  * @param payload The payload set when callback function was specified
- * @param klass The git_error_t value describing the module issuing the warning
- * @param message The message with the specific warning being issued
- * @param repo The repository involved (may be NULL if problem is in a
- *      system config file, not a repo config file)
- * @param otype The type of object with bad data if applicable - GIT_OBJ_ANY
- *		will be used if none of the other types actually apply
- * @param object The object and/or raw data involved which will vary depending
- *		on the specific warning message being issued
  * @return 0 to continue, <0 to convert the warning to an error
  */
 typedef int (*git_warning_callback)(
-	void *payload,
-	git_error_t klass,
+	git_warning_t warning,
 	const char *message,
-	git_repository *repo,
-	git_otype otype,
-	const void *object);
+	void *payload);
+
+/**
+ * Set the callback to be invoked when an invalid but recoverable
+ * scenario occurs.
+ *
+ * @param callback The git_warning_callback to be invoked
+ * @param payload The payload parameter for the callback function
+ */
+GIT_EXTERN(void) git_warning_set_callback(
+	git_warning_callback callback,
+	void *payload);
 
 GIT_END_DECL
 
diff --git a/src/commit.c b/src/commit.c
index a23e7087d..255debe82 100644
--- a/src/commit.c
+++ b/src/commit.c
@@ -274,7 +274,6 @@ int git_commit_amend(
 int git_commit__parse(void *_commit, git_odb_object *odb_obj)
 {
 	git_commit *commit = _commit;
-	git_repository *repo = git_commit_owner(commit);
 	const char *buffer_start = git_odb_object_data(odb_obj), *buffer;
 	const char *buffer_end = buffer_start + git_odb_object_size(odb_obj);
 	git_oid parent_id;
@@ -304,16 +303,14 @@ int git_commit__parse(void *_commit, git_odb_object *odb_obj)
 	commit->author = git__malloc(sizeof(git_signature));
 	GITERR_CHECK_ALLOC(commit->author);
 
-	if (git_signature__parse(
-		commit->author, &buffer, buffer_end, "author ", '\n', repo) < 0)
+	if (git_signature__parse(commit->author, &buffer, buffer_end, "author ", '\n') < 0)
 		return -1;
 
 	/* Always parse the committer; we need the commit time */
 	commit->committer = git__malloc(sizeof(git_signature));
 	GITERR_CHECK_ALLOC(commit->committer);
 
-	if (git_signature__parse(
-		commit->committer, &buffer, buffer_end, "committer ", '\n', repo) < 0)
+	if (git_signature__parse(commit->committer, &buffer, buffer_end, "committer ", '\n') < 0)
 		return -1;
 
 	/* Parse add'l header entries */
diff --git a/src/refdb_fs.c b/src/refdb_fs.c
index 671029e0c..bb26fb66d 100644
--- a/src/refdb_fs.c
+++ b/src/refdb_fs.c
@@ -1274,9 +1274,7 @@ static int reflog_parse(git_reflog *log, const char *buf, size_t buf_size)
 		while (*buf && *buf != '\t' && *buf != '\n')
 			seek_forward(1);
 
-		if (git_signature__parse(
-				entry->committer, &ptr, buf + 1, NULL, *buf,
-				log && log->db ? log->db->repo : NULL) < 0)
+		if (git_signature__parse(entry->committer, &ptr, buf + 1, NULL, *buf) < 0)
 			goto fail;
 
 		if (*buf == '\t') {
diff --git a/src/settings.c b/src/settings.c
index de1df1fa8..9308f94ec 100644
--- a/src/settings.c
+++ b/src/settings.c
@@ -123,13 +123,6 @@ int git_libgit2_opts(int key, ...)
 	case GIT_OPT_SET_TEMPLATE_PATH:
 		error = git_sysdir_set(GIT_SYSDIR_TEMPLATE, va_arg(ap, const char *));
 		break;
-
-	case GIT_OPT_SET_WARNING_CALLBACK:
-		{
-			git_warning_callback cb = va_arg(ap, git_warning_callback);
-			void *payload = va_arg(ap, void *);
-			git_warning_set_callback(cb, payload);
-		}
 	}
 
 	va_end(ap);
diff --git a/src/signature.c b/src/signature.c
index 386fe2deb..dbaa9bd17 100644
--- a/src/signature.c
+++ b/src/signature.c
@@ -157,8 +157,7 @@ int git_signature_default(git_signature **out, git_repository *repo)
 
 int git_signature__parse(
 	git_signature *sig, const char **buffer_out,
-	const char *buffer_end, const char *header, char ender,
-	git_repository *repo)
+	const char *buffer_end, const char *header, char ender)
 {
 	const char *buffer = *buffer_out;
 	const char *email_start, *email_end;
@@ -193,17 +192,18 @@ int git_signature__parse(
 		const char *time_end;
 
 		if (git__strtol64(&sig->when.time, time_start, &time_end, 10) < 0) {
+			/* set timestamp to max value */
+			sig->when.time = (uint64_t)-1L;
+
+			/* skip over invalid timestamp data */
+			time_end = git__skip_over_to_space(time_start);
+
+			/* warn (and return error if requested) */
 			if (git_warning(
-					GITERR_OBJECT, repo, GIT_OBJ_ANY, time_start,
-					"invalid timestamp") < 0)
+					GIT_WARNING_INVALID_SIGNATURE_TIMESTAMP,
+					"invalid signature timestamp '%.*s'",
+					(int)(time_end - time_start), time_start) < 0)
 				return signature_error("invalid Unix timestamp");
-			else {
-				sig->when.time = (uint64_t)-1L;
-				/* skip over invalid timestamp data */
-				time_end = time_start;
-				while (git__isspace(*time_end)) ++time_end;
-				while (!git__isspace(*time_end)) ++time_end;
-			}
 		}
 
 		/* do we have a timezone? */
@@ -215,12 +215,19 @@ int git_signature__parse(
 
 			if ((tz_start[0] != '-' && tz_start[0] != '+') ||
 				git__strtol32(&offset, tz_start + 1, &tz_end, 10) < 0) {
+
+				/* set offset to inoffensive value */
+				offset = 0;
+
+				/* skip over invalid offset data */
+				tz_end = git__skip_over_to_space(tz_start);
+
+				/* warn (and return error if requested) */
 				if (git_warning(
-						GITERR_OBJECT, repo, GIT_OBJ_ANY, tz_start,
-						"invalid timezone") < 0)
+						GIT_WARNING_INVALID_SIGNATURE_TIMEZONE,
+						"invalid signature timezone '%.*s'",
+						(int)(tz_end - tz_start), tz_start) < 0)
 					return signature_error("invalid timezone");
-				else
-					offset = 0;
 			}
 
 			hours = offset / 100;
diff --git a/src/signature.h b/src/signature.h
index 5a09bb8e4..a3bfa3599 100644
--- a/src/signature.h
+++ b/src/signature.h
@@ -13,10 +13,8 @@
 #include <time.h>
 
 int git_signature__parse(
-	git_signature *sig,
-	const char **buffer_out, const char *buffer_end,
-	const char *header, char ender,
-	git_repository *repo);
+	git_signature *sig, const char **buffer_out, const char *buffer_end,
+	const char *header, char ender);
 
 void git_signature__writebuf(
 	git_buf *buf, const char *header, const git_signature *sig);
diff --git a/src/tag.c b/src/tag.c
index ee5e8db62..5abfb90ce 100644
--- a/src/tag.c
+++ b/src/tag.c
@@ -130,9 +130,7 @@ static int tag_parse(git_tag *tag, const char *buffer, const char *buffer_end)
 		tag->tagger = git__malloc(sizeof(git_signature));
 		GITERR_CHECK_ALLOC(tag->tagger);
 
-		if (git_signature__parse(
-				tag->tagger, &buffer, buffer_end,
-				"tagger ", '\n', git_tag_owner(tag)) < 0)
+		if (git_signature__parse(tag->tagger, &buffer, buffer_end, "tagger ", '\n') < 0)
 			return -1;
 	}
 
diff --git a/src/util.h b/src/util.h
index e378786d9..d34c657fd 100644
--- a/src/util.h
+++ b/src/util.h
@@ -310,6 +310,13 @@ GIT_INLINE(bool) git__iswildcard(int c)
 	return (c == '*' || c == '?' || c == '[');
 }
 
+GIT_INLINE(const char *) git__skip_over_to_space(const char *str)
+{
+	while (git__isspace(*str)) ++str;
+	while (*str && !git__isspace(*str)) ++str;
+	return str;
+}
+
 /*
  * Parse a string value as a boolean, just like Core Git does.
  *
diff --git a/src/warning.c b/src/warning.c
index 1884e3cac..52b71f272 100644
--- a/src/warning.c
+++ b/src/warning.c
@@ -19,10 +19,7 @@ void git_warning_set_callback(git_warning_callback cb, void *payload)
 }
 
 int git_warning(
-	git_error_t klass,
-	git_repository *repo,
-	git_otype otype,
-	const void *object,
+	git_warning_t warning,
 	const char *fmt,
 	...)
 {
@@ -39,8 +36,7 @@ int git_warning(
 	va_end(arglist);
 
 	if (!error)
-		error = cb(
-			_warning_payload, klass, git_buf_cstr(&buf), repo, otype, object);
+		error = cb(warning, git_buf_cstr(&buf), _warning_payload);
 
 	git_buf_free(&buf);
 
diff --git a/src/warning.h b/src/warning.h
index 86fc30c6b..dc9308c74 100644
--- a/src/warning.h
+++ b/src/warning.h
@@ -10,13 +10,8 @@
 #include "common.h"
 #include "git2/sys/warning.h"
 
-extern void git_warning_set_callback(git_warning_callback cb, void *payload);
-
 extern int git_warning(
-	git_error_t klass,
-	git_repository *repo,
-	git_otype otype,
-	const void *object,
+	git_warning_t warning,
 	const char *fmt,
 	...);
 
diff --git a/tests/commit/parse.c b/tests/commit/parse.c
index 416b7b033..8bd261c03 100644
--- a/tests/commit/parse.c
+++ b/tests/commit/parse.c
@@ -147,7 +147,7 @@ static void assert_signature_parses(passing_signature_test_case *passcase)
 	size_t len = strlen(passcase->string);
 	struct git_signature person = {0};
 
-	cl_git_pass(git_signature__parse(&person, &str, str + len, passcase->header, '\n', NULL));
+	cl_git_pass(git_signature__parse(&person, &str, str + len, passcase->header, '\n'));
 	cl_assert_equal_s(passcase->name, person.name);
 	cl_assert_equal_s(passcase->email, person.email);
 	cl_assert_equal_sz(passcase->time, person.when.time);
@@ -161,7 +161,7 @@ static void assert_signature_doesnt_parse(failing_signature_test_case *failcase)
 	size_t len = strlen(failcase->string);
 	git_signature person = {0};
 
-	cl_git_fail(git_signature__parse(&person, &str, str + len, failcase->header, '\n', NULL));
+	cl_git_fail(git_signature__parse(&person, &str, str + len, failcase->header, '\n'));
 	git__free(person.name); git__free(person.email);
 }
 
@@ -177,24 +177,26 @@ void test_commit_parse__signature(void)
 		assert_signature_doesnt_parse(failcase);
 }
 
-static int fail_on_warn(
-	void *p, git_error_t k, const char *m, git_repository *r, git_otype t, const void *o)
+static int fail_on_warn(git_warning_t w, const char *m, void *p)
 {
-	GIT_UNUSED(p); GIT_UNUSED(k); GIT_UNUSED(m); GIT_UNUSED(r); GIT_UNUSED(t); GIT_UNUSED(o);
+	GIT_UNUSED(w); GIT_UNUSED(m); GIT_UNUSED(p);
 	return -1;
 }
 
 void test_commit_parse__signature_semivalid(void)
 {
 	passing_signature_test_case passcase = {"author Vicent Marti <tanoku@gmail.com> 9999999999998589934592 \n", "author ", "Vicent Marti", "tanoku@gmail.com", -1, 0};
-	failing_signature_test_case failcase = {"author Vicent Marti <tanoku@gmail.com> 9999999999998589934592 \n", "author "};
+	failing_signature_test_case failcase1 = {"author Vicent Marti <tanoku@gmail.com> 9999999999998589934592 \n", "author "};
+	failing_signature_test_case failcase2 = {"author Vicent Marti <tanoku@gmail.com> 998589934592 +123412341234123412341234 \n", "author "};
 
 	assert_signature_parses(&passcase);
 
-	git_libgit2_opts(GIT_OPT_SET_WARNING_CALLBACK, fail_on_warn, NULL);
-	assert_signature_doesnt_parse(&failcase);
+	git_warning_set_callback(fail_on_warn, NULL);
 
-	git_libgit2_opts(GIT_OPT_SET_WARNING_CALLBACK, NULL, NULL);
+	assert_signature_doesnt_parse(&failcase1);
+	assert_signature_doesnt_parse(&failcase2);
+
+	git_warning_set_callback(NULL, NULL);
 }