Gather the reflog entry content tests by tiennou · Pull Request #4298 · libgit2/libgit2

tiennou · 2017-07-07T17:15:50Z

This is is splitted from #4294 with just the test case wrangling.

tiennou · 2017-07-07T17:17:46Z

@pks-t I took care of your review comments, apart from the header thing : AFAICT we don't have headers on test files.

pks-t · 2017-07-10T06:36:44Z

Two issues:

there's one use of C99-style comments in "tests/refs/reflog/messages.c", which we usually avoid
there's a declaration after code in reflog_print

Looks fine otherwise, thanks!

tiennou · 2017-07-17T23:52:18Z

Good to go ! Travis tripped up on proxy things. Again 😉.

pks-t

Thanks for your adjustments and sorry for another batch of change requests 😅

pks-t · 2017-07-21T07:19:14Z

+		clar__fail(file, line, "Reflog entry mismatch", git_buf_cstr(&result), 1);
 	}

 	git_reflog_free(log);


git_buf_free missing here for the result buffer

pks-t · 2017-07-21T07:24:55Z

+				git_oid_fmt(&err[1], git_object_id(obj));
+				git_oid_fmt(&err[47], git_reflog_entry_id_new(entry));
+
+				git_buf_printf(&result, "\tNew OID: %s\n", err);


Huh, this seems a bit fragile. How about

git_oid__writebuf(&result, "\tNew OID: \"", git_object_id(obj)); git_oid__writebuf(&result, "\" != \", git_reflog_entry_id_new(entry)); git_buf_puts(&result, '"\n');

This would save us from the hardcoded indices. Same applies below.

And above, as well.

Looks better, thanks !

pks-t · 2017-07-21T07:26:34Z

+	}
+	if (git_buf_len(&result) != 0) {
+		clar__fail(file, line, "Reflog entry mismatch", git_buf_cstr(&result), 1);
 	}


No need for braces around single-line statements (lines 85 to 98)

pks-t · 2017-07-21T07:27:57Z

-		git_buf_printf(&buf, "%ld: %s %s %s %s\n", idx, old_oid, new_oid, "somesig", git_reflog_entry_message(entry));
-		fprintf(stderr, "%s", git_buf_cstr(&buf));
-		git_buf_free(&buf);
+		fprintf(stderr, "%s\n", reflog_entry_tostr(entry));


This is a memory leak as reflog_entry_tostr returns a string detached from its buffer. I would instead propose an interface int reflog_entry_tostr(git_buf *out, const git_reflog_entry *entry), which makes the need for memory management more obvious.

pks-t · 2017-07-21T07:28:44Z


-	if (email) {
-		cl_assert_equal_s(email, git_reflog_entry_committer(entry)->email);
+	if (email && strcasecmp(email, git_reflog_entry_committer(entry)->email) != 0) {


Should this comparison really be case-insensitive?

It should not. Fixed the other occurrence below too.

pks-t · 2017-07-21T07:29:34Z

+	if (entry == NULL) {
+		clar__fail(file, line, "Reflog has no such entry", NULL, 1);
+	}



No breaces required around single-line statements.

pks-t · 2017-07-21T07:31:09Z

 	cl_git_pass(git_repository_set_head(g_repo, "refs/tags/test"));            /* 1 */
 	cl_git_pass(git_repository_set_head(g_repo, "refs/remotes/test/master"));  /* 0 */

-	reflog_check_entry(g_repo, GIT_HEAD_FILE, 4,


I'd prefer if these whitespace changes were put inside another commit, which only changes indentantion. Furthermore, instead of adding more indentation, I'd break with our usual style here and decrease indentation to only one tab on the newline. Otherwise, lines get very long and the statements become quite unreadable.

Good idea. Folded the indentation change inside the "gathering" step.

tiennou · 2017-07-21T17:55:37Z

Rebased, squashing review fixups inside their respective commits.

tiennou · 2017-11-14T23:38:20Z

@pks-t Just rebased, ready when you are 😉.

pks-t

Just two small memory leaks. Otherwise this looks good to go for me. 👍

pks-t · 2017-11-21T13:27:03Z

+	git_reference *branch;
+
+	cl_git_pass(git_revparse_single(&obj, g_repo, "e90810b8df3"));
+	cl_git_pass(git_commit_lookup(&target, g_repo, git_object_id(obj)));


The target is leaking.

pks-t · 2017-11-21T13:27:50Z

+
+	cl_git_pass(git_revparse_single(&obj, g_repo, "e90810b8df3"));
+	cl_git_pass(git_commit_lookup(&target, g_repo, git_object_id(obj)));
+	git_object_free(obj);


I'd prefer if you moved all freeing functions to the end. It makes it a lot easier to see if everything has been taken care of by just comparing the list of declarations and free's at the end (at least in most cases).

pks-t · 2017-11-21T13:28:10Z

+	git_buf_clear(&buf);
+
+	cl_git_pass(git_annotated_commit_from_revspec(&annotated, g_repo, "e90810b8df3"));
+	cl_git_pass(git_branch_create_from_annotated(&branch, g_repo, NEW_BRANCH_NAME, annotated, true));


branch is also leaking

tiennou · 2017-11-22T22:51:01Z

Fixed. I had to append some numbers so there was no variable reused, but I like how clearer it makes those lifetimes.

pks-t · 2017-11-24T10:37:25Z

Thanks a lot for this, @tiennou!

tiennou force-pushed the gather-reflog-messages-tests branch 2 times, most recently from d41c3a0 to fe92535 Compare July 17, 2017 20:41

pks-t requested changes Jul 21, 2017

View reviewed changes

tiennou force-pushed the gather-reflog-messages-tests branch from fe92535 to 9a810e8 Compare July 21, 2017 17:52

tiennou force-pushed the gather-reflog-messages-tests branch from b653e27 to 2ec6668 Compare July 21, 2017 20:02

tiennou force-pushed the gather-reflog-messages-tests branch from 2ec6668 to 6820799 Compare September 16, 2017 22:05

tiennou added 4 commits November 15, 2017 00:36

tests: gather the reflog entry content tests

5367212

tests: clarify which steps corresponds to each checks

b9b1f9f

tests: make reflog_check_entry more clar-y

f01a858

tests: move static method to the top

ebe5d8e

tiennou force-pushed the gather-reflog-messages-tests branch from 6820799 to ebe5d8e Compare November 14, 2017 23:37

pks-t requested changes Nov 21, 2017

View reviewed changes

tests: move free functions at the end

da635ed

pks-t approved these changes Nov 24, 2017

View reviewed changes

pks-t merged commit 59ffb51 into libgit2:master Nov 24, 2017

tiennou deleted the gather-reflog-messages-tests branch November 26, 2017 17:59

Uh oh!

Conversation

tiennou commented Jul 7, 2017

Uh oh!

tiennou commented Jul 7, 2017

Uh oh!

pks-t commented Jul 10, 2017

Uh oh!

tiennou commented Jul 17, 2017

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiennou commented Jul 21, 2017

Uh oh!

tiennou commented Nov 14, 2017

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiennou commented Nov 22, 2017

Uh oh!

pks-t commented Nov 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants