Difference between revisions of "Version Control Systems 101"

From Mpich
Jump to: navigation, search
(Review Process)
(General Rules of MPICH Code)
Line 61: Line 61:
=== General Rules of MPICH Code ===
=== General Rules of MPICH Code ===
* Read and follow the [[Coding Standards]]
* Use 4 space indents, not tabs
* Use 4 space indents, not tabs
* '''Don't leave extra whitespace!'''
* '''Don't leave extra whitespace!'''

Revision as of 13:58, 9 August 2013

To help get everyone up to speed on the way we use VCS, specifically Git on projects like MPICH, we've put together this Wiki page as a reference. Some of the content here may be remedial, but please go through all of it to make sure you're contributing code in the way that everyone else on the team expects. This tutorial will cover:

  • What Goes into Making Each VCS Commit?
  • What Makes a Good Commit?
  • Interoperation with Trac
  • Git workflow (This is a short version of an excellent and much more extensive post (Git)

I will assume that you're familiar with what a Version Control System is and why we use it. If you need a good introduction on VCS in general, you might try one of the many tutorials available on the web (citation needed).

What Goes into Making Each VCS Commit?

Sample Git Commit
  1. Header - This includes:
    • The hash identifying the commit. We usually only use the first 8 characters as this is all that Trac looks at. You can get those by adding --abbrev-commit to your git log command.
    • The original author of the commit. This may be different than the person who originally wrote the commit if they do not have write privileges.
    • The date of the original commit. Again, this might be different than when it was actually pushed.
  2. The Subject of the commit. This describes the actual changes in the commit and is critical to be both textually useful and correctly formatted. More on this later.
  3. The sign-off from another developer (if necessary). Anything that's not a trivial change (and sometimes even those) needs to be reviewed by another developer who is qualified to certify that your code is correct and formatted well.
  4. The code diff. This is the patch itself.

What Makes a Good Commit?

Single Idea Per Commit
  • Only have a single idea per commit
    • Often just a single file or a few lines.
    • Makes automated testing much easier
      • If your commit includes features A & B and we have to roll it back to fix B, we don't want to lose A as well.
    • Code refactoring should be separate from everything else.
      • If you need to change API calls, that should be completely separate.
  • Don't pollute code that isn't yours - Try to make the least intrusive version of your change as possible. Don't try to fix things like whitespace in the same commit as code changes. If it's absolutely necessary to reformat existing code to make it more readable, that should be a separate commit.
    • Doing this makes it hard to separate out who actually wrote the code.
  • Format the code in the same style as the existing code
    • If the existing code uses tabs instead of spaces, continue to use tabs. Again, if the code needs to be reformatted, make that a separate commit.
Bad Whitespace Example
    • This includes new whitespace at the end of the line or lines that only contain whitespace.
    • Catch this early by turning on highlighting in your editor or using git show.
      • VIMRC
        • autocmd ColorScheme * highlight ExtraWhitespace ctermbg=red guibg=red (this goes before colorscheme)
          highlight ExtraWhitespace ctermbg=red guibg=red
          match ExtraWhitespace /\s\+$/
          autocmd BufWinEnter * match ExtraWhitespace /\s\+$/
          autocmd InsertEnter * match ExtraWhitespace /\s\+\%#\@<!$/
          autocmd InsertLeave * match ExtraWhitespace /\s\+$/
          autocmd BufWinLeave * call clearmatches()
      • EMACS?
Good Commit Message
  • Write a good commit description
    • The first line should be a short (<50 characters) description of the change.
      • Git uses this for the short version of the log
    • Use a blank line between the first line and the rest of the description
    • Write a complete description of the code change in the body.
    • Use correct punctuation, spelling, and cApItaLizAtIOn
    • Good blog post about how to do this correctly
Commit Messages to Trac
  • Refer to Trac tickets in your commit messages
    • Using keywords makes Trac do magic to automatically attach commits to tickets
      • Fix #1234
      • Fixes #1234
      • Resolves #1234
      • Ticket #1234
      • See #1234
      • #1234

General Rules of MPICH Code

  • Read and follow the Coding Standards
  • Use 4 space indents, not tabs
  • Don't leave extra whitespace!
  • Comment everything that isn't completely obvious
  • Wrap code text at 80 characters
    • 72 for Git commit descriptions
    • 50 for Git subject lines
  • Use established naming conventions
  • Check all return values!

Using Git with MPICH

Git is huge, but this will cover the most important pieces for contributing to MPICH. Refer to the wiki page for more details.

Git graph with multiple repositories and branches
  • Git is not SVN. It's not linear. It's a graph. Use it accordingly.
  • Multiple repositories can intermingle with multiple branches.
    • Often of an origin or mainline repository with a review queue in another repository
  • Most changes shouldn't go into the main repository initially.
    • They need to be reviewed by another developer who will sign off on them first.
    • After being signed off, they can be pushed to the main MPICH repository.

MPICH Git Workflow

If you have commit privileges to mpich and mpich-review, this is how the workflow will be. If you don't have those rights, your workflow will probably include submitting patches or something else. If you are submitting patches, make sure you send git formatted patches which include the hash and original author so you will get credit and they are easy for us to apply.

  • Do work locally and commit.
  • Push to remote review repository
    • git push mpich-review a1b2c3d4:branch-name
  • Someone else will review the code and sign-off (may require multiple signoffs)
    • git checkout mpich-review/branch-name
    • git commit --amend --signoff
    • git push -f mpich-review branch-name
  • If you want to sign off on a bunch of commits at once from a feature branch and apply them onto master, you can do that too:
    • git checkout mpich master
    • git cherry-pick --signoff a1b2c3d4^..e5f6g7h8
      • The ^ makes the range inclusive
    • git push mpich master
  • Then it can be pushed to the main repository and the branch can be deleted
    • git push mpich branch-name:master
    • git push mpich-review :branch-name

Review Process

When your commit goes to be reviewed by someone else, the reviewer should look at these things:

  • Does the code compile?
  • Does it accomplish the goals set forth in the commit message?
  • Can the code be meaningfully broken into multiple commits?
  • Is the commit message useful and sufficient?
  • Is there extra whitespace at the beginning/end of the line?
  • Does this code meet the coding standards of the team?
  • Is the formatting consistent with the existing code?
  • Does this code introduce obvious new bugs?
  • Does this code perform well (if applicable)?