From aa384be3ea591c710781a1d9e9e971e7a41d24fc Mon Sep 17 00:00:00 2001 From: Sudharaka Date: Mon, 14 Jun 2021 14:55:31 -0700 Subject: [PATCH 1/7] feat: propose RFC for merging PRs --- rfcs/012-merge-rebase-strategy.md | 34 +++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 rfcs/012-merge-rebase-strategy.md diff --git a/rfcs/012-merge-rebase-strategy.md b/rfcs/012-merge-rebase-strategy.md new file mode 100644 index 000000000..9a3738155 --- /dev/null +++ b/rfcs/012-merge-rebase-strategy.md @@ -0,0 +1,34 @@ +# RFC 012 - Merge Settings/Merge Strategies + +## Affected projects + +All the projects under the [Open Collective organization](https://github.com/opencollective). + +## Motivation + +It's not always clear what merge strategy to use when merging PRs. Currently, core contributors are free to use +whatever they prefer when merging PRs. GitHub allows three ways, + +1) Create a merge commit - All commits from the branch will be added to the base branch via a merge commit. + +2) Squash and merge - All commits will be combined into one commit and added to the base branch. + +3) Rebase and merge - All the commits will be rebased and added to the base branch. + +This RFC proposes a solution in terms of GitHub settings so that we stick to one method. + +## Best Practice + +I feel there's no best practice on this matter but different workflows that are suitable for different teams. +For Open Collective projects only core team members are merging, and we cannot possibly enforce all the +community to interactively rebase Git history. Hence, currently we are mostly relying on squashing and merging. + +## Solution + +[Suggest disabling `Create a merge commit` from the GitHub settings](https://docs.github.com/en/github/administering-a-repository/configuring-pull-request-merges/configuring-commit-squashing-for-pull-requests) +as it's probably not suitable for our use. Using that option always will create merge commits and we will not have +a clean Git history. We could possibly also disable `Rebase and merge` as well although I would argue leaving it in case someone would like to explicitly include +several commits to the `main` branch. The squash and merge option should work for us in all cases. + +As an alternative or a stricter enforcement of this standard we can also add the [`Require linear history` branch protection rule +to `main` branch](https://docs.github.com/en/github/administering-a-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule). \ No newline at end of file From 94f3ae154692dd8442cff15210b6822fa841c6ce Mon Sep 17 00:00:00 2001 From: Sudharaka Date: Wed, 16 Jun 2021 07:10:38 -0700 Subject: [PATCH 2/7] feat: modify the RFC to include more details and downsides --- rfcs/012-merge-rebase-strategy.md | 33 ++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/rfcs/012-merge-rebase-strategy.md b/rfcs/012-merge-rebase-strategy.md index 9a3738155..ad6b0e977 100644 --- a/rfcs/012-merge-rebase-strategy.md +++ b/rfcs/012-merge-rebase-strategy.md @@ -15,7 +15,27 @@ whatever they prefer when merging PRs. GitHub allows three ways, 3) Rebase and merge - All the commits will be rebased and added to the base branch. -This RFC proposes a solution in terms of GitHub settings so that we stick to one method. +### Benefits + +- Core contributors can choose any particular method that they see fit. Sometimes a PR would contain unnecessary +commits which will pollute the version history and thus a rebase or a squash makes sense. Other times it maybe that + all the commits in the PR adds value. + +### Downsides + +The main downside of this is that we are assuming that the person who made the branch/PR will keep the Git history clean +on that branch. For this purpose we have guidelines; https://github.com/opencollective/opencollective-api/blob/main/CONTRIBUTING.md. + +These guidelines propose interactive rebasing which comes with the following downsides. + +1) Most new contributors and especially ones who are not familiar with Git will find this confusing. + +2) Doing interactive re-bases for larger PRs can be time consuming when there are conflicts. Since the conflicts +needs to be solved for each commit separately rather than as a whole. While merging seems easier since it takes the whole diff + and applies it in one go. + +For the reasons mentioned above, this RFC proposes a solution in terms of GitHub settings so that we stick to one method +whilst giving contributors the ability to choose whatever method they prefer. ## Best Practice @@ -23,6 +43,17 @@ I feel there's no best practice on this matter but different workflows that are For Open Collective projects only core team members are merging, and we cannot possibly enforce all the community to interactively rebase Git history. Hence, currently we are mostly relying on squashing and merging. +However, given that enforcing interactive re-bases for all branches are impossible I would argue we need to enforce a standard +on our end (when merging). Whether contributors use interactive re-basing or not, or if they choose to use simple merging, we could +still have a cleaner Git history if we always make sure to squash and merge PRs. + +### Downside of this approach + +One downside is that when squashing and merging the person who merge needs to verify the commit title and description and make sure +it makes sense. The GitHub interface automatically creates a commit description by concatenating all the commits in the branch. +Although I don't see this as a problem since even for an interactively re-based branch; while merging it's probably good practice +to see if the commit(s) make sense. + ## Solution [Suggest disabling `Create a merge commit` from the GitHub settings](https://docs.github.com/en/github/administering-a-repository/configuring-pull-request-merges/configuring-commit-squashing-for-pull-requests) From 00aa770491273f4cd08674611b84199ab099ccc1 Mon Sep 17 00:00:00 2001 From: Sudharaka Date: Thu, 17 Jun 2021 17:16:01 -0700 Subject: [PATCH 3/7] feat: modify RFC to eliminate fist person language --- rfcs/012-merge-rebase-strategy.md | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/rfcs/012-merge-rebase-strategy.md b/rfcs/012-merge-rebase-strategy.md index ad6b0e977..6ac1eefc0 100644 --- a/rfcs/012-merge-rebase-strategy.md +++ b/rfcs/012-merge-rebase-strategy.md @@ -34,31 +34,29 @@ These guidelines propose interactive rebasing which comes with the following dow needs to be solved for each commit separately rather than as a whole. While merging seems easier since it takes the whole diff and applies it in one go. -For the reasons mentioned above, this RFC proposes a solution in terms of GitHub settings so that we stick to one method -whilst giving contributors the ability to choose whatever method they prefer. +For the reasons mentioned above, this RFC proposes a solution in terms of GitHub settings in order to stick to one method when merging PRs, +whilst giving contributors the ability to choose a method they prefer(merging, interactive rebasing etc) for their own branches/PRs. ## Best Practice -I feel there's no best practice on this matter but different workflows that are suitable for different teams. -For Open Collective projects only core team members are merging, and we cannot possibly enforce all the -community to interactively rebase Git history. Hence, currently we are mostly relying on squashing and merging. +There's no best practice on this matter but different workflows that are suitable for different teams. +For Open Collective projects only core team members are merging. It's hard or probably impossible to enforce all the +community to interactively rebase Git history. Hence, currently we are mostly relying on squashing and merging PRs which are not +interactively rebased and have multiple commits, or those which have merge commits from main branch. -However, given that enforcing interactive re-bases for all branches are impossible I would argue we need to enforce a standard -on our end (when merging). Whether contributors use interactive re-basing or not, or if they choose to use simple merging, we could -still have a cleaner Git history if we always make sure to squash and merge PRs. +Whether contributors use interactive re-basing, or if they choose to use simple merging, a cleaner Git history could still be +achieved if PRs are squashed before merging. ### Downside of this approach One downside is that when squashing and merging the person who merge needs to verify the commit title and description and make sure it makes sense. The GitHub interface automatically creates a commit description by concatenating all the commits in the branch. -Although I don't see this as a problem since even for an interactively re-based branch; while merging it's probably good practice -to see if the commit(s) make sense. +Although this seems to work in most cases it's probably a good practice to see if the commit(s) make sense while merging. ## Solution -[Suggest disabling `Create a merge commit` from the GitHub settings](https://docs.github.com/en/github/administering-a-repository/configuring-pull-request-merges/configuring-commit-squashing-for-pull-requests) -as it's probably not suitable for our use. Using that option always will create merge commits and we will not have -a clean Git history. We could possibly also disable `Rebase and merge` as well although I would argue leaving it in case someone would like to explicitly include +[Disable `Create a merge commit` from the GitHub settings](https://docs.github.com/en/github/administering-a-repository/configuring-pull-request-merges/configuring-commit-squashing-for-pull-requests) +as it's probably not suitable for use. Using that option will create merge commits and will prevent a clean Git history. Additionally, suggest also disabling `Rebase and merge` as well although it could be argued to leave it in case someone would like to explicitly include several commits to the `main` branch. The squash and merge option should work for us in all cases. As an alternative or a stricter enforcement of this standard we can also add the [`Require linear history` branch protection rule From 72f2bb707cf9cd6235695c91796ea5da0461e258 Mon Sep 17 00:00:00 2001 From: Sudharaka Date: Mon, 21 Jun 2021 10:39:24 -0700 Subject: [PATCH 4/7] fix: change working to remove that merge commits produce unclean git history --- rfcs/012-merge-rebase-strategy.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/012-merge-rebase-strategy.md b/rfcs/012-merge-rebase-strategy.md index 6ac1eefc0..c160b3367 100644 --- a/rfcs/012-merge-rebase-strategy.md +++ b/rfcs/012-merge-rebase-strategy.md @@ -56,7 +56,7 @@ Although this seems to work in most cases it's probably a good practice to see i ## Solution [Disable `Create a merge commit` from the GitHub settings](https://docs.github.com/en/github/administering-a-repository/configuring-pull-request-merges/configuring-commit-squashing-for-pull-requests) -as it's probably not suitable for use. Using that option will create merge commits and will prevent a clean Git history. Additionally, suggest also disabling `Rebase and merge` as well although it could be argued to leave it in case someone would like to explicitly include +to help maintain a linear Git history. Additionally, suggest also disabling `Rebase and merge` as well although it could be argued to leave it in case someone would like to explicitly include several commits to the `main` branch. The squash and merge option should work for us in all cases. As an alternative or a stricter enforcement of this standard we can also add the [`Require linear history` branch protection rule From 53c5b227d6af0dbd38bd28cb282b6f043f3804ad Mon Sep 17 00:00:00 2001 From: Sudharaka Date: Mon, 21 Jun 2021 22:48:45 -0700 Subject: [PATCH 5/7] fix: add some examples for squash and merge approach by major repos --- rfcs/012-merge-rebase-strategy.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/rfcs/012-merge-rebase-strategy.md b/rfcs/012-merge-rebase-strategy.md index c160b3367..892baaa07 100644 --- a/rfcs/012-merge-rebase-strategy.md +++ b/rfcs/012-merge-rebase-strategy.md @@ -47,6 +47,24 @@ interactively rebased and have multiple commits, or those which have merge commi Whether contributors use interactive re-basing, or if they choose to use simple merging, a cleaner Git history could still be achieved if PRs are squashed before merging. +Some major projects such as NextJS, React and Babel seem to use the same approach in maintaining their Git histories. Some examples are given below. + +For NextJS; https://github.com/vercel/next.js/pull/25198. In this case the contributor uses lot of commit messages and also merge from `canary` (the default branch of NextJS) to their branch. +However, the final result is just a single commit without any merges. + +![image](https://user-images.githubusercontent.com/12435965/122803708-d158cc00-d27b-11eb-8f99-ac6d7a26fa43.png) + +Similarly, here's a recent example for React; https://github.com/facebook/react/pull/21648. This seems a squash merge; + +![image](https://user-images.githubusercontent.com/12435965/122802824-b20d6f00-d27a-11eb-8db9-10ba529bb9a6.png) + +For Babel: https://github.com/babel/babel/pull/13419. Again this seems to be a squash merge; + +![image](https://user-images.githubusercontent.com/12435965/122803003-f39e1a00-d27a-11eb-8f00-f34d480af450.png) + +Therefore, it could be argued that the squash and merge approach is suitable and works in maintaining a linear Git history while +also making sure the contributors get full flexibility to choose how they arrange their branch commits. + ### Downside of this approach One downside is that when squashing and merging the person who merge needs to verify the commit title and description and make sure From 90f0610ee09862bea76f0326df2d68b1ccc752d6 Mon Sep 17 00:00:00 2001 From: Sudharaka Palamakumbura Date: Tue, 22 Jun 2021 05:45:43 -0700 Subject: [PATCH 6/7] fix: change title of the motivation section MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: François Hodierne --- rfcs/012-merge-rebase-strategy.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rfcs/012-merge-rebase-strategy.md b/rfcs/012-merge-rebase-strategy.md index 892baaa07..37f192e77 100644 --- a/rfcs/012-merge-rebase-strategy.md +++ b/rfcs/012-merge-rebase-strategy.md @@ -4,7 +4,7 @@ All the projects under the [Open Collective organization](https://github.com/opencollective). -## Motivation +## Current Practice It's not always clear what merge strategy to use when merging PRs. Currently, core contributors are free to use whatever they prefer when merging PRs. GitHub allows three ways, @@ -78,4 +78,4 @@ to help maintain a linear Git history. Additionally, suggest also disabling `Reb several commits to the `main` branch. The squash and merge option should work for us in all cases. As an alternative or a stricter enforcement of this standard we can also add the [`Require linear history` branch protection rule -to `main` branch](https://docs.github.com/en/github/administering-a-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule). \ No newline at end of file +to `main` branch](https://docs.github.com/en/github/administering-a-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule). From 811bb09d9d48d4af784ad4c49727384a07f24dd1 Mon Sep 17 00:00:00 2001 From: Sudharaka Date: Fri, 25 Jun 2021 07:56:06 -0700 Subject: [PATCH 7/7] fix wording of the RFC for make sure it sounds definite --- rfcs/012-merge-rebase-strategy.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/rfcs/012-merge-rebase-strategy.md b/rfcs/012-merge-rebase-strategy.md index 37f192e77..8b219d127 100644 --- a/rfcs/012-merge-rebase-strategy.md +++ b/rfcs/012-merge-rebase-strategy.md @@ -74,8 +74,9 @@ Although this seems to work in most cases it's probably a good practice to see i ## Solution [Disable `Create a merge commit` from the GitHub settings](https://docs.github.com/en/github/administering-a-repository/configuring-pull-request-merges/configuring-commit-squashing-for-pull-requests) -to help maintain a linear Git history. Additionally, suggest also disabling `Rebase and merge` as well although it could be argued to leave it in case someone would like to explicitly include +to help maintain a linear Git history. But keep the `Rebase and merge` option in case someone would like to explicitly include several commits to the `main` branch. The squash and merge option should work for us in all cases. -As an alternative or a stricter enforcement of this standard we can also add the [`Require linear history` branch protection rule -to `main` branch](https://docs.github.com/en/github/administering-a-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule). +Also add the [`Require linear history` branch protection rule +to `main` branch](https://docs.github.com/en/github/administering-a-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule) to +avoid merge comments being written to the main branch. \ No newline at end of file