Warnings that default to being errors
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.
diff --git a/include/git2/sys/warning.h b/include/git2/sys/warning.h
index ec1262a..02adfbe 100644
--- a/include/git2/sys/warning.h
+++ b/include/git2/sys/warning.h
@@ -11,8 +11,10 @@
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 @@
* 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 a513b0e..a57d9c4 100644
--- a/src/signature.c
+++ b/src/signature.c
@@ -177,17 +177,38 @@
}
email_start = git__memrchr(buffer, '<', buffer_end - buffer);
- email_end = git__memrchr(buffer, '>', buffer_end - buffer);
+ 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");
- if (!email_start || !email_end || email_end <= email_start)
- return signature_error("malformed 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 @@
/* 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 @@
/* 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 1066c2c..b0bc7c0 100644
--- a/src/warning.c
+++ b/src/warning.c
@@ -19,18 +19,18 @@
}
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 @@
int git_warn(
git_warning_t type,
+ int default_rval,
const char *fmt,
...)
{
@@ -48,12 +49,12 @@
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_invalid_data(
git_warning_t type,
+ int default_rval,
const char *data,
int datalen,
const char *fmt,
@@ -71,7 +73,7 @@
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 @@
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 6d25783..8c14e28 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 07f304c..ff44648 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 @@
}
-// Header parsing
+/* Header parsing */
typedef struct {
const char *line;
const char *header;
@@ -70,7 +70,7 @@
}
-// Signature parsing
+/* Signature parsing */
typedef struct {
const char *string;
const char *header;
@@ -89,27 +89,27 @@
{"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 @@
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 @@
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 @@
}
-// 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 @@
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);
