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

Fix and update the pull request merge procedure #97

Merged
merged 93 commits into from
Jan 31, 2023

Conversation

rptb1
Copy link
Member

@rptb1 rptb1 commented Jan 7, 2023

This is a branch to deal with #96 by creating a procedure for merging pull requests, but also to work on general git migration relating to merges.

You might wonder at the amount of effort that has gone in to this piece of bureaucracy. This is based on long experience with the MPS in defect prevention. This procedure (and others like it) are the final gatekeeper on the quality of the source code, and have been key to prevention in the past. GC bugs are very very nasty, especially when they happen to end-users of your user, who is likely a compiler developer, in a way that can't be seen or reproduced, let alone diagnosed and fixed.

@rptb1 rptb1 self-assigned this Jan 7, 2023
…of what comes out Git Fusion from Perforce, and is closer to our migration target.
… also discards information about who did this procedure, which we don't want.
…exisitng licenses unless expressly stated otherwise in the contribution request.
…ossibility of insisting on a clean trial merge before even starting this procedure.
@rptb1 rptb1 marked this pull request as ready for review January 14, 2023 13:08
@rptb1 rptb1 added git-migration Project migration from Ravenbrook internal Perforce infrastructure to public git repo process To do with process or procedure essential Will cause failure to meet customer requirements. Assign resources. labels Jan 14, 2023
@rptb1 rptb1 added this to the Perforce Polarity milestone Jan 16, 2023
@rptb1
Copy link
Member Author

rptb1 commented Jan 16, 2023

@thejayps merged #93 using this procedure (with me supervising) and yet today we were surprised that some branches were included that @thejayps didn't remember reviewing. That's because when he ran the checklist, he remembered reviewing "it" but "it" was in fact, quite a lot older than what he was merging.

So there's a flaw in the checklist, because it just asks whether a review exists, not what it applied to.

Fixed in 99ff0d0

@rptb1
Copy link
Member Author

rptb1 commented Jan 19, 2023

The procedure doesn't deal with the case where the response to an issue is to merge the branch more than once: to master (for the long term) and to one or more version branches (to fix their defects). GitHub's pull request concept may not deal with this well either, given that Git itself doesn't seem to.

procedure/pull-request-merge.rst Outdated Show resolved Hide resolved
procedure/pull-request-merge.rst Show resolved Hide resolved
@rptb1
Copy link
Member Author

rptb1 commented Jan 31, 2023

Executing proc.merge.pull-request.

  1. An automated test case for Merge procedure creates bogus Perforce branch locations #96 is not feasible because it would need to pollute the Perforce namespace via Git Fusion and check whether that happened.
  2. There has not been an approving review, but all review issues have been dealt with. The procedure is thoroughly tested and risk is low.
  3. There is a tiny merge conflict that I'm deliberately using to test the conflict resolution in the procedure itself.

And it's a good job since I found an error in the command in this case. Fixed in e36c8f9 . Then undid and repeated the procedure.

I didn't wait for CI results on https://github.com/Ravenbrook/mps/tree/merge/2023-01-31/pull-request-merge-procedure because I know there are no code changes, and there's no CI checker for reStructuredText yet (see #112 ).

Merge procedure took 22 minutes.

rptb1 added a commit that referenced this pull request Jan 31, 2023
@rptb1 rptb1 merged commit de31021 into master Jan 31, 2023
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. git-migration Project migration from Ravenbrook internal Perforce infrastructure to public git repo process To do with process or procedure
Development

Successfully merging this pull request may close these issues.

Merge procedure creates bogus Perforce branch locations
4 participants