-
Notifications
You must be signed in to change notification settings - Fork 4
Add merge commit #53
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.
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
Add merge commit #53
Conversation
3200228 to
98ee0d4
Compare
98ee0d4 to
1c4b6d9
Compare
src/subcommand/merge_subcommand.cpp
Outdated
| else | ||
| { | ||
| msg.append("commit "); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two "if/else" sections can be merged into a single one. Or you could use the ternary operator to initilaze the message in a single line:
std::string msg = merge_ref ? "Merge branch" : "Merge commit'";
src/subcommand/merge_subcommand.cpp
Outdated
| } | ||
|
|
||
| void create_merge_commit(repository_wrapper& repo, const index_wrapper& index, std::vector<std::string> m_branches_to_merge, | ||
| const annotated_commit_list_wrapper& commits_to_merge, size_t num_commits_to_merge) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create_merge_commit could be a method of the merge_subcommand class; this way it would have access to the m_changes_to_merge member, and this parameter can be removed. If we prefer to keep it as a free function, then the parameter should be renamed in branches_to_merge (the m_ prefix should be reserved for class members).
ianthomas23
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @SandrineP
Add merge commit