Warnings that default to being errorsrb/warnings-for-commit-headers

This is a try at extending the warning API to include warnings
that would be errors unless the user callback decides to demote
them to warnings.  This allows for relaxed parsing logic that will
default to strict behavior but can continue if possible.

5 files changed, 135 insertions, 51 deletions

diff --git a/include/git2/sys/warning.h b/include/git2/sys/warning.h
index ec1262a6b..02adfbe40 100644
--- a/include/git2/sys/warning.h
+++ b/include/git2/sys/warning.h
@@ -11,8 +11,10 @@ GIT_BEGIN_DECL
 
 typedef enum {
 	GIT_WARNING_NONE = 0,
-	GIT_WARNING_INVALID_DATA__SIGNATURE_TIMESTAMP,
-	GIT_WARNING_INVALID_DATA__SIGNATURE_TIMEZONE,
+	GIT_WARNING_INVALID_DATA__SIGNATURE_TIMESTAMP, /* default continue */
+	GIT_WARNING_INVALID_DATA__SIGNATURE_TIMEZONE,  /* default continue */
+	GIT_WARNING_INVALID_DATA__SIGNATURE_EMAIL_MISSING, /* default error */
+	GIT_WARNING_INVALID_DATA__SIGNATURE_EMAIL_UNTERMINATED, /* error */
 } git_warning_t;
 
 /**
@@ -42,10 +44,13 @@ typedef struct {
  * It will be passed a warning structure describing the problem.
  *
  * @param warning A git_warning structure for the specific situation
+ * @param default_rval Default return code (i.e. <0 if warning defaults
+ *                     to error, 0 if defaults to no error)
  * @param payload The payload set when callback function was specified
  * @return 0 to continue, <0 to convert the warning to an error
  */
-typedef int (*git_warning_callback)(git_warning *warning, void *payload);
+typedef int (*git_warning_callback)(
+	git_warning *warning, int default_rval, void *payload);
 
 /**
  * Set the callback to be invoked when an invalid but recoverable
diff --git a/src/signature.c b/src/signature.c
index a513b0e65..a57d9c4ac 100644
--- a/src/signature.c
+++ b/src/signature.c
@@ -177,17 +177,38 @@ int git_signature__parse(
 	}
 
 	email_start = git__memrchr(buffer, '<', buffer_end - buffer);
-	email_end = git__memrchr(buffer, '>', buffer_end - buffer);
-
-	if (!email_start || !email_end || email_end <= email_start)
-		return signature_error("malformed e-mail");
+	if (!email_start) {
+		if (git_warn_invalid_data(
+				GIT_WARNING_INVALID_DATA__SIGNATURE_EMAIL_MISSING, -1,
+				buffer, (int)(buffer_end - buffer),
+				"missing signature %semail", header) < 0)
+			return signature_error("missing e-mail");
+
+		/* just stop now with everything as name */
+		sig->name = extract_trimmed(buffer, buffer_end - buffer);
+		sig->email = git__strdup("");
+		*buffer_out = buffer_end + 1;
+		return 0;
+	}
 
+	sig->name = extract_trimmed(buffer, email_start - buffer);
 	email_start += 1;
-	sig->name = extract_trimmed(buffer, email_start - buffer - 1);
-	sig->email = extract_trimmed(email_start, email_end - email_start);
+
+	email_end = git__memrchr(email_start, '>', buffer_end - email_start);
+	if (!email_end) {
+		if (git_warn_invalid_data(
+				GIT_WARNING_INVALID_DATA__SIGNATURE_EMAIL_UNTERMINATED, -1,
+				email_start, (int)(buffer_end - email_start),
+				"unterminated signature %semail", header) < 0)
+			return signature_error("malformed e-mail");
+
+		sig->email = extract_trimmed(email_start, buffer_end - email_start);
+	} else {
+		sig->email = extract_trimmed(email_start, email_end - email_start);
+	}
 
 	/* Do we even have a time at the end of the signature? */
-	if (email_end + 2 < buffer_end) {
+	if (email_end != NULL && email_end + 2 < buffer_end) {
 		const char *time_start = email_end + 2;
 		const char *time_end;
 
@@ -200,7 +221,7 @@ int git_signature__parse(
 
 			/* warn (and return error if requested) */
 			if (git_warn_invalid_data(
-					GIT_WARNING_INVALID_DATA__SIGNATURE_TIMESTAMP,
+					GIT_WARNING_INVALID_DATA__SIGNATURE_TIMESTAMP, 0,
 					time_start, (int)(time_end - time_start),
 					"invalid signature %stimestamp", header) < 0)
 				return signature_error("invalid Unix timestamp");
@@ -224,7 +245,7 @@ int git_signature__parse(
 
 				/* warn (and return error if requested) */
 				if (git_warn_invalid_data(
-						GIT_WARNING_INVALID_DATA__SIGNATURE_TIMEZONE,
+						GIT_WARNING_INVALID_DATA__SIGNATURE_TIMEZONE, 0,
 						tz_start, (int)(tz_end - tz_start),
 						"invalid timezone in signature %s", header) < 0)
 					return signature_error("invalid timezone");
diff --git a/src/warning.c b/src/warning.c
index 1066c2c8d..b0bc7c03d 100644
--- a/src/warning.c
+++ b/src/warning.c
@@ -19,18 +19,18 @@ void git_warning_set_callback(git_warning_callback cb, void *payload)
 }
 
 static int git_warning__send(
-	git_warning *warning, const char *fmt, va_list ap)
+	git_warning *warning, int default_rval, const char *fmt, va_list ap)
 {
 	int error = 0;
 	git_buf buf = GIT_BUF_INIT;
 	git_warning_callback cb = _warning_cb;
 
 	if (!cb)
-		return 0;
+		return default_rval;
 
 	if (!(error = git_buf_vprintf(&buf, fmt, ap))) {
 		warning->message = git_buf_cstr(&buf);
-		error = cb(warning, _warning_payload);
+		error = cb(warning, default_rval, _warning_payload);
 	}
 
 	git_buf_free(&buf);
@@ -40,6 +40,7 @@ static int git_warning__send(
 
 int git_warn(
 	git_warning_t type,
+	int default_rval,
 	const char *fmt,
 	...)
 {
@@ -48,12 +49,12 @@ int git_warn(
 	git_warning warning;
 
 	if (!_warning_cb)
-		return 0;
+		return default_rval;
 
 	warning.type = type;
 
 	va_start(ap, fmt);
-	error = git_warning__send(&warning, fmt, ap);
+	error = git_warning__send(&warning, default_rval, fmt, ap);
 	va_end(ap);
 
 	return error;
@@ -61,6 +62,7 @@ int git_warn(
 
 int git_warn_invalid_data(
 	git_warning_t type,
+	int default_rval,
 	const char *data,
 	int datalen,
 	const char *fmt,
@@ -71,7 +73,7 @@ int git_warn_invalid_data(
 	git_warning_invalid_data warning;
 
 	if (!_warning_cb)
-		return 0;
+		return default_rval;
 
 	warning.base.type = type;
 	warning.invalid_data = git__strndup(data, datalen);
@@ -79,7 +81,7 @@ int git_warn_invalid_data(
 	warning.invalid_data_len = datalen;
 
 	va_start(ap, fmt);
-	error = git_warning__send((git_warning *)&warning, fmt, ap);
+	error = git_warning__send((git_warning *)&warning, default_rval, fmt, ap);
 	va_end(ap);
 
 	git__free((char *)warning.invalid_data);
diff --git a/src/warning.h b/src/warning.h
index 6d25783f4..8c14e2869 100644
--- a/src/warning.h
+++ b/src/warning.h
@@ -10,13 +10,26 @@
 #include "common.h"
 #include "git2/sys/warning.h"
 
+/**
+ * Use this to raise a warning
+ *
+ * @param warning A git_warning_t code from include/git2/sys/warning.h
+ * @param default_rval Default return value (i.e. error code or zero)
+ * @param fmt Printf-style format string for warning message
+ * @return 0 to continue, less than 0 to raise error
+ */
 int git_warn(
 	git_warning_t warning,
+	int default_rval,
 	const char *fmt,
 	...);
 
+/**
+ * Raise a warning about invalid data, via a git_warning_invalid_data struct
+ */
 int git_warn_invalid_data(
 	git_warning_t warning,
+	int default_rval,
 	const char *data,
 	int datalen,
 	const char *fmt,
diff --git a/tests/commit/parse.c b/tests/commit/parse.c
index 07f304ce3..ff4464845 100644
--- a/tests/commit/parse.c
+++ b/tests/commit/parse.c
@@ -3,7 +3,7 @@
 #include "commit.h"
 #include "signature.h"
 
-// Fixture setup
+/* Fixture setup */
 static git_repository *g_repo;
 void test_commit_parse__initialize(void)
 {
@@ -15,7 +15,7 @@ void test_commit_parse__cleanup(void)
 }
 
 
-// Header parsing
+/* Header parsing */
 typedef struct {
 	const char *line;
 	const char *header;
@@ -70,7 +70,7 @@ void test_commit_parse__header(void)
 }
 
 
-// Signature parsing
+/* Signature parsing */
 typedef struct {
 	const char *string;
 	const char *header;
@@ -89,27 +89,27 @@ passing_signature_test_case passing_signature_cases[] = {
 	{"committer Vicent Marti <tanoku@gmail.com> 123456 +0000 \n", "committer ", "Vicent Marti", "tanoku@gmail.com", 123456, 0},
 	{"committer Vicent Marti <tanoku@gmail.com> 123456 +0100 \n", "committer ", "Vicent Marti", "tanoku@gmail.com", 123456, 60},
 	{"committer Vicent Marti <tanoku@gmail.com> 123456 -0100 \n", "committer ", "Vicent Marti", "tanoku@gmail.com", 123456, -60},
-	// Parse a signature without an author field
+	/* Parse a signature without an author field */
 	{"committer <tanoku@gmail.com> 123456 -0100 \n", "committer ", "", "tanoku@gmail.com", 123456, -60},
-	// Parse a signature without an author field
+	/* Parse a signature without an author field */
 	{"committer  <tanoku@gmail.com> 123456 -0100 \n", "committer ", "", "tanoku@gmail.com", 123456, -60},
-	// Parse a signature with an empty author field
+	/* Parse a signature with an empty author field */
 	{"committer   <tanoku@gmail.com> 123456 -0100 \n", "committer ", "", "tanoku@gmail.com", 123456, -60},
-	// Parse a signature with an empty email field
+	/* Parse a signature with an empty email field */
 	{"committer Vicent Marti <> 123456 -0100 \n", "committer ", "Vicent Marti", "", 123456, -60},
-	// Parse a signature with an empty email field
+	/* Parse a signature with an empty email field */
 	{"committer Vicent Marti < > 123456 -0100 \n", "committer ", "Vicent Marti", "", 123456, -60},
-	// Parse a signature with empty name and email
+	/* Parse a signature with empty name and email */
 	{"committer <> 123456 -0100 \n", "committer ", "", "", 123456, -60},
-	// Parse a signature with empty name and email
+	/* Parse a signature with empty name and email */
 	{"committer  <> 123456 -0100 \n", "committer ", "", "", 123456, -60},
-	// Parse a signature with empty name and email
+	/* Parse a signature with empty name and email */
 	{"committer  < > 123456 -0100 \n", "committer ", "", "", 123456, -60},
-	// Parse an obviously invalid signature
+	/* Parse an obviously invalid signature */
 	{"committer foo<@bar> 123456 -0100 \n", "committer ", "foo", "@bar", 123456, -60},
-	// Parse an obviously invalid signature
+	/* Parse an obviously invalid signature */
 	{"committer    foo<@bar> 123456 -0100 \n", "committer ", "foo", "@bar", 123456, -60},
-	// Parse an obviously invalid signature
+	/* Parse an obviously invalid signature */
 	{"committer <>\n", "committer ", "", "", 0, 0},
 	{"committer Vicent Marti <tanoku@gmail.com> 123456 -1500 \n", "committer ", "Vicent Marti", "tanoku@gmail.com", 123456, 0},
 	{"committer Vicent Marti <tanoku@gmail.com> 123456 +0163 \n", "committer ", "Vicent Marti", "tanoku@gmail.com", 123456, 0},
@@ -177,25 +177,27 @@ void test_commit_parse__signature(void)
 		assert_signature_doesnt_parse(failcase);
 }
 
-static int pass_on_warn(git_warning *warning, void *payload)
+static int pass_on_warn(git_warning *warning, int rval, void *payload)
 {
 	git_warning *expected = payload;
+	GIT_UNUSED(rval);
 	cl_assert_equal_i(expected->type, warning->type);
 	if (expected->message)
 		cl_assert(strstr(warning->message, expected->message) != NULL);
 	return 0;
 }
 
-static int fail_on_warn(git_warning *warning, void *payload)
+static int fail_on_warn(git_warning *warning, int rval, void *payload)
 {
 	git_warning *expected = payload;
+	GIT_UNUSED(rval);
 	cl_assert_equal_i(expected->type, warning->type);
 	if (expected->message)
 		cl_assert(strstr(warning->message, expected->message) != NULL);
 	return -1;
 }
 
-void test_commit_parse__signature_semivalid(void)
+void test_commit_parse__signature_time_warnings(void)
 {
 	git_warning expected = { 0 };
 	passing_signature_test_case passcase = {"author Vicent Marti <tanoku@gmail.com> 9999999999998589934592 \n", "author ", "Vicent Marti", "tanoku@gmail.com", -1, 0};
@@ -228,67 +230,108 @@ void test_commit_parse__signature_semivalid(void)
 	git_warning_set_callback(NULL, NULL);
 }
 
+static int default_on_warn(git_warning *warning, int rval, void *payload)
+{
+	git_warning *expected = payload;
+	cl_assert_equal_i(expected->type, warning->type);
+	if (expected->message)
+		cl_assert(strstr(warning->message, expected->message) != NULL);
+	return rval;
+}
+
+void test_commit_parse__signature_email_warnings(void)
+{
+	git_warning expected = { 0 };
+	passing_signature_test_case passcase1 = {"author Vicent Marti tanoku@gmail.com> 98589934592 \n", "author ", "Vicent Marti tanoku@gmail.com> 98589934592", "", 0, 0};
+	passing_signature_test_case passcase2 = {"committer Vicent Marti <tanoku@gmail.com 98589934592 \n", "committer ", "Vicent Marti", "tanoku@gmail.com 98589934592", 0, 0};
+
+	failing_signature_test_case failcase1 = {"author Vicent Marti tanoku@gmail.com> 98589934592 \n", "author "};
+	failing_signature_test_case failcase2 = {"committer Vicent Marti <tanoku@gmail.com 98589934592 \n", "author "};
+
+	git_warning_set_callback(default_on_warn, &expected);
+
+	expected.type = GIT_WARNING_INVALID_DATA__SIGNATURE_EMAIL_MISSING;
+	expected.message = "author";
+	assert_signature_doesnt_parse(&failcase1);
+
+	expected.type = GIT_WARNING_INVALID_DATA__SIGNATURE_EMAIL_UNTERMINATED;
+	expected.message = "committer";
+	assert_signature_doesnt_parse(&failcase2);
+
+	git_warning_set_callback(pass_on_warn, &expected);
+
+	expected.type = GIT_WARNING_INVALID_DATA__SIGNATURE_EMAIL_MISSING;
+	expected.message = "author";
+	assert_signature_parses(&passcase1);
+
+	expected.type = GIT_WARNING_INVALID_DATA__SIGNATURE_EMAIL_UNTERMINATED;
+	expected.message = "committer";
+	assert_signature_parses(&passcase2);
+
+	git_warning_set_callback(NULL, NULL);
+}
+
 
 static char *failing_commit_cases[] = {
-// empty commit
+/* empty commit */
 "",
-// random garbage
+/* random garbage */
 "asd97sa9du902e9a0jdsuusad09as9du098709aweu8987sd\n",
-// broken endlines 1
+/* broken endlines 1 */
 "tree f6c0dad3c7b3481caa9d73db21f91964894a945b\r\n\
 parent 05452d6349abcd67aa396dfb28660d765d8b2a36\r\n\
 author Vicent Marti <tanoku@gmail.com> 1273848544 +0200\r\n\
 committer Vicent Marti <tanoku@gmail.com> 1273848544 +0200\r\n\
 \r\n\
 a test commit with broken endlines\r\n",
-// broken endlines 2
+/* broken endlines 2 */
 "tree f6c0dad3c7b3481caa9d73db21f91964894a945b\
 parent 05452d6349abcd67aa396dfb28660d765d8b2a36\
 author Vicent Marti <tanoku@gmail.com> 1273848544 +0200\
 committer Vicent Marti <tanoku@gmail.com> 1273848544 +0200\
 \
 another test commit with broken endlines",
-// starting endlines
+/* starting endlines */
 "\ntree f6c0dad3c7b3481caa9d73db21f91964894a945b\n\
 parent 05452d6349abcd67aa396dfb28660d765d8b2a36\n\
 author Vicent Marti <tanoku@gmail.com> 1273848544 +0200\n\
 committer Vicent Marti <tanoku@gmail.com> 1273848544 +0200\n\
 \n\
 a test commit with a starting endline\n",
-// corrupted commit 1
+/* corrupted commit 1 */
 "tree f6c0dad3c7b3481caa9d73db21f91964894a945b\n\
 parent 05452d6349abcd67aa396df",
-// corrupted commit 2
+/* corrupted commit 2 */
 "tree f6c0dad3c7b3481caa9d73db21f91964894a945b\n\
 parent ",
-// corrupted commit 3
+/* corrupted commit 3 */
 "tree f6c0dad3c7b3481caa9d73db21f91964894a945b\n\
 parent ",
-// corrupted commit 4
+/* corrupted commit 4 */
 "tree f6c0dad3c7b3481caa9d73db21f91964894a945b\n\
 par",
 };
 
 
 static char *passing_commit_cases[] = {
-// simple commit with no message
+/* simple commit with no message */
 "tree 1810dff58d8a660512d4832e740f692884338ccd\n\
 author Vicent Marti <tanoku@gmail.com> 1273848544 +0200\n\
 committer Vicent Marti <tanoku@gmail.com> 1273848544 +0200\n\
 \n",
-// simple commit, no parent
+/* simple commit, no parent */
 "tree 1810dff58d8a660512d4832e740f692884338ccd\n\
 author Vicent Marti <tanoku@gmail.com> 1273848544 +0200\n\
 committer Vicent Marti <tanoku@gmail.com> 1273848544 +0200\n\
 \n\
 a simple commit which works\n",
-// simple commit, no parent, no newline in message
+/* simple commit, no parent, no newline in message */
 "tree 1810dff58d8a660512d4832e740f692884338ccd\n\
 author Vicent Marti <tanoku@gmail.com> 1273848544 +0200\n\
 committer Vicent Marti <tanoku@gmail.com> 1273848544 +0200\n\
 \n\
 a simple commit which works",
-// simple commit, 1 parent
+/* simple commit, 1 parent */
 "tree 1810dff58d8a660512d4832e740f692884338ccd\n\
 parent e90810b8df3e80c413d903f631643c716887138d\n\
 author Vicent Marti <tanoku@gmail.com> 1273848544 +0200\n\
@@ -367,7 +410,7 @@ void test_commit_parse__entire_commit(void)
 }
 
 
-// query the details on a parsed commit
+/* query the details on a parsed commit */
 void test_commit_parse__details0(void) {
 	static const char *commit_ids[] = {
 		"a4a7dce85cf63874e984719f4fdd239f5145052f", /* 0 */
@@ -415,7 +458,7 @@ void test_commit_parse__details0(void) {
 			old_parent = parent;
 			cl_git_pass(git_commit_parent(&parent, commit, p));
 			cl_assert(parent != NULL);
-			cl_assert(git_commit_author(parent) != NULL); // is it really a commit?
+			cl_assert(git_commit_author(parent) != NULL); /* is it really a commit? */
 		}
 		git_commit_free(old_parent);
 		git_commit_free(parent);