Gather the reflog entry content tests #4298

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

pks-t merged 5 commits into libgit2:master from tiennou:gather-reflog-messages-tests

Nov 24, 2017

Contributor

tiennou commented Jul 7, 2017

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

Contributor Author

tiennou commented Jul 7, 2017

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

Member

pks-t commented Jul 10, 2017

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 force-pushed the gather-reflog-messages-tests branch 2 times, most recently from d41c3a0 to fe92535 Compare

July 17, 2017 20:41

Contributor Author

tiennou commented Jul 17, 2017

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

pks-t requested changes

View reviewed changes

Member

pks-t left a comment

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

tests/refs/reflog/reflog_helpers.c

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

Member

pks-t Jul 21, 2017

git_buf_free missing here for the result buffer

Contributor Author

tiennou Jul 21, 2017

Fixed

tests/refs/reflog/reflog_helpers.c Outdated

+              				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);

Member

pks-t Jul 21, 2017

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.

Member

pks-t Jul 21, 2017

And above, as well.

Contributor Author

tiennou Jul 21, 2017

Looks better, thanks !

tests/refs/reflog/reflog_helpers.c Outdated

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

Member

pks-t Jul 21, 2017

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

tests/refs/reflog/reflog_helpers.c Outdated

-              		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));

Member

pks-t Jul 21, 2017

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.

Contributor Author

tiennou Jul 21, 2017

Fixed.

tests/refs/reflog/reflog_helpers.c Outdated

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

Member

pks-t Jul 21, 2017

Should this comparison really be case-insensitive?

Contributor Author

tiennou Jul 21, 2017

It should not. Fixed the other occurrence below too.

tests/refs/reflog/reflog_helpers.c

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

Member

pks-t Jul 21, 2017

No breaces required around single-line statements.

tests/refs/reflog/messages.c Outdated

               	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,

Member

pks-t Jul 21, 2017

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.

Contributor Author

tiennou Jul 21, 2017

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

tiennou force-pushed the gather-reflog-messages-tests branch from fe92535 to 9a810e8 Compare

July 21, 2017 17:52

Contributor Author

tiennou commented Jul 21, 2017

Rebased, squashing review fixups inside their respective commits.

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


          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

Contributor Author

tiennou commented Nov 14, 2017

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

pks-t requested changes

View reviewed changes

Member

pks-t left a comment

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

tests/refs/reflog/messages.c

+              	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)));

Member

pks-t Nov 21, 2017

The target is leaking.

tests/refs/reflog/messages.c

+              	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);

Member

pks-t Nov 21, 2017

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).

tests/refs/reflog/messages.c Outdated

+              	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));

Member

pks-t Nov 21, 2017

branch is also leaking


          tests: move free functions at the end

da635ed

Contributor Author

tiennou commented Nov 22, 2017

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

pks-t approved these changes

View reviewed changes

pks-t merged commit 59ffb51 into libgit2:master

Member

pks-t commented Nov 24, 2017

Thanks a lot for this, @tiennou!

tiennou deleted the gather-reflog-messages-tests branch

November 26, 2017 17:59

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet