Crucible vs. Bitbucket for Code Review

CyanWorlds.com Engine Project Management
Paradox
Member
Posts: 15
Joined: Sun Jul 10, 2011 10:37 pm

Re: Crucible vs. Bitbucket for Code Review

Post by Paradox »

rarified wrote:
Christian Walther wrote:
rarified wrote:it avoids the issue of committing a change to the repository before it gets review exposure, so it doesn't have to be patched out
I don’t see why this is an issue. On the contrary, by committing the change to a repository before it gets reviewed, you ensure that what’s reviewed is exactly what’s going to end up in the mainline if approved. If not approved, I wouldn’t say it needs to be patched out either. It’s either amended by more commits on top of it, then merged into the mainline after all, or it’s replaced or definitely rejected and can just stay there as an open head in the submitter’s repository.
Here is where I would disagree, on the basis that we're trying to follow a single-commit-per-fix pattern. And why I was debating using patch queues until deciding it may be more confusing to less experienced developers than the straightforward rule.
This seems to run contrary to the commonly cited rule of "Commit early, commit often". By committing often and pushing often, other developers can see your work and offer informal review, while ensuring that there is not duplication of effort.
It's also usually preferable to have a bunch of small atomic commits than a single giant patch, because if there's a bug in any of those small commits, they can easily be reverted without losing the entire changeset.

As an example, Deledrius's fixes for localization: https://github.com/H-uru/Plasma/pull/55
Rather than a single large commit that changed several unrelated pieces of code, he kept each change as its own commit so that there would both be context in the commit message, and easily reversible changes.

You can also see an example of our code review process there (although that was a more general discussion than a code review).
A better example might be https://github.com/H-uru/Plasma/pull/52 or https://github.com/H-uru/Plasma/pull/163.

I'd also like to add that we frequently rewrite history or rebase commits in our personal forks, and doing so often results in a cleaner merge, and a more organized history. One of the main reasons that we switched a lot of our projects from Mercurial over to git is because of how difficult Mercurial makes it to shuffle commits around.


As a general question, has anyone tried to have a discussion with Cyan regarding how they actually want changes handled? I think trying to get some sort of answer is better than guessing.
Even if they aren't using hg internally, it shouldn't be difficult to generate a patch that contains a number of commits.
User avatar
JWPlatt
Member
Posts: 1137
Joined: Sun Dec 07, 2008 7:32 pm
Location: Everywhere, all at once

Re: Crucible vs. Bitbucket for Code Review

Post by JWPlatt »

I talk to Mark when he is about to pull proposed changesets or push the approved update. He seems to appreciate simplicity, but has not specified it. He is flexible, but is quick to want things easier if he sees something getting complicated. Cyan uses Subversion internally and it's a bit of a pain for him to do the double work from any DVCS into Subversion. i.e., the quicker he can do things the better. So I'm not specifying anything here either - just giving you a little more sense of things to continue your discussion.

Personally, I'd agree with everything you say about frequent commits to keep community development at pace, but I always expected the end product to be a single, stable, reviewed commit to the CWE/MOULSCRIPT-ou repos from the main branch of whomever is doing the work (e.g., H'uru or a BB pull request), or a set of commits that split a larger project of independently functional modules if they could be incrementally tested. But I'm not a repo expert, so I'm not any kind of lead on this. I just talk blue sky. ;)
Perfect speed is being there.
Paradox
Member
Posts: 15
Joined: Sun Jul 10, 2011 10:37 pm

Re: Crucible vs. Bitbucket for Code Review

Post by Paradox »

Yeah, I didn't really expect Cyan to stray from Subversion, especially at this point.

Maybe I'm misunderstanding the intended use of Crucible vs Bitbucket pull requests, but couldn't the code review be done on Bitbucket, and then the entire BitBucket pull request imported as a single diff into Crucible for tracking? I know with github you can take any pull request and easily get diff of all the commits, just by adding .diff or .patch to the end of the URL (like this).

In theory, that would provide a way for code review to happen on either github or BitBucket, and once approved by the developer community on those sites, the changes could be imported as a single patch into Crucible for tracking/further review before being committed to CWE-ou.
In an ideal world, the pull requests could be merged into CWE-ou directly, but that won't work for github changes anyways.
Christian Walther
Member
Posts: 317
Joined: Sat Dec 13, 2008 10:54 am

Re: Crucible vs. Bitbucket for Code Review

Post by Christian Walther »

rarified wrote:we're trying to follow a single-commit-per-fix pattern
I strongly agree with Paradox here. Whoever “we” is that wants a single-commit-per-fix pattern, I’m not part of it. (I think we all agree on single-fix-per-commit, but that’s a different thing.) The right way to go about this (and the one that is naturally encouraged by the fork/pull-request model) is single-branch-per-fix (“feature branches”, “topic branches”). That way you get bite-sized chunks of changes, easier to understand, review, and test, inside the branch, and you get the whole fix in a single commit, that can also easily be exported as a single patch, in the commit that merges the feature branch back into the mainline.

(This is in fact what we have been doing in CWE-ou so far, only there have never been more than two commits in a feature branch.)
rarified wrote:Crucible can take a patch-formatted text file (diff listing), and apply it against one or more repositories associated with a project to create a "temporary" repository with the differences already committed. It then displays the changeset in a similar way that BB/GitHub show the pending pull request, but without the need for additional commits. If the changes need to be altered, either a new all-encompassing patch can be uploaded replacing the original one, or incremental deltas applied to the review copy, again without requiring a commit on the main repository until the review is complete. Crucible can then consolidate the diffs into a single patch you can apply to your repository fork so you have the "exact" changes that were reviewed.
This is really no different from the GitHub/Bitbucket pull request model, only with a bit more manual work (and the associated opportunity for human error, hence my comment about exactness). In particular regarding the part I emphasized: no commit appears on the main (upstream) repository until the review is complete on GitHub/Bitbucket too, they stay solely in the contributor’s fork during that time. But again, I’m not here to argue about this. Using Crucible is fine with me, what I find inefficient is using both Bitbucket and Crucible on the same input.
User avatar
JWPlatt
Member
Posts: 1137
Joined: Sun Dec 07, 2008 7:32 pm
Location: Everywhere, all at once

Re: Crucible vs. Bitbucket for Code Review

Post by JWPlatt »

Paradox wrote:In theory, that would provide a way for code review to happen on either github or BitBucket, and once approved by the developer community on those sites, the changes could be imported as a single patch into Crucible for tracking/further review before being committed to CWE-ou.
Review within the clone wherever it resides before pushing the final, reviewed code to CWE/MOULSCRIPT-ou. That's been the intention. Bitbucket needs to catch up on that in features, but that's the vision. I know GitHub does some of that. Bitbucket is catching up.

I'll go a bit off topic here and go out on a limb to predict some partnership between Bitbucket and Github, if not an outright acquisition, due to Atlassian's obvious willingness to integrate their plugin with GitHub. It seems remarkable to me that they would understand the demand and invest in GitHub. It begs for a merger of effort. GitHub is a one trick pony, but it's doing a really good trick. Atlassian wants to be repo agnostic and support everything so I suspect that's the direction both will take together, even though, or especially because, Bitbucket already supports Git. If they are both really smart, that's what they'll do. The choice of social repo is not going to matter from the point of view of our tools here at OU.
Christian Walther wrote:Using Crucible is fine with me, what I find inefficient is using both Bitbucket and Crucible on the same input.
Bitbucket needs to catch up to what I believe is the goal as I see it for Atlassian's integration with social repositories.

I'd like to take the long view as we work through these things, knowing that what is going on right now is temporary and getting better all the time. Eventually, there should be no difference from your point of view.
Christian Walther wrote:You’re expecting a tool that will synchronize between Bitbucket pull requests and Crucible reviews (or otherwise mirror Bitbucket pull requests in whatever is designated as the central entry point)? That would certainly solve the problem.
Right.
Perfect speed is being there.
Post Reply

Return to “Management”