reflog: Fix reflog writer/reader

 - Use a space to separate oids and signature
 - Enforce test coverage
 - Make test run in a temporary folder in order not to alter the test repository

8 files changed, 64 insertions, 48 deletions

diff --git a/src/commit.c b/src/commit.c
index fc4848733..a6c19e596 100644
--- a/src/commit.c
+++ b/src/commit.c
@@ -209,12 +209,12 @@ int commit_parse_buffer(git_commit *commit, const void *data, size_t len)
 	}
 
 	commit->author = git__malloc(sizeof(git_signature));
-	if ((error = git_signature__parse(commit->author, &buffer, buffer_end, "author ")) < GIT_SUCCESS)
+	if ((error = git_signature__parse(commit->author, &buffer, buffer_end, "author ", '\n')) < GIT_SUCCESS)
 		return git__rethrow(error, "Failed to parse buffer");
 
 	/* Always parse the committer; we need the commit time */
 	commit->committer = git__malloc(sizeof(git_signature));
-	if ((error = git_signature__parse(commit->committer, &buffer, buffer_end, "committer ")) < GIT_SUCCESS)
+	if ((error = git_signature__parse(commit->committer, &buffer, buffer_end, "committer ", '\n')) < GIT_SUCCESS)
 		return git__rethrow(error, "Failed to parse buffer");
 
 	/* parse commit message */
diff --git a/src/reflog.c b/src/reflog.c
index ce88c101e..da61fd7d6 100644
--- a/src/reflog.c
+++ b/src/reflog.c
@@ -74,9 +74,11 @@ static int reflog_write(git_repository *repo, const char *ref_name,
 		return git__throw(GIT_ERROR, "Failed to write reflog. `%s` is directory", log_path);
 
 	git_buf_puts(&log, oid_old);
+	git_buf_putc(&log, ' ');
+
 	git_buf_puts(&log, oid_new);
 
-	git_signature__writebuf(&log, NULL, committer);
+	git_signature__writebuf(&log, " ", committer);
 	log.size--; /* drop LF */
 
 	if (msg) {
@@ -122,10 +124,10 @@ static int reflog_parse(git_reflog *log, const char *buf, size_t buf_size)
 			return GIT_ENOMEM;
 
 		entry->oid_old = git__strndup(buf, GIT_OID_HEXSZ);
-		seek_forward(GIT_OID_HEXSZ+1);
+		seek_forward(GIT_OID_HEXSZ + 1);
 
 		entry->oid_cur = git__strndup(buf, GIT_OID_HEXSZ);
-		seek_forward(GIT_OID_HEXSZ+1);
+		seek_forward(GIT_OID_HEXSZ + 1);
 
 		ptr = buf;
 
@@ -137,7 +139,7 @@ static int reflog_parse(git_reflog *log, const char *buf, size_t buf_size)
 		if (entry->committer == NULL)
 			return GIT_ENOMEM;
 
-		if ((error = git_signature__parse(entry->committer, &ptr, buf + buf_size, NULL)) < GIT_SUCCESS)
+		if ((error = git_signature__parse(entry->committer, &ptr, buf + 1, NULL, *buf)) < GIT_SUCCESS)
 			goto cleanup;
 
 		if (*buf == '\t') {
diff --git a/src/signature.c b/src/signature.c
index d7c1b2d3e..cc55d1dc7 100644
--- a/src/signature.c
+++ b/src/signature.c
@@ -253,7 +253,7 @@ int parse_time(git_time_t *time_out, const char *buffer)
 }
 
 int git_signature__parse(git_signature *sig, const char **buffer_out,
-		const char *buffer_end, const char *header)
+		const char *buffer_end, const char *header, char ender)
 {
 	const char *buffer = *buffer_out;
 	const char *line_end, *name_end, *email_end, *tz_start, *time_start;
@@ -261,7 +261,7 @@ int git_signature__parse(git_signature *sig, const char **buffer_out,
 
 	memset(sig, 0x0, sizeof(git_signature));
 
-	if ((line_end = memchr(buffer, '\n', buffer_end - buffer)) == NULL)
+	if ((line_end = memchr(buffer, ender, buffer_end - buffer)) == NULL)
 		return git__throw(GIT_EOBJCORRUPTED, "Failed to parse signature. No newline given");
 
 	if (header) {
diff --git a/src/signature.h b/src/signature.h
index c2e7e7815..2fe6cd7c9 100644
--- a/src/signature.h
+++ b/src/signature.h
@@ -6,7 +6,7 @@
 #include "repository.h"
 #include <time.h>
 
-int git_signature__parse(git_signature *sig, const char **buffer_out, const char *buffer_end, const char *header);
+int git_signature__parse(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);
 
 #endif
diff --git a/src/tag.c b/src/tag.c
index 92b30ba87..e2b0cf438 100644
--- a/src/tag.c
+++ b/src/tag.c
@@ -144,7 +144,7 @@ static int parse_tag_buffer(git_tag *tag, const char *buffer, const char *buffer
 	if (tag->tagger == NULL)
 		return GIT_ENOMEM;
 
-	if ((error = git_signature__parse(tag->tagger, &buffer, buffer_end, "tagger ")) != 0) {
+	if ((error = git_signature__parse(tag->tagger, &buffer, buffer_end, "tagger ", '\n')) != 0) {
 		free(tag->tag_name);
 		git_signature_free(tag->tagger);
 		return git__rethrow(error, "Failed to parse tag");
diff --git a/tests/t04-commit.c b/tests/t04-commit.c
index a0d24dab1..1c390824a 100644
--- a/tests/t04-commit.c
+++ b/tests/t04-commit.c
@@ -157,7 +157,7 @@ BEGIN_TEST(parse1, "parse the signature line in a commit")
 	const char *ptr = _string; \
 	size_t len = strlen(_string);\
 	git_signature person = {NULL, NULL, {0, 0}}; \
-	must_pass(git_signature__parse(&person, &ptr, ptr + len, _header));\
+	must_pass(git_signature__parse(&person, &ptr, ptr + len, _header, '\n'));\
 	must_be_true(strcmp(_name, person.name) == 0);\
 	must_be_true(strcmp(_email, person.email) == 0);\
 	must_be_true(_time == person.when.time);\
@@ -169,7 +169,7 @@ BEGIN_TEST(parse1, "parse the signature line in a commit")
 	const char *ptr = _string; \
 	size_t len = strlen(_string);\
 	git_signature person = {NULL, NULL, {0, 0}}; \
-	must_fail(git_signature__parse(&person, &ptr, ptr + len, _header));\
+	must_fail(git_signature__parse(&person, &ptr, ptr + len, _header, '\n'));\
 	free(person.name); free(person.email);\
 }
 
diff --git a/tests/t10-refs.c b/tests/t10-refs.c
index e004625bd..aab21dea8 100644
--- a/tests/t10-refs.c
+++ b/tests/t10-refs.c
@@ -997,17 +997,40 @@ BEGIN_TEST(list1, "try to list only the symbolic references")
 END_TEST
 
 static const char *new_ref = "refs/heads/test-reflog";
+#define commit_msg "commit: bla bla"
 
-BEGIN_TEST(reflog0, "write a reflog for a given reference")
-	git_repository *repo;
-	git_reference *ref;
+static int assert_signature(git_signature *expected, git_signature *actual)
+{
+	if (actual == NULL)
+		return GIT_ERROR;
+
+	if (strcmp(expected->name, actual->name) != 0)
+		return GIT_ERROR;
+
+	if (strcmp(expected->email, actual->email) != 0)
+		return GIT_ERROR;
+
+	if (expected->when.offset != actual->when.offset)
+		return GIT_ERROR;
+
+	if (expected->when.time != actual->when.time)
+		return GIT_ERROR;
+
+	return GIT_SUCCESS;
+}
+
+BEGIN_TEST(reflog0, "write a reflog for a given reference and ensure it can be read back")
+	git_repository *repo, *repo2;
+	git_reference *ref, *lookedup_ref;
 	git_oid oid;
 	git_signature *committer;
+	git_reflog *reflog;
+	git_reflog_entry *entry;
 
-	git_oid_fromstr(&oid, current_master_tip);
-
-	must_pass(git_repository_open(&repo, REPOSITORY_FOLDER));
+	must_pass(open_temp_repo(&repo, REPOSITORY_FOLDER));
 
+	/* Create a new branch pointing at the HEAD */
+	git_oid_fromstr(&oid, current_master_tip);
 	must_pass(git_reference_create_oid(&ref, repo, new_ref, &oid, 0));
 	must_pass(git_reference_lookup(&ref, repo, new_ref));
 
@@ -1015,43 +1038,36 @@ BEGIN_TEST(reflog0, "write a reflog for a given reference")
 
 	must_pass(git_reflog_write(ref, NULL, committer, NULL));
 	must_fail(git_reflog_write(ref, NULL, committer, "no\nnewline"));
-	must_pass(git_reflog_write(ref, &oid, committer, "commit: bla bla"));
+	must_pass(git_reflog_write(ref, &oid, committer, commit_msg));
 
 	git_repository_free(repo);
-END_TEST
 
-BEGIN_TEST(reflog1, "read a reflog for a given reference")
-	unsigned int i;
-	git_repository *repo;
-	git_reference *ref;
-	git_reflog *reflog;
-	git_reflog_entry *GIT_UNUSED(entry);
+	/* Reopen a new instance of the repository */
+	must_pass(git_repository_open(&repo2, TEMP_REPO_FOLDER));
 
-	must_pass(git_repository_open(&repo, REPOSITORY_FOLDER));
+	/* Lookup the preivously created branch */
+	must_pass(git_reference_lookup(&lookedup_ref, repo2, new_ref));
 
-	must_pass(git_reference_lookup(&ref, repo, new_ref));
+	/* Read and parse the reflog for this branch */
+	must_pass(git_reflog_read(&reflog, lookedup_ref));
+	must_be_true(reflog->entries.length == 2);
 
-	must_pass(git_reflog_read(&reflog, ref));
-
-	for (i=0; i<reflog->entries.length; ++i) {
-		entry = git_vector_get(&reflog->entries, i);
-		/*
-		fprintf(stderr, "\nold:  %s\n", entry->oid_old);
-		fprintf(stderr, "cur:  %s\n", entry->oid_cur);
-		fprintf(stderr, "name: %s\n", entry->committer->name);
-		fprintf(stderr, "mail: %s\n", entry->committer->email);
-		if (entry->msg)
-			fprintf(stderr, "msg:  %s\n", entry->msg);
-		*/
-	}
+	entry = (git_reflog_entry *)git_vector_get(&reflog->entries, 0);
+	must_pass(assert_signature(committer, entry->committer));
+	must_be_true(strcmp("0000000000000000000000000000000000000000", entry->oid_old) == 0);
+	must_be_true(strcmp(current_master_tip, entry->oid_cur) == 0);
+	must_be_true(entry->msg == NULL);
 
-	git_reflog_free(reflog);
+	entry = (git_reflog_entry *)git_vector_get(&reflog->entries, 1);
+	must_pass(assert_signature(committer, entry->committer));
+	must_be_true(strcmp(current_master_tip, entry->oid_old) == 0);
+	must_be_true(strcmp(current_master_tip, entry->oid_cur) == 0);
+	must_be_true(strcmp(commit_msg, entry->msg) == 0);
 
-	must_pass(git_reference_delete(ref));
-	git_repository_free(repo);
+	git_reflog_free(reflog);
+	close_temp_repo(repo2);
 END_TEST
 
-
 BEGIN_SUITE(refs)
 	ADD_TEST(readtag0);
 	ADD_TEST(readtag1);
@@ -1097,5 +1113,4 @@ BEGIN_SUITE(refs)
 	ADD_TEST(list1);
 
 	ADD_TEST(reflog0);
-	ADD_TEST(reflog1);
 END_SUITE
diff --git a/tests/t18-status.c b/tests/t18-status.c
index 3fdcce9c9..385de7b6c 100644
--- a/tests/t18-status.c
+++ b/tests/t18-status.c
@@ -209,5 +209,4 @@ BEGIN_SUITE(status)
 	ADD_TEST(statuscb0);
 	ADD_TEST(singlestatus0);
 	ADD_TEST(singlestatus1);
-END_SUITE
-
+END_SUITE
\ No newline at end of file