18

We typically used Perforce and SmartBear's Code Collaborator at Big Corp and now we're also going to be using Mercurial for certain projects.

Code Collaborator support Mercurial (we are using version 5) and I'm trying to determine when the best time (during the commit/push to server) process is the best/efficient time for a code review

thanks

cbrulak
  • 357
  • 2
  • 8
  • You should probably separate the two questions. (a) belongs here, but (b) probably would belong on stackoverflow or serverfault – blueberryfields Feb 15 '11 at 22:20
  • Thanks @blueberryfields. i actually fixed the issue, the problem was with the bin/hg.cmd file being in the path and no the exe. – cbrulak Feb 15 '11 at 23:16

2 Answers2

22

We actually went through nearly the exact same thing at my company recently. Here's what we did:

  • We keep a central definitive copy of all our repositories on one server. When developers want to "check out" code, they go to this server and clone from the repositories there. Likewise, when the development cycle is complete, code gets pushed into the appropriate repository there also.

  • We separate stable repositories from development repositories. We require that code be reviewed before it is pushed into a stable repository. (This is significant because we also require that our stable repositories contain the code that is currently running in production, differeing only by pending code promotions.)

To enforce the code review, we wrote a pretxnchangegroup hook (documented in the HG Book). We leverage the fact that when this hook runs, it can see the repository as-if the code changes were permanent, yet also gives us the ability to prevent the push. Basically, the process is as follows:

  1. The developer initiates a push to the stable repository (yes, this really is the first step)
  2. The hook runs and grabs a list of all the changesets included in the transaction (by running HG log). It then queries a database we built to see if those changesets were included in a code review. (The table matches the hash of a changeset with a code review ID).
    • If this is the first time any of these changesets have been seen, then we create a new Code Review (using the Code Collaborator command line), and then record these changesets in the database with that code review's ID.
    • If we've seen some (but not all) of the changesets, we run the (Code Collaborator) command to attach the new changesets to the existing review and record these new changesets in the database.
    • If all of the changes were found in the database (ie, they've all been added to the code review), then we verify that the status of the code review is Complete. However, if there were any new changesets (or the code review isn't complete), the hook exits with a non-zero status code (causing Mercurial to roll back the transaction) and outputs a friendly message on Standard Error explaining to the developer that the code review needs to be finished.

In essence, this provides the developer with a pretty streamlined process (all they have to do is an hg push) and completely automates the creation of the code review (and uploading additional changed files to the review), while ensuring that all code gets reviewed.

Note: This is a fairly simple process (and relatively new for us), so it may not work for everyone, and there may be some design bugs that we haven't run into yet. But so far, it's worked quite nicely.

Ryan
  • 389
  • 2
  • 7
  • Would you explain your decision for checking into your stable environment prior to code review? To me, stable seems to be a misnomer. – Jordan Apr 25 '11 at 06:55
  • 1
    It probably wasn't clear from the answer, but it doesn't actually make it into the repository unless all of the changes have been added to the code review and the review is complete. If the hook exits with a non-zero exit code, Mercurial rolls back all of the changes that were being pushed. Thus, that particular hook provides a very convenient place to not only get the diffs for the review, but also to enforce the review before the changes are allowed into the repository. – Ryan Apr 27 '11 at 13:38
  • 1
    Wow. Can I come work for you? – Rich Nov 08 '11 at 05:06
  • @Ryan - How do we implement pretxnchangegroup hook, the link you provide does not give detail explanation on how it can be implement, does not give the sort of function template we should follow, where to put the hook. I have no python experience. Please could you redirect me to a correct source or the template for pretxnchangegroup hook. Ta – Simple-Solution Oct 23 '13 at 10:37
2

It depends on how you have your repository structure and what you are trying to accomplish. We prefer to do "pre-commit" reviews, which in the world of DVCS really means "pre-push". DVCS's are nicer in this environment (when compared to traditional SCMs) because they have built-in functionality for saving your local changes and getting your workspace back so you can work on something else.

If you want to do post-push reviews, the ideal workflow depends heavily on your repository structure. For example, let's assume a repository structure that looks like the one discussed in this article on Git repository layouts. In this case, you may want to review changes that are being merged into develop. Individual commits on feature branches may not make sense to review. Obviously all hotfixes must also be reviewed along with the merges into master.

If instead you have a single integration branch where people are checking in directly, you would want to review all pushes to that branch. That is probably slightly less efficient, but could still work. In this environment, you would have to ensure that all the changes that had been pushed are reviewed before you cut a release. That can be trickier.

As for b) the only thing I would suggest is emailing SmartBear support (support@smartbear.com) directly. We'll (yes, I work for SmartBear) be happy to help you work out your path issues, but there's not enough information in this question to fix your issue. The normal process is to just run the installer and everything just works. Apparently something has gone wrong in that process.

Brandon DuRette
  • 474
  • 2
  • 5