What are some good practices before checking in source code?

Question

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.

score 149 · Accepted Answer · answered Feb 08 '11 at 14:26

149

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.

answered Feb 08 '11 at 14:26

Cassie Dee

361
1
4
4

46

+1 it's obvious, but if anyone out there is not doing this, then they are doing it wrong! – David Heffernan Feb 08 '11 at 14:28
1

+1: Absolutely recommend. I always look at the diffs and do catch stuff that I forgot to do. – Gabriel Magana Feb 08 '11 at 14:57
1

+1: It is obvious but it is amazing what you can catch. – DBlackborough Feb 08 '11 at 15:13
6

+1 Actually, it is not that obvious but if you don't do it you'll be sorry. – Nemanja Trifunovic Feb 08 '11 at 16:50
14

+1 Also, if you think this is too much work, you are probably committing too much at once. – mpeterson Feb 08 '11 at 17:53
5

Also, looking at the diffs makes it easier to know what to put into the descriptive note about what you've been trying to achieve with your changes, especially in case you did multiple fixes. – Jonas Feb 08 '11 at 19:16
1

+1: I've gotten in the habit of doing this in the last few years, and it's amazing how much less cruft I check in. – Bob Murphy Feb 08 '11 at 19:37
4

If it's not worth looking through, it's probably not worth checking in. – Robert Jeppesen Feb 08 '11 at 22:35
1

It's amazing how many times I've added comments, or decided not to check in after looking doing my diff-before-checkin. – Chuck Feb 09 '11 at 16:29

Oded · Answer 2 · 2011-02-08T15:07:52.113

63

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:

Get latest
Fix merge conflicts
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:

Review the list of files to check-in to ensure they are the expected files.
Review the changes for each file (deletes/additions/diffs)

edited Feb 08 '11 at 15:07

answered Feb 08 '11 at 14:29

Oded

53,326
19
166
181

I did a double take here. Perhaps you mean 'commented-out code'? Myself, I'd lean towards never checking in uncommented code! – Pontus Gagge Feb 08 '11 at 14:32
11

+1 - that's a pretty complete list right there! DON'T BREAK THE BUILD!! – ozz Feb 08 '11 at 15:12
About commented-out code: What if you are in the process of refactoring a large bunch of code? Today I refactored a project, porting it from Postgis to Spatialite and I had to comment some SQLs partially, I commented some function calls because I need to discuss those with my boss + I did some if (false) { old_code... } else { System.out.println("Watch out!"); }... ;-) My point was that I wanted a first working version + needed too check in to prevent merge conflicts – Philip Feb 08 '11 at 21:21
1

@Philip - So long as you _know_ that this is not good practice and so long as this is a simple _short term_ intermediary, then this is one of the few cases to break that rule. I find it much more concerning when people check in commented out code so they "won't lose it". – Oded Feb 08 '11 at 21:34
2

@Philip, That's why git is nice. You can commit those WIP changes locally, as often as you like, then before you push to the main repository, you `rebase -i` and clean up your local history, squashing commits as necessary, so the mainline has no ugly work-in-progress commits. – Alex Budovski Feb 09 '11 at 02:12
2

@james http://gobarbra.com/hit/new-d1bbb9dcf1a7fdbec622e107ca25e16c :P – pkoch Feb 09 '11 at 02:46
@pkoch: I hope that's included in your build break emails. – idbrii Feb 09 '11 at 23:21

score 20 · Answer 3 · answered Feb 08 '11 at 19:00

20

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...

answered Feb 08 '11 at 19:00

AviD

490
4
12

5

+1 for commenting on the checkin. It's not policy in my shop, but I try always to leave a descriptive note, if only to jog my memory later on. – PSU Feb 08 '11 at 19:10
Agreed - I imagine that [Oded's workflow](http://programmers.stackexchange.com/questions/45479/what-is-a-good-practices-before-check-in-source-code/45481#45481) could benefit a lot from version control between each of the steps, or, at the very least, between each of the loops. – Kevin Vermeer Feb 08 '11 at 19:14
7

Don't all of those steps just move from when you checkin, to when you push? – user13278 Feb 08 '11 at 19:30
@user13278 some of them do, but differently. E.g. Merging is a completely different experience - and, you do while pushing, no need for a getlatest-merge-tryagain cycle. And you can do it for a whole bunch of changesets, and not have to remerge every checkin. In general, a lot of those steps don't have much to do with the checking in anymore - e.g. you pull when you want, not because you're checking in (or pushing). Of course you still need to test - but that can be on its own timeframe. Pushing still remains a lot more lightweight, but of course you want to make sure you dont push crap. – AviD Feb 08 '11 at 19:36
2

+1. Associating with a work item is the one hard thing to do in Git or Hg. You'll need to run a whole package, like Kiln. This is the (only) area in which TFS is good. It is harmful for version control though. – Robert Jeppesen Feb 08 '11 at 22:39
@Robert, I agree about Kiln - actually, that's what I'm using, and it is FANTASTIC. Of course, the integration with FB doesn't hurt... – AviD Feb 08 '11 at 23:50
@Robert can't you just refer to the work item id in your commit messages? You could even add a reminder for it in your commit message with `git config commit.template '~/template-file'` – idbrii Feb 09 '11 at 23:14
@AviD (original answer): don't you still test your code and check your changes before you commit? While I agree that Git is great for having tiny and frequent commits, I still want to vet what gets committed so I can easily roll back to known good changes. Make sure you're improving the code and whatnot. – idbrii Feb 09 '11 at 23:19
@pydave you could, and you *should* - but you don't have to, since it's not that important. In fact, some say that you should commit *before* you start testing and such, just like you would always *save* beforehand. This way, there is a record of what you originally wrote, what was wrong with it, how you fixed it, etc. But in general I would agree with you, though that's mostly because my head isnt completely wrapped around DVCS yet... – AviD Feb 10 '11 at 07:18
@AviD Good point. Although the output of those tests (even if they're failures) is good content for your commit message. Especially since I haven't had much experience with `git bisect`. – idbrii Feb 11 '11 at 02:59

score 8 · Answer 4 · answered Feb 09 '11 at 02:10

Three things that I didn't see in other answers:

Include new files

Look for new files that aren't part of your changelist
May be specific to SCMs like Perforce -- you have to tell it everything that's in your change.

Revert unchanged files

I hate when I'm looking at other people's changes and there is a changelist with nine files but only three of them have been modified.
I also revert files with whitespace or otherwise meaningless changes.

Check your submitted commit

Make sure that the build stays green after your commit.
I used to have a second machine that I would sync, build, and run after my commits so if something went wrong, I could easily jump in and fix it.

Two things when I use Git:

Atomic commits:

Only stage individual functional changes for commit.
Make commits as small as possible. Make them easy to patch, revert, and understand.
I use git add --patch to split my change up into multiple parts if necessary.

View diffs while summarizing

I always check in with 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.)
This makes it much easier to go through your changes and describe the whole commit. Occasionally, I catch unintended changes at this stage. (Describing your change helps you think about it.)

+1 for being the only person to mention atomic commits. – Stephen Paulger Feb 09 '11 at 12:50 — Stephen Paulger, Feb 09 '11 at 12:50

score 7 · Answer 5 · answered Feb 08 '11 at 14:33

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

I like this answer. As QA, sometimes we trace a bug back to the commit that caused it to appear, and it is nice to have descriptive comments available. Also at release time, our shop creates something called release nores, which is a distillation of new features and changes, and the checkin notes are an important source of this information. — Omega Centauri, Feb 08 '11 at 21:02

score 7 · Answer 6 · answered Feb 08 '11 at 20:43

7

Search and replace curse words ;-)

answered Feb 08 '11 at 20:43

Throwback1986

259
2
2

Just be sure not to make the clbuttic mistake. – Tullo_x86 Jul 31 '12 at 23:29

score 4 · Answer 7 · answered Feb 08 '11 at 15:08

4

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.

answered Feb 08 '11 at 15:08

Fanatic23

7,533
4
31
56

4

Checking for the ^M characters only makes sense if a UNIX box is actually involved in the development process in any way. Some companies are all-Windows shops. – Dima Feb 08 '11 at 17:44
1

Exactly. This is why you don't use tabs. – Alex Budovski Feb 09 '11 at 02:16
Some SCMs handle line endings for you (some do better than others). Perforce (http://kb.perforce.com/?article=063), git (core.eol in git config), svn (svn:eol-style), etc. – idbrii Feb 09 '11 at 02:28
@Alex: Or you consistently use tabs everywhere. It doesn't matter which you do so long as you're _consistent_. – Donal Fellows Jul 13 '11 at 15:05
@donal, no. Herein is the problem; tabs are subject to your editor's configuration and hence inherently inconsistent. Some "editors" are inconfigurable, such as cmd.exe, and most Linux consoles, so you might even be inconsistent with yourself. – Alex Budovski Jul 17 '11 at 21:39

score 4 · Answer 8 · answered Feb 08 '11 at 16:46

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.

score 4 · Answer 9 · answered Feb 08 '11 at 17:57

4

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)

answered Feb 08 '11 at 17:57

mpeterson

647
4
10

2

This can be set as a check-in policy, so that no code can be checked in _without_ associating to a work item. – John Saunders Feb 08 '11 at 19:59
Good point, John. I am actually hoping to do this very soon where I work. – mpeterson Feb 08 '11 at 20:07
Enforcing stuff is usually counter-productive. Make sure your people understand that it's good for them instead. – Robert Jeppesen Feb 08 '11 at 22:42

score 3 · Answer 10 · answered Feb 08 '11 at 20:01

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.

edelwater · Answer 11 · 2011-02-09T02:07:49.463

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.

the answer from histumness happens 2 times : in step 2 and 6
the answer from Oded on check-in dance: idem but an extra layer of deliver and rebase on check in / check out to make sure you work isolated and errors can always easily be taken out even in later stages
the answer from answered guildsbounty: get latest is step 2. For the build: it really depends if you HAVE a build... in my world you have inputs from data models, word documents, requirement sheets, config data from informatica, siebel, etc.. , and yes also java code, .net code etc... that all should mingle together. So there is no really "a build" here but more a integration higher up depending if that single e.g. build from your "code" integrates with all the rest of the stuff since you can not know for sure if it is integration stuff and depending on their test environments it could be compiled stuff is needed or at higher deliveries another build because it needs something from another team.
the answer from guildsbounty on commenting and required: i think every environment in which you do not have integration of VERSIONS and Changes in change sets (and type: defects, RFC, hotfi) is not mature. I think its chaos if you can not e.g. automate release notes with the amount of bugs and rfcs submitted which are clickable through to the exact versions that are touched (since e.g. version 1 and version 3 of hello.c could very well be delivered but version 2 should not have been delivered because that stuff in there would be part of a later release but some noob already put it in) (so it means a manual decision IF you also want to take out version 3 of hello.c BUT taking version 3 out means you also have to take out all other version touched by that RFC/defect so you need to be able easily and fast with a tool to take the complete thing out)(even if multiple developers worked on parts of that same RFC) but at least you need stuff around it to decide on it etc...). Extra documentation is always handy but by associating change sets you get the full circle: a version <-- a change set <-- work items <-- a ticket/rfc/defect <-- a requirement. Meaning: you know which requirements are fully or completely delivered: one requirement has multiple RFC's or bugs or whatever. The RFC has multiple work items for multiple persons. that work item corresponds to a change set which exists of a set of versions (e.g. version 1 and 3 of hello.c on the integration stream that are ofcourse NOT the versions 1,2,3,4,5 in your development stream that version 3 in the integration stream corresponds to) (hence a version tree and tools to operate on it)
the comment from luis.espinal: dont break the build is double checked in rebase and deliver still... there are higher up deliveries for 'release managers and build meisters' who should see change sets and baselines as their info. " Never work directly on main branch" yes, the stream structure can be big or simple but in essence: developers have their own stream, they deliver to a team stream who deliver to a release stream. --> so that the deliveries from all teams (e.g. documentation team, requirements team, development teams, test team) are in their team streams and a release manager or configuration manager grabs the baselines that should go in a release and so makes sure the correct documentation with the correct test versions docs/outcomes with the correct requirement with the correct code versions are all in line for a release.

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.

score 2 · Answer 12 · answered Feb 08 '11 at 21:17

Some of the following apply more than others (or in different forms) depending on your SCM, so here they go:

Don't break the build - check only code that compiles.
Comment your check ins (and possibly your check outs if your SCM gives you that option.)
Don't keep stuff unchecked for long periods of time.
Check in often (several times a day if possible.)
Label meaningfully.
Label regularly.
Never work directly on the main branch.
Every release to production must have its own label (and a read-only branch off the main branch if possible). When possible do the same for UAT/Integration/Pre-production test releases.
You should be able to build exactly what it's on production from what is in your SCM and from a label.

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.

score 2 · Answer 13 · answered Feb 08 '11 at 21:52

2

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.

answered Feb 08 '11 at 21:52

ctrl-alt-delor

570
4
9

score 1 · Answer 14 · answered Feb 08 '11 at 17:43

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.

score 1 · Answer 15 · answered Feb 08 '11 at 20:57

1

Run your unit tests you've been so diligently working on. Green is good.

answered Feb 08 '11 at 20:57

Naftuli Kay

1,601
2
16
23

score 1 · Answer 16 · answered Feb 09 '11 at 09:08

1

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.

answered Feb 09 '11 at 09:08

Rune Aamodt

101
3

score 1 · Answer 17 · answered Feb 09 '11 at 13:51

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.

score 1 · Answer 18 · answered Feb 09 '11 at 22:36

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.

score 0 · Answer 19 · answered Feb 09 '11 at 00:58

0

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).

answered Feb 09 '11 at 00:58

Jason

397
1
8

git helps you clean your mess before committing publicly. Unfortunately, centralised VCSs do not. – Alex Budovski Feb 09 '11 at 02:19

score 0 · Answer 20 · answered Feb 10 '11 at 00:01

I keep a local hg repo for my work.

Whenever I check something in, I look over the diff and make sure that all changes are acceptable.
I try to note the key feature of the checkin.
I try to keep each commit size to one key feature.

I don't claim these are the best, but they do work for me.

score 0 · Answer 21 · answered Feb 20 '11 at 08:47

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.

score 0 · Answer 22 · answered Feb 20 '11 at 18:30

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?

What are some good practices before checking in source code?

22 Answers22

Linked