NOTE: Most developers do not have commit privilege on the official ModeShape Git repository. If you are not, please follow the normal ModeShape Development Workflow.
ModeShape uses Git for its version control system, and GitHub.com for the "official" upstream repository. All contributors use the GitHub to create a fork of the official repository, do all work within their own fork, and then generate pull-requests for the commits you'd like to go into the ModeShape codebase. (For details of this process, see our Development Process).
As mentioned above, ModeShape's integration managers watch the pull-requests and review, approve and merge all proposed changes to the ModeShape codebase. Just a few people have this responsibility, helping to ensure that the official repository remains buildable and runnable, and to ensure that the commits are made against the appropriate branches.
This page documents how a project Integration Manager sets up their environment and performs the integration from the pull-requests.
Setup
All development is to be done on forks and not in the official repository, but whenever you work on the official repository, you should use a completely separate local git repository. This helps make it clear what role you're serving, and helps keep your dev changes outside of the official until the appropriate time.
So create a clone of the "official" repository, which we'll call 'official':
$ git clone git@github.com/ModeShape/modeshape official $ cd official
See what's there:
$ git branch -a $ git status
Again, never do your development in this repository, and use it only for merging pull-requests.
Notifications of Pull-Requests
There are several ways of watching for pull-requests.
The first and perhaps easiest is via e-mail from GitHub. All people that are in the Committer membership group should automatically get email notifications from GitHub, unless you've changed your default settings in your profile.
The second is by using an RSS reader and subscribing to the "Pull Reqests" page of the appropriate GitHub repositories.
The third way is to watch for JIRA email notifications of additions of pull-request URLs to specific issues.
Reviewing Pull-Requests
As soon as somebody commits a change to their fork and generates a pull-request, that pull-request will appear as an open pull request on the "Pull Requests" tab on the ModeShape repository page. (Note that the number of open pull requests is always shown in the tab title.) The page also has a filter to show open or closed requests.
Click on a pull-request to see the details. The description is hopefully complete enough to provide you with the context and to reference the JIRA issue(s) to which this change applies. There will also be one or more commits; simply click on a commit to review the changed files and lines that would be merged should the pull-request be approved.
There are lots of things to look for, but here are a few important ones:
- Does the pull-request fix the issue? If not, is it clear that the pull-request contains only a partial solution and that additional changes will be forthcoming?
- Does the pull-request really address the cause of a problem, or does it merely fix a symptom? The latter will often feel like a hack or a workaround, and should not be accepted.
- Does the pull-request unnecessarily change lots of lines (usually because of formatting differences)? Such changes are generally frowned upon, because they tend to obscure the meaningful changes.
- Are there more than a few commits? If so, ask the author whether those distinct commits are really useful. (Often they are useful for the author during development, but once in the official history a single commit is often preferred.) If not, ask the user to squash their multiple commits into a single commit, and (forcibly) push the new commit to their fork's branch. The pull-request should be updated automatically.
- Are the changes adequately documented?
- Do the pull-request description/comments and JIRA comments adequately explain the changes? Both should follow our conventions.
- Consider how the changed code might fail, and whether/how various error conditions might occur. Are null conditions checked? Are assertions made to declare that expected/assumed conditions?
- Consider the changes' impact on concurrency. Do the changes made to concurrent classes/methods use proper concurrency patterns and techniques? If there are new classes, do they properly make use of the @ThreadSafe, @Immutable, and @NotThreadSafe annotations?
- Do the changes adhere to our conventions? Do they include adequate and meaningful JavaDoc and in-line comments? Do they use our naming conventions? Do the changes look like changes you might make? (This is not a requirement, but it is a goal that all of our code appears to have been written by nobody in particular.) Is there a code smell?
Some of these are very subjective, but do your best and try to be consistent in how you review all pull-requests. There's a fine line between being too picky and too leanient. Error on the side of being helpful, courteous and verbose. Be more diligent with pull-requests from new contributors. Likewise, usually give more leeway to contributors with more time on the project, but at the same time look for those simple mistakes.
Discussing Pull-Requests
After reviewing the proposed changes, you may have some comments. If so, leave specific, detailed and courteous comments and line notes on the pull-request. The author can easily reply to your questions/comments via the pull-request. Pull request comments are Markdown-enabled, so you can embed images, preformatted blocks, and use other Markdown formatting techniques.
If you expect the author to make additional changes, fully describe what the author needs to do to fix the problems. If you're not sure, ask the author before assuming - and appearing pretentious. It's even possible to request a change by phrasing it in the form of a question: "Did you consider what the concurrency implications are?" If your remarks are non-trivial, be sure to add them to the JIRA issue as well. The author can make the changes on their local topic branch, and when they push those changes into their fork they will be automatically added to the pull-request. (They may alternatively choose to rewrite their commits and push the new commits into their fork; these too will automatically be added to the pull-request.)
This cycle should continue until the pull-request is acceptable or a decision is made to reject the pull-request. If you believe the pull-request should be rejected, cordially explain the reasoning via pull-request comments and on the JIRA issue. Sometimes, the author decides to try a completely different approach and may close the pull-request or ask that you close it. Again, please be sure the JIRA is updated to reflect the decision.
If the pull-request is acceptable, then you can try merging it into the appropriate branch.
Rejecting Pull-Requests
If you are not going to approve the pull-request, be sure to add a comment that gives sufficient feedback and suggestions for what needs to be corrected. Above all, be cordial -- you are getting a suggested improvement or bug fix from the community, and a cordial and encouraging tone will go a long way toward keeping your community interested and willing to make the effort.
The submitter can then make the changes that you requested and commit those to the pull-request's branch. As noted above, the comments for these commits will appear in the pull request.
If the pull-request contains changes that will simply not be merged into the "official" repository, you can manually close the pull-request.
Testing Pull-Requests
If the changes in a pull-request are visually acceptable, then it's time to try merging the changes and running a full build. The first step is to be sure your local git repo is up-to-date by pulling from "official". The following changes assume that the changes are to be merged onto the 'master' branch, but use the correct branch for the pull-request.
$ git pull origin master # or just 'git pull'
The GitHub pull-request page has a "?" button that displays useful git commands for performing the merge. Often it's easiest to simply copy the commands, as they are specific to the pull-request.
First, create a topic branch for merging and testing the changes. Note that we want a clean branch, so use a unique name. The GitHub pull-request page help uses a branch name with the author's username:
$ git checkout -b authorname-mode-1234 master # or the correct parent branch
Now we need to get the proposed changes, and several ways to do this.
The first and easiest is what the pull-request page's help displays:
$ git pull https://github.com/authorname/modeshape.git mode-1234
where "mode-1234" is the branch in the author's fork from which the pull-request was made.
One alternative is to add a remote for the repository where the commits in this pull-request were published, fetch the changes from that remote, merge the changes onto the appropriate branch, fix any conflicts, and push the newly-merged branch to the "official" repository.
$ git remote add jack git://github.com/smith/modeshape.git
$ git pull jack mode-1234
The other approach works better when the submitter doesn't often make pull-requests, and you don't want to add a remote for their repository. In this case, it's maybe easier to use the "git-am" command. Every pull-request has a URL that points to the patch file for the changes:
$ curl http://github.com/github/modeshape/pull/25.patch | git am
At this point, check the history. If there have been other commits in the official repository since the author created the pull-request, the history will show a merge commit. If this is the case, rebase the branch to alter the author's commmits to be as if they were applied to the now-latest commit, and double-check the history to verify the merge commit is no longer there.
$ git rebase master # or the correct parent branch $ git log
If the pull-request contains numerous commits, consider squashing the commits -- especially if you're going to have to apply the pull-request to the multiple branches. This is done with interactive rebase, and is not shown here. Perhaps you should have asked the user to squash their commits earlier in the process (see the "Reviewing" section above).
Once the commits are fixed, run a full build with the changes so that all unit and integration tests are run. This may seem like overkill, but since you're doing the merge it is your responsibility to make sure that the changes don't break anything. For ModeShape 2.x, the command is:
$ mvn clean install -Pintegration
while for ModeShape 3.x, the command is currently:
$ mvn clean install -Plegacy
If the build fails, be sure that the failure was a result of the pull-request. For example, you might try rebuilding, or checking your environment. If the build fails again, switch to the parent branch and run the build again (without the pull-request's changes). If that build fails, something is wrong in your environment, so fix that first and then try building on the pull-request's branch.
If the build succeeds, the next step is to merge the changes onto 'master' branch (e.g., or '2.5.x' or '3.x' as appropriate) and push to 'origin'. Again, these commands are in the pull-request's help section:
$ git checkout master # or the correct parent branch $ git merge authorname-mode-1234 $ git push origin master # or the correct parent branch
The pull-request is automatically closed when the requested commits are successfully merged into the "official" repository, and an event is generated to notify all of the followers that the merge occurred.
Don't forget to delete the local topic branch:
$ git branch -D mode-1234
Merging onto multiple branches
Some pull-request are to be applied to multiple branches. First, find out the IDs of the pull-requests commits. On the 'master' branch:
$ git log
and find the commit IDs for the pull-request (they might have changed from the pull-request if you rebased or squashed when merging) and keep them handy.
Then, repeat the following process for each branch onto which the changes are to be applied. The following examples use the '3.x' branch.
Creat a topic branch with a unique name (note the "-b"):
$ git checkout -b authorname-mode-1234-b 3.x
and apply the changes with the Git "cherry-pick" command. You'll need to cherry-pick each of the commit IDs:
$ git cherry-pick <commitId>
This may result in a merge conflict (especially if there are multiple commits to cherry-pick). If this is the case, you have to resolve them using the standard git procedure. (Remember that git tells you what to do in the output.)
After cherry-picking all of the commits and resolving all merges, you can then run a build:
$ mvn clean install
If successful, you can merge onto the parent branch:
$ git checkout 3.x # or the correct parent branch $ git merge authorname-mode-1234-b $ git push origin 3.x # or the correct parent branch
Close the pull-request
Once the pull-request has been merged onto the various branches, be sure to describe what you did using a comment to the pull-request. For example:
"Nice work, Jane. Merged the changes onto the 'master' branch and cherry-picked onto the '3.x' branch."
Be sure the pull-request is closed. (This is done automatically unless you rebased or squashed the original commits.)
Update the JIRA issue
Finally, update the JIRA. If this is the final pull-request for the issue, use "Workflow->Pull Request Closed" and add a comment. Note that this automatically transitions the issue to the "Resolved" state.
If, on the other hand, there are additional pull-requests that will be made for this issue, use "Edit" instead and simply add your comment (stating which pull-request you merged).
Comments