My team uses Team Foundation Server for source control, and today I fixed some bugs and smoke test application before I checked it in but I forgot to comment some code. (This code made the UI a little bit strange.)
I want to know what good practices there are before checking code in - I don't want to make this kind of mistake again.
One thing I have gotten in the habit of doing is always looking at the diffs of every file I'm about to check in, right before I check them in.
You should never check-in commented-out code. If you have code that needs commenting out before check-ins, you are doing it wrong.
As for rules:
Build
3.1 Fix build errors
Run tests
4.1 Fix broken tests
Go to 1 (until there is nothing new to get)
Only check in when all steps are complete.
See check-in dance.
Other good practices:
rebase -i and clean up your local history, squashing commits as necessary, so the mainline has no ugly work-in-progress commits.
Commented
Feb 9, 2011 at 2:12
I'm not trying to be too much of a pantsweasel here, but the assumption in this question (and all but one of the answers) mostly applies to Centralized VCS, such as TFS, SVN, Perforce, etc.
Fair enough, it's what the OP is using.
On the other hand, however, when using DVCS (such as Mercurial and Git), you usually shouldn't wait to checkin, and most of the things mentioned in answers - such as diffs, get latest, merge, etc - are not necessary. Even things like code reviews and tests are better to do after the checkin (though perhaps before pushing, depending...)
The one exception I saw here (so far) is associating with a work item. Of course, commenting on the checkin is also good...
Three things that I didn't see in other answers:
Include new files
Revert unchanged files
Check your submitted commit
Two things when I use Git:
Atomic commits:
git add --patch to split my change up into multiple parts if necessary.View diffs while summarizing
git commit --verbose so I can see a diff of my change while I'm typing in my commit message. (Or I use my patched git-vim to show the diff.)A few items of 'good practice' that I enforce on my team's servers are pretty straight forward. First, before you check in, you should always get latest and run a local build, to ensure that no one else has checked anything in that you code will clash with. Additionally, take care of any code conflicts on your local machine, not the server. Once your code, with the latest code downloaded, has been confirmed to build and work properly you are ready for the next step. Run any automated tests then begin your check-in to ensure they still function properly. Then, just to be sure, get latest again.
It's possible, as a TFS Admin, to enforce commenting on all check-ins. I would recommend always putting in check-in comments for your work regardless of if it is enforced or not. If you have the option to do so, enforce it. Make sure the comments are, at the least, a general summary of what you changed since the last time you checked your code in. That way, if something goes wrong, you can look through the check-ins and see, roughly, what was changed in that check-in. It makes debugging a broken build much easier.
Additionally, if you have TFS Admin privileges, enforce rolling builds on check-ins (to make sure everyone else knows right away if their check-in breaks something), and you can set up the server to either perform a gated check-in (if the checked in code breaks the build, the server rejects it), or you can simply have it create a bug and assign it to whoever broke the build.
There are a few other options you can turn on or off to keep everything well in order, or to suggest to your TFS-Admin to turn on to keep things nice and clean...but they are largely preference
Search and replace curse words ;-)
If you are checking in from Windows, check if your code does not have those invisible ^M characters -- editors in UNIX often give errors/warnings 'cause of that.
Also try and replace tabs -- different users will end up seeing tabstops differently some with 4 spaces, some 8 and not good for code readability.
Best approach IMHO is to have a pre-defined script check your code against the coding guidelines of your organization. Loads of source control systems have this functionality.
At my company we use check-in reviews. These don't have to be detailed, but just showing someone the diffs in your changelist and talking through them will sometimes highlight the odd typo you've missed in testing.
Our source control server won't allow you to check in unless you note the reviewer's name in the comments (in the form !initials, eg !BW if Bruce Wayne did your review). The reviewer gets an email noting that they helped review. This is open to obvious exploitation but seems to work pretty well.
Whenever possible, I like to associate a check-in with a work item. This gives you some contextual information about WHY this was changed, not just WHAT was changed. TFS has a fairly decent work item tracking system, so this is rather trivial to do at the time of check-in.
(this is in addition to reviewing the diffs of my changes)
One small thing I do is to not check in files which haven't really changed. These files may have been modified inadvertently, or may have been involved in refactorings which have either been rolled back or otherwise been made moot.
This way, your changeset (associated with a work item) contains exactly the files necessary to satisfy the work item.
To combine all answers here and give a complete checklist
[check in / check out] you should not check in directly to the stream others are working in. You should have a stream strategy: e.g. per developer a stream in which you can check in and check out independently without bothering others: your work will be safe but in your own development stream so [only in your own development stream]. With every check in you associate it with a change record so that your changes are atomic against that change called the change set (so you can distribute individual rfc's / bugs etc... without having to deliver ' everything ' ).
[then rebase with your team stream] it means you get the changes from others in your own stream. During that operation you can see in the merge dialog all the "diffs" and go through them or... if there are thousands and/or you are using not code but also e.g. data models / siebel projects etc... rely on either non trivial merges , trivial-automatic and trivial manual merges , the last category contains the difficult ones. Remember that you are still working in your own stream.
[complete rebase] If all is ok then check in all the changes you just got from the team stream: your own stream is now up-to-date
[deliver] now deliver your work to the team stream. IF you dont want to deliver everything you can also select e.g. 1 specific RFC with that specific versions of files or a set of RFC's / defects solved.
[test deliver] it should go ok but since the chance exists that someone in the meanwhile delivered changes also: you can test if your work works with the latest changes on the team stream. With the same merge dialogs showing differences.
[complete deliver] complete your deliver and your work is now in the team stream.
To make it more complex: Since there is still the chance that the work you delivered = ok BUT you are already working on a further version you should baseline always after deliver and indicate which baseline that one is that is preferred for other users to rebase from. That makes sure that other developers get a recommended one and not the latest version on the stream (if you are working in that scenario). That is also a Triple check so that even IF the latest versions in the team stream are "bad" they are still not the ones that others rebase to or look at and your configuration manager can than merge the previous version to the next version to undo your delivery.
In your example you give that you forgot to commented out code. Mistakes happen. The configuration management system around it should take care of it. It can really one be that e.g. thousands of changes come in and "builds" and "integrations" take place in a hierarchy of streams on different servers chained and processed in time. So even if after 5 months your commented out code is tested on an integration server because your code needs integration with other code and systems it should still be possible to atomically take out your change set and still carry on. So the best practice is more or less on a higher level. The overall design of the configuration management streams should take care of it. For individual developers the best practice is ofcourse to validate / unit test. But from the big picture to " make it work " the best practice is to follow the system and always provide that meta information of related change sets for the guys later on in the chain.
Some of the following apply more than others (or in different forms) depending on your SCM, so here they go:
NOTE : some of the items above seem rather obvious, but you would not believe how many people actually work on the main branch or make changes on production first and then manually create deltas to go on version control... directly on the main branch... and with labels. Sweet like fermented bile mixed with unwashed armpit juice... yeah, like that.
Have a personal check list. Start it empty when you mess up, at an entry. When it becomes second nature remove it from the list.
Run the tests. If they pass check it in. If you mess up and some thing gets past the tests, then write a test.
We do the following ...
Test - We want to make sure that it works. At the very least, we want to know that it does not break anything.
Code review, or at least a buddy check - This is a great way to ensure that the knowledge gets spread around and that people are kept up to date. It also helps catch bugs before checking things in.
Send an advance notice - An advance notice is sent to the group before checkin. The purpose is not only to let others know which files or areas are changing, but it gives them a heads up (should they choose to take notice) in case those changes are expected to affect them.
Some hours after sending the advance notice, the check in is performed, and the group is informed via email. Everyone in the group can know when the particular work on a bug or feature is done.
A copy of the checkin notice is pasted into the fix record associated with the bug or feature. When searching through the records, we find that it is very handy to have an idea of what the fix/feature entailed.
Make sure your code is formatted properly (e.g. for Java: select your code and press Ctrl-Shift-F in Eclipse). But be careful doing the same for an entire document.
Always, always, always, check that whatever changes you've done doesn't break the build. Especially minor changes that may seem trivial.
Once I made a very minor change that I didn't think would cause any problems right before I left work for the weekend. Sure enough, that little change broke the build and no nightly test runs were executed for our project. The Q&A chief wasn't too happy about that, and rightly so.
Look for portions of your changes that can go in as standalone units.
Often, by the time I have a working fix or enhancement to the code, there are quite a few changes. Some of them are specific to the behavior change I'm going for; others are refactorings I did to make that change cleaner.
I prefer to check in each refactoring separately, with its own change description, like this:
REFACTORING: Rename X to Y
X made sense before because ... but now it should be Y. This is related to work for issue #9.
Then, once each good refactoring is checked in, the final behavior change is often trivial.
Also, some changes affect many lines of code but aren't very interesting, while other changes are very localized but have an important impact. If these changes are checked in together, it can be hard to read the diffs. So, I keep them separate.
Later, when someone is reading through the change history, it's obvious how things got to the current state of affairs, and why they are this way. It's also trivial to undo my behavior change because it's not tangled up with tons of other edits.
Do what you would do when returning something you borrowed from someone. Make sure it is clean and in good shape. If you did a mess, be sure to clean before returning the code to its owner (in most case, your employer).
I keep a local hg repo for my work.
I don't claim these are the best, but they do work for me.
When I write code that I know isn't meant to be checked in, I add a line before it containing "//TEMP:" and after it with "//END TEMP.". This, together with doing diff before checking in, promises that I won't check that code in by mistake.
Thoroughly test everything you added or changed. Try every possible case you can think of to try. Don't leave testing to QA. If I had a sandwich for every time I've made some minor change, and then tried some test cases just to be on the safe side, and immediately saw problems, I'd have a lot of sandwiches. I usually say out loud to myself "I'm really glad I tried that..."
You say the UI became strange after your change. If you'd only ran the program and looked at the UI before checking in, would you have noticed the problem?