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

Clarify that we use merge commits when mergeing PRs #4220

Merged
merged 3 commits into from Aug 18, 2021

Conversation

daschuer
Copy link
Member

No description provided.

@Swiftb0y
Copy link
Member

I was not aware we had this policy. Moreover, there have been exceptions in the past where I merged when I should've rather squashed and merge. See: #4108 (comment)

@Holzhaus
Copy link
Member

I was not aware we had this policy. Moreover, there have been exceptions in the past where I merged when I should've rather squashed and merge. See: #4108 (comment)

Yes. In general, we should always merge. Squashing should only be done in these cases:

  • The PR contains broken (non-building) commits and the contributor is not willing to fix them up.
  • Minor changes with lots of back-and-forth where the commits are not worth keeping (e. g. a PR that updates the changelog does not need a separate commit for every typo pointed out during review).
  • Controller mapping PRs where the contributor failed to use meaningful commit messages (e. g. "Update Foo.midi.xml", "fixes", "changed stuff", "hurr durr ima sheep")

@mixxxdj mixxxdj deleted a comment from coveralls Aug 18, 2021
@Swiftb0y
Copy link
Member

Yes. In general, we should always merge. Squashing should only be done in these cases:

Thats fine, but we should write down these exceptions too instead of just saying to always merge.

@daschuer
Copy link
Member Author

I have rephrased the suggestions without the "is not willing" part, because that is another issue where we will find an individual solution.

In general we should stick with the "Merge pull request .. " commits whenever possible for consistency when crawling through the git tree.

@daschuer
Copy link
Member Author

Does this fit to your needs?

CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. pre-commit complains about trailing whitespace. Please fix, then we can merge IMHO.

Co-authored-by: Jan Holthuis <holthuis.jan@googlemail.com>
@daschuer
Copy link
Member Author

Done

@Swiftb0y Swiftb0y merged commit babfa2a into mixxxdj:main Aug 18, 2021
@Be-ing
Copy link
Contributor

Be-ing commented Aug 18, 2021

Whoa there. I didn't even have a chance to comment before this was merged. I am opposed to this. In my experience this policy is unusual and unwarranted. Most projects do not use merge commits; they rebase.

@Holzhaus
Copy link
Member

Holzhaus commented Aug 18, 2021

We shall we revert until the discussion is concluded? Unsure, because in my understanding using merge commits was always our policy until now. Telling people to rebase would be a change IMHO. In any case, squash-and-merge should generally be discouraged.

@Be-ing Can you open a topic on Zulip? I'm pretty much against rebasing because you need to retest every single commit, but let's not discuss that here.

@Be-ing
Copy link
Contributor

Be-ing commented Aug 18, 2021

Let's continue the discussion on Zulip. No need to rush a reversion.

@daschuer daschuer deleted the dont_sqash branch September 26, 2021 17:44
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

Successfully merging this pull request may close these issues.

None yet

4 participants