-
Notifications
You must be signed in to change notification settings - Fork 77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updating the merge procedure to removing synchronisation with Perforce repository #228
base: master
Are you sure you want to change the base?
Conversation
…tions that were formerly in a Perfofce push step.
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 .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 .plan - not aware of specific checking rates for manuals. Continue Entry finished at 1632 total 29 mins Continuing express review: .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) " q1 Will hyperlinks to the merging procedure still work properly after |
Stop Express review: major defect M1: In section 6: 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. |
Stopped at 1710 |
I think s/perforce/github/ fixes this but I'll want to validate those cases given Git's constant violation of POLA. |
Yep, Git astonished again. |
…merge commit. Git violates POLA once again.
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 #228 (comment) 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. |
Progress towards #98 .