Skip to content
This repository was archived by the owner on Oct 23, 2020. It is now read-only.

Satisfying Pull Requests

Jump to bottom
Xylar Asay-Davis edited this page Jun 30, 2016 · 15 revisions

This article will detail the steps for satisfying a pull request. Note: You can only satisfy pull requests for repositories which you have write access to.

Step 0 is only done once. Step 1 will take the longest, and may stretch over days or even weeks if revisions to the branch are required. Steps 2-5 should all happen at once when the branch is ready to merge and should take minutes.

0. Git config settings (one-time setup)

Before you begin, you should make sure you have two git config settings established:

  1. git config --global merge.ff false This will force a no-fast-forward merge even if a fast-forward merge is possible. Our MPAS workflow requires this. Note this is equivalent to always including the --no-ff flag on the command line when using git merge.

  2. git config --global merge.summary true This adds a summary of what commits are in the merge to the commit message associated with the merge.

  3. git config --global push.default nothing This ensures you only push branches that you explicitly list to push. There are other options than 'nothing' that are more useful, but we'll consider this an advanced topic you can explore on your own.

1. Review the code and the Pull Request

Now we can start reviewing the pull request. The first step is reviewing the pull request was created in a proper way. To begin with, make sure you are assigned to the PR, and that the PR is labeled properly. After that, ensure the pull request has a useful name (note, branch names are not useful so if the pull request is named core/feature_xyz, change it or ask the developer to change it). After the name has been reviewed, review the description of the pull request. This should be a description of the changes the pull request is introducing, not a description of how the changes were tested (those should go in a comment below the pull request description). If modifications need to be made, ask the developer to do them.

After the pull request has been reviewed, the code changes can be reviewed. Start by getting a local copy of the branch in question. For the purposes of this guide, we will call the branch corename/feature1, and the username will be mpas_developer.

  1. Add a remote to the user's fork if you don't already have one: git remote add mpas_developer/MPAS [email protected]:mpas_developer/MPAS.git
  2. Fetch the history of the user's fork if you needed to add their remote: git fetch --all -p
  3. Checkout a local copy of the branch: git checkout -b corename/feature1 mpas_developer/corename/feature1 (Note that git has a shorthand for this command: git checkout -b corename/feature1 I.e., if you try to create a new branch that matches a remote-tracking branch, you will get a local branch that is a copy of the remote-tracking branch. This can be confusing if you don't expect it - or handy if you do.)

Once you are on the corename/feature1 branch, you can review it. A review typically includes exploring the code modifications to make sure they are all appropriate, in addition to building and/or testing the models affected.

Note: You could also choose to do the code review in a detached head so as not to add local branches that you don't need lying around. To do so, instead do the final step as git checkout mpas_developer/corename/feature1. Also note, you can do a lot of code review through the Github website. The Pull Request page has three tabs that allow you to 1. make comments, 2. see the commits in the PR, and 3. see the integrated diff of the entire branch vs. the development branch. Tab 3 is particularly useful, and you can make inline comments about specific lines of code by pressing the blue + symbol that appears as you hover over the code.

2. Get an up-to-date local version of the development branch receiving the merge

To begin, make sure you have a local version of the branch you want to merge into. This is either CORE/develop or develop, depending on which type of branch you're merging.

  1. Fetch the history of the shared repository: git fetch --all -p (Assumes you have already added the appropriate remotes, see this article for more information)
  2. Checkout your local version of the branch you are merging into: git checkout CORE/develop or git checkout develop
  3. Reset this branch to point to the shared version of the branch: git reset --hard MPAS-Dev/MPAS/CORE/develop or git reset --hard MPAS-Dev/MPAS/develop

3. Merge the feature branch into the development branch

Once the review is complete, the merge can be satisfied. To satisfy a merge, you need to be on the branch that you want to merge feature1 into. In this case, we'll assume you want to merge it into develop but the other situation is you want to merge it into CORE/develop. (Following text assumes develop.)

  1. You should already be on the local develop branch (which you had updated above) after previous step.
  2. Merge corename/feature1 into develop: git merge mpas_developer/corename/feature1 (Note the remote version of their branch is specified here in case you never created a copy of it in a local branch - and to avoid the situation where your local version of their branch is out of date.)

During the merge, any conflicts that exist will have to be resolved manually.

The preferred format for the commit message results in something like this (Notes are below about specific edits to make):

Merge branch 'landice/extern_dycore_cleanup' into landice/develop

These commits add the interface code, Makefile changes, and modifications
to the LI core to allow the LI core to be built with an external dycore
library written in C/C++. The resulting model has been validated for the
circular shelf, confined shelf, dome, and var-res dome test cases.

* remotes/matthewhoffman/MPAS/landice/extern_dycore_cleanup2:
  LI: Fix typos/cleanup comments in extern dycore code
  LI: modified algorithm to compute HO normal velocity on edges
  LI: fix dirichlet conditions for parallel jobs (use global number of vertices instead of local one).
  LI: bug fix for extension of fields to boundaries in interface_velocity_solver

    Conflicts:
        src/core_landice/Registry.xml
        src/core_landice/mpas_li_diagnostic_vars.F

Specific edits that are sometimes made:

  • If merged from a remote branch, the remote-tracking portion of "Merge remote-tracking branch" is removed (it's unnecessary, and just makes the commit subject longer)
  • If merged from a remote branch, the remote name is removed from the branch name. e.g. if you used git merge developer/core/feature, developer is deleted from this. This is because the name of a remote is specific to a certain repository, and is useless information to other developers.

Below the title, the summary of the Pull Request was copy and pasted from the PR description on the Github website (written by the author when the PR was created).

Beneath the description is a summary of the commits that are being merged (via the git config --global merge.summary true command described at the top of this page). Additionally, if there were conflicts that needed to be resolved during the merge, those are listed in the message. This list will be automatically generated by git, and it is recommended that this information is left in the commit message.

Following our policies on pull requests and merges detailed in the developers guide, this should not result in a "fast-forward" merge - and the git config settings described at the beginning of this page will ensure that. (This is equivalent to using the --no-ff flag with git merge.)

If you need to undo this merge, you simply have to reset your local develop branch to the shared repository's develop branch via: git reset --hard MPAS-Dev/MPAS/develop

4. Test the updated development branch after the merge

Even if tests passed on the feature branch, that does not guarantee tests will pass on the development branch after the merge (except in the case where all commits on the feature branch are ahead of the head of develop).

After you have a locally merged branch, you should test it again by doing some build/run tests, ideally testing that all cores build at a minimum. Additionally, core/develop branches may have their own specific testing protocols that should also be satisfied.

5. Push the updated development branch to the MPAS-Dev/MPAS repository

Once your testing of the merged branch is complete, you can push it to the shared repository:

git push MPAS-Dev/MPAS develop:develop

The Github website will detect that the requested set of commits are now in the history of the development branch and automatically close the Pull Request for you.

6. Delete the development branch

After a pull request has been merged into develop, you should delete the branch the pull request was based on. This can be done with the purple 'delete branch' button on the Pull Request webpage (it appears after the merge is completed), or it can be done from the command line. Also remember to consider deleting your local copy of the branch if you created one.

Clone this wiki locally