The exact details on how to submit changes for merging depends, of course, on the development workflow that the project is using. Various classes of possible workflows are described in Chapter 5, Collaborative Development with Git.
If the project has a dedicated maintainer or, at least, if it has someone responsible to merge the proposed changes into the official version, you would need also to describe submitted changes as a whole (in addition to describing each commit in the series). This can be done in the form of a cover letter for the patch series while sending changes as patches via e-mail; or it can be comments in the pull request while using collocated contributor repositories model; or it can be the description in an e-mail with a pull request, which already includes the URL and the branch in your public repository with changes (generated with git request-pull).
This cover letter or a pull request should include the description of the purpose of the patch series or the pull request. Consider providing there an overview of why the work is taking place (with any relevant links and a summary of the discussion). Be explicit with stating that it is a work in progress, saying this in the description of changes.
In the dispersed contributor model, where changes are submitted for review as patches or patch series, usually to the mailing list, you should use Git-based tools such as git format-patch and, if possible, git send-email. Multiple related patches should be grouped together, for example, in their own e-mail thread. The convention is to send them as replies to an additional cover letter message, which should describe the feature as a whole.
If the changes are sent to the mailing list, it is a common convention to prefix your subject line with [PATCH] or with [PATCH m/n] (where m is the patch number in the series of the n patches). This lets people easily distinguish patch submissions from other e-mails. This part can be done with git format-patch. What you need to decide yourself is to whether to use additional markers after PATCH to mark the nature of the series, for example, PATCH/RFC (RFC means here Request for Comments, that is an idea for a feature with an example of its implementation; such patch series should be examined if the idea is worthy; it is not ready to be applied/merged but provided only for the discussion among developers).
In the collocated contributor repositories model, where all the developers use the same Git hosting website or software (for example, GitHub, Bitbucket, GitLab, or a private instance of it, and so on), you would push changes to your own public repository, a fork of the official version. Then, you would create a merge request or a pull request, usually via a web interface of the hosting service, again describing the changes as a whole there.
In the case of using the central repository (perhaps, in a shared maintenance model), you would push changes to a separate and possibly new branch in the integration repository, and then send an announcement to the maintainer so that he or she would be able to find where the changes to merge are. The details of this step depends on the exact setup; sending announcement might be done via e-mail, via some kind of internal messaging mechanism, or even via tickets (or via the comments in the tickets).
The development documentation might include rules specifying to where and to what place to send announcements and/or changes to. It is considered a courtesy to notify the people who are involved in the area of code you are touching about the new changes (here you can use git blame and git shortlog to identify these people; see Chapter 2, Exploring Project History). These people are important; they can write a comment about the change and help reviewing it.
Tip
Crediting people and signing your work
Some open source projects, in order to improve the tracking provenance of the code, use the sign-off procedure borrowed from the Linux kernel called Digital Certificate of Origin. The sign-off is a simple line at the end of the commit message, saying for example:
Signed-off-by: Random Developer <[email protected]>
By adding this line, you certify that the contribution is either created as a whole or in part by you, or is based on the previous work, or was provided directly to you, and that everybody in the chain have the right to submit it under appropriate license. If your work is based on the work by somebody else, or if you are just passing somebody's work, then there can be multiple sign-off lines, forming a chain of provenance.
In order to credit people who helped with creating the commit, you can append to the commit message other trailers, such as Reported-by:, Reviewed-by:, Acked-by: (this one states that it was liked by the person responsible for the area covered by the change), or Tested-by:.
Completing a peer review of changes is time-consuming (but so is using version control), but the benefits are huge: better code quality, reducing the time needed for quality assurance testing, transfer of knowledge, and so on. The change can be reviewed by a peer developer, or reviewed by a community (requiring consensus), or reviewed by the maintainer or one of his/her lieutenants.
Before beginning the code review process, you should read through the description of the proposed changes to discover why the change was proposed, and decide whether you are the correct person to perform the review (that is one of reasons why good commit messages are so important). You need to understand the problem that the change tries to solve. You should familiarize yourself with the context of the issue, and with the code in the area of changes.
The first step is to reproduce the state before the change and check whether the program works as described (for example, that the bug in a bugfix can be reproduced). Then, you need to check out the topic branch with proposed changes and verify that the result works correctly. If it works, review the proposed changes, creating a comprehensive list of everything wrong (though if there are errors early in the process, it might be unnecessary to go deeper), as follows:
- Are the commit messages descriptive enough? Is the code easily understood?
- Is the contribution architected correctly? Is it architecturally sound?
- Does the code comply with project's coding standards and with the agreed upon coding conventions?
- Are the changes limited to the scope described in the commit message?
- Does the code follow the industry's best practices? Is it safe and efficient?
- Is there any redundant or duplicate code? Is the code as modular as possible?
- Does the code introduce any regressions in the test suite? If it is a new feature, does the change include the tests for the new feature, both positive and negative?
- Is the new code as performing the way it did before the change (within the project's tolerances)?
- Are all the words spelled correctly, and does the new version follow the formatting guidelines for the content?
This is only one possible proposal for such code review checklist. Depending on the specifics of the project, there might be more questions that need to be asked as a part of the review; make the team write your own checklist. You can find good examples online, such as Fog Creek's Code Review Checklist.
Divide the problems that you have found during reviews into the following categories:
- Wrong problem: This feature does not lie within the scope of project. It is used sometimes for the bug that cannot be reproduced. Is the idea behind the contribution sound? If so, eject changes with or without prejudice, do not continue the analysis for the review.
- Does not work: This does not compile, introduces a regression, doesn't pass the test suite, doesn't fix the bug, and so on. These problems absolutely must be fixed.
- Fails best practices: This does not follow the industry guidelines or the project's coding conventions. Is the contribution polished? These are pretty important to fix, but there might be some nuances on why it is written the way it is.
- Does not match reviewer preferences. Suggest modifications but do not require changes, or alternatively ask for a clarification.
Minor problems, for example, typo fixes or spelling errors, can be fixed immediately by the reviewer. If the exact problem repeats, however, consider asking the original author for the fix and resubmissions; this is done to spread knowledge. You should not be making any substantive edits in the review process (barring extenuating circumstances).
Ask, don't tell. Explain your reasoning about why the code should be changed. Offer ways to improve the code. Distinguish between facts and opinions. Be aware of negative bias with the online documentation.
Not always are the changes accepted on the first try. You can and will get suggestions for improvement (and other comments) from the maintainer, from the code reviewer, and from other developers. You might even get these comments in the patch form, or in a fixup commit form.
First, consider leading your response with an expression of appreciation to take time to perform a review. If anything in the review is unclear, do ask for clarification; and if there is a lack of understanding between you and the reviewer, offer clarification.
In such case, the next step is often to polish and refine changes. Then, you should resubmit them (perhaps, marking them as v2). You should respond to the review for each commit and for the whole series.
If you are responding to the comments in a pull request, reply in the same way. In the case of patch submissions via e-mail, you can put the comments for a new version (with a response to the review, or a description of the difference from the previous attempt), either between three dashes --- and the diffstat, or at the top of an e-mail separated from what is to be in the commit message by the "scissors" line, for example, ------- >8 -------. An explanation of the changes that stays constant between iterations, but nevertheless should be not included in the commit message, can be kept in the git notes (see Chapter 8, Keeping History Clean) and inserted automatically via git format-patch --notes.
Depending on the project's governance structure, you would have to wait for the changes to be considered good and ready for the inclusion. This can be the decision of a benevolent dictator for life in open-source projects, or the decision of the team leader, a committee, or a consensus. It is considered a good practice to summarize the discussion while submitting a final version of a feature.
Note that the changes that got accepted might nevertheless go through few more stages, before finally graduating to the stable branch and be present in the project.