Skip to content
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

Merge pull-requests by commit disabled? #280

Closed
v4hn opened this issue Jan 21, 2019 · 6 comments
Closed

Merge pull-requests by commit disabled? #280

v4hn opened this issue Jan 21, 2019 · 6 comments

Comments

@v4hn
Copy link
Contributor

v4hn commented Jan 21, 2019

I just wanted to merge #278 and found the Create a merge commit field disabled:

Create a merge commit
All commits from this branch will be added to the base branch via a merge commit.
Not enabled for this repository

@davetcoleman @mlautman @rhaschke Did someone disable this option?
I guess it is still possible to do this by hand in git, right?
It's not a big deal for this request, but in general I would prefer to have this possibility.

@davetcoleman
Copy link
Member

I don't remember exactly, but this was probably me. Why do we want this enabled, though? We never use that functionality as it makes a sloppy git history. Our policy is:

Squash-Merge If Possible
Pull-requests with a single commit should always be squash-merged (github supports this via a drop-down list on the “merge” button). If a request contains “fixup commits” it should also be squash-merged. If you are unsure this is to the requestor’s liking, ask them.

@rhaschke
Copy link
Contributor

The citation primarily refers to single commits and fixup commits, which indeed should not clutter the git history. Merge-commits make a lot of sense if there are multiple, clearly distinguishable commits in a single PR!

@davetcoleman
Copy link
Member

For clearly distinguishable commits in a single PR we've all agreed we want rebase merging:

image

@rhaschke
Copy link
Contributor

Often that's a viable option, yes. However, rebase-merging also disguises the extension of a merge (you don't see the start and end of the related commits anymore) and the PR id is not reported by github.
For this reason, I prefer merge commits in other projects, although I try not to merge branches that were branched off very deep in the past. In this case, I first rebase and then merge-commit.

@v4hn
Copy link
Contributor Author

v4hn commented Jan 24, 2019

For clearly distinguishable commits in a single PR we've all agreed we want rebase merging:

I'm not aware of an agreement to never use merge commits in moveit and neither Robert nor I seem to agree.

  • single commits - > squash
  • multiple fixup-style commits - > squash
  • multiple independent commits -> rebase and merge
  • multiple clean commits with common ground and some relation - > merge commit.

one of git's design goals are short-living feature branches that can be merged.
So I really don't think it makes sense to disable merges...

@davetcoleman
Copy link
Member

Sounds good, I've re-enabled merging. Can you copy that policy your wrote out on our Pull Request page please? You can just edit on Github:
https://github.com/ros-planning/moveit.ros.org/blob/gh-pages/documentation/contributing/pullrequests/index.markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants