New Fun Blog – Scott Bilas

This is the fifth in a series of posts on our peer code review process at Loose Cannon. In the third, I briefly mentioned a tool I created to bridge the gap between Perforce pending changelists and Crucible. Two great tastes that taste great together. A key part of our review process. In this post, I’ll talk about how it works in excruciating detail!

Caution: this might be the most boring post ever.

About The Tool

The “tool” is actually a command crucreate that is part of a Perforce-based tool I wrote called P4X for adding small utility functions. P4X is built on top of p4.net, which is itself built on top of a native lower layer. Unfortunately, p4.net is rather weak on supporting specific P4 “forms”, preferring only to give general form access. I assume this is to prevent issues with changes to forms across P4 versions. So of course I had to make yet a third layer that wraps up p4.net to build real classes instead of generic forms and arrays, like ChangelistSpec and FileSpec.

P4X comes in two flavors: p4x.exe, the command line tool (uses stdin/out/err), and p4xwin.exe, the gui tool (uses Windows dialogs). The command line tool is meant to be used from scripts and console apps including a command shell. It behaves similarly to p4.exe, mimicking P4’s simple command line format for consistency (prefix params for user/pass etc., command name + command args, ‘help’ for each, etc.).

The gui tool is meant to be hooked into P4V/P4Win, taking advantage of context-sensitive features, or as a simple external tool from editors and so on. For example, it includes a “blame” feature that receives a filename and line number from the editor, which in turn opens up FishEye to the given file and line (FishEye does a much better job than P4 annotate or P4Web IMO).

Installing P4X

Installing support for P4X commands is easy. For P4Win, you double-click a .reg file we have checked into our depot. And for P4V, you import custom tools from a .xml file (also in the depot). This adds in a variety of commands, including the one we care about:

Name = Create Crucible Review from Changelist… Command = p4xwin.exe Arguments = –c $c –p $p –u $u crucreate %c AddToContext = true

This will install a “Create Crucible Review from Changelist…” command to the Tools menu, but it will only be enabled if the user currently has a changelist focused. And by using AddToContext = true, we can right-click on any changelist and the command will show up as enabled. Running it passes in the changelist number in place of %c (or 0 if default). Note that it also passes in the current user/password/port settings. The crucreate command can’t ever assume a global set of connection settings. If these aren’t passed in, then P4X just inherits whatever the environment’s settings are.

It’s really convenient to be able to just right-click a pending changelist and get a review. My personal workflow goes like this for any given change:

1. Code and test.
2. Play Geo Defense.
3. Move bare minimum of files involved in the change to a new pending changelist if not there already.
4. Select all the files and diff to look for stupid things like #if xyzzy.
5. Play Geo Defense again.
6. As I go through each diff, I update the changelist’s description with why I did what I did.
7. Once done, proceed with the process detailed in the third post.
8. Play more Geo Defense.

Note that after creating the review, I will additionally go through the diffs again in the new review in Crucible to make any necessary comments inline.

How It Talks To Crucible

The Crucible Web API

Crucible uses an XML REST API for communications. This is a recent change, Atlassian having removed the previous SOAP interface. I understand that SOAP has all kinds of annoying problems, I really do. But nothing is simpler than telling Visual Studio to add a “web reference” to the WSDL’s URL, then suddenly being able to get full Intellisense on your new Crucible SOAP object as you’re writing code. I miss that.

Hopefully the industry will settle on a WSDL type thing for REST (there are several serious proposals getting attention, so it should be soon), Atlassian will add support for it, and then I will be able to use that! For now, though, I’m maintaining a C# wrapper to talk to Crucible’s REST API. Intellisense is something I just can’t live without. API’s should be explorable and obvious enough to use without having to resort to opening some HTML file and copying names into code. I heart static code analysis from generated files.

It’s not a huge problem in general, although for every new release of Crucible I download, I do need to review my usage of the API to deal with any changes. Atlassian is good about backwards-compatibility but, without a generated spec that I can type-check against statically, I can’t guarantee that there aren’t typos (on either side of the equation).

Again, not a huge problem, but worth mentioning.

Authentication Needs Improvement

Ok, well there is another frustrating issue with the API. I don’t have a way to reuse the user’s Crucible credentials from their web browser or Windows login. I have to send user/pass in plaintext through a GET auth-v1/login. Not cool. We use LDAP hosted by our Active Directory for all our services, and the whole point of that is to not ask the user for their credentials over and over. I don’t want to do a “save password” checkbox where it stores it in the registry either. It’s just a bad practice.

So instead, we have a “cruciblerobot” user with a password hard coded into p4x and is used for all API calls. It needs “Edit Review Details” to do its job which is normally only granted to admins, moderators, and creators.

I’m assuming this situation has been improved in 2.0, but haven’t had a chance to look at it yet. Ok, well, we, um, don’t have the money yet to renew our maintenance. But I’d really like a way to either reuse the browser’s cookie, or tell Crucible to trust the user’s existing domain login token somehow. Perhaps Crowd could help here, but we’re very happy building everything on top of AD (which is already paid for).

How It Works

What follows is a detailed doc on how the current version of crucreate works. I have a mile long list of improvements and bug fixes I’d like to make to it, but it works well enough now so it’s been a low priority to get past “good enough”.

Creating Reviews From Submitted Changelists

When P4X crucreate is run, it first checks to see if it was given a submitted changelist. If that’s the case, it takes a cheap shortcut and just has Crucible create it itself using a POST to cru/create. [Note that this may not work if crucreate was run after the changelist was submitted but before Crucible has noticed. The user gets an error and has to wait a few minutes for the scanner to pick it up, then try again.]

It would be better if it handled pending and submitted changelists the same. Unfortunately, it would have been more work to implement, and it’s rare enough that we need to do this that it wasn’t worth the time. Even in emergencies where a checkin has to happen immediately, the review is still always required to be created before the checkin.

Creating Reviews From Pending Changelists

If it’s a pending changelist, here’s what crucreate automatically does next:

1. Log into Crucible.
   • Do a GET to auth-v1/login with the user/pass for cruciblerobot.
   • Set a cookie “remember” with the resulting login token.
   • Make sure to use the cookie container on any web request from now on.
2. Check if the changelist already has a review by scanning its description for a review tag using a regex.
   • We need to know if we should offer to do an incremental review.
   • We also want to query from Crucible who the reviewers are on the other review, so it can pre-select them in the reviewer chooser dialog.
3. Query Crucible for the list of possible reviewers.
   • Do a GET to reviews-v1/[review]/reviewers.
   • The user submitting the review is removed from the list as it would be redundant.
   • Fake users like cruciblerobot are removed as well.
   • In 1.x (maybe fixed in 2.0), there is no way to query group membership, so for now I have the primary reviewer group hard coded. It rarely changes so I haven’t even bothered putting the list in the XML file.
4. A dialog comes up with available reviewers.
   • For totally new reviews, a primary reviewer is pre-selected at random.
     
   • For existing reviews (identified in step 2) then the reviewers from the most recent review are pre-selected instead. The user is also given the option of creating a new review or adding onto the current one as an incremental update.
     
   • Note that the Go button isn’t enabled until at least one primary reviewer is selected.
     • I know, it’s a more usable UI to always be enabled and do an error instead on a click, but I haven’t gotten around to fixing that.
5. If the user hits Go, we get to work. This is where we split down a couple code paths.

Creating New Reviews

If the user has chosen to create a new review, this is what crucreate does next:

1. Create an empty file to hold the patch.
2. For every file in the changelist…
   • Skip if binary file type.
   • Check if the file is scheduled for resolve and error out if that’s the case, telling the user to resolve them. We want a clean set of files to work with.
   • Create two tempfiles – a “before” and an “after”.
   • Print the file the user is sync’d to (the “have”, not the “head”) from P4 into a temp file, if it is not an ‘add’ action.
     • We want the “have” because we only want the changes the engineer made. If we use the head revision then changes from other people will get mixed in, which will ruin the review.
     • If an engineer wants to review their changes from the head revision, then they need to sync and merge, then run crucreate.
   • Copy the local file into another temp file, if it is not a ‘delete’ action.
   • Run diff.exe
     • Use these options:
       –unified=1000000
       –text
       –strip-trailing-cr
       –minimal
       <tempfile1> <tempfile2>
     • Note that the million lines of context ensures that Crucible has the full contents of each file. This is important! Crucible will still only show a few lines of context or whatever according to per-user prefs, but we need the –unified=1000000 so Crucible has what it needs in case the user wants to expand the diffs to full.
   • Skip if result is empty. It means there was no diff and there’s no point adding it to the review.
     • Note that for an add or a delete, the diff against a 0-length file will force every line to be different. This is great because you can see the full contents of the file being added or deleted. It’s a lot more useful than the simple “file was deleted/added” tag because you can make per-line comments like any other file.
     • This is an area where I think our solution is better than Crucible’s creation of post-submit reviews (as well as FishEye’s browsing of changelists) where they don’t let you expand the file inline.
   • Replace the first line of the diff with something that will make more sense to the reviewer than a couple of temp filenames. We use a format like “— ///depot/path/to/file.txt\trev. #123”.
   • Append the contents of the diff to the patch file we’re building, plus an extra \n just to be sure the diff was terminated.
     • Note that a patch file is just one or more diff outputs stuck together.
     • I could submit a patch per file, but it’s simpler if I have a single patch per changelist to send to Crucible. Less clutter and easier incremental updates later on.
3. Skip entire review and put up a message to user if the patch file is empty. It means every file in the changelist is either binary or did not change from the depot version.
4. Create a Crucible review via a POST to reviews-v1, remembering the ID we get back for the new review.
   • Creator = author = moderator = the current user.
     • Annoyingly, Crucible apparently has case-sensitive usernames, at least through certain API’s (regardless of server setting for lowercasing usernames). Have to ensure the username is lowercase here or Crucible will create a new user.
     • This seems to be a common problem with cross-platform tools. Perforce has historically had a lot of issues with case-sensitivity too. It’s the “unix way”. People can’t even spell things right – expecting them to get the case correct too is just out of the question. I have a policy of lowercasing as much as I can, enforcing if possible through policy (branch names, client names, folder names..).
   • Description = the pending changelist description from P4.
   • Name = the trimmed, truncated first line of the changelist. People typically put some kind of overall summary in the description so this works out pretty well as a review name.
   • Project Key = currently hard coded. I need to have a way to associate metadata to a project. I’m thinking maybe just a simple projectinfo.txt in the client root (Jira name, Crucible name, Confluence home, etc.).
   • Allow Reviewers To Join = sure, why not. Not sure why you’d want to limit this.
   • Patch = attach the patch file generated previously.
5. Update the P4 pending changelist with the ID of the new review and an http link to it. Ours looks like “[Review: TAC-1234 ( http://crucible/cru/TAC-1234 )]”. Note that I had to put spaces around the URL otherwise some scanners pick up the trailing parenthesis as part of the link.
6. Open the web page of the new review!

So with a few clicks, and all the above automation, the review is in draft mode. The user can now proceed with their review process. Not much can be automated past this point!

Creating Incremental Reviews

Assuming a changelist was already in review, and the user has chosen to update it with an incremental via the review dialog, we go to this route instead. In a way there’s less work to do, but it’s a lot more complicated.

In building the new patch file, we have to reconstruct the state of each file as it existed before the most recent changes, diff that with its current state into a patch, and upload that to the Crucible review. Because multiple incrementals can be tacked onto a review, we need to run each file through each existing patch to progressively get to the n-1 state.

It works like this:

1. Create an empty file to hold the incremental patch.
2. Fetch the review from a GET to reviews-v1/[review]/details.
3. Fetch all the patches from the review via each patchUrl in reviewItems.
   • If this doesn’t work, make sure you’re using the cookie container from the original login. Took me a while to figure this out.
   • I store the patches in a table mapping URL to patch data. Useful for what comes next.
4. For each patch…
   • Break patch down with a regex by the individual file diffs in it.
   • Look up the matching P4 filespec from the diff’s first line “header”.
   • Skip diffs for files that aren’t in the pending changelist. Someone may have reverted a file since the patch was created.
   • Store in a table mapping by filespec to the diff data.
5. For every file in the changelist, do the same thing as in “Creating New Reviews” Step 2 above, except we need to rebuild the “old” version of the data. So if the filespec we’re working on exists in the patch/diff table…
   • Create a temp directory to work in so we can name the files the same as its name in the patch file and not worry about collisions. This is so patch.exe works.
   • Print out the “have” state for the filespec from Perforce to a file in the newly created directory. If it didn’t exist, print out a zero-length file instead.
     • It’s important to check that the “have” hasn’t changed from the last time a diff was done. If the user has done a sync-and-merge, it breaks the whole incremental patch chain.
     • I’m, um, not checking for this at present. This results in a lot of screwy inexplicable results from the incremental feature of crucreate making people not trust it. Oops. Must fix.
   • Follow the patch chain. For each patch that is associated with this filespec, in the same order added to the review…
     • Call patch.exe with these options:
       -t
       –g0
       (stdin) = stream in the patch data (make sure last line is \n terminated!)
       (working dir) = temp directory (patch works directly on the file there)
     • Note that the –g0 is required because we need to disable the weird automatic Perforce support in patch.exe. Took me a while to figure out what was going on here. WTF GNU?
   • Take the final patched file and use it as the “from” in the diff, doing the diff the same as with a new review.
   • When replacing the first line in the diff as with new reviews, append “UPDATE 1” or “UPDATE 2” etc. depending on how many incrementals we’ve done so far. Without this, there’s no way to tell the difference between the original and the incrementals in the review.
   • Take the resulting diff and append to the patch file that we’re building for the update.
6. If the filespec didn’t exist in the patch/diff table, then the user must have added/edited/deleted a file new to the changelist since the last review. Do the same as if it was in a new review.
   • When doing the first-line diff rename, still append the UPDATE # with the filename so it is considered part of the overall incremental update.
7. Skip incremental update and put up a message to user if the patch file is empty. It means every file in the changelist is either binary or did not change from when the review or last incremental was made.
8. Add the patch to the review using a POST to reviews-v1/[review]/addPatch.

The review is now updated to have an additional patch attached. Reviewers will see files changed since the review (new and old) in the file list with everything else, except having “UPDATE” on their names to see the incremental progression.

Now, at this point, the reviewee has to notify everyone on the review that they added an incremental. Ideally I’d have crucreate make every reviewer un-complete the review (which would also trigger some notification emails), but when I do this I get an exception from Crucible.

I suppose if this isn’t fixed in v2 I’ll update crucreate to just send notifying emails itself. But I’d be really surprised if this isn’t fixed or at least improved, given all the changes I’ve seen in the previews I read about v2 features!

Special Diff Hacks

One of the things on my list is to find something better than GNU diff and patch. It is incredibly old and hacky feeling, particularly the weird Perforce support. There are so many better diff algorithms out there too. In particular, I wish I could use the truly excellent diff in Beyond Compare, but only maybe half the team uses that…maybe I will run a “diff server”.

Anyway, in order to make diffs work right for Crucible via GNU, I have to do the following…

• Check diff/patch versions.
  • Command line options vary across versions and I want to make sure people don’t accidentally have some other diff.exe/patch.exe in their path before our standard tools that are sync’d via P4 (or they’re out of sync).
  • Just run the exe with ‘-v’ and check against a hard coded version. We’re supporting 2.8.7 for diff and 2.5.9 for patch.
• When fetching a file from P4, do it as a binary. Don’t want P4 doing any of its screwy translations.
• Convert to ASCII. Gnu diff apparently doesn’t know what a byte order marker is or anything about unicode or code pages.
• Split by line.
• Trim trailing whitespace. Don’t want this stuff cluttering the diff.
• Replace the contents of RCS keywords ($Id, $Header, $Date, $DateTime, $Change, $Revision) with “<ignored>”. This is generated code and we don’t want it cluttering every single diff.
• If a file ends in .lua, then prefix every line with a dot (“.”). I don’t know how to work around this problem, but Lua comments start with dash-dash (“—”) and that confuses the hell out of diff/patch. The leading dot is a little ugly but getting an empty diff is worse.
• Rejoin by line and write out. This ensures 100% consistent line endings on both sides.

Maybe I’m reading the (rather awful) docs wrong, and maybe diff/patch can do what I need. But I can’t figure out how to make it do these things on its own.

Final Thoughts

This tool works pretty well. The process looks big and nasty and complicated, and a lot of it is, but the end result to the users comes down to three or four clicks to make a review. 99% automated. With Crucible 2.0 I’ll make some changes, remove duplicate functionality and so on. But it feels like it will work fine for us, well into the future.

Still, there are a lot of things I plan to work on as I find time on the side:

• Upload the source to a site like SourceForge or CodePlex. Lots of cleanup to do first, not to mention seeing if anyone is remotely interested in using it in their studio (this is part of the reason I’m posting this).
• Enhancements to the dialog to query each of the primary reviewers to see what their workload is like. Number of outstanding reviews and so on, so the random choice and reviewee can balance out the load better.
• Rebuild the guts on top of PowerShell cmdlets so we can reuse it in other ways. In particular, the patch and diff management stuff was a pain to get right and would be useful in implementing a ‘stash’ feature in P4X. I heart PowerShell.
• Lots and lots of bug fixes and little enhancements.

It’s my hope that Atlassian will eventually build such a bridge on their own and eliminate the need for me to keep maintaining something like this. Although, now that we have it built, we can continue to tune it to meet our exact needs.

I’m in Brisbane for just a few more days, then it’s back to Seattle where my life can finally return to a normal pace. Sushi in the Park Saturday the 15th at Cal Anderson!

Series

My full series on code reviews:

• Part 1: Peer Code Reviews At Loose Cannon
• Part 2: Peer Code Reviews: First Attempts
• Part 3: Peer Code Reviews: Success!
• Part 4: Peer Code Reviews: Good Commenting Practices
• Part 5: About Our Crucible-Perforce Bridge
• Part 6: Peer Code Reviews: How Did We Do?