Shelvesets and Code Review

Question

I don't really understand how this works. I'm a developer, I change some code, instead of checking it in I shelve it so that it can be reviewed by a team leader. So the team leader reviews it and then what? How can the team leader mark this code as acceptable for checking in?

Thank you,
Sammy

Answer 1

Dear Sammy:

  The shelve is that you can save your work without actually checking in.  Here are some scenarios that the shelve is useful for:

     1. While some code isn’t complete but you have to do high priority tasks immediately. You can set aside pending changes using the shelve. The incomplete code can be saved and protected but doesn’t affect the other’s work because it doesn’t really check in.
     2. Sometimes the project needs a daily build. The code checked in should be complete or at least pass the compile. If the task is too long to complete in one day, you should not check in the incomplete code, then you can use the shelve to avoid changing the old version of code in the server.

  In your scenario there is no special mark needed for the shelved code, if your team leader reviews the code, and accepts the code, you can un-shelve the code and check them in the server.

 

Answer 2

If you want a defined workflow for the code reviews, you can check this codeplex project which does exactly that.

http://www.codeplex.com/TFSCodeReviewFlow

Otherwise, some teams actually disable checkin for most team members and the team leaders are the only ones that can checkin, so in such a situation the team leader would perform the actual checkin (potentially with the /author flag from the command-line to say who the original author of the changes was, assuming they have the CheckinOther permission)

Since you own the shelveset, the team leader can't modify it, so communicating that it's been approved is out-of-band with respect to the shelvesets itself.  Communicating it via work items is one option (and what the workflow does), but you could use OneNote, SharePoint, email, etc. as well.  It certainly would be useful if others could add their own metadata to your shelvesets, but that's not currently in the plans for a future version.

Answer 3

Well thank you very much that was very helpful. Yet I cannot seem to find the right solution. Unfortunately, the TFSCodeReviewFlow is incomplete. For example what would prevent a user from unshelving the reviewed shelveset, modifying it, and checking it in?

Answer 4

Nothing would prevent that outside of not allowing those users checkin permissions, outside of telling them not to.

At the end of the day, no system of permissions will save you if you're giving write access to the wrong people :)  TFS can't magically tell which checkins are "valid" and which aren't.

We *have* gotten the request to let people checkin shelvesets directly, but that doesn't fit with our current architecture since checkin policies are evaluated on the client, so we can't do an all-server-side operation like that without failing to check the policies.

Answer 5

We just use the simple option of shelving, creating an entry in a sharepoint excel spreadsheet (or create a work item), someone else reviews the shelveset and then marks it as passed in the spreadsheet / resolves the work item. The originator then just unshelves and checks the changes in.

It would be good to have some kind of cross between this process and Microsoft's Gauntlet system (which is probably overkill for most people). Checking shelvesets in directly would be good, and I note you can evalute check-in policies BEFORE shelving, and so the reviewer could just click a button to check in the shelveset if the policies were all satisfied at shelving time and if so, check the changes in? Then the checkin could automatically have the owner set to the shelveset owner and the committer set to the code reviewer?

Answer 6

We've talked about Team Build including something similar to Gauntlet, but it won't make it into Orcas.  IIRC, we don't currently track on the server whether the client policies were satisfied on a shelveset, so that (and "directly checkin a shelveset") would need to be new functionality on the server first, so this isn't going to change for Orcas.

Good idea, though, and we've gotten the feedback from multiple customers, so it's definitely under consideration for a future version.

Answer 7

The other (less clever) idea is to have a check in policy that disallows check ins unless they are on another user's behalf. Therefore no-one is allowed to check their own code in and has to get it reviewed and checked in by someone else. The only problem is that people might just start checking their own changes in under someone else's name!

This hit a brick wall anyway, as there is no check in owner property available in the PendingCheckin object.