Skip to content
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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

rptb1
Copy link
Member

@rptb1 rptb1 commented May 30, 2023

Progress towards #98 .

@rptb1 rptb1 added process To do with process or procedure essential Will cause failure to meet customer requirements. Assign resources. labels May 30, 2023
@rptb1 rptb1 added this to the Perforce Divorce milestone May 30, 2023
@rptb1 rptb1 requested review from UNAA008 and thejayps May 30, 2023 13:08
@rptb1 rptb1 self-assigned this May 30, 2023
@thejayps
Copy link
Contributor

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

@thejayps
Copy link
Contributor

Begin checking at 1646, resume at 1700 with 10 mins checking and small break

@UNAA008
Copy link
Contributor

UNAA008 commented Oct 25, 2023

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?

@thejayps
Copy link
Contributor

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.

@thejayps
Copy link
Contributor

Stopped at 1710

@rptb1
Copy link
Member Author

rptb1 commented Oct 25, 2023

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.

@rptb1
Copy link
Member Author

rptb1 commented Nov 6, 2023

...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.

@thejayps
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
essential Will cause failure to meet customer requirements. Assign resources. process To do with process or procedure
Projects
Development

Successfully merging this pull request may close these issues.

3 participants