From 35949db3b4639f47a6b4599a96617d7fa49fc73f Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Tue, 30 May 2023 14:04:46 +0100 Subject: [PATCH 1/6] Removing synchronisation with Perforce repository. --- procedure/pull-request-merge.rst | 134 +++++-------------------------- 1 file changed, 20 insertions(+), 114 deletions(-) diff --git a/procedure/pull-request-merge.rst b/procedure/pull-request-merge.rst index eea2ec42d3..b643c5fe66 100644 --- a/procedure/pull-request-merge.rst +++ b/procedure/pull-request-merge.rst @@ -52,8 +52,8 @@ The purpose of this procedure is: 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 +3. maintain and protect the permanent record of the MPS in 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`_". @@ -101,11 +101,7 @@ When you finish the checklist, decide whether to start #. 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.* + [FIXME: Should this refer to proc.review.exit? RB 2023-05-30] #. Has the contributor licensed their work? @@ -181,25 +177,6 @@ These steps will only rarely need repeating. #. 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/ - - This is the public key that the SSH used by your Git will use to - connect to perforce.ravenbrook.com. - - **NOTE**: As of 2022-11, you may not be able to connect to Perforce - Git Fusion without specifying an old public key algorithm. See - `"Connecting to Git Fusion" - `__. - -#. 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" [#github]_. This will give you access to CI to build and test the merge. (If you're an MPS developer you can use @@ -207,21 +184,6 @@ These steps will only rarely need repeating. git clone -o github git@github.com:Ravenbrook/mps.git -#. Set your e-mail address for commits to the repo to match the one in - your Perforce user record, e.g. from within the "mps" repo - directory :: - - git config user.email spqr@ravenbrook.com - - and possibly your name if you don't have that set in Git globally :: - - git config user.name 'Julius Cesar' - -#. Add the Git Fusion mps-public repo, which is the interface to - Ravenbrook's Perforce. From within the "mps" repo directory :: - - git remote add perforce ssh://git@perforce.ravenbrook.com:1622/mps-public - .. [#github] There's nothing special about this name -- it's just assumed in the examples in the procedure. @@ -263,35 +225,11 @@ working repo before that point. git remote add captain-contrib https://gitlab.com/captcontrib/mps.git git fetch captain-contrib mps-speed-hax:branch/2023-01-06/speed-hax - Double check you've got the branch name right. Using the wrong - branch naming `causes permanent pollution in the Ravenbrook - Perforce repository - `_. - 2. Optionally, let other people know that you're working on a merge into master. Negotiate to avoid racing them to push to the master codeline (step 7) because that will create extra merging work. -3. Ensure your local master is up to date with Perforce:: - - 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. - - 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:: +3. Merge the branch in to your local master:: git merge --no-ff branch/2023-01-06/speed-hax @@ -310,7 +248,7 @@ working repo before that point. branch. If you still can't resolve conflicts, this procedure fails. -5. Maybe build and test locally. If either +4. Maybe build and test locally. If either - the merge was non-trivial - there has been any rebasing (see step 7) @@ -329,7 +267,7 @@ working repo before that point. 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. Maybe build and test using CI. As with step 5, if either +5. 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) @@ -347,52 +285,17 @@ working repo before that point. See build (step 5) about what to do if tests do not pass. -7. Submit your merged master and the branch to Perforce:: - - 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. - - 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:: +6. Submit your merged master and the branch 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. - -9. Eyeball the pull request and related issues on GitHub to make sure +7. 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. -10. Edit the comment you made in `3. Pre-merge checklist`_ to record - the end time of the merge and how long you spent merging, like:: +8. Edit the comment you made in `3. Pre-merge checklist`_ to record + the end time of the merge and how long you spent merging, like:: 6. End time 11:20. Merge took 17 mins. @@ -451,10 +354,10 @@ 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. -Currently, Perforce is the authoritative home of the MPS, and the Git -repository is a mirror. +In the past, we could not use the GitHub pull request merge button +because it would have put the GitHub master branch out of sync with +(ahead of) Perforce. However, Git is now the authoritative home of +the MPS, and the Perforce repository is mothballed. According to `GitHub's "About pull request merges" `_: @@ -477,13 +380,15 @@ 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 4 and 5, the procedure, but not +Now that `Git has becomes the home +`_ we could potentially +use 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 `_. +[FIXME: Create a GitHub issue for this. RB 2023-05-30] + .. _durable branch naming convention: @@ -553,6 +458,7 @@ B. Document History 2023-01-23 RB_ Adding measurements. 2023-01-25 RB_ Responding to mini-review_. 2023-01-31 RB_ Adding instructions for recording the merge. +2023-05-30 RB_ Perforce Divorce: removing synchronisation with Perforce repository. ========== ===== ================================================== .. _RB: mailto:rb@ravenbrook.com From 9dddbc5f9443b5dd8adfbce81bfe454237b9f3e2 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Tue, 30 May 2023 14:17:18 +0100 Subject: [PATCH 2/6] Resolving FIXME comments with links to procedures and issues. --- procedure/pull-request-merge.rst | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/procedure/pull-request-merge.rst b/procedure/pull-request-merge.rst index b643c5fe66..28bc67f6ef 100644 --- a/procedure/pull-request-merge.rst +++ b/procedure/pull-request-merge.rst @@ -101,7 +101,14 @@ When you finish the checklist, decide whether to start #. Is the merge approved by an approving review? - [FIXME: Should this refer to proc.review.exit? RB 2023-05-30] + [Technically, the work to be merged must have passed + `proc.review.exit + `_ + but at the time of writing, proc.review has not itself been + merged. It is pending in `GitHub pull request #123 + `_. In practice, we + are applying that procedure. Once we merge the review procedure + itself, this comment should be updated. RB 2023-05-30] #. Has the contributor licensed their work? @@ -386,8 +393,9 @@ use 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 `_. - -[FIXME: Create a GitHub issue for this. RB 2023-05-30] +[See `this comment on GitHub issue #98 +`__. +RB 2023-05-30] .. _durable branch naming convention: From 72b1f653ab1f4d75793da6c873baed048ed84f7e Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Mon, 12 Jun 2023 13:57:13 +0100 Subject: [PATCH 3/6] Correcting cross-references to steps and reintroducing rebase instructions that were formerly in a Perfofce push step. --- procedure/pull-request-merge.rst | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/procedure/pull-request-merge.rst b/procedure/pull-request-merge.rst index 28bc67f6ef..38847bdd92 100644 --- a/procedure/pull-request-merge.rst +++ b/procedure/pull-request-merge.rst @@ -200,7 +200,7 @@ These steps will only rarely need repeating. 5. Merging procedure -------------------- -Note: At any point before a successful "push" in step 7, this +Note: At any point before a successful "push" in step 6, this procedure can be abandoned without harm. All work is local to your working repo before that point. @@ -234,7 +234,7 @@ working repo before that point. 2. Optionally, let other people know that you're working on a merge into master. Negotiate to avoid racing them to push to the master - codeline (step 7) because that will create extra merging work. + codeline (step 6) because that will create extra merging work. 3. Merge the branch in to your local master:: @@ -258,7 +258,7 @@ working repo before that point. 4. Maybe build and test locally. If either - the merge was non-trivial - - there has been any rebasing (see step 7) + - there has been any rebasing (see step 6) - there were failed or missing build results from CI then build and test the merge result locally if possible. For @@ -270,14 +270,14 @@ working repo before that point. platforms. If tests do not pass, review your conflict resolution from the - merge (step 4), and if that doesn't fix 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! -5. Maybe build and test using CI. As with step 5, if either +5. Maybe build and test using CI. As with step 4, if either - the merge was non-trivial - - there has been any rebasing (see step 7) + - there has been any rebasing (see step 6) - there were failed or missing build results from CI then push the merge to a fresh branch in the `Ravenbrook MPS repo @@ -290,12 +290,29 @@ working repo before that point. 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. + See build (step 4) about what to do if tests do not pass. 6. Submit your merged master and the branch to GitHub:: git push github 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. + + You can attempt to rebase your work on those changes:: + + git pull --rebase perforce + + then go back to testing (step 4). + + Alternatively, you could undo your merging work:: + + git reset --hard perforce/master + + then go back to merging (step 3). + 7. 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 From 38e2a57a67cb3aea509852415ef75cd0512781ff Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Mon, 6 Nov 2023 13:12:29 +0000 Subject: [PATCH 4/6] Correcting references to "perforce" remote when resolving conflict with remote. --- procedure/pull-request-merge.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/procedure/pull-request-merge.rst b/procedure/pull-request-merge.rst index 38847bdd92..9864e9a3a1 100644 --- a/procedure/pull-request-merge.rst +++ b/procedure/pull-request-merge.rst @@ -303,13 +303,13 @@ working repo before that point. You can attempt to rebase your work on those changes:: - git pull --rebase perforce + git pull --rebase github then go back to testing (step 4). Alternatively, you could undo your merging work:: - git reset --hard perforce/master + git reset --hard github/master then go back to merging (step 3). From d9576418ec5a5408db0565f0d80bb10cde2d876c Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Mon, 6 Nov 2023 13:42:01 +0000 Subject: [PATCH 5/6] Correcting invocation of `git pull --rebase` to avoid discarding the merge commit. Git violates POLA once again. --- procedure/pull-request-merge.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/procedure/pull-request-merge.rst b/procedure/pull-request-merge.rst index 9864e9a3a1..52176490d0 100644 --- a/procedure/pull-request-merge.rst +++ b/procedure/pull-request-merge.rst @@ -303,7 +303,7 @@ working repo before that point. You can attempt to rebase your work on those changes:: - git pull --rebase github + git pull --rebase=merges github then go back to testing (step 4). From dfec8bcaa50c9be237196cdadbbcf5cfa7262eb1 Mon Sep 17 00:00:00 2001 From: Richard Brooksby Date: Mon, 6 Nov 2023 13:50:00 +0000 Subject: [PATCH 6/6] Generalising instructions for obtaining CI results. --- procedure/pull-request-merge.rst | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/procedure/pull-request-merge.rst b/procedure/pull-request-merge.rst index 52176490d0..e73994bc57 100644 --- a/procedure/pull-request-merge.rst +++ b/procedure/pull-request-merge.rst @@ -166,10 +166,6 @@ When you finish the checklist, decide whether to start .. _pull request merge branch: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request -.. _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 - .. _MPS interface: https://www.ravenbrook.com/project/mps/master/manual/html/topic/interface.html @@ -286,9 +282,8 @@ working repo before that point. 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 - `GitHub workflows for the repo`_. + You will need to wait for `results from CI + <../design/tests.txt#continuous-integration>`_. See build (step 4) about what to do if tests do not pass.