Git merge request journey

Overview

Merge requests are the means by which developers can propose changes to the RISC OS source code. You might also have seen the term pull request which is very similar. There are three topic areas to think about with merge requests handled by RISC OS Open:

• Guidance on a good merge request
• The lifetime of a merge request
• Finding merge requests that need a reviewer

Guidance on a good merge request

While the acceptance rate is relatively high, around 90%, there’s no guarantee that the change will be accepted following review. Here are a few guidelines on what makes a ‘good’ merge request, and a few things to avoid.

• Consider breaking up large/complicated changes into a set of smaller commits, this makes them easier to review
• Try to follow the layout style of the existing source code, such as placement of braces and indentation
• Collect related commits together in one merge request, rather than opening several each with only one change
  • Use the summary paragraph to describe why they go together and to ‘sell’ the benefit of the submission as a whole
  • See if there’s another similar merge request pending for that component, it may be worth collaborating with that developer on the same area
  • We don’t offer a prize for most merge requests opened!
• Generally, the following classes of changes are unlikely to be accepted
  • Merge requests that merely churn the code without substance, because they confuse the change history (for example, lowercasing all comments in a component)
  • Merge requests that only benefit one port to the detriment of others (for example, slowing down the Kernel’s SWI despatch in order to support an emulator)
  • Merge requests that contain debug or temporary code left enabled; remember to diff your changes before committing and remove any remnants
  • For those components which have Continuous Integration enabled, merge requests that cause a CI failure where previously it passed

The lifetime of a merge request

Developers are encouraged to get involved in the reviewing process by looking out for other developers’ work in areas they’re familiar with to return the favour. The final step of merging a change into the central RISC OS sources can only be done by ROOL, with pending changes checked for multiple times per week and typically to the following schedule:

• For trivial build fixes and important security updates these may be accepted at the next opportunity
• For small submissions, say less than 100 lines, at least 1 reviewer must be satisfied and no objections within a week
• For larger submissions, a more considered discussion is expected and this could span several weeks requiring multiple updates
• If a submission has attracted no interest or comments after 1 month, ROOL may step in as a default reviewer if time permits to consider the changes on their merits
• If a submission has pending questions or resistance from reviewers, but the author has not followed up or resolved them within 3 months, ROOL may step in as a default reviewer to try to progress things
• If there is no activity at all and little prospect of a resolution after 6 months the request will be demoted to ‘Draft’ status signalling that the developer has more work to do to bring things up to standard
• At 12 months of inactivity the merge request is closed

The fastest way to progress a submission is usually to buddy with a reviewer who also has a pending merge request, and is knowledgeable in the area of your merge request. Review their work first and have them review yours in return. You should not rely on ROOL as a default reviewer – we manage the submission process but don’t offer any guarantee we’ll even look at your work other than to close it after 12 months.

Finding merge requests that need a reviewer

On the top bar of every GitLab screen is an icon depicting a merge. Click on this to search merge requests. You can filter merge requests in a number of ways. If you want to see a complete list, you can try a search term of Assignee:any.