mps
mps copied to clipboard
Ravenbrook
Updating the merge procedure to removing synchronisation with Perforce repository
executing proc.review.entry with @UNAA008
Start time 1555
entry.express: looks low risk and @UNAA008 is available.
entry.universal applies, entry.impl applies as rst is code.
.source-approved the issue relating to this change has not been reviewed, however it has been effectively been mini-reviewed and understood during prioritisation. Also, by the time a PR has been created to address an issue, the correctness of the issue should have been established. See #232
rule.style applies, rule.generic applies rule.code does apply
.rules-approved - there is no evidence that any rule document has been approved. See #232
.brief-check - @thejayps looked at the rendered output and found no obvious major defects
1620- paused for technical issue with videocall Resumed 1628
.plan - not aware of specific checking rates for manuals. Continue
Entry finished at 1632 total 29 mins
Continuing express review: .express.time - We spent 29 minutes doing entry very meticulously, but @thejayps interprets the 30 mins as being from the point someone was called in ( .express.call )
.express.rule - @thejayps and @UNAA008 don't know all the applicable rules by heart.
.express.improve. We should stop technically, this is the first time @thejayps has run an express review, there is an inevitable extra objective in going up the express learning curve. It's not really an improvement though
Begin checking at 1646, resume at 1700 with 10 mins checking and small break
m1 "Ravenbrook is currently (2023-01) " The edits have simplified the procedure already. Delete this paragraph, or mention the migration for historical note.
q1 Will hyperlinks to the merging procedure still work properly after merging?
Stop Express review: major defect M1:
In section 6: " git pull --rebase perforce then go back to testing (step 4).
Alternatively, you could undo your merging work:
git reset --hard perforce/master "
These commands make reference to a token (remote name) "perforce". The purpose of this change is explicitly to remove synchronisation with perforce. Anything that still refers to "perforce" should either explain why the reference must still exist, or change the reference. If not done, this is sufficiently unclear that even a very experienced git/perforce user who could figure out what was going on would probably abort the procedure to be safe.
Stop Express review: major defect M1:
In section 6: " git pull --rebase perforce then go back to testing (step 4).
Alternatively, you could undo your merging work:
git reset --hard perforce/master "
These commands make reference to a token (remote name) "perforce". The purpose of this change is explicitly to remove synchronisation with perforce. Anything that still refers to "perforce" should either explain why the reference must still exist, or change the reference. If not done, this is sufficiently unclear that even a very experienced git/perforce user who could figure out what was going on would probably abort the procedure to be safe.
I think s/perforce/github/ fixes this but I'll want to validate those cases given Git's constant violation of POLA.
...validate those cases given Git's constant violation of POLA.
Yep, Git astonished again. git pull --rebase discards merge commits, changing the topology of what is rebased and effectively fast-forwarding master. You have to say git pull --rebase=merges.
Attempt 2 at express review:
Executing proc.review.entry
Start time 1350
entry.express: still looks low risk, await @UNAA008
1400 @UNAA008 arrives
Most comments relating to entry and planning here https://github.com/Ravenbrook/mps/pull/228#issuecomment-1779568832 remain the same, however the restructured text syntax check and FIXME checks now fail for the most recent commit to #123, so we are aware that there is some degradation in the source document for the review process. However we strongly suspect that this is cosmetic and/or superficial, and should not hinder an express review
.express.major We are performing an express review again after a major defect was found. The author @rptb1 has made edits to fix the major defect found, and we believe express review is appropriate following this. Is the underlying procedure completely correct?
Lots of discussion points are coming up about the appropriateness of express review - on balance this is probably not for express review now.
We discussed an idea to test the merge procedure "in anger" on a small low risk change, then do a full or express review after that.
@UNAA008 points out that changes like this one, which document processes for maintaining the MPS, should be exercised as part of their inspection.