From 8ce22343db79f152395b102ef5ea9385014c81f7 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 7 Jan 2023 18:15:34 +0000 Subject: [PATCH 01/93] Comprehensive revision of pull request merge procedure after discussion with pnj and gdr. still draft, but ready for comment. --- mps/procedure/pull-request-merge.rst | 129 +++++++++++++++++---------- 1 file changed, 84 insertions(+), 45 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 3c5c0f84ad5..83c16c1e7fb 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -17,15 +17,18 @@ Memory Pool System pull request merge procedure This document contains a procedure for merging a branch received via a GitHub "pull request". -This is a draft procedure based on: -- "Re: Possible MPS Help" [GDR_2014-01-09]_ -- "Memory Pool System branching and merging procedures" [GDR_2020-09-03]_. +This document was created as a combination of the process improvement +steps from our Perforce-based "Memory Pool System branching and +merging procedures" [GDR_2020-09-03]_ with Gareth Rees' email +containing the working steps for git / GitHub merges, "Re: Possible +MPS Help" [GDR_2014-01-09]_. -Some of the questions that need resolving are noted in square -brackets. +The document is still draft. Some of the questions that need +resolving are noted in square brackets. Ravenbrook is currently (2023-01) migrating the MPS project to git -(and GitHub) and this is likely to modify this procedure. +(and GitHub) and this is likely to modify this procedure. [Insert +reference to that project. RB 2023-01-07] 2. Pre-merge checklist @@ -76,7 +79,21 @@ omitted for now. RB 2023-01-07] git clone -o ravenbrook ssh://git@perforce.ravenbrook.com:1622/mps-public -#. Add the GitHub repo as a new remote:: + [We can migrate to cloning the repo from GitHub or wherever it is + hosted, as long as it's equivalent. RB 2023-01-07] + +#. Add the repo that contains the branch to be merged as a remote. + For example, if it in the Ravenbrook MPS git repo:: + + git remote add github git@github.com:Ravenbrook/mps.git + + but if it's from a third-party repo (such as a GitHub fork) then + add that instead, with an appropriate name, e.g.:: + + git remote add captain-contrib https://gitlab.com/captcontrib/mps.git + +#. Add the Ravenbrook MPS GitHub repository as a remote in order to + make use of Travis CI to build and test the merge. git remote add github git@github.com:Ravenbrook/mps.git @@ -85,60 +102,82 @@ omitted for now. RB 2023-01-07] ------------------------------- 1. Ensure that the contributor has executed the appropriate assignment - of copyright. + of copyright. [And do what if they haven't? RB 2023-01-07] [This + needs updating now that the MPS is using the BSD licence. RB + 2023-01-07] -2. Fetch the branch that you are going to merge from GitHub, for - example:: +2. Fetch the branch that you are going to merge, e.g.:: - BRANCH=branch/2020-08-23/walk-docs - git fetch github "$BRANCH” + git fetch captain-contrib mps-speed-hax -3. Make a local tracking branch:: +3. Make an equivalent local branch using the MPS durable branch naming + convention, "branch/DATE/TOPIC", e.g.:: - git checkout "$BRANCH” + git checkout -b branch/2023-01-06/speed-hax captain-contrib/mps-speed-hax -4. Decide whether you are going to merge by fast-forward or not. This - all depends on how you want the result of the merge to appear in - Perforce. + Double check you've got this right. Using the wrong branch naming + `causes permanent pollution in the Ravenbrook Perforce repository + `_. - A fast-forward merge means that the individual commits from the - branch are replayed on top of master and so each commit from the - branch becomes a separate change in Perforce, with its own change - description. This would be appropriate when each commit stands on - its own. To make a fast-forward merge, rebase the branch on master, - resolve any conflicts, and force-push it back to GitHub:: +4. Merge master with the branch:: - git rebase ravenbrook/master - # resolve conflicts here - git push --force-with-lease github "$BRANCH:$BRANCH" + git merge ravenbrook/master - A non-fast-forward merge makes a single merge commit with two - parent commits (the heads of master and the branch - respectively). This would be appropriate when the commits on the - branch don’t stand on their own and you want to have a single - change in Perforce. + You may need to resolve conflicts. If you can't resolve conflicts + yourself, you may need to involve the original author of the + branch. If you still can't resolve conflicts, this procedure + fails. - [This needs review, and in any case a decision procedure is needed - here. What would be the default outcome from the GitHub interface? - RB 2023-01-07] + [What would be the default outcome from the GitHub interface, + using the "merge" button? Can we allow that? RB 2023-01-07] -5. Merge the branch to master and resolve any conflicts:: - - git checkout master - # resolve conflicts here - git merge "$BRANCH” - -6. Run tests, for example:: +5. Build and test the results locally. For example:: make -C code -f lii6gc.gmk testci testansi testpollnone testmmqa - if you are on Linux. + If tests to not pass, review your conflict resolution from step 4, + and if that doesn't resolve things, the procedure fails, and you + need to go back to the source of the branch, e.g. the pull request + and its original author. Something's wrong! -7. Push master (and if non-fast-forward, the branch) to mps-public:: +6. Push the branch to the Ravenbrook MPS GitHub repository to trigger + building and testing on all target platforms using Travis CI. :: - git push ravenbrook master "$BRANCH” + git push github branch/2023-01-06/speed-hax:branch/2023-01-06/speed-hax -8. Wait for gitpushbot to push the result to GitHub. + You will need to wait for results from Travis CI. [Add details of + how to see them. RB 2023-07-01] [Some sort of ``--force`` option + may be required if we're pushing back to the same branch we started + with, such as when it's a Ravenbrook development branch. RB + 2023-07-01] + + See step 5 about what to do if tests do not pass. + + Note: This potentially creates a branch in the GitHub repo ahead + of Git Fusion doing so, but it will the same name, because of the + Git Fusion mapping, and so the result is the same as if it had come + in via Perforce. + +7. Replace the master with your branch, effecting the merge:: + + git checkout master + git merge --ff-only branch/2023-01-06/speed-hax + + [There should not have been any further changes on master, and + ``--ff-only`` checks for that. The merge commit we want on master + is made in step 4. RB 2023-01-07] + +8. Push master to Perforce via Git Fusion:: + + git push ravenbrook master branch/2023-01-06/speed-hax + + [This could fail if someone else has done something on the master + codeline in Perforce. What do you do in that case? RB + 2023-01-07.] + +8. After a bit [how long? RB 2023-01-07] check that gitpushbot has + pushed the result to the Ravenbrook MPS repo on GitHub. [And do + what if it doesn't? RB 2023-01-07] A. References From 5a8a312720561fdc4d8cc668ba2f4d5c11c32cd7 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 7 Jan 2023 19:09:00 +0000 Subject: [PATCH 02/93] Various fixes in response to review by gdr . --- mps/procedure/pull-request-merge.rst | 50 +++++++++++++++------------- 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 83c16c1e7fb..eebf360d8ad 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -19,9 +19,9 @@ GitHub "pull request". This document was created as a combination of the process improvement steps from our Perforce-based "Memory Pool System branching and -merging procedures" [GDR_2020-09-03]_ with Gareth Rees' email +merging procedures" [GDR_2014-01-09]_ with Gareth Rees' email containing the working steps for git / GitHub merges, "Re: Possible -MPS Help" [GDR_2014-01-09]_. +MPS Help" [GDR_2020-09-03]_ . The document is still draft. Some of the questions that need resolving are noted in square brackets. @@ -82,28 +82,24 @@ omitted for now. RB 2023-01-07] [We can migrate to cloning the repo from GitHub or wherever it is hosted, as long as it's equivalent. RB 2023-01-07] -#. Add the repo that contains the branch to be merged as a remote. - For example, if it in the Ravenbrook MPS git repo:: - - git remote add github git@github.com:Ravenbrook/mps.git - - but if it's from a third-party repo (such as a GitHub fork) then - add that instead, with an appropriate name, e.g.:: - - git remote add captain-contrib https://gitlab.com/captcontrib/mps.git - #. Add the Ravenbrook MPS GitHub repository as a remote in order to make use of Travis CI to build and test the merge. git remote add github git@github.com:Ravenbrook/mps.git +#. If the branch to be merged is in a third-party repo (such as a + GitHub fork), then add that, e.g.:: + + git remote add captain-contrib https://gitlab.com/captcontrib/mps.git + 4. Merging a development branch ------------------------------- 1. Ensure that the contributor has executed the appropriate assignment - of copyright. [And do what if they haven't? RB 2023-01-07] [This - needs updating now that the MPS is using the BSD licence. RB + of copyright. If they haven't, this procedure fails. Talk to them + or get someone to talk to them. [This needs updating now that the + MPS is using the BSD licence. e.g. We mustn't accept GPL code. RB 2023-01-07] 2. Fetch the branch that you are going to merge, e.g.:: @@ -135,7 +131,11 @@ omitted for now. RB 2023-01-07] make -C code -f lii6gc.gmk testci testansi testpollnone testmmqa - If tests to not pass, review your conflict resolution from step 4, + See `design.mps.tests`_ for details and other platforms. + +.. _`design.mps.tests`: ../design/tests.txt + + If tests do not pass, review your conflict resolution from step 4, and if that doesn't resolve things, the procedure fails, and you need to go back to the source of the branch, e.g. the pull request and its original author. Something's wrong! @@ -146,10 +146,10 @@ omitted for now. RB 2023-01-07] git push github branch/2023-01-06/speed-hax:branch/2023-01-06/speed-hax You will need to wait for results from Travis CI. [Add details of - how to see them. RB 2023-07-01] [Some sort of ``--force`` option - may be required if we're pushing back to the same branch we started - with, such as when it's a Ravenbrook development branch. RB - 2023-07-01] + how to see them. RB 2023-07-01] [Some sort of + ``--force-with-lease`` option may be required if we're pushing back + to the same branch we started with, such as when it's a Ravenbrook + development branch. RB 2023-07-01] See step 5 about what to do if tests do not pass. @@ -167,15 +167,17 @@ omitted for now. RB 2023-01-07] ``--ff-only`` checks for that. The merge commit we want on master is made in step 4. RB 2023-01-07] -8. Push master to Perforce via Git Fusion:: +8. Push master and the branch to Perforce via Git Fusion:: git push ravenbrook master branch/2023-01-06/speed-hax - [This could fail if someone else has done something on the master - codeline in Perforce. What do you do in that case? RB - 2023-01-07.] + If this fails because someone else has submitted changes to the + master codeline since you started, pull those changes and go back + to step 4 :: -8. After a bit [how long? RB 2023-01-07] check that gitpushbot has + git pull ravenbrook master + +9. After a bit [how long? RB 2023-01-07] check that gitpushbot has pushed the result to the Ravenbrook MPS repo on GitHub. [And do what if it doesn't? RB 2023-01-07] From e2f4071dc298941afca662f6af0d74975748cd2b Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 7 Jan 2023 19:41:12 +0000 Subject: [PATCH 03/93] Starting by cloning from github, since this is now a strict superset of what comes out git fusion from perforce, and is closer to our migration target. --- mps/procedure/pull-request-merge.rst | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index eebf360d8ad..7c409b69173 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -75,20 +75,19 @@ omitted for now. RB 2023-01-07] in Perforce and matches your Perforce user record, submitting it if necessary. -#. Clone the mps-public repo and name the remote “ravenbrook”:: +#. Clone the Ravenbrook MPS GitHub repository and name the remote + "github", giving (inter alia) access to Travis CI to build and test + the merge. :: - git clone -o ravenbrook ssh://git@perforce.ravenbrook.com:1622/mps-public + git clone -o github git@github.com:Ravenbrook/mps.git - [We can migrate to cloning the repo from GitHub or wherever it is - hosted, as long as it's equivalent. RB 2023-01-07] +#. Add the Git Fusion mps-public repo, which is the interface to + Ravenbrook's Perforce. :: -#. Add the Ravenbrook MPS GitHub repository as a remote in order to - make use of Travis CI to build and test the merge. - - git remote add github git@github.com:Ravenbrook/mps.git + git remote add ravenbrook ssh://git@perforce.ravenbrook.com:1622/mps-public #. If the branch to be merged is in a third-party repo (such as a - GitHub fork), then add that, e.g.:: + fork), then add that, e.g.:: git remote add captain-contrib https://gitlab.com/captcontrib/mps.git From 6382c7b3f30bd30c7a391df6989c2837d81809de Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 7 Jan 2023 19:53:41 +0000 Subject: [PATCH 04/93] Emphasizing that some things are in perforce, because it wasn't immediately clear to pnj. --- mps/procedure/pull-request-merge.rst | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 7c409b69173..eb7a77230d3 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -68,13 +68,14 @@ omitted for now. RB 2023-01-07] 3. Prerequisite steps --------------------- -#. Ensure your public key is in //.git-fusion/users/USER/keys/ in - Perforce, submitting it if necessary. - -#. Ensure your e-mail address is in //.git-fusion/users/p4gf_usermap - in Perforce and matches your Perforce user record, submitting it if +#. Ensure your public key is in Perforce at + //.git-fusion/users/USER/keys/ in Perforce, submitting it if necessary. +#. Ensure your e-mail address is in Perforce at + //.git-fusion/users/p4gf_usermap in Perforce and matches your + Perforce user record, submitting it if necessary. + #. Clone the Ravenbrook MPS GitHub repository and name the remote "github", giving (inter alia) access to Travis CI to build and test the merge. :: From c9a7fe8ae5866be525aa632af9f7b0f0739be16b Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 7 Jan 2023 19:54:15 +0000 Subject: [PATCH 05/93] Pnj pointed out that trainee integrators (people who merge stuff) should be in the readership. --- mps/procedure/pull-request-merge.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index eb7a77230d3..17472197e91 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -7,7 +7,7 @@ Memory Pool System pull request merge procedure :date: 2023-01-07 :confidentiality: public :copyright: See `C. Copyright and License`_ -:readership: MPS developers +:readership: MPS developers, trainee integrators :status: draft From f2a5cc580460d7f4b65573e80d694e1e2c703119 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 7 Jan 2023 20:24:15 +0000 Subject: [PATCH 06/93] Adding reference to "github standard fork & pull request workflow". --- mps/procedure/pull-request-merge.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 17472197e91..29f773b9626 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -185,6 +185,10 @@ omitted for now. RB 2023-01-07] A. References ------------- +.. [Chaser324_2017] "GitHub Standard Fork & Pull Request Workflow"; + Chase Pettit; 2017; + . + .. [GDR_2020-09-03] "Re: Possible MPS help"; Gareth Rees; 2020-09-03; . From aa0fedf2ecb016bafa101d720182b0879959535d Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 7 Jan 2023 20:28:59 +0000 Subject: [PATCH 07/93] Fetching the branch to be merged directly to a local branch, removing a step. --- mps/procedure/pull-request-merge.rst | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 29f773b9626..39843052e65 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -102,20 +102,17 @@ omitted for now. RB 2023-01-07] MPS is using the BSD licence. e.g. We mustn't accept GPL code. RB 2023-01-07] -2. Fetch the branch that you are going to merge, e.g.:: +2. Fetch the branch that you are going to merge to a local branch + using the MPS durable branch naming convention, + "branch/DATE/TOPIC", e.g. :: - git fetch captain-contrib mps-speed-hax - -3. Make an equivalent local branch using the MPS durable branch naming - convention, "branch/DATE/TOPIC", e.g.:: - - git checkout -b branch/2023-01-06/speed-hax captain-contrib/mps-speed-hax + git fetch captain-contrib mps-speed-hax:branch/2023-01-06/speed-hax Double check you've got this right. Using the wrong branch naming `causes permanent pollution in the Ravenbrook Perforce repository `_. -4. Merge master with the branch:: +3. Merge master with the branch:: git merge ravenbrook/master @@ -127,7 +124,7 @@ omitted for now. RB 2023-01-07] [What would be the default outcome from the GitHub interface, using the "merge" button? Can we allow that? RB 2023-01-07] -5. Build and test the results locally. For example:: +4. Build and test the results locally. For example:: make -C code -f lii6gc.gmk testci testansi testpollnone testmmqa @@ -140,7 +137,7 @@ omitted for now. RB 2023-01-07] need to go back to the source of the branch, e.g. the pull request and its original author. Something's wrong! -6. Push the branch to the Ravenbrook MPS GitHub repository to trigger +5. Push the branch to the Ravenbrook MPS GitHub repository to trigger building and testing on all target platforms using Travis CI. :: git push github branch/2023-01-06/speed-hax:branch/2023-01-06/speed-hax @@ -158,7 +155,7 @@ omitted for now. RB 2023-01-07] Git Fusion mapping, and so the result is the same as if it had come in via Perforce. -7. Replace the master with your branch, effecting the merge:: +6. Replace the master with your branch, effecting the merge:: git checkout master git merge --ff-only branch/2023-01-06/speed-hax @@ -167,17 +164,17 @@ omitted for now. RB 2023-01-07] ``--ff-only`` checks for that. The merge commit we want on master is made in step 4. RB 2023-01-07] -8. Push master and the branch to Perforce via Git Fusion:: +7. Push master and the branch to Perforce via Git Fusion:: git push ravenbrook master branch/2023-01-06/speed-hax If this fails because someone else has submitted changes to the master codeline since you started, pull those changes and go back - to step 4 :: + to step 3 :: git pull ravenbrook master -9. After a bit [how long? RB 2023-01-07] check that gitpushbot has +8. After a bit [how long? RB 2023-01-07] check that gitpushbot has pushed the result to the Ravenbrook MPS repo on GitHub. [And do what if it doesn't? RB 2023-01-07] From 5fbcb89841857084a98d4c0c5aef15a4f5a0cca8 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 7 Jan 2023 20:35:31 +0000 Subject: [PATCH 08/93] Answering question about the github merge button. --- mps/procedure/pull-request-merge.rst | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 39843052e65..807b2a93f9e 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -114,6 +114,7 @@ omitted for now. RB 2023-01-07] 3. Merge master with the branch:: + git checkout branch/2023-01-06/speed-hax git merge ravenbrook/master You may need to resolve conflicts. If you can't resolve conflicts @@ -121,8 +122,10 @@ omitted for now. RB 2023-01-07] branch. If you still can't resolve conflicts, this procedure fails. - [What would be the default outcome from the GitHub interface, - using the "merge" button? Can we allow that? RB 2023-01-07] + [What would be the default outcome from the GitHub interface, using + the "merge" button? [Chaser324_2017]_ claims it only works for + fast-forwards, which we don't want, because they discard all + records of the merge happening. RB 2023-01-07] 4. Build and test the results locally. For example:: From 6286943aa64a8b343c16ee08c1bb63360d7322a9 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 7 Jan 2023 20:38:49 +0000 Subject: [PATCH 09/93] Pnj points out that the merge button in github, doing a fast-forward, also discards information about who did this procedure, which we don't want. --- mps/procedure/pull-request-merge.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 807b2a93f9e..c582ad07953 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -124,8 +124,8 @@ omitted for now. RB 2023-01-07] [What would be the default outcome from the GitHub interface, using the "merge" button? [Chaser324_2017]_ claims it only works for - fast-forwards, which we don't want, because they discard all - records of the merge happening. RB 2023-01-07] + fast-forwards, which we don't want, because they discard records of + the merge happening, including who did it. RB 2023-01-07] 4. Build and test the results locally. For example:: From 4d9c048999ca5e66da77d6563f3db82ff18b22c9 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 7 Jan 2023 20:55:56 +0000 Subject: [PATCH 10/93] Validating rst and fixing minor issues. --- mps/procedure/pull-request-merge.rst | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index c582ad07953..df80aa58ac4 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -131,9 +131,8 @@ omitted for now. RB 2023-01-07] make -C code -f lii6gc.gmk testci testansi testpollnone testmmqa - See `design.mps.tests`_ for details and other platforms. - -.. _`design.mps.tests`: ../design/tests.txt + See `design.mps.tests <../design/tests.txt>`_ for details and other + platforms. If tests do not pass, review your conflict resolution from step 4, and if that doesn't resolve things, the procedure fails, and you @@ -236,5 +235,5 @@ THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -.. checked with rst2html -v index.rst > /dev/null +.. checked with rst2html -v pull-request-merge.rst > /dev/null .. end From 9f66e51e13c0c54c14e127270b8abb7cc578d3df Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 7 Jan 2023 22:17:42 +0000 Subject: [PATCH 11/93] Fixing the step for checking licensing of contributions. --- mps/procedure/pull-request-merge.rst | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index df80aa58ac4..865302c8376 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -96,12 +96,29 @@ omitted for now. RB 2023-01-07] 4. Merging a development branch ------------------------------- -1. Ensure that the contributor has executed the appropriate assignment - of copyright. If they haven't, this procedure fails. Talk to them - or get someone to talk to them. [This needs updating now that the - MPS is using the BSD licence. e.g. We mustn't accept GPL code. RB +1. Ensure that the contributor has either: + + - licensed their work under an open source licence that is at least + as permissive as `the MPS licence <../license.txt>`_ (the `BSD + 2-clause license + `_), preferably the + same licence + + - granted Ravenbrook the licence described in `Contributing to the + MPS <../contributing.rst>`_ + + If they haven't, this procedure fails. Talk to them or get someone + to talk to them. + + [We can probably reduce the overhead of this step with a clause + similar to `Syncthing's Licensing clause + `_ + in `our own contributing document <../contributing.rst>`_. RB 2023-01-07] + Note: the `GPL `_ is + *not* compatible with the MPS. + 2. Fetch the branch that you are going to merge to a local branch using the MPS durable branch naming convention, "branch/DATE/TOPIC", e.g. :: From c76660b04e22fa5202d869d7c28c885a36fe7e7d Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 7 Jan 2023 22:20:45 +0000 Subject: [PATCH 12/93] Improving rst formatting. --- mps/contributing.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/mps/contributing.rst b/mps/contributing.rst index e806164f653..0c3a2fcc8f0 100644 --- a/mps/contributing.rst +++ b/mps/contributing.rst @@ -1,11 +1,14 @@ +======================= Contributing to the MPS ======================= + We are very happy to receive contributions to the Memory Pool System so that we can improve it for everyone. Review ------ + The MPS is highly engineered and rigorously controlled in order to prevent defects. This approach has lead to an extremely small number of bugs in production since its first commercial use in 1997. There are a @@ -22,6 +25,7 @@ The style guide in guide.impl.c.format_ contains basic rules for style. Licensing --------- + Prior to the 2020-05 re-licensing of the MPS under the BSD 2-clause license, we required contributors to agree to the following contribution agreement, so that we could continue to commercially @@ -44,4 +48,8 @@ You just need to reply with “I agree.” We apologise for the inconvenience. Thank you --------- + Finally, thank you for making the MPS more useful to everyone. + +.. validated with rst2html -v contributing.rst > /dev/null +.. end From 3eeaaac06723f3ea2abd78292b16e25fb9c92bed Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 7 Jan 2023 22:33:42 +0000 Subject: [PATCH 13/93] Simplifying licensing of contributions by deeming them to conform to exisitng licenses unless expressly stated otherwise in the contribution request. --- mps/contributing.rst | 24 ++++++------------------ mps/procedure/pull-request-merge.rst | 27 ++++++--------------------- 2 files changed, 12 insertions(+), 39 deletions(-) diff --git a/mps/contributing.rst b/mps/contributing.rst index 0c3a2fcc8f0..38de05c8c6c 100644 --- a/mps/contributing.rst +++ b/mps/contributing.rst @@ -26,24 +26,12 @@ The style guide in guide.impl.c.format_ contains basic rules for style. Licensing --------- -Prior to the 2020-05 re-licensing of the MPS under the BSD 2-clause -license, we required contributors to agree to the following -contribution agreement, so that we could continue to commercially -license the MPS and thereby fund future development. We have not yet -decided what may replace this requirement. - - I grant Ravenbrook Ltd an irrevocable, perpetual, worldwide, - non-exclusive licence to do anything with [your contribution] that I - would have the right to do. This includes (but is not limited to): - - 1. reproducing it and doing any other act that is restricted by - copyright; - - 2. the right to sublicence to others the code and any derivative - work. - -A member of Ravenbrook staff may ask you to expressly (in writing) agree. -You just need to reply with “I agree.” We apologise for the inconvenience. +All contributions are deemed to have been made under the same license +as the material to which the contribution is made, unless you +expressly state otherwise in your contribution request. In nearly all +cases this is the `BSD 2-clause license +`_. You retain the +copyright to such contributions. Thank you diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 865302c8376..8611920909a 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -96,28 +96,13 @@ omitted for now. RB 2023-01-07] 4. Merging a development branch ------------------------------- -1. Ensure that the contributor has either: +1. Ensure that the contributor has not expressed any kind of variation + on the licensing of their work from the material to which they are + contributing. (See `"Licensing" in "Contributing to the MPS" + <../contributing.rst#licensing>`.) - - licensed their work under an open source licence that is at least - as permissive as `the MPS licence <../license.txt>`_ (the `BSD - 2-clause license - `_), preferably the - same licence - - - granted Ravenbrook the licence described in `Contributing to the - MPS <../contributing.rst>`_ - - If they haven't, this procedure fails. Talk to them or get someone - to talk to them. - - [We can probably reduce the overhead of this step with a clause - similar to `Syncthing's Licensing clause - `_ - in `our own contributing document <../contributing.rst>`_. RB - 2023-01-07] - - Note: the `GPL `_ is - *not* compatible with the MPS. + If they have, this procedure fails. Talk to them or get someone to + talk to them about the issue. 2. Fetch the branch that you are going to merge to a local branch using the MPS durable branch naming convention, From b2b87cbf82e5c9db02f0b67c57328554cee47b09 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 7 Jan 2023 22:42:15 +0000 Subject: [PATCH 14/93] Clarifying pre-merge checklist. --- mps/procedure/pull-request-merge.rst | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 8611920909a..92abfa47f7b 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -34,18 +34,20 @@ reference to that project. RB 2023-01-07] 2. Pre-merge checklist ---------------------- -#. Is there a document (issue, job) recording the problem that is - solved by the changes in the branch? +#. Is there a permanent visible document (issue, job), referenced by + the branch, recording the problem that is solved by the changes in + the branch? #. Does the branch solve the problem? -#. Is there an automated test case that demonstrates that the problem - is solved? +#. Is there an automated test case that demonstrates the problem + (without the branch) and that that the problem is solved (with the + branch)? -#. If there are interface changes, is there documentation? +#. If there are interface changes, are they documented? -#. If the changes are significant and user-visible, have you updated - the release notes (``manual/source/release.rst``)? +#. If the changes are significant and user-visible, is there an update + to the release notes (``manual/source/release.rst``)? #. Has there been a code review? @@ -55,7 +57,8 @@ reference to that project. RB 2023-01-07] #. Does the branch merge cleanly into the masters? If not, consider a catch-up merge from the masters. [This may be validated by GitHub - and/or Travis CI. Insert details here. RB 2023-01-07] + and/or Travis CI. Insert details here. RB 2023-01-07] [This is + duplicated by step 3 of the procedure. RB 2023-01-07] #. Does a merge of the branch with master build and pass tests on all target platforms? [This may be validated by Travis CI. Insert From 8596294a3e1c7bd8feb328d35a206641ec11d5a4 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 7 Jan 2023 22:58:50 +0000 Subject: [PATCH 15/93] Clear temporal separation of prerequisite steps. further clarification of procedure steps. --- mps/procedure/pull-request-merge.rst | 51 ++++++++++++++-------------- 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 92abfa47f7b..cd9e0e49edf 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -71,6 +71,8 @@ omitted for now. RB 2023-01-07] 3. Prerequisite steps --------------------- +These steps will only rarely need repeating. + #. Ensure your public key is in Perforce at //.git-fusion/users/USER/keys/ in Perforce, submitting it if necessary. @@ -90,24 +92,24 @@ omitted for now. RB 2023-01-07] git remote add ravenbrook ssh://git@perforce.ravenbrook.com:1622/mps-public -#. If the branch to be merged is in a third-party repo (such as a - fork), then add that, e.g.:: - - git remote add captain-contrib https://gitlab.com/captcontrib/mps.git - 4. Merging a development branch ------------------------------- -1. Ensure that the contributor has not expressed any kind of variation +1. Check that the contributor has not expressed any kind of variation on the licensing of their work from the material to which they are contributing. (See `"Licensing" in "Contributing to the MPS" <../contributing.rst#licensing>`.) If they have, this procedure fails. Talk to them or get someone to - talk to them about the issue. + talk to them about the matter. -2. Fetch the branch that you are going to merge to a local branch +2. If the branch to be merged is in a third-party repo (such as a + fork), then add that, e.g.:: + + git remote add captain-contrib https://gitlab.com/captcontrib/mps.git + +3. Fetch the branch that you are going to merge to a local branch using the MPS durable branch naming convention, "branch/DATE/TOPIC", e.g. :: @@ -117,10 +119,11 @@ omitted for now. RB 2023-01-07] `causes permanent pollution in the Ravenbrook Perforce repository `_. -3. Merge master with the branch:: +4. Merge master with the branch:: git checkout branch/2023-01-06/speed-hax - git merge ravenbrook/master + git pull ravenbrook master:master + git merge master You may need to resolve conflicts. If you can't resolve conflicts yourself, you may need to involve the original author of the @@ -132,7 +135,7 @@ omitted for now. RB 2023-01-07] fast-forwards, which we don't want, because they discard records of the merge happening, including who did it. RB 2023-01-07] -4. Build and test the results locally. For example:: +5. Build and test the results locally. For example:: make -C code -f lii6gc.gmk testci testansi testpollnone testmmqa @@ -144,16 +147,13 @@ omitted for now. RB 2023-01-07] need to go back to the source of the branch, e.g. the pull request and its original author. Something's wrong! -5. Push the branch to the Ravenbrook MPS GitHub repository to trigger +6. Push the branch to the Ravenbrook MPS GitHub repository to trigger building and testing on all target platforms using Travis CI. :: git push github branch/2023-01-06/speed-hax:branch/2023-01-06/speed-hax You will need to wait for results from Travis CI. [Add details of - how to see them. RB 2023-07-01] [Some sort of - ``--force-with-lease`` option may be required if we're pushing back - to the same branch we started with, such as when it's a Ravenbrook - development branch. RB 2023-07-01] + how to see them. RB 2023-07-01] See step 5 about what to do if tests do not pass. @@ -162,26 +162,27 @@ omitted for now. RB 2023-01-07] Git Fusion mapping, and so the result is the same as if it had come in via Perforce. -6. Replace the master with your branch, effecting the merge:: +7. Replace the master with your branch, effecting the merge:: git checkout master git merge --ff-only branch/2023-01-06/speed-hax - [There should not have been any further changes on master, and - ``--ff-only`` checks for that. The merge commit we want on master - is made in step 4. RB 2023-01-07] + The ``--ff-only`` flag ensures there have been no changes on master + since step 4, so that the testing is valid for master, and we do + not create a second merge commit. If this fails, go back to + step 4. -7. Push master and the branch to Perforce via Git Fusion:: +8. Push master and the branch to Perforce via Git Fusion:: git push ravenbrook master branch/2023-01-06/speed-hax - If this fails because someone else has submitted changes to the - master codeline since you started, pull those changes and go back - to step 3 :: + If this fails because someone has submitted changes to the master + codeline since you started, pull those changes and go back to step + 4. :: git pull ravenbrook master -8. After a bit [how long? RB 2023-01-07] check that gitpushbot has +9. After a bit [how long? RB 2023-01-07] check that gitpushbot has pushed the result to the Ravenbrook MPS repo on GitHub. [And do what if it doesn't? RB 2023-01-07] From 6799312f8b3a540a43abeff2d03bd2ab94fc6790 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 7 Jan 2023 23:03:28 +0000 Subject: [PATCH 16/93] Adding a step to create a "social lock" on the perforce masters to avoid a race. --- mps/procedure/pull-request-merge.rst | 30 ++++++++++++++++------------ 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index cd9e0e49edf..e8a5de21b0c 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -119,7 +119,11 @@ These steps will only rarely need repeating. `causes permanent pollution in the Ravenbrook Perforce repository `_. -4. Merge master with the branch:: +4. Optionally, let other people know that you're about to merge into + master, and negotiate to avoid racing them to step 9 and making + extra work for everyone. + +5. Merge master with the branch:: git checkout branch/2023-01-06/speed-hax git pull ravenbrook master:master @@ -135,19 +139,19 @@ These steps will only rarely need repeating. fast-forwards, which we don't want, because they discard records of the merge happening, including who did it. RB 2023-01-07] -5. Build and test the results locally. For example:: +6. Build and test the results locally. For example:: make -C code -f lii6gc.gmk testci testansi testpollnone testmmqa See `design.mps.tests <../design/tests.txt>`_ for details and other platforms. - If tests do not pass, review your conflict resolution from step 4, + If tests do not pass, review your conflict resolution from step 5, and if that doesn't resolve things, the procedure fails, and you need to go back to the source of the branch, e.g. the pull request and its original author. Something's wrong! -6. Push the branch to the Ravenbrook MPS GitHub repository to trigger +7. Push the branch to the Ravenbrook MPS GitHub repository to trigger building and testing on all target platforms using Travis CI. :: git push github branch/2023-01-06/speed-hax:branch/2023-01-06/speed-hax @@ -155,36 +159,36 @@ These steps will only rarely need repeating. You will need to wait for results from Travis CI. [Add details of how to see them. RB 2023-07-01] - See step 5 about what to do if tests do not pass. + See step 6 about what to do if tests do not pass. Note: This potentially creates a branch in the GitHub repo ahead of Git Fusion doing so, but it will the same name, because of the Git Fusion mapping, and so the result is the same as if it had come in via Perforce. -7. Replace the master with your branch, effecting the merge:: +8. Replace the master with your branch, effecting the merge:: git checkout master git merge --ff-only branch/2023-01-06/speed-hax The ``--ff-only`` flag ensures there have been no changes on master - since step 4, so that the testing is valid for master, and we do + since step 5, so that the testing is valid for master, and we do not create a second merge commit. If this fails, go back to - step 4. + step 5. -8. Push master and the branch to Perforce via Git Fusion:: +9. Push master and the branch to Perforce via Git Fusion:: git push ravenbrook master branch/2023-01-06/speed-hax If this fails because someone has submitted changes to the master codeline since you started, pull those changes and go back to step - 4. :: + 5. :: git pull ravenbrook master -9. After a bit [how long? RB 2023-01-07] check that gitpushbot has - pushed the result to the Ravenbrook MPS repo on GitHub. [And do - what if it doesn't? RB 2023-01-07] +10. After a bit [how long? RB 2023-01-07] check that gitpushbot has + pushed the result to the Ravenbrook MPS repo on GitHub. [And do + what if it doesn't? RB 2023-01-07] A. References From 11d63a5140bc2662fbefe9849948307e7b9eaaba Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 7 Jan 2023 23:11:55 +0000 Subject: [PATCH 17/93] Fixing link markup. --- mps/procedure/pull-request-merge.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index e8a5de21b0c..1f78c0c1f90 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -99,7 +99,7 @@ These steps will only rarely need repeating. 1. Check that the contributor has not expressed any kind of variation on the licensing of their work from the material to which they are contributing. (See `"Licensing" in "Contributing to the MPS" - <../contributing.rst#licensing>`.) + <../contributing.rst#licensing>`_.) If they have, this procedure fails. Talk to them or get someone to talk to them about the matter. From ff9992970d43f21f6c18d9281849ed8094577ea5 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 7 Jan 2023 23:48:24 +0000 Subject: [PATCH 18/93] Comments become rationale in late-night rant. --- mps/procedure/pull-request-merge.rst | 74 ++++++++++++++++++++++++++-- 1 file changed, 69 insertions(+), 5 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 1f78c0c1f90..e44c737b0f6 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -134,11 +134,6 @@ These steps will only rarely need repeating. branch. If you still can't resolve conflicts, this procedure fails. - [What would be the default outcome from the GitHub interface, using - the "merge" button? [Chaser324_2017]_ claims it only works for - fast-forwards, which we don't want, because they discard records of - the merge happening, including who did it. RB 2023-01-07] - 6. Build and test the results locally. For example:: make -C code -f lii6gc.gmk testci testansi testpollnone testmmqa @@ -191,9 +186,78 @@ These steps will only rarely need repeating. what if it doesn't? RB 2023-01-07] +5. Rationale +------------ + +This section explains why the procedure is like it is. It's intended +for people who want to vary the procedure on the fly, or make +permanent changes to it. In the latter case, update this section! + +5.1. Why not press the GitHub merge button? +------------------------------------------- + +GitHub provides a merge button on pull requests. According to +[Chaser324_2017]_ it only works for branches that can fast-forward +master, and also only creates fast-forwards. + +There are two reasons this is undesirable. + +Firstly, it's quite likely that a pull request has a branch that isn't +at the tip of master and can't be fast-forwarded. It's possible to +rebase such branches only if Perforce has never seen them, because +Perforce does not permit branch history to be rewritten. We could +have a more complicated procedure involving making a new rebased +branch, but the result would be less good. + +Secondly, we would like to avoid rewriting history and the destruction +of information on the grounds that it is bad software engineering, and +so want to discourage rebasing. + +And it's for this reason we also want to avoid fast-forwards of +master. A fast-forward means there is no commit that records the fact +that there has been a merge, by whom, from where, etc. It discards +that information. Therefore we want to discourage fast-forwards of +master in favour of merges. + +Given this, step 8 may seem odd, since it fast-forwards master. But +in fact it's pointing master at the merge commit created in step 5, so +that master has a history including a proper merge. + +5.2. Why the "durable" branch names? +------------------------------------ + +It's common in Git culture to delete branches once they've been +merged [Ardalis_2017]_ but this destroys information that has been +invaluable to MPS quality in the past. + +It destroys the connection between the branch name and a series of +changes made together, intentionally, for a purpose. That makes it +hard to identify those changes together. It makes it hard to *refer* +to those changes from documents and code (referring to the hash of the +last commit is not as good). It makes it hard to investigate the +intention of changes discovered by tools such as ``git blame`` or ``p4 +annotate``. + +Essentially, it throws away history and dissolves the branch into the +big global graph of git commits. That's not good configuration +management. + +The MPS has an ongoing policy of retaining all of its intentional +history, and that includes branch names. Branch names in the MPS +repository are intended to last forever. That is why they have +"durable" names. + +This policy has persisted over decades through more than one SCM +system, and will persist when Git has been replaced by the next one. + + A. References ------------- +.. [Ardalis_2017] "Why Delete Old Git Branches?"; Steve Ardalis; + 2017-07-20; + . + .. [Chaser324_2017] "GitHub Standard Fork & Pull Request Workflow"; Chase Pettit; 2017; . From 65a4f924cfd326d8d7a094c39d53fb4a3ae720dc Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sun, 8 Jan 2023 00:12:43 +0000 Subject: [PATCH 19/93] Improving prerequisite wording. --- mps/procedure/pull-request-merge.rst | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index e44c737b0f6..2b79970e51d 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -73,16 +73,15 @@ omitted for now. RB 2023-01-07] These steps will only rarely need repeating. -#. Ensure your public key is in Perforce at - //.git-fusion/users/USER/keys/ in Perforce, submitting it if - necessary. +#. Ensure your public key is submitted in Perforce at + //.git-fusion/users/USER/keys/ -#. Ensure your e-mail address is in Perforce at - //.git-fusion/users/p4gf_usermap in Perforce and matches your - Perforce user record, submitting it if necessary. +#. Ensure your e-mail address is submitted in Perforce at + //.git-fusion/users/p4gf_usermap and matches your Perforce user + record. #. Clone the Ravenbrook MPS GitHub repository and name the remote - "github", giving (inter alia) access to Travis CI to build and test + "github". This will give you access to Travis CI to build and test the merge. :: git clone -o github git@github.com:Ravenbrook/mps.git From 4c01d17b606c45b1b9a11d09dec36f44ca76ff30 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sun, 8 Jan 2023 08:19:34 +0000 Subject: [PATCH 20/93] Adding an option step to push to github promptly, and what to do if it doesn't happen automatically. --- mps/procedure/pull-request-merge.rst | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 2b79970e51d..b0d21a7b822 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -118,6 +118,10 @@ These steps will only rarely need repeating. `causes permanent pollution in the Ravenbrook Perforce repository `_. + [How does this affect the connection between the branch and the + pull request and issues? We want to retain that connection + forever. RB 2023-01-08] + 4. Optionally, let other people know that you're about to merge into master, and negotiate to avoid racing them to step 9 and making extra work for everyone. @@ -180,9 +184,23 @@ These steps will only rarely need repeating. git pull ravenbrook master -10. After a bit [how long? RB 2023-01-07] check that gitpushbot has - pushed the result to the Ravenbrook MPS repo on GitHub. [And do - what if it doesn't? RB 2023-01-07] +10. If and *only if* step 9 succeeds, you can optionally push to + GitHub:: + + git push github master branch/2023-01-06/speed-hax + + If you don't do this, then within `30 minutes + `_ + check that the merge appears in `the commits in the Ravenbrook MPS + repo on GitHub`_. + + If they do not appear: + + 1. Check email for error messages from gitpushbot and resolve them. + + 2. Check (or ask a sysadmin to check) that gitpushbot is running + on Berunda and restart it if necessary, or ask a sysadmin to do + this. 5. Rationale From 23760e16523ae46b091028027d35b5a898f18df7 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sun, 8 Jan 2023 08:37:18 +0000 Subject: [PATCH 21/93] Adding instructions for checking travis ci results. this raises the possibility of insisting on a clean trial merge before even starting this procedure. --- mps/procedure/pull-request-merge.rst | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index b0d21a7b822..14256e96af5 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -52,17 +52,32 @@ reference to that project. RB 2023-01-07] #. Has there been a code review? #. Does the branch build and pass tests on all target platforms? - [This may be validated by Travis CI. Insert details here. RB - 2023-01-07] + [This may have been validated by Travis CI if the branch is in the + Ravenbrook MPS repo on GitHub. If not, we could add a step to push + it there for this purpose. RB 2023-01-08] #. Does the branch merge cleanly into the masters? If not, consider a catch-up merge from the masters. [This may be validated by GitHub and/or Travis CI. Insert details here. RB 2023-01-07] [This is duplicated by step 3 of the procedure. RB 2023-01-07] -#. Does a merge of the branch with master build and pass tests on all - target platforms? [This may be validated by Travis CI. Insert - details here. RB 2023-01-07] +#. Does the branch merge cleanly in to master and pass tests on all + target platforms? + + Travis CI should have run builds. To check, either: + + - Look for "All checks have passed" in the pull request on GitHub, + expand "Show all checks", and look for build success messages + from Travis CI. + + - Look for a successful build in the `Travis CI build history for + the repo + `_. + + [Failure here is not necessarily a failure of this procedure, since + this procedure includes conflict resolution and testing the result. + Insisiting on this step would move that work upstream, which might + be efficient. RB 2023-01-08] [Checklist items for Customer-specific branches from branch-merge.rst omitted for now. RB 2023-01-07] From 621821886b3b98d0b6db405b2dda9028a23ce54f Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sun, 8 Jan 2023 09:29:22 +0000 Subject: [PATCH 22/93] Explaining prerequisite checks for build results and what to do about them. --- mps/procedure/pull-request-merge.rst | 51 ++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 15 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 14256e96af5..05c54287525 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -51,33 +51,54 @@ reference to that project. RB 2023-01-07] #. Has there been a code review? -#. Does the branch build and pass tests on all target platforms? - [This may have been validated by Travis CI if the branch is in the - Ravenbrook MPS repo on GitHub. If not, we could add a step to push - it there for this purpose. RB 2023-01-08] +#. Does the branch build and pass tests on all `target platforms + <../readme.txt>`_? -#. Does the branch merge cleanly into the masters? If not, consider a - catch-up merge from the masters. [This may be validated by GitHub - and/or Travis CI. Insert details here. RB 2023-01-07] [This is - duplicated by step 3 of the procedure. RB 2023-01-07] + If the branch is in the Ravenbrook MPS repo on GitHub then Travis + CI should have run builds. Look for a successful build in the + `Travis CI build history for the repo + `_. If + there is a failed build you should not execute this procedure, but + talk to the contributor about fixing the branch. + + If the branch is in the Ravenbrook MPS repo on GitHub and Travis + builds are missing, inform sysadmins that Travis CI isn't + functioning. + + If you have no build and test results, you can still execute this + procedure, with caution. #. Does the branch merge cleanly in to master and pass tests on all target platforms? - Travis CI should have run builds. To check, either: + Travis CI should have run builds of the pull request (i.e. `of a + merge with master + `_). + To check, either: - - Look for "All checks have passed" in the pull request on GitHub, - expand "Show all checks", and look for build success messages + - Look for "All checks have passed" in the pull request on GitHub. + Expand "Show all checks", and look for build success messages from Travis CI. - Look for a successful build in the `Travis CI build history for the repo `_. - [Failure here is not necessarily a failure of this procedure, since - this procedure includes conflict resolution and testing the result. - Insisiting on this step would move that work upstream, which might - be efficient. RB 2023-01-08] + Success by Travis CI is a strong indication that this procecure + will be quick and successful. + + If Travis builds failed, you can still execute this procedure if + you believe that the failure is due to merge conflicts that you are + willing to resolve. + + If Travis builds are missing, inform sysadmins that Travis CI isn't + functioning. + + If you have no build and test results for the merge, then you can + still execute this procedure if: + # you believe there are only merge conflicts, + # you're willing to try to resolve those conflicts, and + # you're prepared to test on all target platforms. [Checklist items for Customer-specific branches from branch-merge.rst omitted for now. RB 2023-01-07] From d48bd2343a8f17144bfc78e0fd9f747dbf18518a Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sun, 8 Jan 2023 09:32:22 +0000 Subject: [PATCH 23/93] Fixing rst validation errors. --- mps/procedure/pull-request-merge.rst | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 05c54287525..54f27be71d4 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -56,10 +56,9 @@ reference to that project. RB 2023-01-07] If the branch is in the Ravenbrook MPS repo on GitHub then Travis CI should have run builds. Look for a successful build in the - `Travis CI build history for the repo - `_. If - there is a failed build you should not execute this procedure, but - talk to the contributor about fixing the branch. + `Travis CI build history for the repo`_. If there is a failed + build you should not execute this procedure, but talk to the + contributor about fixing the branch. If the branch is in the Ravenbrook MPS repo on GitHub and Travis builds are missing, inform sysadmins that Travis CI isn't @@ -81,8 +80,7 @@ reference to that project. RB 2023-01-07] from Travis CI. - Look for a successful build in the `Travis CI build history for - the repo - `_. + the repo`_. Success by Travis CI is a strong indication that this procecure will be quick and successful. @@ -103,6 +101,8 @@ reference to that project. RB 2023-01-07] [Checklist items for Customer-specific branches from branch-merge.rst omitted for now. RB 2023-01-07] +.. _Travis CI build history for the repo: https://app.travis-ci.com/github/Ravenbrook/mps/builds + 3. Prerequisite steps --------------------- @@ -228,7 +228,7 @@ These steps will only rarely need repeating. If you don't do this, then within `30 minutes `_ check that the merge appears in `the commits in the Ravenbrook MPS - repo on GitHub`_. + repo on GitHub `_. If they do not appear: From 5545f4c9f107802851ca6f93b7ff98938022b78e Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sun, 8 Jan 2023 09:40:55 +0000 Subject: [PATCH 24/93] Clarifying that you can use your existing repo. --- mps/procedure/pull-request-merge.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 54f27be71d4..67e8615cd98 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -118,7 +118,8 @@ These steps will only rarely need repeating. #. Clone the Ravenbrook MPS GitHub repository and name the remote "github". This will give you access to Travis CI to build and test - the merge. :: + the merge. (If you're an MPS developer you can use your existing + repo.) :: git clone -o github git@github.com:Ravenbrook/mps.git From c8a13aae4e3afb12d9c162093ac052d1761fa282 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sun, 8 Jan 2023 09:49:12 +0000 Subject: [PATCH 25/93] Moving licensing check to checklist to simplify main procedure. --- mps/procedure/pull-request-merge.rst | 74 +++++++++++++++------------- 1 file changed, 39 insertions(+), 35 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 67e8615cd98..02727cdef6a 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -51,6 +51,16 @@ reference to that project. RB 2023-01-07] #. Has there been a code review? +#. Has the contributor licensed their work? + + By default, the work is licensed if the contributor has not + expressed any kind of variation on the licensing of their work from + the material to which they are contributing. (See `"Licensing" in + "Contributing to the MPS" <../contributing.rst#licensing>`_.) + + If they have, yalk to them or get someone to talk to them about the + matter. Do not proceed. + #. Does the branch build and pass tests on all `target platforms <../readme.txt>`_? @@ -132,20 +142,12 @@ These steps will only rarely need repeating. 4. Merging a development branch ------------------------------- -1. Check that the contributor has not expressed any kind of variation - on the licensing of their work from the material to which they are - contributing. (See `"Licensing" in "Contributing to the MPS" - <../contributing.rst#licensing>`_.) - - If they have, this procedure fails. Talk to them or get someone to - talk to them about the matter. - -2. If the branch to be merged is in a third-party repo (such as a +1. If the branch to be merged is in a third-party repo (such as a fork), then add that, e.g.:: git remote add captain-contrib https://gitlab.com/captcontrib/mps.git -3. Fetch the branch that you are going to merge to a local branch +2. Fetch the branch that you are going to merge to a local branch using the MPS durable branch naming convention, "branch/DATE/TOPIC", e.g. :: @@ -159,11 +161,11 @@ These steps will only rarely need repeating. pull request and issues? We want to retain that connection forever. RB 2023-01-08] -4. Optionally, let other people know that you're about to merge into - master, and negotiate to avoid racing them to step 9 and making - extra work for everyone. +3. Optionally, let other people know that you're about to merge into + master, and negotiate to avoid racing them to the push (step 8) and + making extra work for everyone. -5. Merge master with the branch:: +4. Merge master with the branch:: git checkout branch/2023-01-06/speed-hax git pull ravenbrook master:master @@ -174,7 +176,7 @@ These steps will only rarely need repeating. branch. If you still can't resolve conflicts, this procedure fails. -6. Build and test the results locally. For example:: +5. Build and test the results locally. For example:: make -C code -f lii6gc.gmk testci testansi testpollnone testmmqa @@ -186,7 +188,7 @@ These steps will only rarely need repeating. need to go back to the source of the branch, e.g. the pull request and its original author. Something's wrong! -7. Push the branch to the Ravenbrook MPS GitHub repository to trigger +6. Push the branch to the Ravenbrook MPS GitHub repository to trigger building and testing on all target platforms using Travis CI. :: git push github branch/2023-01-06/speed-hax:branch/2023-01-06/speed-hax @@ -201,7 +203,7 @@ These steps will only rarely need repeating. Git Fusion mapping, and so the result is the same as if it had come in via Perforce. -8. Replace the master with your branch, effecting the merge:: +7. Replace the master with your branch, effecting the merge:: git checkout master git merge --ff-only branch/2023-01-06/speed-hax @@ -211,33 +213,33 @@ These steps will only rarely need repeating. not create a second merge commit. If this fails, go back to step 5. -9. Push master and the branch to Perforce via Git Fusion:: +8. Push master and the branch to Perforce via Git Fusion:: git push ravenbrook master branch/2023-01-06/speed-hax If this fails because someone has submitted changes to the master - codeline since you started, pull those changes and go back to step - 5. :: + codeline since you started, pull those changes and go back to + merging (step 4). :: git pull ravenbrook master -10. If and *only if* step 9 succeeds, you can optionally push to - GitHub:: +9. If and *only if* the Perforce push (step 8) succeeds, you can + optionally push to GitHub:: - git push github master branch/2023-01-06/speed-hax + git push github master branch/2023-01-06/speed-hax - If you don't do this, then within `30 minutes - `_ - check that the merge appears in `the commits in the Ravenbrook MPS - repo on GitHub `_. + If you don't do this, then within `30 minutes + `_ + check that the merge appears in `the commits in the Ravenbrook MPS + repo on GitHub `_. - If they do not appear: + If they do not appear: - 1. Check email for error messages from gitpushbot and resolve them. + 1. Check email for error messages from gitpushbot and resolve them. - 2. Check (or ask a sysadmin to check) that gitpushbot is running - on Berunda and restart it if necessary, or ask a sysadmin to do - this. + 2. Check (or ask a sysadmin to check) that gitpushbot is running + on Berunda and restart it if necessary, or ask a sysadmin to do + this. 5. Rationale @@ -273,9 +275,11 @@ that there has been a merge, by whom, from where, etc. It discards that information. Therefore we want to discourage fast-forwards of master in favour of merges. -Given this, step 8 may seem odd, since it fast-forwards master. But -in fact it's pointing master at the merge commit created in step 5, so -that master has a history including a proper merge. +Given this, the replace (step 7) may seem odd, since it fast-forwards +master. But in fact it's pointing master at the merge commit created +earlier (step 5), so that master has a history including a proper +merge. + 5.2. Why the "durable" branch names? ------------------------------------ From 4acce47ff005a20e3ebf61954e44696aa49045a3 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sun, 8 Jan 2023 09:52:14 +0000 Subject: [PATCH 26/93] Updating master first, so that problems with that are more likely to occur in context. --- mps/procedure/pull-request-merge.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 02727cdef6a..6899b1b03f3 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -167,8 +167,8 @@ These steps will only rarely need repeating. 4. Merge master with the branch:: - git checkout branch/2023-01-06/speed-hax git pull ravenbrook master:master + git checkout branch/2023-01-06/speed-hax git merge master You may need to resolve conflicts. If you can't resolve conflicts From 76b93abb11a9ee301bf5ea1dad56924a0b9eaefc Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sun, 8 Jan 2023 09:56:07 +0000 Subject: [PATCH 27/93] Correcting step cross-references. --- mps/procedure/pull-request-merge.rst | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 6899b1b03f3..6997ff6f3f0 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -183,10 +183,10 @@ These steps will only rarely need repeating. See `design.mps.tests <../design/tests.txt>`_ for details and other platforms. - If tests do not pass, review your conflict resolution from step 5, - and if that doesn't resolve things, the procedure fails, and you - need to go back to the source of the branch, e.g. the pull request - and its original author. Something's wrong! + If tests do not pass, review your conflict resolution from the + merge (step 4), and if that doesn't resolve things, the procedure + fails, and you need to go back to the source of the branch, + e.g. the pull request and its original author. Something's wrong! 6. Push the branch to the Ravenbrook MPS GitHub repository to trigger building and testing on all target platforms using Travis CI. :: @@ -196,7 +196,7 @@ These steps will only rarely need repeating. You will need to wait for results from Travis CI. [Add details of how to see them. RB 2023-07-01] - See step 6 about what to do if tests do not pass. + See build (step 5) about what to do if tests do not pass. Note: This potentially creates a branch in the GitHub repo ahead of Git Fusion doing so, but it will the same name, because of the @@ -209,9 +209,9 @@ These steps will only rarely need repeating. git merge --ff-only branch/2023-01-06/speed-hax The ``--ff-only`` flag ensures there have been no changes on master - since step 5, so that the testing is valid for master, and we do - not create a second merge commit. If this fails, go back to - step 5. + since merging (step 4), so that the testing is valid for master, + and we do not create a second merge commit. If this fails, go back + to merging (step 4). 8. Push master and the branch to Perforce via Git Fusion:: @@ -224,7 +224,7 @@ These steps will only rarely need repeating. git pull ravenbrook master 9. If and *only if* the Perforce push (step 8) succeeds, you can - optionally push to GitHub:: + also push to GitHub:: git push github master branch/2023-01-06/speed-hax From 0494e3017d07d74219f9ecb2a4d02a3eca2b5e82 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sun, 8 Jan 2023 10:00:50 +0000 Subject: [PATCH 28/93] Fixing typo. --- mps/procedure/pull-request-merge.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 6997ff6f3f0..b1d30afa936 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -58,7 +58,7 @@ reference to that project. RB 2023-01-07] the material to which they are contributing. (See `"Licensing" in "Contributing to the MPS" <../contributing.rst#licensing>`_.) - If they have, yalk to them or get someone to talk to them about the + If they have, talk to them or get someone to talk to them about the matter. Do not proceed. #. Does the branch build and pass tests on all `target platforms From 77ca5d370b6492575a8a98e68a142dc8c650cadc Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sun, 8 Jan 2023 15:24:20 +0000 Subject: [PATCH 29/93] Fixing list markup. --- mps/procedure/pull-request-merge.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index b1d30afa936..bfce18b7765 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -104,9 +104,9 @@ reference to that project. RB 2023-01-07] If you have no build and test results for the merge, then you can still execute this procedure if: - # you believe there are only merge conflicts, - # you're willing to try to resolve those conflicts, and - # you're prepared to test on all target platforms. + #. you believe there are only merge conflicts, + #. you're willing to try to resolve those conflicts, and + #. you're prepared to test on all target platforms. [Checklist items for Customer-specific branches from branch-merge.rst omitted for now. RB 2023-01-07] From 77570c933ba15d1573e47830b9d40135b8901991 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sun, 8 Jan 2023 15:25:48 +0000 Subject: [PATCH 30/93] Adding reference to git migration issue. --- mps/procedure/pull-request-merge.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index bfce18b7765..80d91e6da2c 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -26,9 +26,9 @@ MPS Help" [GDR_2020-09-03]_ . The document is still draft. Some of the questions that need resolving are noted in square brackets. -Ravenbrook is currently (2023-01) migrating the MPS project to git -(and GitHub) and this is likely to modify this procedure. [Insert -reference to that project. RB 2023-01-07] +Ravenbrook is currently (2023-01) `migrating the MPS project to git +(and GitHub) `_ and this +is likely to modify and simplify this procedure. 2. Pre-merge checklist From e508a9828b17073e1392350b746b0f66dd70a7a1 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Mon, 9 Jan 2023 08:18:13 +0000 Subject: [PATCH 31/93] Fixing numbered list markup. --- mps/procedure/pull-request-merge.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 80d91e6da2c..76a8d11abba 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -104,6 +104,7 @@ is likely to modify and simplify this procedure. If you have no build and test results for the merge, then you can still execute this procedure if: + #. you believe there are only merge conflicts, #. you're willing to try to resolve those conflicts, and #. you're prepared to test on all target platforms. From 9a76be9323536ef91a657f93e6b3281a671df335 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Mon, 9 Jan 2023 08:21:40 +0000 Subject: [PATCH 32/93] If there's a race on master we must back out our update. there might be a better method. --- mps/procedure/pull-request-merge.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 76a8d11abba..7b8d2ce2bd0 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -219,10 +219,10 @@ These steps will only rarely need repeating. git push ravenbrook master branch/2023-01-06/speed-hax If this fails because someone has submitted changes to the master - codeline since you started, pull those changes and go back to - merging (step 4). :: + codeline since you started. Replace your master with those changes + and go back to merging (step 4). :: - git pull ravenbrook master + git pull --force ravenbrook master:master 9. If and *only if* the Perforce push (step 8) succeeds, you can also push to GitHub:: From 66bc29f94816928801651c696f1b0cd215d00281 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Mon, 9 Jan 2023 08:34:21 +0000 Subject: [PATCH 33/93] Removing redundant branch name in push command. --- mps/procedure/pull-request-merge.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 7b8d2ce2bd0..283ee658650 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -192,7 +192,7 @@ These steps will only rarely need repeating. 6. Push the branch to the Ravenbrook MPS GitHub repository to trigger building and testing on all target platforms using Travis CI. :: - git push github branch/2023-01-06/speed-hax:branch/2023-01-06/speed-hax + git push github branch/2023-01-06/speed-hax You will need to wait for results from Travis CI. [Add details of how to see them. RB 2023-07-01] From f6798d378bb08ce4d6c3d2189340e9cea63c12ea Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Mon, 9 Jan 2023 08:39:26 +0000 Subject: [PATCH 34/93] Naming the perforce git fusion remote "perforce" for clarity. --- mps/procedure/pull-request-merge.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 283ee658650..ec529e2675b 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -137,7 +137,7 @@ These steps will only rarely need repeating. #. Add the Git Fusion mps-public repo, which is the interface to Ravenbrook's Perforce. :: - git remote add ravenbrook ssh://git@perforce.ravenbrook.com:1622/mps-public + git remote add perforce ssh://git@perforce.ravenbrook.com:1622/mps-public 4. Merging a development branch @@ -168,7 +168,7 @@ These steps will only rarely need repeating. 4. Merge master with the branch:: - git pull ravenbrook master:master + git pull perforce master:master git checkout branch/2023-01-06/speed-hax git merge master @@ -216,13 +216,13 @@ These steps will only rarely need repeating. 8. Push master and the branch to Perforce via Git Fusion:: - git push ravenbrook master branch/2023-01-06/speed-hax + git push perforce master branch/2023-01-06/speed-hax If this fails because someone has submitted changes to the master codeline since you started. Replace your master with those changes and go back to merging (step 4). :: - git pull --force ravenbrook master:master + git pull --force perforce master:master 9. If and *only if* the Perforce push (step 8) succeeds, you can also push to GitHub:: From 7cc5256cabca8b8e35f84c613fb69731848072f6 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Mon, 9 Jan 2023 08:44:24 +0000 Subject: [PATCH 35/93] Clarifying what replacing master achieves. --- mps/procedure/pull-request-merge.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index ec529e2675b..d609debc511 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -204,7 +204,7 @@ These steps will only rarely need repeating. Git Fusion mapping, and so the result is the same as if it had come in via Perforce. -7. Replace the master with your branch, effecting the merge:: +7. Replace the master with your merged branch:: git checkout master git merge --ff-only branch/2023-01-06/speed-hax From 2ef7d58767bbf89c3c9682b444348a625ceb81d5 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Mon, 9 Jan 2023 09:00:47 +0000 Subject: [PATCH 36/93] Pulling the branch to be merged directly from the pr ref at github, removing a step. --- mps/procedure/pull-request-merge.rst | 57 ++++++++++++++-------------- 1 file changed, 28 insertions(+), 29 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index d609debc511..8753c8051b6 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -143,30 +143,27 @@ These steps will only rarely need repeating. 4. Merging a development branch ------------------------------- -1. If the branch to be merged is in a third-party repo (such as a - fork), then add that, e.g.:: +1. `Fetch the pull request branch`_ to a local branch using the MPS + durable branch naming convention, "branch/DATE/TOPIC", e.g. :: + + git fetch github pull/93/head:branch/2022-12-23/hardened-runtime + + If the branch to be merged is in a third-party repo, such as a fork + not on GitHub, you can fetch it usina a remote, e.g.:: git remote add captain-contrib https://gitlab.com/captcontrib/mps.git - -2. Fetch the branch that you are going to merge to a local branch - using the MPS durable branch naming convention, - "branch/DATE/TOPIC", e.g. :: - git fetch captain-contrib mps-speed-hax:branch/2023-01-06/speed-hax - Double check you've got this right. Using the wrong branch naming - `causes permanent pollution in the Ravenbrook Perforce repository + Double check you've got the branch name right. Using the wrong + branch naming `causes permanent pollution in the Ravenbrook + Perforce repository `_. - [How does this affect the connection between the branch and the - pull request and issues? We want to retain that connection - forever. RB 2023-01-08] +2. Optionally, let other people know that you're working on a merge + into master. Negotiate to avoid racing them to the push (step 7) + because that will create extra merging work. -3. Optionally, let other people know that you're about to merge into - master, and negotiate to avoid racing them to the push (step 8) and - making extra work for everyone. - -4. Merge master with the branch:: +3. Merge master with the branch:: git pull perforce master:master git checkout branch/2023-01-06/speed-hax @@ -177,7 +174,7 @@ These steps will only rarely need repeating. branch. If you still can't resolve conflicts, this procedure fails. -5. Build and test the results locally. For example:: +4. Build and test the results locally. For example:: make -C code -f lii6gc.gmk testci testansi testpollnone testmmqa @@ -185,11 +182,11 @@ These steps will only rarely need repeating. platforms. If tests do not pass, review your conflict resolution from the - merge (step 4), and if that doesn't resolve things, the procedure + merge (step 3), and if that doesn't resolve things, the procedure fails, and you need to go back to the source of the branch, e.g. the pull request and its original author. Something's wrong! -6. Push the branch to the Ravenbrook MPS GitHub repository to trigger +5. Push the branch to the Ravenbrook MPS GitHub repository to trigger building and testing on all target platforms using Travis CI. :: git push github branch/2023-01-06/speed-hax @@ -197,34 +194,34 @@ These steps will only rarely need repeating. You will need to wait for results from Travis CI. [Add details of how to see them. RB 2023-07-01] - See build (step 5) about what to do if tests do not pass. + See build (step 4) about what to do if tests do not pass. Note: This potentially creates a branch in the GitHub repo ahead of Git Fusion doing so, but it will the same name, because of the Git Fusion mapping, and so the result is the same as if it had come in via Perforce. -7. Replace the master with your merged branch:: +6. Replace the master with your merged branch:: git checkout master git merge --ff-only branch/2023-01-06/speed-hax The ``--ff-only`` flag ensures there have been no changes on master - since merging (step 4), so that the testing is valid for master, + since merging (step 3), so that the testing is valid for master, and we do not create a second merge commit. If this fails, go back - to merging (step 4). + to merging (step 3). -8. Push master and the branch to Perforce via Git Fusion:: +7. Push master and the branch to Perforce via Git Fusion:: git push perforce master branch/2023-01-06/speed-hax If this fails because someone has submitted changes to the master codeline since you started. Replace your master with those changes - and go back to merging (step 4). :: + and go back to merging (step 3). :: git pull --force perforce master:master -9. If and *only if* the Perforce push (step 8) succeeds, you can +8. If and *only if* the Perforce push (step 7) succeeds, you can also push to GitHub:: git push github master branch/2023-01-06/speed-hax @@ -242,6 +239,8 @@ These steps will only rarely need repeating. on Berunda and restart it if necessary, or ask a sysadmin to do this. +.. _Fetch the pull request branch: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally#modifying-an-inactive-pull-request-locally + 5. Rationale ------------ @@ -276,9 +275,9 @@ that there has been a merge, by whom, from where, etc. It discards that information. Therefore we want to discourage fast-forwards of master in favour of merges. -Given this, the replace (step 7) may seem odd, since it fast-forwards +Given this, the replace (step 6) may seem odd, since it fast-forwards master. But in fact it's pointing master at the merge commit created -earlier (step 5), so that master has a history including a proper +earlier (step 4), so that master has a history including a proper merge. From b786e7be3221b53ca922472bfb0644957c421695 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Mon, 9 Jan 2023 09:21:32 +0000 Subject: [PATCH 37/93] Simplifying the procedure by avoiding pushing via the local master, removing the strange fast-forward step. --- mps/procedure/pull-request-merge.rst | 38 ++++++++++------------------ 1 file changed, 13 insertions(+), 25 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 8753c8051b6..422804329b8 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -160,8 +160,8 @@ These steps will only rarely need repeating. `_. 2. Optionally, let other people know that you're working on a merge - into master. Negotiate to avoid racing them to the push (step 7) - because that will create extra merging work. + into master. Negotiate to avoid racing them to push to the master + codeline (step 7) because that will create extra merging work. 3. Merge master with the branch:: @@ -182,7 +182,7 @@ These steps will only rarely need repeating. platforms. If tests do not pass, review your conflict resolution from the - merge (step 3), and if that doesn't resolve things, the procedure + merge (step 3), and if that doesn't fix things, the procedure fails, and you need to go back to the source of the branch, e.g. the pull request and its original author. Something's wrong! @@ -201,30 +201,23 @@ These steps will only rarely need repeating. Git Fusion mapping, and so the result is the same as if it had come in via Perforce. -6. Replace the master with your merged branch:: +6. Submit your merged branch to Perforce:: - git checkout master - git merge --ff-only branch/2023-01-06/speed-hax + git push Perforce branch/2023-01-06/speed-hax - The ``--ff-only`` flag ensures there have been no changes on master - since merging (step 3), so that the testing is valid for master, - and we do not create a second merge commit. If this fails, go back - to merging (step 3). +7. Submit your merged branch to the Perforce master codeline:: -7. Push master and the branch to Perforce via Git Fusion:: + git push perforce branch/2023-01-06/speed-hax:master - git push perforce master branch/2023-01-06/speed-hax + **Important**: Do *not* force this push. - If this fails because someone has submitted changes to the master - codeline since you started. Replace your master with those changes - and go back to merging (step 3). :: + If this fails, someone has submitted changes to the master codeline + since you started. Go back to merging (step 3). - git pull --force perforce master:master +8. Optionally, if and *only if* the Perforce push (step 7) succeeded, + you can also push to GitHub:: -8. If and *only if* the Perforce push (step 7) succeeds, you can - also push to GitHub:: - - git push github master branch/2023-01-06/speed-hax + git push github branch/2023-01-06/speed-hax:master If you don't do this, then within `30 minutes `_ @@ -275,11 +268,6 @@ that there has been a merge, by whom, from where, etc. It discards that information. Therefore we want to discourage fast-forwards of master in favour of merges. -Given this, the replace (step 6) may seem odd, since it fast-forwards -master. But in fact it's pointing master at the merge commit created -earlier (step 4), so that master has a history including a proper -merge. - 5.2. Why the "durable" branch names? ------------------------------------ From bf04002ce45250d534298cfd5697bc93cc551e96 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Mon, 9 Jan 2023 09:22:54 +0000 Subject: [PATCH 38/93] Updating old procedure section title inherited from [gdr_2014-01-09]. --- mps/procedure/pull-request-merge.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 422804329b8..74dfaecabf8 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -140,8 +140,8 @@ These steps will only rarely need repeating. git remote add perforce ssh://git@perforce.ravenbrook.com:1622/mps-public -4. Merging a development branch -------------------------------- +4. Merging procedure +-------------------- 1. `Fetch the pull request branch`_ to a local branch using the MPS durable branch naming convention, "branch/DATE/TOPIC", e.g. :: From 0e8a92ead9b0bceb76930127c47fe58e653871c0 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Mon, 9 Jan 2023 09:29:18 +0000 Subject: [PATCH 39/93] Avoid renaming correctly named branches. --- mps/procedure/pull-request-merge.rst | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 74dfaecabf8..7916be5b93d 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -146,7 +146,11 @@ These steps will only rarely need repeating. 1. `Fetch the pull request branch`_ to a local branch using the MPS durable branch naming convention, "branch/DATE/TOPIC", e.g. :: - git fetch github pull/93/head:branch/2022-12-23/hardened-runtime + git fetch github pull/93/head:branch/2023-01-06/speed-hax + + If the pull request is from the Ravenbrook MPS repo on GitHub then + its branch may already have a conventional name. Use the existing + name. If the branch to be merged is in a third-party repo, such as a fork not on GitHub, you can fetch it usina a remote, e.g.:: From aa27d852fefe408c16ac9c22137b10b0c1e11757 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Mon, 9 Jan 2023 09:31:09 +0000 Subject: [PATCH 40/93] Improving language and fixing a typo. --- mps/procedure/pull-request-merge.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 7916be5b93d..381d212a01d 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -148,12 +148,12 @@ These steps will only rarely need repeating. git fetch github pull/93/head:branch/2023-01-06/speed-hax - If the pull request is from the Ravenbrook MPS repo on GitHub then - its branch may already have a conventional name. Use the existing + If the pull request is from the Ravenbrook MPS repo on GitHub, and + its branch already has a conventional name, then use the existing name. If the branch to be merged is in a third-party repo, such as a fork - not on GitHub, you can fetch it usina a remote, e.g.:: + not on GitHub, you can fetch it using a remote, e.g.:: git remote add captain-contrib https://gitlab.com/captcontrib/mps.git git fetch captain-contrib mps-speed-hax:branch/2023-01-06/speed-hax From c2126e37d9d201c24f3ba50bac79898e0f01d984 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Tue, 10 Jan 2023 08:08:55 +0000 Subject: [PATCH 41/93] Adding note about [chaser324_2017]_ being misleading about the merge button. --- mps/procedure/pull-request-merge.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 381d212a01d..72abee500df 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -253,6 +253,10 @@ GitHub provides a merge button on pull requests. According to [Chaser324_2017]_ it only works for branches that can fast-forward master, and also only creates fast-forwards. +[This might not be true. See `About merge methods on GitHub +`_. +RB 2023-01-09] + There are two reasons this is undesirable. Firstly, it's quite likely that a pull request has a branch that isn't From 4269b3ef15bf25948713ca7baa86f0720453af3e Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Tue, 10 Jan 2023 08:10:53 +0000 Subject: [PATCH 42/93] Attempt to disable builds for this branch, branch/2023-01-07/pull-request-merge-procedure, in response to travis ci support case 42422. --- mps/.travis.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/mps/.travis.yml b/mps/.travis.yml index 8b62dfe0d72..3ab2d602d40 100644 --- a/mps/.travis.yml +++ b/mps/.travis.yml @@ -1,5 +1,11 @@ # .travis.yml -- Travis CI configuration for the MPS # $Id$ + +# Some branches don't need builds. Add them here to avoid using build +# resources and unnecessary build messages. See +# . +if: NOT branch IN (branch/2023-01-07/pull-request-merge-procedure) + # See . language: c os: From b8c867842f200eba594005f4c5f81c8069ac9a65 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Tue, 10 Jan 2023 08:54:34 +0000 Subject: [PATCH 43/93] I was mislead by [chaser324_2017]. removed it and arguments from it. improved the rationale. explained how and when we can use the github merge button. --- mps/procedure/pull-request-merge.rst | 86 +++++++++++++++++++--------- 1 file changed, 59 insertions(+), 27 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 72abee500df..367fb0afa94 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -246,38 +246,74 @@ This section explains why the procedure is like it is. It's intended for people who want to vary the procedure on the fly, or make permanent changes to it. In the latter case, update this section! -5.1. Why not press the GitHub merge button? + +5.1. Why not rebase or squash merge? +------------------------------------ + +We would like to avoid rewriting history and the destruction of +information on the grounds that it destroys information that could be +important to the engineering of the MPS, such as tracking down +defects, comprehending the intention of changes. So want to +discourage rebasing or squashing. + +We want to avoid fast-forwards of master. A fast-forward means there +is no commit that records the fact that there has been a merge, by +whom, from where, for what purpose, etc. It discards that +information. Therefore we want to discourage fast-forwards of master +in favour of merges. (Annoyingly, GitHub only provides `branch +protection that enforces the opposite +`_!) +See also `5.3. Why the "durable" branch names?`_. + +We also want to avoid `squash merges +`_. +A squash merge compresses development history into a single commit, +destroying the record of what happened during development and the +connection to the branch. + +The main motivation for fast-forwards and squashes appears to be to +simplify the branching history so that it's easier to understand. +Better tools and interfaces are no doubt required for analysing Git +history. These will emerge. And they will be able to analyse the +history that we are creating today. + +There is also a strong tendency among developers to "correct" mistakes +and edit history to reflect "what should have happened" or "what I +meant to do", treating history like code. But it's the function of +version control to protect software against well-intentioned mistakes. +Git is bad at remembering changes to history (it has no meta-history) +and so we should not edit it. + + +5.2. Why not press the GitHub merge button? ------------------------------------------- -GitHub provides a merge button on pull requests. According to -[Chaser324_2017]_ it only works for branches that can fast-forward -master, and also only creates fast-forwards. +We cannot use the GitHub pull request merge button because it would +put the GitHub master branch out of sync with (ahead of) Perforce. +Currently, Perforce is the authoritative home of the MPS, and the Git +repository is a mirror. -[This might not be true. See `About merge methods on GitHub -`_. -RB 2023-01-09] +According to `GitHub's "About pull request merges" +`_: -There are two reasons this is undesirable. + When you click the default Merge pull request option on a pull + request on GitHub.com, all commits from the feature branch are added + to the base branch in a merge commit. -Firstly, it's quite likely that a pull request has a branch that isn't -at the tip of master and can't be fast-forwarded. It's possible to -rebase such branches only if Perforce has never seen them, because -Perforce does not permit branch history to be rewritten. We could -have a more complicated procedure involving making a new rebased -branch, but the result would be less good. +`Travis CI builds and tests this merge in advance `_: -Secondly, we would like to avoid rewriting history and the destruction -of information on the grounds that it is bad software engineering, and -so want to discourage rebasing. + Rather than build the commits that have been pushed to the branch + the pull request is from, we build the merge between the source + branch and the upstream branch. -And it's for this reason we also want to avoid fast-forwards of -master. A fast-forward means there is no commit that records the fact -that there has been a merge, by whom, from where, etc. It discards -that information. Therefore we want to discourage fast-forwards of -master in favour of merges. +So, `once Git becomes the home +`_ we will be able to use +the button to to replace sections 3 and 4, the procedure, but not +section 2, the pre-merge checklist. We may be able to incorporate the +checklist into GitHub's interface. -5.2. Why the "durable" branch names? +5.3. Why the "durable" branch names? ------------------------------------ It's common in Git culture to delete branches once they've been @@ -312,10 +348,6 @@ A. References 2017-07-20; . -.. [Chaser324_2017] "GitHub Standard Fork & Pull Request Workflow"; - Chase Pettit; 2017; - . - .. [GDR_2020-09-03] "Re: Possible MPS help"; Gareth Rees; 2020-09-03; . From af6d0732a2012d8043db3b0781d69b161ac90195 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Tue, 10 Jan 2023 09:04:28 +0000 Subject: [PATCH 44/93] Minor clarifications. --- mps/procedure/pull-request-merge.rst | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 367fb0afa94..0fb6f63c323 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -17,6 +17,10 @@ Memory Pool System pull request merge procedure This document contains a procedure for merging a branch received via a GitHub "pull request". +Ravenbrook is currently (2023-01) `migrating the MPS project to git +(and GitHub) `_ and that +will greatly simplify this procedure. + This document was created as a combination of the process improvement steps from our Perforce-based "Memory Pool System branching and merging procedures" [GDR_2014-01-09]_ with Gareth Rees' email @@ -26,10 +30,6 @@ MPS Help" [GDR_2020-09-03]_ . The document is still draft. Some of the questions that need resolving are noted in square brackets. -Ravenbrook is currently (2023-01) `migrating the MPS project to git -(and GitHub) `_ and this -is likely to modify and simplify this procedure. - 2. Pre-merge checklist ---------------------- @@ -120,7 +120,7 @@ omitted for now. RB 2023-01-07] These steps will only rarely need repeating. -#. Ensure your public key is submitted in Perforce at +#. Ensure your public SSH key is submitted in Perforce at //.git-fusion/users/USER/keys/ #. Ensure your e-mail address is submitted in Perforce at From d2625504b21a478df90806b90889624e3d1812ce Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Tue, 10 Jan 2023 09:05:03 +0000 Subject: [PATCH 45/93] Removing comment about customer-specific branches. this case is covered by asking whether the contribution is licensed. --- mps/procedure/pull-request-merge.rst | 3 --- 1 file changed, 3 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 0fb6f63c323..1d9dc117adc 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -109,9 +109,6 @@ resolving are noted in square brackets. #. you're willing to try to resolve those conflicts, and #. you're prepared to test on all target platforms. -[Checklist items for Customer-specific branches from branch-merge.rst -omitted for now. RB 2023-01-07] - .. _Travis CI build history for the repo: https://app.travis-ci.com/github/Ravenbrook/mps/builds From 5f0a241514c0de4d84d9aaa4c2847cefa757b73f Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Tue, 10 Jan 2023 10:17:38 +0000 Subject: [PATCH 46/93] Adding reference to github branch protection rules. --- mps/procedure/pull-request-merge.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 1d9dc117adc..f00d1d1d588 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -337,6 +337,14 @@ repository are intended to last forever. That is why they have This policy has persisted over decades through more than one SCM system, and will persist when Git has been replaced by the next one. +Note: `GitHub branch protection rules`_ are `enabled +`_ on the +`Ravenbrook MPS repo on GitHub`_ and should prevent deletion. + +.. _Ravenbrook MPS repo on GitHub: https://github.com/Ravenbrook/mps + +.. _GitHub branch protection rules: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/about-protected-branches#require-linear-history + A. References ------------- From b05ce68ac5ec02c849317f5da48f28a1241475a2 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Tue, 10 Jan 2023 10:19:11 +0000 Subject: [PATCH 47/93] Linking refs to github repo for ease-of-use. --- mps/procedure/pull-request-merge.rst | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index f00d1d1d588..3eb4a73a723 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -64,13 +64,13 @@ resolving are noted in square brackets. #. Does the branch build and pass tests on all `target platforms <../readme.txt>`_? - If the branch is in the Ravenbrook MPS repo on GitHub then Travis - CI should have run builds. Look for a successful build in the - `Travis CI build history for the repo`_. If there is a failed + If the branch is in the `Ravenbrook MPS repo on GitHub`_ then + Travis CI should have run builds. Look for a successful build in + the `Travis CI build history for the repo`_. If there is a failed build you should not execute this procedure, but talk to the contributor about fixing the branch. - If the branch is in the Ravenbrook MPS repo on GitHub and Travis + If the branch is in the `Ravenbrook MPS repo on GitHub`_ and Travis builds are missing, inform sysadmins that Travis CI isn't functioning. @@ -145,9 +145,9 @@ These steps will only rarely need repeating. git fetch github pull/93/head:branch/2023-01-06/speed-hax - If the pull request is from the Ravenbrook MPS repo on GitHub, and - its branch already has a conventional name, then use the existing - name. + If the pull request is from the `Ravenbrook MPS repo on GitHub`_, + and its branch already has a conventional name, then use the + existing name. If the branch to be merged is in a third-party repo, such as a fork not on GitHub, you can fetch it using a remote, e.g.:: From d416e05f5edeb34f1eaa1037e4033f908d4aed13 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Tue, 10 Jan 2023 10:19:38 +0000 Subject: [PATCH 48/93] Removing draft status. --- mps/procedure/pull-request-merge.rst | 4 ---- 1 file changed, 4 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 3eb4a73a723..fa0b372cd5f 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -8,7 +8,6 @@ Memory Pool System pull request merge procedure :confidentiality: public :copyright: See `C. Copyright and License`_ :readership: MPS developers, trainee integrators -:status: draft 1. Introduction @@ -27,9 +26,6 @@ merging procedures" [GDR_2014-01-09]_ with Gareth Rees' email containing the working steps for git / GitHub merges, "Re: Possible MPS Help" [GDR_2020-09-03]_ . -The document is still draft. Some of the questions that need -resolving are noted in square brackets. - 2. Pre-merge checklist ---------------------- From 7ed8b2b3bd58f5c6c6e1a5d520eee7c8980992d2 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Tue, 10 Jan 2023 11:04:56 +0000 Subject: [PATCH 49/93] Adding link to pull request template docs. --- mps/procedure/pull-request-merge.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index fa0b372cd5f..6275644dbbe 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -303,7 +303,8 @@ So, `once Git becomes the home `_ we will be able to use the button to to replace sections 3 and 4, the procedure, but not section 2, the pre-merge checklist. We may be able to incorporate the -checklist into GitHub's interface. +checklist into GitHub's interface using a `pull request template +`_. 5.3. Why the "durable" branch names? From 02c6e59b542800fa9f5448f77f70a1c300c2dd3c Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Tue, 10 Jan 2023 13:34:37 +0000 Subject: [PATCH 50/93] Adding note about varying the procedure for pull requests not on github. --- mps/procedure/pull-request-merge.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 6275644dbbe..46dd1e258d6 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -20,6 +20,11 @@ Ravenbrook is currently (2023-01) `migrating the MPS project to git (and GitHub) `_ and that will greatly simplify this procedure. +This procedure assumes a pull request has been received via GitHub, +but that dependency is light. The procedure can be varied for other +sources, even where the "pull request" is received by email or some +other traceable document. + This document was created as a combination of the process improvement steps from our Perforce-based "Memory Pool System branching and merging procedures" [GDR_2014-01-09]_ with Gareth Rees' email From 85f8165fa9860de361a96fe44ea7a9071bb6b667 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Tue, 10 Jan 2023 22:13:07 +0000 Subject: [PATCH 51/93] Using travis ci's "blocklisting" to exclude branches, rather than a conditional expression, for easier management and diffs. --- mps/.travis.yml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/mps/.travis.yml b/mps/.travis.yml index 3ab2d602d40..1c2e08d5a71 100644 --- a/mps/.travis.yml +++ b/mps/.travis.yml @@ -3,8 +3,10 @@ # Some branches don't need builds. Add them here to avoid using build # resources and unnecessary build messages. See -# . -if: NOT branch IN (branch/2023-01-07/pull-request-merge-procedure) +# . +branches: + except: + - branch/2023-01-07/pull-request-merge-procedure # See . language: c From 960e8bd855b7c32a4faf2c8c3a3886ff6b6290e1 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Wed, 11 Jan 2023 02:08:30 +0000 Subject: [PATCH 52/93] Adding instructions for the commit message on the merge. --- mps/procedure/pull-request-merge.rst | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 46dd1e258d6..5d220e82658 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -171,6 +171,16 @@ These steps will only rarely need repeating. git checkout branch/2023-01-06/speed-hax git merge master + Edit the commit message to say something like: + + Merging branch/2023-01-06/speed-hax for pull request 93 + . + + Do *not* just say "pull request 93" without a link, because that + number is local to, and only valid on GitHub. Bear this in mind + for other references. Do add any other links that would increase + traceability. + You may need to resolve conflicts. If you can't resolve conflicts yourself, you may need to involve the original author of the branch. If you still can't resolve conflicts, this procedure From 34946c79355c431dfcdbd7582dfb56d844b62ff5 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Thu, 12 Jan 2023 18:15:35 +0000 Subject: [PATCH 53/93] Attempt to prevent github rewriting links to github by using a code quotation. --- mps/procedure/pull-request-merge.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 5d220e82658..3ca480b1604 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -171,7 +171,7 @@ These steps will only rarely need repeating. git checkout branch/2023-01-06/speed-hax git merge master - Edit the commit message to say something like: + Edit the commit message to say something like:: Merging branch/2023-01-06/speed-hax for pull request 93 . From 443f656564ed3418c64b42bf506cc3d29210cd0b Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Thu, 12 Jan 2023 21:34:30 +0000 Subject: [PATCH 54/93] Implementing a more natural strategy by separating the use of ci from updating the branch. see also . --- mps/procedure/pull-request-merge.rst | 52 +++++++++++++++++++--------- 1 file changed, 35 insertions(+), 17 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 3ca480b1604..bdee18f929c 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -165,15 +165,21 @@ These steps will only rarely need repeating. into master. Negotiate to avoid racing them to push to the master codeline (step 7) because that will create extra merging work. -3. Merge master with the branch:: +3. Ensure your local master is up to date with Perforce:: - git pull perforce master:master - git checkout branch/2023-01-06/speed-hax - git merge master + git pull --ff-only perforce master + + If this doesn't succeed, then GitHub's master and Perforce's master + are in out of sync, and this procedure fails. [It may be possible + to quickly fix that here and now and continue. RB 2023-01-12] + +4. Merge the branch in to your local master:: + + git merge --no-ff branch/2023-01-06/speed-hax Edit the commit message to say something like:: - Merging branch/2023-01-06/speed-hax for pull request 93 + Merging branch/2023-01-06/speed-hax for GitHub pull request 93 . Do *not* just say "pull request 93" without a link, because that @@ -186,7 +192,9 @@ These steps will only rarely need repeating. branch. If you still can't resolve conflicts, this procedure fails. -4. Build and test the results locally. For example:: +5. [This step is only necessary if the merge was non-trivial, there + has been rebasing, or CI results are not available. RB 2023-01-12] + Build and test the results locally. For example:: make -C code -f lii6gc.gmk testci testansi testpollnone testmmqa @@ -194,11 +202,13 @@ These steps will only rarely need repeating. platforms. If tests do not pass, review your conflict resolution from the - merge (step 3), and if that doesn't fix things, the procedure + merge (step 4), and if that doesn't fix things, the procedure fails, and you need to go back to the source of the branch, e.g. the pull request and its original author. Something's wrong! -5. Push the branch to the Ravenbrook MPS GitHub repository to trigger +6. [This step is only necessary if the merge was non-trivial, there + has been rebasing, or CI results are not available. RB 2023-01-12] + Push the branch to the Ravenbrook MPS GitHub repository to trigger building and testing on all target platforms using Travis CI. :: git push github branch/2023-01-06/speed-hax @@ -206,30 +216,38 @@ These steps will only rarely need repeating. You will need to wait for results from Travis CI. [Add details of how to see them. RB 2023-07-01] - See build (step 4) about what to do if tests do not pass. + See build (step 5) about what to do if tests do not pass. Note: This potentially creates a branch in the GitHub repo ahead of Git Fusion doing so, but it will the same name, because of the Git Fusion mapping, and so the result is the same as if it had come in via Perforce. -6. Submit your merged branch to Perforce:: +7. Submit your merged master and the branch to Perforce:: - git push Perforce branch/2023-01-06/speed-hax - -7. Submit your merged branch to the Perforce master codeline:: - - git push perforce branch/2023-01-06/speed-hax:master + git push perforce master branch/2023-01-06/speed-hax **Important**: Do *not* force this push. If this fails, someone has submitted changes to the master codeline - since you started. Go back to merging (step 3). + since you started. + + You can attempt to rebase your work on those changes:: + + git pull --rebase perforce + + then go back to testing (step 5). + + Alternatively, you could undo your merging work:: + + git reset --hard perforce/master + + then go back to merging (step 4). 8. Optionally, if and *only if* the Perforce push (step 7) succeeded, you can also push to GitHub:: - git push github branch/2023-01-06/speed-hax:master + git push github master branch/2023-01-06/speed-hax If you don't do this, then within `30 minutes `_ From 687ae30cf186cc71c49027fc6cd42841cc0aa68d Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Thu, 12 Jan 2023 21:53:51 +0000 Subject: [PATCH 55/93] Forgot to push to a fresh branch in the previous change. --- mps/procedure/pull-request-merge.rst | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index bdee18f929c..1f1f374fd5c 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -208,10 +208,11 @@ These steps will only rarely need repeating. 6. [This step is only necessary if the merge was non-trivial, there has been rebasing, or CI results are not available. RB 2023-01-12] - Push the branch to the Ravenbrook MPS GitHub repository to trigger - building and testing on all target platforms using Travis CI. :: + Push the merge to a fresh branch in the Ravenbrook MPS GitHub + repository to trigger building and testing on all target platforms + using Travis CI. :: - git push github branch/2023-01-06/speed-hax + git push github merge/2023-01-06/speed-hax You will need to wait for results from Travis CI. [Add details of how to see them. RB 2023-07-01] From 6261aa4c9165e60eae7443fc0ad91e2f0a26c896 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 13 Jan 2023 04:39:45 +0000 Subject: [PATCH 56/93] Adding instructions for the checklist. --- mps/procedure/pull-request-merge.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 1f1f374fd5c..94d911a2b65 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -35,6 +35,11 @@ MPS Help" [GDR_2020-09-03]_ . 2. Pre-merge checklist ---------------------- +The answers to these questions should be "yes". If the answer to a +question isn't "yes", record that, and why, in response to the pull +request (and maybe suggest what to do about it). When you finish the +checklist, decide whether to continue with the procedure. + #. Is there a permanent visible document (issue, job), referenced by the branch, recording the problem that is solved by the changes in the branch? From fa26d064c1d517b23a8039658b4f1804b65ad73d Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 13 Jan 2023 04:43:00 +0000 Subject: [PATCH 57/93] Clarifying what to do if contributor varies licensing. --- mps/procedure/pull-request-merge.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 94d911a2b65..588ef165635 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -64,8 +64,8 @@ checklist, decide whether to continue with the procedure. the material to which they are contributing. (See `"Licensing" in "Contributing to the MPS" <../contributing.rst#licensing>`_.) - If they have, talk to them or get someone to talk to them about the - matter. Do not proceed. + If they have expressed a variation, talk to them or get someone to + talk to them about the matter. *Do not proceed with merging*. #. Does the branch build and pass tests on all `target platforms <../readme.txt>`_? From b5d4a14b6bc5d3fcad41cc2e69597c15cbb7c438 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 13 Jan 2023 04:46:56 +0000 Subject: [PATCH 58/93] Adding step to configure git email address. --- mps/procedure/pull-request-merge.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 588ef165635..2635a046085 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -136,6 +136,12 @@ These steps will only rarely need repeating. repo.) :: git clone -o github git@github.com:Ravenbrook/mps.git + cd mps + +#. Set your e-mail address for commits to the repo to match the one in + your Perforce user record. :: + + git config user.email spqr@ravenbrook.com #. Add the Git Fusion mps-public repo, which is the interface to Ravenbrook's Perforce. :: From f5cffe8a97d644926542d002020272d55365da8d Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 13 Jan 2023 04:49:52 +0000 Subject: [PATCH 59/93] Adding example command for when the branch is already in the ravenbrook repo. --- mps/procedure/pull-request-merge.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 2635a046085..ce9a31587a8 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -159,7 +159,9 @@ These steps will only rarely need repeating. If the pull request is from the `Ravenbrook MPS repo on GitHub`_, and its branch already has a conventional name, then use the - existing name. + existing name, e.g. :: + + git fetch github branch/2023-01-06/speed-hax:branch/2023-01-06/speed-hax If the branch to be merged is in a third-party repo, such as a fork not on GitHub, you can fetch it using a remote, e.g.:: From ad8448741bb325c9475257efa4fdc92be6c12386 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 13 Jan 2023 04:55:22 +0000 Subject: [PATCH 60/93] Clarifying where to get the branch from and moving common case to top. --- mps/procedure/pull-request-merge.rst | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index ce9a31587a8..7b90fade11a 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -153,16 +153,18 @@ These steps will only rarely need repeating. -------------------- 1. `Fetch the pull request branch`_ to a local branch using the MPS - durable branch naming convention, "branch/DATE/TOPIC", e.g. :: + durable branch naming convention, "branch/DATE/TOPIC". - git fetch github pull/93/head:branch/2023-01-06/speed-hax - - If the pull request is from the `Ravenbrook MPS repo on GitHub`_, - and its branch already has a conventional name, then use the - existing name, e.g. :: + If branch is in the `Ravenbrook MPS repo on GitHub`_ and already + has a conventional name, then use the existing name, e.g. :: git fetch github branch/2023-01-06/speed-hax:branch/2023-01-06/speed-hax + Otherwise, if the pull request is from the `Ravenbrook MPS repo on + GitHub`_, fetch it from the pull request, e.g. :: + + git fetch github pull/93/head:branch/2023-01-06/speed-hax + If the branch to be merged is in a third-party repo, such as a fork not on GitHub, you can fetch it using a remote, e.g.:: From fd6409a346d45e4e696ef5b93f28a34b238ec977 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 13 Jan 2023 05:05:34 +0000 Subject: [PATCH 61/93] Explaining when to run tests. --- mps/procedure/pull-request-merge.rst | 34 ++++++++++++++++------------ 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 7b90fade11a..5642d766888 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -207,9 +207,14 @@ These steps will only rarely need repeating. branch. If you still can't resolve conflicts, this procedure fails. -5. [This step is only necessary if the merge was non-trivial, there - has been rebasing, or CI results are not available. RB 2023-01-12] - Build and test the results locally. For example:: +5. If either + + - the merge was non-trivial + - there has been any rebasing (see step 7) + - you don't have pull request build results from CI + + then build and test the merge result locally if possible. For + example:: make -C code -f lii6gc.gmk testci testansi testpollnone testmmqa @@ -221,24 +226,23 @@ These steps will only rarely need repeating. fails, and you need to go back to the source of the branch, e.g. the pull request and its original author. Something's wrong! -6. [This step is only necessary if the merge was non-trivial, there - has been rebasing, or CI results are not available. RB 2023-01-12] - Push the merge to a fresh branch in the Ravenbrook MPS GitHub - repository to trigger building and testing on all target platforms - using Travis CI. :: +6. If either + + - the merge was non-trivial + - there has been any rebasing (see step 7) + - you don't have pull request build results from CI + + then push the merge to a fresh branch in the `Ravenbrook MPS repo + on GitHub`_ to trigger CI to build and testing on all target + platforms. :: git push github merge/2023-01-06/speed-hax - You will need to wait for results from Travis CI. [Add details of - how to see them. RB 2023-07-01] + You will need to wait for results from CI. [Add details of how to + see them. RB 2023-07-01] See build (step 5) about what to do if tests do not pass. - Note: This potentially creates a branch in the GitHub repo ahead - of Git Fusion doing so, but it will the same name, because of the - Git Fusion mapping, and so the result is the same as if it had come - in via Perforce. - 7. Submit your merged master and the branch to Perforce:: git push perforce master branch/2023-01-06/speed-hax From 705b904e2f79b0a1a202a4b3204d0093d7db070b Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 13 Jan 2023 05:07:43 +0000 Subject: [PATCH 62/93] Updating document history. --- mps/procedure/pull-request-merge.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 5642d766888..e2ab33cddb7 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -417,6 +417,7 @@ B. Document History ========== ===== ================================================== 2023-01-07 RB_ Created. +2023-01-13 RB_ Updates after first attempt at execution. ========== ===== ================================================== .. _RB: mailto:rb@ravenbrook.com From 7603749803153799d0c092abd8614ba4e6a0b638 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 13 Jan 2023 11:37:57 +0000 Subject: [PATCH 63/93] Clarifying why we might pull from a pull request not a branch. --- mps/procedure/pull-request-merge.rst | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index e2ab33cddb7..2b8ce6d6379 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -139,7 +139,7 @@ These steps will only rarely need repeating. cd mps #. Set your e-mail address for commits to the repo to match the one in - your Perforce user record. :: + your Perforce user record, e.g. :: git config user.email spqr@ravenbrook.com @@ -155,16 +155,21 @@ These steps will only rarely need repeating. 1. `Fetch the pull request branch`_ to a local branch using the MPS durable branch naming convention, "branch/DATE/TOPIC". - If branch is in the `Ravenbrook MPS repo on GitHub`_ and already - has a conventional name, then use the existing name, e.g. :: + If the branch already has a conventional name, and it's in the + `Ravenbrook MPS repo on GitHub`_ then fetch it with the existing + name, e.g. :: git fetch github branch/2023-01-06/speed-hax:branch/2023-01-06/speed-hax - Otherwise, if the pull request is from the `Ravenbrook MPS repo on - GitHub`_, fetch it from the pull request, e.g. :: + Otherwise, if the pull request is in the `Ravenbrook MPS repo on + GitHub`_, fetch it from the pull request and give it a conventional + name, e.g. :: git fetch github pull/93/head:branch/2023-01-06/speed-hax + (This could happen if either the pull request is from a fork or the + branch has an unconventional name.) + If the branch to be merged is in a third-party repo, such as a fork not on GitHub, you can fetch it using a remote, e.g.:: From c00935463fdd33c684fcd0fa67199f5845c56475 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 13 Jan 2023 11:48:58 +0000 Subject: [PATCH 64/93] Clarifying what failure of pull might look like. --- mps/procedure/pull-request-merge.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 2b8ce6d6379..d85615f5797 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -189,9 +189,9 @@ These steps will only rarely need repeating. git pull --ff-only perforce master - If this doesn't succeed, then GitHub's master and Perforce's master - are in out of sync, and this procedure fails. [It may be possible - to quickly fix that here and now and continue. RB 2023-01-12] + If you get an error, then GitHub's master and Perforce's master are + in out of sync, and this procedure fails. [It may be possible to + quickly fix that here and now and continue. RB 2023-01-12] 4. Merge the branch in to your local master:: From 870427e899e1dc079bc017764c985f8871f09f07 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 13 Jan 2023 11:58:12 +0000 Subject: [PATCH 65/93] Explaining why we edit the merge comment. --- mps/procedure/pull-request-merge.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index d85615f5797..761425b910c 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -197,7 +197,8 @@ These steps will only rarely need repeating. git merge --no-ff branch/2023-01-06/speed-hax - Edit the commit message to say something like:: + Edit the commit message to link it to *why* you are merging. Say + something like:: Merging branch/2023-01-06/speed-hax for GitHub pull request 93 . From 74110d174573881db216f649a90abe0c3bb7426e Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 13 Jan 2023 13:06:05 +0000 Subject: [PATCH 66/93] Clarifying what "not having" ci results means. --- mps/procedure/pull-request-merge.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 761425b910c..6c3648ca912 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -217,7 +217,7 @@ These steps will only rarely need repeating. - the merge was non-trivial - there has been any rebasing (see step 7) - - you don't have pull request build results from CI + - you haven't checked pull request build results from CI then build and test the merge result locally if possible. For example:: @@ -236,7 +236,7 @@ These steps will only rarely need repeating. - the merge was non-trivial - there has been any rebasing (see step 7) - - you don't have pull request build results from CI + - you haven't checked pull request build results from CI then push the merge to a fresh branch in the `Ravenbrook MPS repo on GitHub`_ to trigger CI to build and testing on all target From aad77fafc871e7ef5d8f38e0cf4bbda5b476a049 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 13 Jan 2023 17:48:18 +0000 Subject: [PATCH 67/93] Clarifying not to start the merging procedure when there are licensing issues to resolve. --- mps/procedure/pull-request-merge.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 6c3648ca912..bfc5c6a4699 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -65,7 +65,8 @@ checklist, decide whether to continue with the procedure. "Contributing to the MPS" <../contributing.rst#licensing>`_.) If they have expressed a variation, talk to them or get someone to - talk to them about the matter. *Do not proceed with merging*. + talk to them about the matter. *Do not start the merging + procedure*. #. Does the branch build and pass tests on all `target platforms <../readme.txt>`_? From 22cc388cc380c83831bb4d5678d57df246a2e4f2 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 13 Jan 2023 18:07:14 +0000 Subject: [PATCH 68/93] Explaining more clearly where to find build results, and generalising from travis ci to ci in general. --- mps/procedure/pull-request-merge.rst | 56 ++++++++++++++++++---------- 1 file changed, 36 insertions(+), 20 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index bfc5c6a4699..2d66cf26674 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -71,15 +71,26 @@ checklist, decide whether to continue with the procedure. #. Does the branch build and pass tests on all `target platforms <../readme.txt>`_? - If the branch is in the `Ravenbrook MPS repo on GitHub`_ then - Travis CI should have run builds. Look for a successful build in - the `Travis CI build history for the repo`_. If there is a failed - build you should not execute this procedure, but talk to the - contributor about fixing the branch. + If the branch is in the `Ravenbrook MPS repo on GitHub`_ then CI + should have run builds of the branch. To check, either: - If the branch is in the `Ravenbrook MPS repo on GitHub`_ and Travis - builds are missing, inform sysadmins that Travis CI isn't - functioning. + - Look for build results for the branch (not the pull request) in + the "checks" section of the pull request on GitHub. Expand "Show + all checks", and look for build success messages *for the + branch*. + + - Look for a the most recent build of the branch in the `Travis CI + build history for the repo`_. + + - Look for the most recent build of the branch in the `GitHub + workflows for the repo`_. + + If there is a failed build you should not execute this procedure, + but talk to the contributor about fixing the branch. + + If the branch is in the `Ravenbrook MPS repo on GitHub`_ and builds + are missing from the "checks" section of pull request, inform + sysadmins that CI isn't functioning. If you have no build and test results, you can still execute this procedure, with caution. @@ -87,26 +98,29 @@ checklist, decide whether to continue with the procedure. #. Does the branch merge cleanly in to master and pass tests on all target platforms? - Travis CI should have run builds of the pull request (i.e. `of a - merge with master + CI should have run builds of the pull request (i.e. `of a merge + with master `_). To check, either: - - Look for "All checks have passed" in the pull request on GitHub. - Expand "Show all checks", and look for build success messages - from Travis CI. + - Look for build results for the pull request (not the branch) in + the pull request on GitHub. Expand "Show all checks", and look + for build success messages *for the pull request*. - - Look for a successful build in the `Travis CI build history for - the repo`_. + - Look for a successful build of the pull request in the `Travis CI + build history for the repo`_. - Success by Travis CI is a strong indication that this procecure - will be quick and successful. + - Look for the most recent build of the pull request in the `GitHub + workflows for the repo`_. - If Travis builds failed, you can still execute this procedure if - you believe that the failure is due to merge conflicts that you are + Success by CI is a strong indication that this procecure will be + quick and successful. + + If CI builds failed, you can still execute this procedure if you + believe that the failure is due to merge conflicts that you are willing to resolve. - If Travis builds are missing, inform sysadmins that Travis CI isn't + If CI builds are missing, inform sysadmins that CI isn't functioning. If you have no build and test results for the merge, then you can @@ -118,6 +132,8 @@ checklist, decide whether to continue with the procedure. .. _Travis CI build history for the repo: https://app.travis-ci.com/github/Ravenbrook/mps/builds +.. _GitHub workflows for the repo: https://github.com/Ravenbrook/mps/actions + 3. Prerequisite steps --------------------- From 8c2c7b9506a50949ab40b1c1f6b8d6002a153a3c Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 13 Jan 2023 18:12:45 +0000 Subject: [PATCH 69/93] Clarifying that "this procedure" in the checklist means the merging procedure, and linking it. --- mps/procedure/pull-request-merge.rst | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 2d66cf26674..8b8b734456f 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -85,15 +85,15 @@ checklist, decide whether to continue with the procedure. - Look for the most recent build of the branch in the `GitHub workflows for the repo`_. - If there is a failed build you should not execute this procedure, - but talk to the contributor about fixing the branch. + If there is a failed build you should not execute `the merging + procedure`_, but talk to the contributor about fixing the branch. If the branch is in the `Ravenbrook MPS repo on GitHub`_ and builds are missing from the "checks" section of pull request, inform sysadmins that CI isn't functioning. - If you have no build and test results, you can still execute this - procedure, with caution. + If you have no build and test results, you can still execute `the + merging procedure`_, with caution. #. Does the branch merge cleanly in to master and pass tests on all target platforms? @@ -116,15 +116,15 @@ checklist, decide whether to continue with the procedure. Success by CI is a strong indication that this procecure will be quick and successful. - If CI builds failed, you can still execute this procedure if you - believe that the failure is due to merge conflicts that you are - willing to resolve. + If CI builds failed, you can still execute `the merging procedure`_ + if you believe that the failure is due to merge conflicts that you + are willing to resolve. If CI builds are missing, inform sysadmins that CI isn't functioning. If you have no build and test results for the merge, then you can - still execute this procedure if: + still execute `the merging procedure`_ if: #. you believe there are only merge conflicts, #. you're willing to try to resolve those conflicts, and @@ -166,6 +166,8 @@ These steps will only rarely need repeating. git remote add perforce ssh://git@perforce.ravenbrook.com:1622/mps-public +.. _the merging procedure: + 4. Merging procedure -------------------- From 34baf1b0b3e8fb1af2d2c5c654980b4a2f12aa66 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 13 Jan 2023 18:16:57 +0000 Subject: [PATCH 70/93] Making it clearer how to do a fetch from a pull request by number. --- mps/procedure/pull-request-merge.rst | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 8b8b734456f..2f0c954cc52 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -182,7 +182,11 @@ These steps will only rarely need repeating. Otherwise, if the pull request is in the `Ravenbrook MPS repo on GitHub`_, fetch it from the pull request and give it a conventional - name, e.g. :: + name, like this :: + + git fetch github pull/$PR/head:$BRANCH + + For example :: git fetch github pull/93/head:branch/2023-01-06/speed-hax From a01b755b2095569b3996aeb624bf60c26532b846 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 13 Jan 2023 18:57:03 +0000 Subject: [PATCH 71/93] Further generalisation of ci to include both travis and github. --- mps/procedure/pull-request-merge.rst | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 2f0c954cc52..e0dc2db9168 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -38,7 +38,7 @@ MPS Help" [GDR_2020-09-03]_ . The answers to these questions should be "yes". If the answer to a question isn't "yes", record that, and why, in response to the pull request (and maybe suggest what to do about it). When you finish the -checklist, decide whether to continue with the procedure. +checklist, decide whether to start `the merging procedure`_. #. Is there a permanent visible document (issue, job), referenced by the branch, recording the problem that is solved by the changes in @@ -148,8 +148,8 @@ These steps will only rarely need repeating. record. #. Clone the Ravenbrook MPS GitHub repository and name the remote - "github". This will give you access to Travis CI to build and test - the merge. (If you're an MPS developer you can use your existing + "github". This will give you access to CI to build and test the + merge. (If you're an MPS developer you can use your existing repo.) :: git clone -o github git@github.com:Ravenbrook/mps.git @@ -381,6 +381,14 @@ According to `GitHub's "About pull request merges" the pull request is from, we build the merge between the source branch and the upstream branch. +When we use a GitHub CI on pull requests, that's also run on the merge +results. As `GitHub's pull request event documentation +`_ +says: + + GITHUB_SHA for this event is the last merge commit of the pull + request merge branch. + So, `once Git becomes the home `_ we will be able to use the button to to replace sections 3 and 4, the procedure, but not From 2357c2d23c927ac28e9dba8ae0ba0038875a20d4 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 14 Jan 2023 10:52:52 +0000 Subject: [PATCH 72/93] Fixing typo . --- mps/procedure/pull-request-merge.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index e0dc2db9168..dc32ab5eba1 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -113,7 +113,7 @@ checklist, decide whether to start `the merging procedure`_. - Look for the most recent build of the pull request in the `GitHub workflows for the repo`_. - Success by CI is a strong indication that this procecure will be + Success by CI is a strong indication that this procedure will be quick and successful. If CI builds failed, you can still execute `the merging procedure`_ From 95910ada301bb468e5e0ceb5c530cbd57a95e16e Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 14 Jan 2023 11:09:36 +0000 Subject: [PATCH 73/93] More neutral wording about fast-forwards and squashes, suggested in . --- mps/procedure/pull-request-merge.rst | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index dc32ab5eba1..d2c0df5e176 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -346,11 +346,11 @@ A squash merge compresses development history into a single commit, destroying the record of what happened during development and the connection to the branch. -The main motivation for fast-forwards and squashes appears to be to -simplify the branching history so that it's easier to understand. -Better tools and interfaces are no doubt required for analysing Git -history. These will emerge. And they will be able to analyse the -history that we are creating today. +Many developers use fast-forwards and squashes to simplify the +branching history so that it's easier to understand. Better tools and +interfaces are no doubt required for analysing Git history. These +will emerge. And they will be able to analyse the history that we are +creating today. There is also a strong tendency among developers to "correct" mistakes and edit history to reflect "what should have happened" or "what I From a970319cae8f57f5aa35f7f838303add93c43a9a Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 14 Jan 2023 11:47:37 +0000 Subject: [PATCH 74/93] Adding check for approval in response to . --- mps/procedure/pull-request-merge.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index d2c0df5e176..717761ec609 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -57,6 +57,14 @@ checklist, decide whether to start `the merging procedure`_. #. Has there been a code review? +#. Is the merge approved by an approving review? + + The `Ravenbrook MPS repo on GitHub`_ is set to require an approving + review and GitHub will block a push (step 8) without one. If you + push an unapproved merge to Perforce, this will cause gitpushbot to + fail, and leave Perforce and GitHub out of sync. *Do not push to + Perforce without GitHub approval.* + #. Has the contributor licensed their work? By default, the work is licensed if the contributor has not From 0f2eae8904e66edd2ee9b0cb158405f0ca1caf5b Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 14 Jan 2023 11:54:09 +0000 Subject: [PATCH 75/93] Updating document history with references back to github conversations. --- mps/procedure/pull-request-merge.rst | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 717761ec609..967d224f5ae 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -462,11 +462,16 @@ B. Document History ========== ===== ================================================== 2023-01-07 RB_ Created. -2023-01-13 RB_ Updates after first attempt at execution. +2023-01-13 RB_ Updates after `first attempt at execution`_. +2023-01-14 RB_ Updates after `second (successful) execution`_. ========== ===== ================================================== .. _RB: mailto:rb@ravenbrook.com +.. _first attempt at execution: https://github.com/Ravenbrook/mps/pull/97#issuecomment-1380206348 + +.. _second (successful) execution: https://github.com/Ravenbrook/mps/pull/97#issuecomment-1381771818 + C. Copyright and License ------------------------ From b0abeba6193f621e6ef38dc09193342c14341954 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 14 Jan 2023 12:04:06 +0000 Subject: [PATCH 76/93] Stating purpose of procedure in response to . --- mps/procedure/pull-request-merge.rst | 39 +++++++++++++++++++++------- 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 967d224f5ae..632618f4ecc 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -32,7 +32,23 @@ containing the working steps for git / GitHub merges, "Re: Possible MPS Help" [GDR_2020-09-03]_ . -2. Pre-merge checklist +2. Purpose +---------- + +The purpose of this procedure is: + +1. get required changes in to the MPS + +2. protect the MPS from introduction of defects + +3. maintain and protect the permanent record of the MPS in Perforce + and Git, so that defects can be discovered, fixed, and prevented + +As with any procedure, you can vary this one to meet this purpose, but +you should probably read section "`6. Rationale`_". + + +3. Pre-merge checklist ---------------------- The answers to these questions should be "yes". If the answer to a @@ -143,7 +159,7 @@ checklist, decide whether to start `the merging procedure`_. .. _GitHub workflows for the repo: https://github.com/Ravenbrook/mps/actions -3. Prerequisite steps +4. Prerequisite steps --------------------- These steps will only rarely need repeating. @@ -176,7 +192,7 @@ These steps will only rarely need repeating. .. _the merging procedure: -4. Merging procedure +5. Merging procedure -------------------- 1. `Fetch the pull request branch`_ to a local branch using the MPS @@ -322,15 +338,18 @@ These steps will only rarely need repeating. .. _Fetch the pull request branch: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally#modifying-an-inactive-pull-request-locally -5. Rationale +6. Rationale ------------ This section explains why the procedure is like it is. It's intended for people who want to vary the procedure on the fly, or make permanent changes to it. In the latter case, update this section! +[This section should argue the case in terms of section "`2. Purpose`_". +RB 2023-01-14] -5.1. Why not rebase or squash merge? + +6.1. Why not rebase or squash merge? ------------------------------------ We would like to avoid rewriting history and the destruction of @@ -346,7 +365,7 @@ information. Therefore we want to discourage fast-forwards of master in favour of merges. (Annoyingly, GitHub only provides `branch protection that enforces the opposite `_!) -See also `5.3. Why the "durable" branch names?`_. +See also `6.3. Why the "durable" branch names?`_. We also want to avoid `squash merges `_. @@ -368,7 +387,7 @@ Git is bad at remembering changes to history (it has no meta-history) and so we should not edit it. -5.2. Why not press the GitHub merge button? +6.2. Why not press the GitHub merge button? ------------------------------------------- We cannot use the GitHub pull request merge button because it would @@ -399,13 +418,13 @@ says: So, `once Git becomes the home `_ we will be able to use -the button to to replace sections 3 and 4, the procedure, but not -section 2, the pre-merge checklist. We may be able to incorporate the +the button to to replace sections 4 and 5, the procedure, but not +section 3, the pre-merge checklist. We may be able to incorporate the checklist into GitHub's interface using a `pull request template `_. -5.3. Why the "durable" branch names? +6.3. Why the "durable" branch names? ------------------------------------ It's common in Git culture to delete branches once they've been From b3c4ba106759587daf7678ffd770da8609325191 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 14 Jan 2023 12:20:08 +0000 Subject: [PATCH 77/93] Adding more thorough check that perforce is in sync with github in response to item 4. --- mps/procedure/pull-request-merge.rst | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 632618f4ecc..f32ae2d3447 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -237,8 +237,19 @@ These steps will only rarely need repeating. git pull --ff-only perforce master If you get an error, then GitHub's master and Perforce's master are - in out of sync, and this procedure fails. [It may be possible to - quickly fix that here and now and continue. RB 2023-01-12] + in out of sync, and this procedure fails. + + Ensure your local master is not ahead of Perforce:: + + git push --dry-run perforce + + If this shows anything other than "Everything up-to-date." then + GitHub's master and Perforce's master are in out of sync, and this + procedure fails. + + [It may be possible to fix that here and now and continue. That's + a subject for a whole nother procedure that we don't currently + have. RB 2023-01-12] 4. Merge the branch in to your local master:: From bdf879193c5d4c0915154ba4fabf9c66616d3dbe Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 14 Jan 2023 12:37:11 +0000 Subject: [PATCH 78/93] Unifying and simplifying checklist item for build results in response to . --- mps/procedure/pull-request-merge.rst | 69 ++++++++-------------------- 1 file changed, 19 insertions(+), 50 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index f32ae2d3447..327dcf9b58e 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -92,63 +92,32 @@ checklist, decide whether to start `the merging procedure`_. talk to them about the matter. *Do not start the merging procedure*. -#. Does the branch build and pass tests on all `target platforms - <../readme.txt>`_? +#. Does the branch, and its merge, build and pass tests? - If the branch is in the `Ravenbrook MPS repo on GitHub`_ then CI - should have run builds of the branch. To check, either: + CI should have run builds of both the branch, and a *trial merge* + of the pull request with master. Success by CI is a strong + indication that `the merging procedure`_ will be quick and + successful. - - Look for build results for the branch (not the pull request) in - the "checks" section of the pull request on GitHub. Expand "Show - all checks", and look for build success messages *for the - branch*. + Look for build results in the pull request on GitHub. Expand "Show + all checks", and look for build success messages for both the + branch and for the pull request (the trial merge). If these + results are missing, inform sysadmins that CI isn't functioning. - - Look for a the most recent build of the branch in the `Travis CI - build history for the repo`_. + You can also look for a build results in the `Travis CI build + history for the repo`_ and in the `GitHub workflows for the repo`_. - - Look for the most recent build of the branch in the `GitHub - workflows for the repo`_. + If there is a failed build *of the branch* you should not execute + `the merging procedure`_, but talk to the contributor about fixing + the branch. - If there is a failed build you should not execute `the merging - procedure`_, but talk to the contributor about fixing the branch. - - If the branch is in the `Ravenbrook MPS repo on GitHub`_ and builds - are missing from the "checks" section of pull request, inform - sysadmins that CI isn't functioning. - - If you have no build and test results, you can still execute `the - merging procedure`_, with caution. - -#. Does the branch merge cleanly in to master and pass tests on all - target platforms? - - CI should have run builds of the pull request (i.e. `of a merge - with master - `_). - To check, either: - - - Look for build results for the pull request (not the branch) in - the pull request on GitHub. Expand "Show all checks", and look - for build success messages *for the pull request*. - - - Look for a successful build of the pull request in the `Travis CI - build history for the repo`_. - - - Look for the most recent build of the pull request in the `GitHub - workflows for the repo`_. - - Success by CI is a strong indication that this procedure will be - quick and successful. - - If CI builds failed, you can still execute `the merging procedure`_ - if you believe that the failure is due to merge conflicts that you - are willing to resolve. - - If CI builds are missing, inform sysadmins that CI isn't - functioning. + If the branch built OK but there is a failed build *of the trial + merge*, you can still execute `the merging procedure`_ if you + believe that the failure is due to merge conflicts that you are + willing to resolve. If you have no build and test results for the merge, then you can - still execute `the merging procedure`_ if: + still try to execute `the merging procedure`_ if: #. you believe there are only merge conflicts, #. you're willing to try to resolve those conflicts, and From 7481604d840da41a0ab28bbfe70766ca48dcd3a8 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 14 Jan 2023 12:41:47 +0000 Subject: [PATCH 79/93] Clarifying that local/ci build step conditions are the same, in response to . --- mps/procedure/pull-request-merge.rst | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 327dcf9b58e..8f385688bcd 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -92,6 +92,8 @@ checklist, decide whether to start `the merging procedure`_. talk to them about the matter. *Do not start the merging procedure*. +.. _checked pull request build results from CI: + #. Does the branch, and its merge, build and pass tests? CI should have run builds of both the branch, and a *trial merge* @@ -240,11 +242,11 @@ These steps will only rarely need repeating. branch. If you still can't resolve conflicts, this procedure fails. -5. If either +5. Maybe build and test locally. If either - the merge was non-trivial - there has been any rebasing (see step 7) - - you haven't checked pull request build results from CI + - you haven't `checked pull request build results from CI`_ then build and test the merge result locally if possible. For example:: @@ -259,11 +261,11 @@ These steps will only rarely need repeating. fails, and you need to go back to the source of the branch, e.g. the pull request and its original author. Something's wrong! -6. If either +6. Maybe build and test using CI. As with step 5, if either - the merge was non-trivial - there has been any rebasing (see step 7) - - you haven't checked pull request build results from CI + - you haven't `checked pull request build results from CI`_ then push the merge to a fresh branch in the `Ravenbrook MPS repo on GitHub`_ to trigger CI to build and testing on all target From 53c4da918a5ecd91fcf8a0dadcf7f92b883af661 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 14 Jan 2023 12:45:46 +0000 Subject: [PATCH 80/93] Short explanation of basis for deciding after checklist, in response to . --- mps/procedure/pull-request-merge.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 8f385688bcd..83d604ab4ac 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -54,7 +54,8 @@ you should probably read section "`6. Rationale`_". The answers to these questions should be "yes". If the answer to a question isn't "yes", record that, and why, in response to the pull request (and maybe suggest what to do about it). When you finish the -checklist, decide whether to start `the merging procedure`_. +checklist, decide whether to start `the merging procedure`_ by +considering `2. Purpose`_. #. Is there a permanent visible document (issue, job), referenced by the branch, recording the problem that is solved by the changes in From 6faa049c7e16108ca72ccefba414630731fb5947 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 14 Jan 2023 12:54:25 +0000 Subject: [PATCH 81/93] Unfortunately, inserting this reference resets the numbered list. backed out for now. --- mps/procedure/pull-request-merge.rst | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 83d604ab4ac..feb2c92a23b 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -93,8 +93,6 @@ considering `2. Purpose`_. talk to them about the matter. *Do not start the merging procedure*. -.. _checked pull request build results from CI: - #. Does the branch, and its merge, build and pass tests? CI should have run builds of both the branch, and a *trial merge* @@ -247,7 +245,7 @@ These steps will only rarely need repeating. - the merge was non-trivial - there has been any rebasing (see step 7) - - you haven't `checked pull request build results from CI`_ + - there were failed or missing build results from CI then build and test the merge result locally if possible. For example:: @@ -266,7 +264,7 @@ These steps will only rarely need repeating. - the merge was non-trivial - there has been any rebasing (see step 7) - - you haven't `checked pull request build results from CI`_ + - there were failed or missing build results from CI then push the merge to a fresh branch in the `Ravenbrook MPS repo on GitHub`_ to trigger CI to build and testing on all target From 5f4a75df15144a286fcad6fe156b70700967af26 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 14 Jan 2023 12:58:49 +0000 Subject: [PATCH 82/93] Explaining where to look for build results in step 6. --- mps/procedure/pull-request-merge.rst | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index feb2c92a23b..251d27d23ec 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -267,13 +267,14 @@ These steps will only rarely need repeating. - there were failed or missing build results from CI then push the merge to a fresh branch in the `Ravenbrook MPS repo - on GitHub`_ to trigger CI to build and testing on all target - platforms. :: + on GitHub`_. This should trigger CI to build and testing on all + target platforms. :: git push github merge/2023-01-06/speed-hax - You will need to wait for results from CI. [Add details of how to - see them. RB 2023-07-01] + You will need to wait for results from CI. Look for a build + results in the `Travis CI build history for the repo`_ and in the + `GitHub workflows for the repo`_. See build (step 5) about what to do if tests do not pass. From e0566912dff5130b3c80dc89df5d238f1026a89b Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Sat, 14 Jan 2023 13:07:28 +0000 Subject: [PATCH 83/93] Linking justification of durable branch naming convention. adding step to eyeball results and protect against losing issues. --- mps/procedure/pull-request-merge.rst | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 251d27d23ec..35a85baaa40 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -166,7 +166,7 @@ These steps will only rarely need repeating. -------------------- 1. `Fetch the pull request branch`_ to a local branch using the MPS - durable branch naming convention, "branch/DATE/TOPIC". + `durable branch naming convention`_, "branch/DATE/TOPIC". If the branch already has a conventional name, and it's in the `Ravenbrook MPS repo on GitHub`_ then fetch it with the existing @@ -319,6 +319,11 @@ These steps will only rarely need repeating. .. _Fetch the pull request branch: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally#modifying-an-inactive-pull-request-locally +9. Eyeball the pull request and related issues on GitHub to make sure + the merge was recorded correctly. Check that any issues *not + completely resolved* by the merge were not closed. Re-open them if + necessary. + 6. Rationale ------------ @@ -406,6 +411,8 @@ checklist into GitHub's interface using a `pull request template `_. +.. _durable branch naming convention: + 6.3. Why the "durable" branch names? ------------------------------------ From 2f0b3be67b1696a067d008cba94cd434a77c9a3d Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Mon, 16 Jan 2023 22:24:11 +0000 Subject: [PATCH 84/93] Process improvement: check that the code review is recent. response to . --- mps/procedure/pull-request-merge.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 35a85baaa40..cae8d7cc5db 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -72,7 +72,7 @@ considering `2. Purpose`_. #. If the changes are significant and user-visible, is there an update to the release notes (``manual/source/release.rst``)? -#. Has there been a code review? +#. Has there been a code review, and only review edits since? #. Is the merge approved by an approving review? From 11429749ccc30f6ff36fdff2344d15291f875a96 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Thu, 19 Jan 2023 12:54:14 +0000 Subject: [PATCH 85/93] Adding instruction to record the revision of the procedure being followed in the pull request. --- mps/procedure/pull-request-merge.rst | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index cae8d7cc5db..9306676f611 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -53,9 +53,10 @@ you should probably read section "`6. Rationale`_". The answers to these questions should be "yes". If the answer to a question isn't "yes", record that, and why, in response to the pull -request (and maybe suggest what to do about it). When you finish the -checklist, decide whether to start `the merging procedure`_ by -considering `2. Purpose`_. +request (and maybe suggest what to do about it). Include a permalink +to the revision of this procedure that you are following. When you +finish the checklist, decide whether to start `the merging procedure`_ +by considering `2. Purpose`_. #. Is there a permanent visible document (issue, job), referenced by the branch, recording the problem that is solved by the changes in From d4c58298918e9ff3d0edbec266dfbbefb44bce62 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Fri, 20 Jan 2023 10:27:47 +0000 Subject: [PATCH 86/93] Adding document tag proc.merge.pull-request. --- mps/procedure/pull-request-merge.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 9306676f611..85ae4069118 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -2,6 +2,7 @@ Memory Pool System pull request merge procedure =============================================== +:tag: proc.merge.pull-request :author: Richard Brooksby :organization: Ravenbrook Limited :date: 2023-01-07 From 849ea9f4ddcccda6eb14367785ad4949f1f7c55e Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Mon, 23 Jan 2023 09:40:47 +0000 Subject: [PATCH 87/93] Adding time-to-execute estimates based on measurements. --- mps/procedure/pull-request-merge.rst | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 85ae4069118..596b3feed11 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -17,6 +17,16 @@ Memory Pool System pull request merge procedure This document contains a procedure for merging a branch received via a GitHub "pull request". +Time to execute: + +- first time: 2 hours +- second time: 30 minutes +- with practice: < 10 minutes + +these are measurements_ taken during simple merges that passed automated tests. + +.. _measurements: https://github.com/Ravenbrook/mps/pull/97#issuecomment-1381771818 + Ravenbrook is currently (2023-01) `migrating the MPS project to git (and GitHub) `_ and that will greatly simplify this procedure. @@ -474,6 +484,7 @@ B. Document History 2023-01-07 RB_ Created. 2023-01-13 RB_ Updates after `first attempt at execution`_. 2023-01-14 RB_ Updates after `second (successful) execution`_. +2023-01-23 RB_ Adding measurements. ========== ===== ================================================== .. _RB: mailto:rb@ravenbrook.com From ca219b51a9057e0b707db12b4f957da3926ccbcb Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Wed, 25 Jan 2023 21:22:29 +0000 Subject: [PATCH 88/93] Clarifying what "interface" means, in response to comment by @unaa008 . --- mps/procedure/pull-request-merge.rst | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 596b3feed11..c602ad17df2 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -79,7 +79,7 @@ by considering `2. Purpose`_. (without the branch) and that that the problem is solved (with the branch)? -#. If there are interface changes, are they documented? +#. If there changes to the `MPS interface`_, are they documented? #. If the changes are significant and user-visible, is there an update to the release notes (``manual/source/release.rst``)? @@ -140,6 +140,8 @@ by considering `2. Purpose`_. .. _GitHub workflows for the repo: https://github.com/Ravenbrook/mps/actions +.. _MPS interface: https://www.ravenbrook.com/project/mps/master/manual/html/topic/interface.html + 4. Prerequisite steps --------------------- @@ -329,13 +331,13 @@ These steps will only rarely need repeating. on Berunda and restart it if necessary, or ask a sysadmin to do this. -.. _Fetch the pull request branch: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally#modifying-an-inactive-pull-request-locally - 9. Eyeball the pull request and related issues on GitHub to make sure the merge was recorded correctly. Check that any issues *not completely resolved* by the merge were not closed. Re-open them if necessary. +.. _Fetch the pull request branch: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally#modifying-an-inactive-pull-request-locally + 6. Rationale ------------ From 28e1e17b6dbf3f93de71237bc7f88fa2ae3dd65d Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Wed, 25 Jan 2023 21:39:44 +0000 Subject: [PATCH 89/93] Clarifying levels of competence needed and risks of executing the procedure in response to mini-review by @thejayps and @unaa008 . --- mps/procedure/pull-request-merge.rst | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index c602ad17df2..af7820eafa2 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -148,6 +148,16 @@ by considering `2. Purpose`_. These steps will only rarely need repeating. +#. You need basic competence with Git: enough to understand the + commands in `the merging procedure`_. + +#. If the merge has conflicts, you will need competence in using Git + to resolve merge conflicts. + +#. If you want to vary the procedure, you will need to understand how + Perforce Git Fusion [Perforce_2017]_ connects Ravenbrook's Perforce + repository to the `Ravenbrook MPS repo on GitHub`_. + #. Ensure your public SSH key is submitted in Perforce at //.git-fusion/users/USER/keys/ @@ -179,6 +189,10 @@ These steps will only rarely need repeating. 5. Merging procedure -------------------- +Note: At any point before a successful "push" in step 7, this +procedure can be abandoned without harm. All work is local to your +working repo before that point. + 1. `Fetch the pull request branch`_ to a local branch using the MPS `durable branch naming convention`_, "branch/DATE/TOPIC". @@ -478,6 +492,10 @@ A. References , . +.. [Perforce_2017] "HelixCode Git Fusion Guide (2017.2)"; Perforce + Software; 2017; + . + B. Document History ------------------- @@ -487,6 +505,7 @@ B. Document History 2023-01-13 RB_ Updates after `first attempt at execution`_. 2023-01-14 RB_ Updates after `second (successful) execution`_. 2023-01-23 RB_ Adding measurements. +2023-01-25 RB_ Responding to mini-review_. ========== ===== ================================================== .. _RB: mailto:rb@ravenbrook.com @@ -495,6 +514,8 @@ B. Document History .. _second (successful) execution: https://github.com/Ravenbrook/mps/pull/97#issuecomment-1381771818 +.. _mini-review: https://github.com/Ravenbrook/mps/pull/97#discussion_r1085584810 + C. Copyright and License ------------------------ From 45b3e59795550c631156f8590b7408b972ffaaed Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Wed, 25 Jan 2023 21:43:11 +0000 Subject: [PATCH 90/93] Fixing heading markup of subsection headings in section 6. --- mps/procedure/pull-request-merge.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index af7820eafa2..2ab6b47d104 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -365,7 +365,7 @@ RB 2023-01-14] 6.1. Why not rebase or squash merge? ------------------------------------- +.................................... We would like to avoid rewriting history and the destruction of information on the grounds that it destroys information that could be @@ -403,7 +403,7 @@ and so we should not edit it. 6.2. Why not press the GitHub merge button? -------------------------------------------- +........................................... We cannot use the GitHub pull request merge button because it would put the GitHub master branch out of sync with (ahead of) Perforce. @@ -442,7 +442,7 @@ checklist into GitHub's interface using a `pull request template .. _durable branch naming convention: 6.3. Why the "durable" branch names? ------------------------------------- +.................................... It's common in Git culture to delete branches once they've been merged [Ardalis_2017]_ but this destroys information that has been From c6841a10a69e9569b1b9a3c887c67dd14323c1b0 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Thu, 26 Jan 2023 07:45:00 +0000 Subject: [PATCH 91/93] Clarifying what i mean by "basic competence with git". --- mps/procedure/pull-request-merge.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 2ab6b47d104..587f36f21b9 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -148,8 +148,8 @@ by considering `2. Purpose`_. These steps will only rarely need repeating. -#. You need basic competence with Git: enough to understand the - commands in `the merging procedure`_. +#. You need basic competence with Git: enough to understand what the + commands in `the merging procedure`_ do. #. If the merge has conflicts, you will need competence in using Git to resolve merge conflicts. From f7d5b80af681e406f2c246b474b9ffcb602d38e0 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Tue, 31 Jan 2023 18:20:04 +0000 Subject: [PATCH 92/93] Adding instructions for recording the merge. --- mps/procedure/pull-request-merge.rst | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 587f36f21b9..0e24ea53628 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -62,12 +62,22 @@ you should probably read section "`6. Rationale`_". 3. Pre-merge checklist ---------------------- -The answers to these questions should be "yes". If the answer to a -question isn't "yes", record that, and why, in response to the pull -request (and maybe suggest what to do about it). Include a permalink -to the revision of this procedure that you are following. When you -finish the checklist, decide whether to start `the merging procedure`_ -by considering `2. Purpose`_. +Start by making a record for the merge. Make a comment on the pull +request with a permalink to the procedure you're following (this one), +like:: + + Executing [proc.merge.pull-request](https://github.com/Ravenbrook/mps/blob/973fc087c9abff01a957b85bd17c4a2be434ae73/procedure/pull-request-merge.rst) + +The answers to the checklist questions should be "yes". If the answer +to a question isn't "yes", record that, and why (and maybe suggest +what to do about it). For example:: + + 2. An automated test case isn't feasible as it would mean + introducing broken shell scripts to GitHub and watching for CI + errors. + +When you finish the checklist, decide whether to start +`the merging procedure`_ by considering `2. Purpose`_. #. Is there a permanent visible document (issue, job), referenced by the branch, recording the problem that is solved by the changes in @@ -506,6 +516,7 @@ B. Document History 2023-01-14 RB_ Updates after `second (successful) execution`_. 2023-01-23 RB_ Adding measurements. 2023-01-25 RB_ Responding to mini-review_. +2023-01-31 RB_ Adding instructions for recording the merge. ========== ===== ================================================== .. _RB: mailto:rb@ravenbrook.com From 80cd84ef1319e71b3b3dcc6ea83e078093ac8e8b Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Tue, 31 Jan 2023 18:40:07 +0000 Subject: [PATCH 93/93] Fixing git command for pushing a fresh ci branch. --- mps/procedure/pull-request-merge.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mps/procedure/pull-request-merge.rst b/mps/procedure/pull-request-merge.rst index 0e24ea53628..5d83ad18b12 100644 --- a/mps/procedure/pull-request-merge.rst +++ b/mps/procedure/pull-request-merge.rst @@ -308,7 +308,7 @@ working repo before that point. on GitHub`_. This should trigger CI to build and testing on all target platforms. :: - git push github merge/2023-01-06/speed-hax + git push github HEAD:merge/2023-01-06/speed-hax You will need to wait for results from CI. Look for a build results in the `Travis CI build history for the repo`_ and in the