Propose RFC for Merging PRs

[SudharakaP]

This proposes a slight addition to the way we merge PRs as per the following discussion; https://opencollective.slack.com/archives/CEZUS9WH3/p1623691549000100 🐨

[kewitz]

Correct me if I'm wrong, but I got the impression the issue was caused by the merge of main into the PR branch and not the opposite.
I'm OK with merge PRs into main without squashing, I feel like there's value in seeing commit-by-commit.
What I recommend avoiding is the merge of main into PR (which will mess the tree when we merge the PR back into main).

[SudharakaP]

Correct me if I'm wrong, but I got the impression the issue was caused by the merge of main into the PR branch and not the opposite.
I'm OK with merge PRs into main without squashing, I feel like there's value in seeing commit-by-commit.
What I recommend avoiding is the merge of main into PR (which will mess the tree when we merge the PR back into main).

@kewitz : Yes, but the thing is that's upto the individual contributor; it's their branch right; so we can never enforce such a thing I feel, especially for external contributors. 😄 🤔 🤔

[Betree]

I'm not against this change as I see a value in having a clean linear Git history, even though I agree with @kewitz that it's sometimes useful to merge multiple commits.

However, the RFC lacks clear motivations for this change - why are we making it now? The RFC should also mention the downsides of the proposed solution.

[znarf]

I think it's great to clarify our practices here. The default for the team has always been "Create a merge commit" which is also the GitHub default. PRs can have multiple commits and there is no need to squash unless the Git history is dirty. Team members should normally never have a dirty Git history in their PR because they know how to make it clean using Git rebase.

Also agree that we should never merge main into our feature branches, we use git pull --rebase for this.

There are some interesting guidelines for this in our Contributing.md

[SudharakaP]

I think it's great to clarify our practices here. The default for the team has always been "Create a merge commit" which is also the GitHub default. PRs can have multiple commits and there is no need to squash unless the Git history is dirty. Team members should normally never have a dirty Git history in their PR because they know how to make it clean using Git rebase.

Also agree that we should never merge main into our feature branches, we use git pull --rebase for this.

There are some interesting guidelines for this in our Contributing.md

@znarf : Thanks for the feedback. I've added some more details to the RFC. Let me know any thoughts you have. 🐨 My intention in this RFC is to make the Git workflow more flexible for all contributors. My understanding is squash merging can give a cleaner Git history on our end whether contributors choose to do interactive rebases, or merges from main, etc. 🐨

[SudharakaP]

@znarf : Thanks for pointing it out. I've updated it to remove first person language. Hope it looks good now. 😄

[Betree]

I'm personally enthusiastic about this change. I feel like the benefit of making sure that we keep a clean history surpasses the downside of not being able to merge multiple commits at once. Especially since:

• Being open-source, we get contributions from very different people with different practices
• A PR is not supposed to contain unrelated changes
• It's still possible to rollback and to see the history for individual commits as Github keeps the info even when you squash & merge

[znarf]

To be more convinced, I would like to see how if the rest of the industry is approaching this, what are the current practices on popular projects in our ecosystem such as Next.js, React or Babel. How do they approach this? What can we learn from their experience?

[SudharakaP]

To be more convinced, I would like to see how if the rest of the industry is approaching this, what are the current practices on popular projects in our ecosystem such as Next.js, React or Babel. How do they approach this? What can we learn from their experience?

@znarf :

All three repos (Next.js, React and Babel) seems to use this same practice. They let contributors do whatever they like in terms of commit messages and PRs. But if you notice the Git history there's not a single merge commit. For example let us take some recent PRs from each of these repos; 😄 🐨

For NextJS; vercel/next.js#25198. As you can see the contributor uses lot of commit messages and also does merges from canary (the default branch of NextJS) to their branch. But the final result is just a single commit without any merges.

Similarly here's a recent example for React; facebook/react#21648. This seems a squash merge;

For Babel: babel/babel#13419. Again this seems like a squash merge;

Furthermore, although all these projects have linear Git history they don't seem to enforce much standard in terms of the commit messages themselves. The two examples given above seems to indicate that when they merge, if it's squashed they go along with the default squash description provided by GitHub. We could potentially do better by employing conventional commit messages. That is when the core contributor merges it, we could follow the conventional commit guidelines as much as we could. This way our change-log will have consistent formatting and will be very pretty in my opinion. Although I wasn't sure to write this down as well in the RFC so just though maybe I'll suggest that in the future. 😉 😉

cc: @Betree

[znarf]

@SudharakaP thank you for doing the research (Next.js, React, Babel, ...), please include those findings in the RFC, as a proof of industry usage of the practice.

It's about time we talk about conventional commit, we're using it for quite some time now and I have not found the practice useful for us. If we decide to move to Squash and Merge, maybe we can also consider a new simpler convention for commit messages.

[SudharakaP]

@SudharakaP thank you for doing the research (Next.js, React, Babel, ...), please include those findings in the RFC, as a proof of industry usage of the practice.

It's about time we talk about conventional commit, we're using it for quite some time now and I have not found the practice useful for us. If we decide to move to Squash and Merge, maybe we can also consider a new simpler convention for commit messages.

@znarf : Thanks, I've added that as well. Yes I agree, if you notice the NextJS, Babel and React repos also don't strictly adhere to the conventional commit scheme. Maybe we could use a subset of keywords that works for our need. Let me think about it. 🤔

[SudharakaP]

@znarf @kewitz : Are we all good with this? Anything else to add or any comments? 😄 🐨