Fix: make reflog include "(merge)" for merge commits#4143
Fix: make reflog include "(merge)" for merge commits#4143pks-t merged 2 commits intolibgit2:masterfrom
Conversation
pks-t
left a comment
There was a problem hiding this comment.
The implementation looks good, despite the stylistic comment, thanks! 👍
There'd be extra kudos for you if you're willing to write some small tests for this behavior 😄
src/refs.c
Outdated
| return error; | ||
| } | ||
|
|
||
| const char *git_reference__commit_type(const git_commit *commit) |
There was a problem hiding this comment.
This function is used inside of this file only and will probably never be exposed to other callers, as it is very specific. As such, this function should be marked as static as well as drop the git_reference__ prefix.
There was a problem hiding this comment.
Thanks for the review, I will make these changes.
There was a problem hiding this comment.
Ahh, it will take me a little while longer to add some tests, I'll look into what's needed, thanks!
There was a problem hiding this comment.
Fixed style issues and added a test. Have verified that test fails on revert of this fix.
This fixes issue libgit2#4094
27598a7 to
7ea61c2
Compare
pks-t
left a comment
There was a problem hiding this comment.
Thanks for the test! :)
I think it'd be more suited inside the reflog tests, though, and could be simplified by removing the real merge logic as we only want to test if the reflog actually works correctly (which your test already does nicely).
tests/commit/merge.c
Outdated
| @@ -0,0 +1,66 @@ | |||
| #include "clar_libgit2.h" | |||
There was a problem hiding this comment.
I hate to be nitpicky, but I would have expected the test to be part of tests/refs/reflog/reflog.c. Sorry to not state this earlier
There was a problem hiding this comment.
No worries, it could be simpler I guess. I'll rework this later.
tests/commit/merge.c
Outdated
| cl_git_pass(git_index_read(index, 1)); | ||
| cl_assert_(!git_index_has_conflicts(index), "Index has conflicts!"); | ||
| cl_git_pass(git_index_write_tree(&tree_oid, index)); | ||
| cl_git_pass(git_tree_lookup(&tree, _repo, &tree_oid)); |
There was a problem hiding this comment.
What we really want to test is the reflog logic. So you could discard the "real" merge logic and simply create a merge commit re-using the tree of one of both commits. This would make the test a lot simpler and more to the point
tests/commit/merge.c
Outdated
|
|
||
| cl_git_pass(git_reflog_read(&log, _repo, "refs/heads/branchB-1")); | ||
| entry = git_reflog_entry_byindex(log, 0); | ||
| cl_assert_equal_s(merge_reflog_message, git_reflog_entry_message(entry)); |
There was a problem hiding this comment.
You unfortunately have to clean up all allocated memory after the test ;)
This test ensures that the string '(merge)' is included in the reflog when a merge commit is made.
7ea61c2 to
397cf1a
Compare
|
Looks good to me now! Thanks again for working on it |
|
Thanks, @richardipsum ! 🎉 |
This fixes issue #4094