Just jotting down a few important points or commandments one (you) should keep in mind so that your commit reviewer has comparatively less hair loss on the following day.
1. Review your changes before you hit commit
Review the files you are committing and make sure there are no debug calls, no commented code blocks etc. Also, make sure you are not committing log files, no `.bak`, no `.tmp` etc.
2. Write better commit logs
Visualize you were reading the commit log and doing a review of the commit by just looking at the diff alone. Would it make any sense that time if I go ahead with the comment I just wrote? Do I need to put more details? Is it too verbose? Be in your reviewer’s shoes while you write a commit log. Explain briefly, to the point what you fixed and how. A few bullets perhaps, with proper references. That makes a great commit log.
3. Never commit the code that is broken
Once you complete coding, please test it at least once before you commit. It might be a one line fix but you might have forgotten a comma or you put a dot instead of a plus in JavaScript because you are so familiar with PHP (happens with me). Once you commit, somebody else might get stuck and will have to spend time debugging a new bug you just introduced. Be kind to your fellow coders, please do unit test. At least make sure the positive flow is functional before you commit.
4. Commit one bug fix at a time, whenever possible
It makes life so easier, to deal with one issue at a time. I know some people who work just eight hours straight on a bunch of issues, no commits in between and just before they leave office it finally arrives. A jurassic sized commit. The commit diff looks more like diarrhea with 158+ diffs. Somebody must be thinking it’s goddamn easy to figure out everything by just looking at such a diff.
Unless your one issue is so tied to another that you’ve to commit half way baked code, don’t do it. Once you’re done fixing an issue, unit test and commit it. The commit log also looks simpler and neat that way.
5. Use adequate references
It’s great to include references to tickets, revisions etc. while you are writing a commit log. For example, I just fixed the bug #14748 which used to make the login username textbox become disabled. While I write my commit log, I shall quote the bug number, like:
Fixed #14748 that caused the account login username text box to become disabled on page log.
If you were the commit reviewer, what is the next question on your mind once you read the above sentence? It’s obvious, “how the hell did he fix it”? So while it’s okay to say you fixed what, it’s also great to give a tip on how you did it. Doesn’t have to be a story detailing your learning curve on the bug, but a simple one liner saying what was the issue and what you did to make it all work.
So even better is,
Fixed #14748 that caused the account login username text box to become disabled on page log. * The username field had `disabled` class assigned which jQuery scanned on-load. Removed the class name.
6. Formatting in svn commit log?
How about,
Fixed #14749 - Implement Google Analytics on all user pages (Enhancement). Cached Google Analytics script locally at webroot/js/ga.js for improved performance. Wrote a cron shell at vendors/shells/refresh_analytics_script.js that fetches latest GA script. Implemented a CakePHP element for GA namely GoogleAnalytics. Modified default.ctp layout to call GA element.
or is even better the one below?
Fixed #14749 - Implement Google Analytics on all user pages (Enhancement) * Cached Google Analytics script locally at webroot/js/ga.js for improved performance. * Wrote a cron shell at vendors/shells/refresh_analytics_script.js that fetches latest GA script. * Implemented a CakePHP element for GA namely GoogleAnalytics. * Modified default.ctp layout to call GA element.
Here I’m trying to use trac formatting so that it will appear nice when you view it via a browser. For people not familiar with track, `space asterisk space` stands for a bulleted list and `hash followed by a number` for referring to a trac bug ticket. Having said that, this is one of those “good to have” things, no pressure.
7. SVN keywords are good but minimize their use
Don’t use the SVN keywords like `Id` or `Log`. We can always pull this information right from svn whenever we need. They cause merge conflicts usually when merging different branches.
That’s all I could think of now, or what I consider important to follow. Have fun!