Crucible vs. Bitbucket for Code Review

CyanWorlds.com Engine Project Management
Christian Walther
Member
Posts: 317
Joined: Sat Dec 13, 2008 10:54 am

Crucible vs. Bitbucket for Code Review

Post by Christian Walther »

Taking a discussion here that started in Crucible on MOULSCRIPT-19 a while ago when rarified opened Crucible reviews for several contributions that had come in as Bitbucket pull requests and had already been commented on there:
Christian Walther wrote:Do we really need Crucible to review this and the other contributions that reviews were opened for today (15-19)? The changes are so simple that Bitbucket seems completely adequate to me, and we have already started reviewing them there. In my opinion, CWE-ou and MOULSCRIPT-ou are already hobbled enough by essentially having only a single maintainer (rarified), so I’d rather not see him bogged down by additional bureaucracy when he’s stretched thin already.
rarified wrote:No we don't need Crucible on these, but I want to try to establish a standard location for people to see pending stuff going into the pipeline. Right now it's pretty cut-and-paste from BB comments to here; I need to extract the changes as a patch anyway to get them into the Minkata-specific repository, so applying the patch here as well gives a way of confirming the transfer was done correctly (if the author is inclined to visit Crucible and eyeball the changes).

I think a discussion of abbreviated process for some classes of changes should be a discussion in the forums, and would be happy to have a dialog about it.
OK, if you don’t feel it’s a burden on you, then that already addresses most of my concern.

However, I still think it’s a bit awkward to have things reviewed in two places. Some people will copy and paste their comments from one place to the other, which is extra work. Some will only comment in one place and miss what is being said in the other. It just doesn’t feel like a process as simple and inviting to contributors as it should be.

My preferred solution would be to keep review of contributions that come in as Bitbucket pull requests on Bitbucket, and only resort to Crucible when Bitbucket’s capabilities turn out to be insufficient (e.g. lack of comments on individual lines of code).

I see your point about having a single central point of entry, but in that case, I wonder, should we discourage Bitbucket pull requests and mandate that all contributions come in through Crucible, to avoid the duplication?


Is there an interest in this discussion from other involved people?
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 »

This, I hope, is a transient discussion as Atlassian brings Bitbucket integration with it's other tools. Tighter integration with JIRA between Bitbucket and GitHub is already available and planned with our next JIRA upgrade to 5.0.

https://plugins.atlassian.com/plugins/c ... .bitbucket

This might not resolve your specific concerns with Crucible at the moment - I don't know all the details - but I think it will get there.
Perfect speed is being there.
User avatar
Hoikas
Member
Posts: 344
Joined: Fri Jun 03, 2011 8:38 pm

Re: Crucible vs. Bitbucket for Code Review

Post by Hoikas »

I took a few minutes to try and figure out how to submit a change to crucible and was somewhat befuddled by it. When I was first pushing for bitbucket repositories, I was hoping that at least the CWE-ou and MOULSCRIPT-ou repositories would be handled on bitbucket, and my experiences with Crucible assure me that my initial position was the correct one. I find pull requests and forks a much simpler workflow than the diffs we have in Crucible. Also, we have the benefit of being able to press the big blue button of doooooooom to update pull requests on bitbucket ;) .
Image
User avatar
rarified
Member
Posts: 1061
Joined: Tue Dec 16, 2008 10:48 pm
Location: Colorado, US

Re: Crucible vs. Bitbucket for Code Review

Post by rarified »

Then perhaps sometime I can do a walkthrough with you on how to submit a Crucible change. It's not really that hard, and 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 (since neither BB nor Mercurial cannot roll back a committed change, and you cannot submit a pull request without having committed to your BB fork).

This is where WebEx would be particularly helpful.

_R
One of the OpenUru toolsmiths... a bookbinder.
User avatar
Mac_Fife
Member
Posts: 1239
Joined: Fri Dec 19, 2008 12:38 am
Location: Scotland
Contact:

Re: Crucible vs. Bitbucket for Code Review

Post by Mac_Fife »

rarified wrote:This is where WebEx would be particularly helpful.
I think I heard my name called?
:lol:
Mac_Fife
OpenUru.org wiki wrangler
User avatar
Hoikas
Member
Posts: 344
Joined: Fri Jun 03, 2011 8:38 pm

Re: Crucible vs. Bitbucket for Code Review

Post by Hoikas »

Code: Select all

hg rollback ; hg commit
According to this useful page, pretty much all of the advanced stuff I would want to do for keeping my changes sane can be done in Mercurial. I suspect that we are both accustomed to different workflows, so this might indeed be a topic for us to discuss at some point during a WebEx... I think that would be more useful than linking to Atlassian or GitHub posts exposing the greatness of the latest feature as those rarely do anything for me. 8-)
Image
User avatar
rarified
Member
Posts: 1061
Joined: Tue Dec 16, 2008 10:48 pm
Location: Colorado, US

Re: Crucible vs. Bitbucket for Code Review

Post by rarified »

Hoikas wrote:

Code: Select all

hg rollback ; hg commit
Unfortunately, those commands are not available to BB forks. Once the BB copy of the repo has a commit, there's no going back.

Also, there's only one level of rollback in Mercurial; if a submitter has multiple changes (embodied in separate pull requests), at least one of those changes is irrevocably committed to their repository. Mercurial's philosophy is clearly biased toward never permitting history to be altered. (Yes there are advanced tools to do so, again not applicable to the BB copies).

_R
One of the OpenUru toolsmiths... a bookbinder.
User avatar
Hoikas
Member
Posts: 344
Joined: Fri Jun 03, 2011 8:38 pm

Re: Crucible vs. Bitbucket for Code Review

Post by Hoikas »

Ugh. Why, Atlassian, why? :?
Image
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:Once the BB copy of the repo has a commit, there's no going back.
Incorrect, fortunately. :) There is a strip option in the repository admin section. Has proven very useful to me on multiple occasions. The usual caveats about removing/replacing revisions that have already been published apply, but people familiar with Git should be well aware of these.

We seem to be drifting a bit off-topic here… trying to weave the threads together again:
JWPlatt wrote:This might not resolve your specific concerns with Crucible at the moment - I don't know all the details - but I think it will get there.
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.

The new JIRA/Bitbucket integration sounds cool, but indeed relatively irrelevant to the question at hand.

(Also, just to be clear: my specific concerns are not with Crucible per se, they are about duplication and burdening people (both OU staff and contributors) with more work than necessary. I admit I haven’t tried entering a contribution through Crucible though. I guess a walkthrough would indeed be helpful.)
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. I think Mercurial even has an option to mark a branch as “closed” to distinguish it from heads that are still “alive”. And if that’s undesired, there’s still the strip option mentioned above, or the contributor can simply delete their fork and make a new one for the next contribution.
User avatar
rarified
Member
Posts: 1061
Joined: Tue Dec 16, 2008 10:48 pm
Location: Colorado, US

Re: Crucible vs. Bitbucket for Code Review

Post by rarified »

Christian Walther wrote:There is a strip option in the repository admin section.
Thanks! I had looked for that capability on several occasions and missed it -- it is right there. But there are caveats with that function, as the admin page points out. If other forks sync up from the repo before the strip of a version, it's likely any pull from one of those repositories will put the stripped out changesets back. And the alternative, "hg backout" is almost as unattractive; the history gets cluttered with versions that cancel each other out. Which is why...
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.

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.

I've been creating the Crucible reviews using this exact process; since I already have to transfer fixes from H'uru/Git to OU/Mercurial as a patch anyway.

[I continue to desire the one-commit-per-fix standard primarily for simplifying the transfer of those fixes to Cyan. As with many of my preferences, this comes from trying to put myself in Chogon's position and imagining what processes make his work easiest, not from any directive that Cyan has imposed explicitly.]

_R
One of the OpenUru toolsmiths... a bookbinder.
Post Reply

Return to “Management”