Make adding proper patches to ReviewBoard easier

RESOLVED WORKSFORME

Status

()

RESOLVED WORKSFORME
5 years ago
5 years ago

People

(Reporter: gps, Unassigned)

Tracking

Production
Dependency tree / graph

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
+++ This bug was initially created as a clone of Bug #944256 +++

In bug 944256, we eliminated the inbound repo from Reviewboard because it is silly. It's 99% redundant with mozilla-central and only serves to confuse people and fragment the mozilla-central reviews among more buckets. It was never a correct solution because we would also need fx-team, services-central, build-system, etc.

The correct solution to this problem involves a unified Mercurial repository - a Mercurial repository pulling from multiple repos and aggregating the changesets under one roof.

The reason we need a unified Mercurial repository is because Reviewboard needs to know about the base revisions for all patches in Reviewboard. If Reviewboard is limited in knowledge to m-c, patches created against inbound will fail to render properly until the "missing" inbound changesets are present (merged) into mozilla-central. This is because RB talks to the repo to find base file content, etc. If RB is unable to find supposedly existing repo content, suckitude happens. This is extremely annoying to say the least. The problem is pretty much unbearable when patches are produced against changesets from a long-unmerged branch/repo (like say Australis): RB will effectively never find these base changesets and intelligent patch rendering will never work.

If RB is given access to all known changesets (via a unified repository), then it will be able to construct patch information for more changesets sooner. It may still miss changesets once in a while, but the problem should be significantly mitigated.

A sufficient solution to this bug involves running a unified repo on the RB server, without having that repo exposed to the world. This is pretty easy to do. Clone central, then have a while (true) loop pulling in central, inbound, fx-team, etc. This is effectively how I manage http://hg.gregoryszorc.com/gecko. I guess technically you could configure RB to point to that repo. However, it's currently hosted on an underpowered EC2 instance and remote access will be too slow and people will likely complain "RB is too slow."

(RB can be configured to query the canonical server remotely or via a local mirror. If using remote access, diff requests result in RB issuing HTTP requests against http://hg.mozilla.org/mozilla-central/raw-file/<rev>/path etc to obtain file content. RB does cache these lookups, but they are still slow. If RB operates against a local mirror, diffs display much, much quicker. I'm not sure what the Mozilla RB server does today.)

Even with the unified repo, clients may still upload patches referencing changesets unknown to the RB unified "mirror." In addition, RBTools may inadvertently pull in inbound (or other) changesets that haven't merged to central yet as part of the uploaded diff. A solution to both of these problems is possible, but it involves some work (the best solution I have so far is for RB reviews to be derived from pushes to try or a try-like repo). Solving these are beyond the scope of this bug.

Hal: This is actually a legit use case for a unified Mercurial repo. Is there any RelEng interest on running a read-only unified repo under hg.mozilla.org? If not, people will continue to reinvent the wheel.
Flags: needinfo?(hwine)
(Reporter)

Comment 1

5 years ago
OK, ReviewBoard is quite inept - at least compared to Phabricator.

ReviewBoard essentially doesn't like it at all when uploaded diffs reference a base that is unknown to ReviewBoard. Say you have the following revisions:

m-c --> X' --> X'' --> R' --> R'' --> R'''

m-c is the current tip of m-c. X' and X'' are patches applied on top of m-c that aren't part of the requested review. R', R'', and R''' are the patches "squashed" together to constitute the RB review.

If I create a RB review with `rbt post --revision-range=X'':R''' (yes, you need to specify parent(R') and not R' because ReviewBoard's revisions are exclusive, not inclusive - a decision I disagree with), rbt will upload the equivalent of `hg diff -r X'' -r R'''`. If R contains files modified by X, ReviewBoard fails to render the diff because it doesn't know about the content in X''. Supposedly you can upload a "parent diff" to ReviewBoard. However, rbt's Mercurial support doesn't support parent diffs. FWIW, I've found rbt's Mercurial support to be extremely lacking. I have no less than 4 patches awaiting review from the ReviewBoard people to make things suck less.

Anyway, I hacked up my local rbt to upload parent diffs. However, I still can't get RB to render the patches properly. Assuming it will work someday, there is still the unfortunate property that rbt will need to be told how to create the parent diff. Effectively, rbt needs to perform `hg diff -r central/default -r "p1(X')"`. This means users would need to pass an argument into rbt. Users would also need to know what changeset is the tip of central. Humans will forget to do this or will do it wrong. So, I think the only reasonable approach to getting reliable output from rbt is to have a tool wrap the command. That's what bug 947604 is for. But even then, I question how good it will be.

This is all a lot of work to get ReviewBoard to work.

Phabricator, by comparison, just works. When Phabricator uploads a diff (via `arc diff`), it uploads the full file content from the base revision. Therefore, Phabricator has all the data needed to construct diffs, even if the Phabricator server doesn't know about the base revision the patch was created against! This approach of content followed by diff is much more robust than ReviewBoard's approach of shuffling diffs around and hoping content can be derived later. I'm starting to seriously doubt whether ReviewBoard will "win" the review tool shootout. I /think/ we can make it work - but's it going to take some time and work and likely some custom Python to glue everything together.
(Reporter)

Comment 2

5 years ago
I should also mention that ReviewBoard uploads "vanilla" diffs from Mercurial. Instead of exporting Git-style diffs, it's using the GNU compatible format. This means permission bits, copy and rename metadata, and binary diffs are not adequately expressed in the diffs ReviewBoard receives. This is quite sad, really. Again, Phabricator does the right thing and uploads a rich patch context.

It just feels like ReviewBoard is operating against the single patch based software development model from the CVS/Subversion days whereas Phabricator has embraced modern workflows enabled by modern tools like Mercurial and Git. Of course, Firefox development generally still operates as if we're using CVS. But we shouldn't let our tools hold us back.

Comment 3

5 years ago
ReviewBoard is currently a patch based tool, and comment 0 and comment 1 explain two things that are going to suck with *any* patch based tool, not just RB.  I'm planning to talk to Mike Conley who is a RB developer to see if we can fix RB to be push based and not patch based later this week.  If the answer turns out to be "yes", then then that's great, and we can probably close this bug as WONTFIX.  If the answer turns out to be "no", then we should look for alternative tools (I am *very* opposed to adopting any new patch based tool for anything new for reasons partly expressed in this bug).  On the surface, Phabricator seems like too much of a big hammer for our needs (unless I'm missing something about how to use only the review tool and not the rest of Phabricator.)

Anyways, in short, I strongly believe that we will not move forward with the existing patch based ReviewBoard.

Comment 4

5 years ago
We can enable or disable various pieces of phabricator, so e.g. only enable the pull/review and repo-watching elements of Phab and disable the bug tracker and other elements.

Comment 5

5 years ago
(In reply to comment #4)
> We can enable or disable various pieces of phabricator, so e.g. only enable the
> pull/review and repo-watching elements of Phab and disable the bug tracker and
> other elements.

OK in that case it might be a viable backup plan.  But let's first make sure that RB cannot be fixed to do this properly.

Comment 6

5 years ago
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> We can enable or disable various pieces of phabricator, so e.g. only enable
> the pull/review and repo-watching elements of Phab and disable the bug
> tracker and other elements.

We'd still need to integrate phabricator with bz. Lets make sure reviewboard can't easily learn about changesets before we commit to customizing a pile of php.
Just for the record, the BMO team has no problems doing the integration work on Phrabicator if we determine that it's the best solution--I assume (and don't mind that) we'll end up owning the chosen review tool.  Agreed with seeing if Review Board can be made to be workable though.
(Reporter)

Comment 8

5 years ago
I agree with Ehsan's statements in comment #3 regarding patch-based vs push-based tools. This is why I'm currently experimenting with some mad scientist like ideas to turn pushes into code reviews automagically.

Essentially, I have a Mercurial server that on push will compare the head to the most recent "mozilla repo" head (central, inbound, fx-team, etc), compute the changesets in between, and turn those changesets into ReviewBoard requests. It even automatically uploads a "redirect" attachment to Bugzilla! It's kinda/sorta working, but only in pristine lab conditions. Of course, it still relies on patches to communicate the ReviewBoard review. However, I do have it submitting parent diffs to ReviewBoard, which is already better than what RBTools does today. I agree with Ehsan that pointing ReviewBoard at some revisions on an existing repository is a much saner approach to solving the problem: patches just aren't as robust as we need them to be. Furthermore, having changesets pushed somewhere also opens doors to much more exciting possibilities. I'll post more details once I have something to demo :)

Anyway, something I need to know is how Mozilla's ReviewBoard server integrates with mozilla-central. In order to upload parent diffs, I need to ensure the base changeset for that diff is accessible to the ReviewBoard instance. If Mozilla's ReviewBoard server is accessing https://hg.mozilla.org/ directly, problem solved. But if it has a local mirror, then I need to know what the heads on that repo are or the parent diff may reference a changeset not yet available to ReviewBoard!

mcote: can you provide me more details about how reviewboard.allizom.org is configured w.r.t. mozilla-central access?
Flags: needinfo?(mcote)
(In reply to Gregory Szorc [:gps] (in southeast Asia until Dec 14) from comment #8)
> mcote: can you provide me more details about how reviewboard.allizom.org is
> configured w.r.t. mozilla-central access?

It uses http://hg.mozilla.org/mozilla-central/

(See: https://reviewboard.allizom.org/api/repositories/1/ )
Flags: needinfo?(mcote)
(Reporter)

Comment 10

5 years ago
(In reply to Steven MacLeod [:smacleod] from comment #9)
> (In reply to Gregory Szorc [:gps] (in southeast Asia until Dec 14) from
> comment #8)
> > mcote: can you provide me more details about how reviewboard.allizom.org is
> > configured w.r.t. mozilla-central access?
> 
> It uses http://hg.mozilla.org/mozilla-central/
> 
> (See: https://reviewboard.allizom.org/api/repositories/1/ )

Oh, right - I didn't realize it exposed the mirror info in the API (it's the empty string, which implies it isn't set). That seems like a security issue since it could leak the server's local paths. I also think it should be using https:// instead of http://. Whatevs.
Hey everyone,

Review Board is pretty heavily patch-based today, as you note, for historical reasons. Back when we were first building it, Subversion was King, GitHub didn't yet exist, and people were asking for Bazaar and Monotone support. So, our design was heavily influenced by traditional patches, but we have no intentions of staying that way. We are fully intending to evolve this, and soon, and I'd love to gather thoughts on how you want it to work. Currently filling up a notebook before I start diving in.

I'm heads down on Review Board 2.0 right now, which will still be patch-based, but we've done a lot of the ground work needed to be able to move toward a better model. After the release (1-2 months away?), this work will be my priority.

Gregory, that sounds like a great solution. I'd love to see it in action.

We're also seriously taking your other comments into consideration for RBTools. Your patches will make this week's release (there's a unit test breakage I'm hitting with one, but the rest are committed), and I'm going to try to get parent diffs for Mercurial working soon after, and rapid-fire a release with that. David Trowbridge (co-creator of RB) is rethinking RBTools with regards to how revision ranges work, and making it easier to use native syntax for choosing ranges, hopefully automatically computing the parent diffs. This will also be a precursor to the DVCS work we'll be doing soon.
(Reporter)

Comment 12

5 years ago
Christian: Thanks for chiming in! I want to be explicitly clear that I want to make ReviewBoard work at Mozilla for various reasons. If nothing else, it being Python is a strong win compared to Phabricator's PHP since Mozilla has an abundance of Python knowledge compared to PHP and I have no doubt we could both integrate and extend ReviewBoard easier than Phabricator.

I can probably submit a RBTools patch for Mercurial + parent diffs - I nearly have one ready to go. I haven't yet looked at all your comments for my existing RBTools patch set. Hopefully tonight.

As for future of ReviewBoard, I think ReviewBoard should just host its own Mercurial/Git/whatever servers and rbtools should just push to those hosted repos. Then, ReviewBoard server can compare changesets between the pushed head and what's available in the canonical repo. This solves the very complicated problem of determining those changesets on the client and should eliminate the "patch doesn't apply" problem. It also makes it very trivial to jump to a "pull request" development model, since the reviewed heads are natively pullable without involving a patch dance. Our procedure for "land a reviewed patch" literally become "hg pull + hg merge/rebase." That's huge. FWIW Phabricator has beta support for hosting repositories (https://secure.phabricator.com/T2230).

Perhaps we should take this discussion to one of the RB mailing lists (http://www.reviewboard.org/mailing-lists/) and/or IRC channel?
Hey,

I'd be glad to look at a patch for parent diffs! It really is long overdue.

The self-hosted servers idea is a pretty interesting one, and is an idea I've played with before. The trick would be to make it work well in larger corporate environments (those scaling RB across several servers and working with hundreds of repositories), RBCommons.com included. It's something we can look into more. I've also considered the idea of a companion server that would be the thing you push to, which can be placed wherever and linked up with one or more RB servers. There's a lot of options to consider.

The reviewboard-dev list would be a great place for discussing this.

Comment 14

5 years ago
For what it's worth, Phabricator is also patch-based (we believe this is a much better workflow, particularly at scale). We just generate patches with `hg diff -U9999 --git` to get context and binary data. Theoretically RBTools could do something similar (with the caveats that I've barely used ReviewBoard and may be misunderstanding the problem).
Hey gps, I'm interested in knowing how your experiment that you mentioned in comment 8 went down. Did you get it working?
Flags: needinfo?(gps)
(Reporter)

Comment 16

5 years ago
(In reply to Mike Conley (:mconley) from comment #15)
> Hey gps, I'm interested in knowing how your experiment that you mentioned in
> comment 8 went down. Did you get it working?

Sort of.

https://gps.pastebin.mozilla.org/3812481 is the server-side hook I've got so far. It's *really* hacky at this juncture. It requires latest Git HEAD of rbtools. It might even require some patches to rbtools I haven't finished upstreaming yet. It also requires a monolithic Mozilla repo (created with my mozext extension) to do head discovery since it's looking up the bookmarks/tags that extension defines. I also think it's referencing some Python modules I haven't pushed anywhere.

Functionality wise, it's half-baked. I still need to hook in support for not "squashing" changesets into a single RB review. My grand vision for this was to have clients issue an `hg review` command which would transparently push to a hardcoded repo URL. The client part is not implemented.

I'm going to try to get this all working during the holidays. (I can't justify the time to work on this during normal work time since other things take priority.)

Ehsan also has ideas of standing up something similar. Although, his vision is slightly different from mine.
Flags: needinfo?(gps)
(Reporter)

Comment 17

5 years ago
Switching one nebulous bug summary for another.
Summary: Reviewboard mozilla-central repo should use unified Mercurial repository → Make adding proper patches to ReviewBoard easier

Comment 18

5 years ago
(In reply to comment #16)
> Ehsan also has ideas of standing up something similar. Although, his vision is
> slightly different from mine.

Indeed.  I won't be using reviewboard for starters.  ;-)
(Reporter)

Comment 19

5 years ago
There are exciting times in ReviewBoard land!

I wrote a patch (pending review) that adds parent diff support to Mercurial (https://reviews.reviewboard.org/r/5207/). I'm also contributing patches (such as https://reviews.reviewboard.org/r/5220/) to make Mercurial integration much better. There's also work to make the CLI interface much friendlier (https://reviews.reviewboard.org/r/5226/). I'm helping to ensure they get Mercurial support right.

I've tested these patches and have got things to play nice with reviewboard.allizom.org! I'm now conducting some reviews on Mozilla's ReviewBoard instance!

At the current pace, I imagine RBTools for Mercurial will be usable in the next week or two. Once that's ready, I'd like to whip together a mach command and/or Mercurial extension that does patch series upload to ReviewBoard + Bugzilla in one command. I may leverage mconley's recent patch to RBTools to support Python entry points so any client can plug into it. Stay tuned.
Flags: needinfo?(hwine)

Comment 20

5 years ago
Greg, we're trying to see if we can avoid building more systems based on patches.  I'd appreciate if you could hold off integrating this into mach for now while we're exploring other possibilities.  If those efforts prove to fail, we can always go back and integrate this into mach at a later time.  Thanks!
(Reporter)

Comment 21

5 years ago
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #20)
> Greg, we're trying to see if we can avoid building more systems based on
> patches.  I'd appreciate if you could hold off integrating this into mach
> for now while we're exploring other possibilities.  If those efforts prove
> to fail, we can always go back and integrate this into mach at a later time.
> Thanks!

I really don't like holding the present hostage of the future. We can always kill the mach command if it's replaced by a superior solution. Or better yet, you just change the mach command to use the new mechanism instead.

Comment 22

5 years ago
(In reply to comment #21)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #20)
> > Greg, we're trying to see if we can avoid building more systems based on
> > patches.  I'd appreciate if you could hold off integrating this into mach
> > for now while we're exploring other possibilities.  If those efforts prove
> > to fail, we can always go back and integrate this into mach at a later time.
> > Thanks!
> 
> I really don't like holding the present hostage of the future. We can always
> kill the mach command if it's replaced by a superior solution. Or better yet,
> you just change the mach command to use the new mechanism instead.

I don't think that we need a mach command here at all *unless* we have decided that we're going to proceed with ReviewBoard, and we're nowhere close to making that call yet.  The reason why I don't want us to add a mach command for this is that I don't want to give any incorrect impression on the state of ReviewBoard to anybody by having people extrapolate endorsement from the existence of the mach command.  Anybody who is interested in trying out ReviewBoard can use the RB command line tools right now.
(Reporter)

Comment 23

5 years ago
The mach command I'm envisioning would be `mach create-review` or similar. It would be review tool agnostic so it could adapt with the times. It could even support multiple review tools!

Remember, mach commands are just Python functions. Whatever I write will be a standalone Python module, suitable for use in a mach command, hg command, custom scripts, etc.

Comment 24

5 years ago
(In reply to comment #23)
> The mach command I'm envisioning would be `mach create-review` or similar. It
> would be review tool agnostic so it could adapt with the times. It could even
> support multiple review tools!
> 
> Remember, mach commands are just Python functions. Whatever I write will be a
> standalone Python module, suitable for use in a mach command, hg command,
> custom scripts, etc.

While we can call that command something that doesn't have RB in its name, it doesn't change the fact that the output of that command will be a ReviewBoard review, which means that this doesn't address the problem I raised in comment 22.  Can you please explain why you're in a rush to add a mach command here?
(Reporter)

Comment 25

5 years ago
I'm not in a rush to add a mach command as much as I'm in a rush to stop using Splinter for code review. Splinter has adversely impacted the ability of me and hundreds of other Mozillians from conducting high quality code reviews and I have zero doubt the quality of code going into Firefox has suffered as a result. Is ReviewBoard perfect? No. But IMO it's better than Splinter.

The way I see it, ReviewBoard isn't getting a fare opportunity for adoption at Mozilla because the tools and process stinks. I'm working to fix that.

My renewed push for ReviewBoard stems from the new year and my lessened tolerance for sub-par "working conditions" that follow any lengthy vacation. Maybe by next week I'll be reconditioned to Splinter's relative suckiness and won't care as much.

Comment 26

5 years ago
(In reply to comment #25)
> I'm not in a rush to add a mach command as much as I'm in a rush to stop using
> Splinter for code review. Splinter has adversely impacted the ability of me and
> hundreds of other Mozillians from conducting high quality code reviews and I
> have zero doubt the quality of code going into Firefox has suffered as a
> result. Is ReviewBoard perfect? No. But IMO it's better than Splinter.

Sure, but we're interested in adopting a system which we don't want to replace very soon.  RB is not that system to the best of my knowledge.

> The way I see it, ReviewBoard isn't getting a fare opportunity for adoption at
> Mozilla because the tools and process stinks. I'm working to fix that.

Nobody wants RB to be adopted by Mozilla yet.  Currently it's being considered for adoption and we are aware of a constraint which is very hard if not impossible to fix as things are right now, which means that RB will almost certainly not be what we will end up adopting.  Therefore, any move towards pretending that RB will be adopted by Mozilla is a mistake.

> My renewed push for ReviewBoard stems from the new year and my lessened
> tolerance for sub-par "working conditions" that follow any lengthy vacation.
> Maybe by next week I'll be reconditioned to Splinter's relative suckiness and
> won't care as much.

Your work on RB is much appreciated, but like I said we should hold off the rest of the work here until we know that we are doing to adopt RB.
(Reporter)

Comment 27

5 years ago
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #26)
> Nobody wants RB to be adopted by Mozilla yet.  Currently it's being
> considered for adoption and we are aware of a constraint which is very hard
> if not impossible to fix as things are right now, which means that RB will
> almost certainly not be what we will end up adopting.  Therefore, any move
> towards pretending that RB will be adopted by Mozilla is a mistake.

The latest patches to RBTools sufficiently work around or eliminate the constraints described earlier. Only https://reviews.reviewboard.org/r/5207/ (due to land in the next day or two) stands in the way of me calling RBTools' Mercurial integration free of major issues.

If you need a demo, https://reviewboard.allizom.org/r/24/diff/ was uploaded with a parent diff. Intradiffs and context expansion both work.

Comment 28

5 years ago
(In reply to Gregory Szorc [:gps] from comment #27)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #26)
> > Nobody wants RB to be adopted by Mozilla yet.  Currently it's being
> > considered for adoption and we are aware of a constraint which is very hard
> > if not impossible to fix as things are right now, which means that RB will
> > almost certainly not be what we will end up adopting.  Therefore, any move
> > towards pretending that RB will be adopted by Mozilla is a mistake.
> 
> The latest patches to RBTools sufficiently work around or eliminate the
> constraints described earlier. Only https://reviews.reviewboard.org/r/5207/
> (due to land in the next day or two) stands in the way of me calling
> RBTools' Mercurial integration free of major issues.

Yes, *you* do not care about the other downsides of using a patch based system, while others do.  Your changes do nothing to move RB away from using patches.  It's OK if you personally don't care about the rest of the pieces of this puzzle, but that doesn't really change anything that I've said so far in this bug.
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #20)
> Greg, we're trying to see if we can avoid building more systems based on
> patches.  I'd appreciate if you could hold off integrating this into mach
> for now while we're exploring other possibilities.  If those efforts prove
> to fail, we can always go back and integrate this into mach at a later time.

I don't see any harm in making purely additive changes to mach (which can always be reverted/adjusted later should some better system come into play). Seems like you're overplaying the cost of having multiple solutions competing against each other.

Comment 30

5 years ago
(In reply to comment #29)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #20)
> > Greg, we're trying to see if we can avoid building more systems based on
> > patches.  I'd appreciate if you could hold off integrating this into mach
> > for now while we're exploring other possibilities.  If those efforts prove
> > to fail, we can always go back and integrate this into mach at a later time.
> 
> I don't see any harm in making purely additive changes to mach (which can
> always be reverted/adjusted later should some better system come into play).
> Seems like you're overplaying the cost of having multiple solutions competing
> against each other.

What competition are you referring to?  We already know that RB will not satisfy our needs, and we're working on an alternative (see bug 961265 for the contract details, let me know if you want to be CCed.)
Hi Ehsan,

Review Board is not a stagnant product, and I think we have a lot of the same goals going forward. It's also a *highly* extensible product, especially in the 2.0 release we're working to push out. Greg's been working with us to improve some of the support you guys need as well.

I've been following the thread closely, but haven't had a really good sense of exactly what you feel is missing from Review Board that cannot be added, or why you think it's not going to grow beyond where it is. It seems starting from the ground up on a new product isn't an optimal use of time when there's already a base that can be built upon.
Perhaps we can pay some contractors to help extend / modify Review Board for our needs?
(Reporter)

Comment 33

5 years ago
The concern is that Review Board is "patch based" instead of "changeset/commit based."

Essentially, we would like our code review process to be more repository centric: instead of emailing/attaching/uploading patches around, we push changesets/commits to a repository and initiate a review based on changeset/commit in a repo.

Having looked over the proposal at https://github.com/ehsan/reviewtool, AFAICT what's being proposed is essentially a database holding changesets and their review status plus a number of hooks to integrate with other Mozilla systems.

As others have mentioned, Review Board is extensible. I'm having trouble understanding why we couldn't use Review Board for at least the basic patch display and review interface. We could modify Review Board to associate necessary metadata with review requests such that when a review is granted, Review Board "pings" our review tracking service (if that's even a separate database - it might all be in the same database as Review Board). Perhaps we're dealing with false assumptions stemming from an ignorance of Review Board's (and Django's) extensibility?
I think Review Board could certainly be used that way without a lot of trouble, and without having to patch Review Board itself.

As of 2.0, you can associate commit IDs with a review request and it'll fetch details from the repository to generate or update the review request (providing the summary, description, branch, diff) based on the committed change. We ship a basic UI for this, but it's just making use of our API, so anything can really provide that support (or enhance it really). You could even replace our UI with a custom one that's better suited for your needs.

As for the same/separate database, Django allows for a project to use multiple databases in any form, and to route requests to Django models to whatever database is most appropriate. So, if you have data in a different database, and you want a Review Board extension to talk to it, that should be easy.

You made me realize I've only talked about how it's extensible without going into specifics. I'll give a bit of an overview on what we provide (as of 2.0). Some of this may be useful to you.

= Extensions =

Extensions are sets of Python modules that Review Board can load in to do.. almost anything, really:

* Attach, store, and look up arbitrary data to review requests, diffs, reviews, or most anything else. (These can also be accessed or modified through the REST API.)
* Add new human-facing fields to a review request (if you need anything at all beyond what we provide out of the box).
* Perform actions of any sort when a review request is created, published, reviewed, updated, or closed. We have signals for any creation, update, and delete operations for basically every database-backed object in Review Board, plus signals for many other things.
* Programmatically generate a diff and attach it to a review request.
* Add new URLs and new pages.
* Add new navigation bar entries.
* Add new pages or columns to the dashboard.
* Add new REST API (which the Python API will automatically be able to make use of, without changes).
* Replace any page, if you really want to (you can even completely replace the diff viewer if it really came down to it).
* Provide new backends for talking to a repository, in order to gather information on commits or whatever.
* Add new user preferences or any sort of customization in the My Account page (completely rewritten in 2.0), if you need to, for example, allow users to link up their own repositories.
* Gather additional information on comments on diffs.
* Provide new review UIs for different sorts of uploaded files or binary files in a diff.
* If you really need to, monkey patch things.


= API =

* Create review requests (either with a local diff or based on a commit on a repository).
* Create reviews, comments, etc.
* Upload file attachments.
* Look up or alter custom data on most major resources, either provided by consumes of the API, or by extensions.
* Work with any custom REST API provided by extensions.
* Query review requests based on a number of different criteria, including commit ID.
* Really, access or manipulate most any data in Review Board. The above four are just more likely to be interesting for those looking to really change things.


A lot of this is new in 2.0. Once 2.0 ships, a lot of my work will be focused on making Review Board more DVCS-friendly. Providing diff improvements such as being able to review a series of commits on one review request, instead of squashing into a single diff. (Some people fake it by pushing each commit as a new diff revision on Review Board, which also works, but my new stuff will be *much* nicer than that).

I'm probably leaving some things out. If there's something we lack that you would need, we're all for adding new levels of extensibility. Our goal for a long time has been for Review Board to work as a solid base for the majority of users, yet allow third parties to enhance it through extensions when there's something more specialized they need.
(Reporter)

Comment 35

5 years ago
Created attachment 8365312 [details] [diff] [review]
Install RBTools from Git

RBTools as released on PyPI has a number of bugs and deficiencies with
the Mercurial implementation. These have been fixed in an unreleased
version of RBTools.

Until RBTools with the desired patches is released to PyPI, we will
install RBTools direct from its source repository.

Please note that this patch doesn't add any new functionality to mach or
anything else. It just unbreaks what was there before. We can always
delete this mach command if we don't end up using Review Board.
Attachment #8365312 - Flags: review?(mconley)
(Reporter)

Updated

5 years ago
Assignee: nobody → gps
Status: NEW → ASSIGNED
(Reporter)

Updated

5 years ago
Assignee: gps → nobody
Status: ASSIGNED → NEW
Comment on attachment 8365312 [details] [diff] [review]
Install RBTools from Git

Review of attachment 8365312 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to me, though I'm not a tools/ reviewer.
Attachment #8365312 - Flags: review?(mconley)
Attachment #8365312 - Flags: review?(ahalberstadt)
Attachment #8365312 - Flags: feedback+
(Reporter)

Comment 37

5 years ago
I don't think you need to be a peer of anything for this code. I was tempted to check it in without review :)
Comment on attachment 8365312 [details] [diff] [review]
Install RBTools from Git

Alright, eff it, let's roll.
Attachment #8365312 - Flags: review?(ahalberstadt)
Attachment #8365312 - Flags: review+
Attachment #8365312 - Flags: feedback+

Comment 40

5 years ago
Christian, thanks for participating in the conversation so far!

First things first, please don't take any of what I've said in this bug as a criticism towards RB.  I think RB is a great tool which solves an interesting set of problems, it's just not the set of problems that we at Mozilla are interested in.  Please let me elaborate.

Going forward, we're trying to design workflows that will no longer be patch-centric.  Mozilla has traditionally used patches a lot, and this usage dates back to the days when we used CVS, and patches were the best tool that we had at our disposal.  After the switch to hg years ago, we kept using some of the workflows which were really inspired by the usage of CVS, and uploading patches to Bugzilla for review is one such example.

Patches have this inherent problem that they are completely detached from our source code repository.  A few examples of the problems that this causes for us is: we don't know what revision of the source code the patch is based on, we lose all of the underlying power of hg and git when dealing with patches, we cannot generate proper interdiffs by looking at the patches, applying the patch on multiple branches is error prone, etc.  As a result, we would like to stop focusing on patches for our workflows going forward, and instead rely on the facilities that hg and git provide us for maintaining trees.  Please note that this general comment applies to more than just our code review tool.

Moving away from using patches, the most natural way of sharing your work for review and other purposes is to push your local tree somewhere.  Projects such as gerrit use this idea successfully for code review.  We would like to adopt the same idea which has been proven useful in that project to our workflow.  Moreover, it would be really nice if our workflow remains bug-centric, so that you can publish different versions of your tree, associate them with a given bug, and then finally let other automated systems take your reviewed tree in a given bug and push it to the central code repository (among other things.)

I did a survey of the existing code review tools, including RB, to see if we can use an existing one (or easily change one to fit it to our needs.)  The workflow that we want to enable in the code review is documented here: https://github.com/ehsan/reviewtool (please see the 10,000 Feet Overview section, as the rest of the document is not complete yet.)

What you've said about the extensibility of RB is completely true, and is something that I have thought of before, as I was not in a rush to reinvent our own tool here.  But RB is almost too extensible for our needs.  For example, support for other repository formats than hg and git doesn't provide much value for us, and indeed hurts the objectives that we would like to achieve (such as having the workflow be centered around the underlying DVCS.)  Another example, the inability to review multiple commits inside the same review breaks one of the most common workflows that we currently have (create multiple smaller commits to fix a single issue.)  Also, the review-centric nature of RB (as opposed to being bug-centric) is something that we would need to change as well.  I have also considered extending other review tools such as gerrit to match our needs, but doing that is also very difficult because of other reasons.  This is why we decided to create our own review tool, basically we decided that fixing any of the existing ones according to our specific needs is going to be more difficult than starting from scratch.

I am aware of some of the post-2.0 RB plans through my conversation with Mike Conley about this, and I'm happy to see that RB is moving in this direction, but I'm not convinced that focusing on RB is going to give us exactly what we want with less effort than creating our own tool from scratch.

Hope this clarifies the reason why I'm resisting the usage of RB in this bug.  Again, I'd like to reiterate that I understand that the RB project may have different goals than us, and it's perfectly fine.  We do have the luxury of having to support a much smaller subset of the functionality that you guys need to support, which affords us the ability to move faster towards a tool which satisfies our needs.  But maintaining a larger software requires taking many other things into consideration.  It's definitely possible, but we need to make a call based on how much effort each one of the alternatives entails, and how soon we can expect to have something working.  I'm sure this kind of trade-off is familiar to you.

Please let me know if you have any questions, or if I have completely misunderstood something about RB which invalidates my assumptions above.  Thanks!
(Reporter)

Comment 41

5 years ago
Comment #34 specifically says RB is working on multiple patches per review request.

CC mhoye (since he likes these kinds of discussions and can add some useful context from the community engagement side)

Comment 42

5 years ago
(In reply to comment #41)
> Comment #34 specifically says RB is working on multiple patches per review
> request.

That is a good response to only one of the points I brought up.  :-)
(Reporter)

Comment 43

5 years ago
I've read your 10,000 ft overview and I can easily picture ReviewBoard as part of the solution.

The existing Bugzilla + RB workflow already implements some of these requirements! See bug 875562 for a demo - if you upload an attachment that contains a URL back to reviewboard.allizom.org, Bugzilla automatically sets the MIME type as a RB review and clicking the attachment redirects you to RB. The big missing piece is linkage back to BZ when activity happens on RB. This is definitely possible with RB - I did it 5 years ago at another company: when a review was marked as complete, RB updated some fields in BZ automatically. We just need someone to write that hook for RB + BZ at Mozilla. RB can initiate all the autoland things you described.

I also take issue with the "comments synced back to Bugzilla" requirement. I hate, hate, hate review clutter getting in the way of multiple patch bugs. See http://gregoryszorc.com/blog/2014/01/07/on-multiple-patches-in-bugs/ for my rant on this. I would strongly prefer a review system where individual patches are reviewed in relative isolation (you can group them, sure). I just don't want interleaved review activity, thank you. This is one of the reasons I prefer RB over Bugzilla - it decouples code review from issue tracking and activity. Bugs often have many followers and many interested parties. Much fewer people care about code review (often only two - sometimes a small handful). I'd argue that as long as review comments are preserved, we shouldn't care too much about where a code review is occurring. (Requirement: code review history is preserved under a Mozilla owned and operated domain.)

I still feel we're dismissing a viable solution (RB) and are under-estimating the cost to build our own.

Anyway, here's how I'd see RB fitting into your proposal:

1) Changesets pushed to a repo
2) Web service call against review service initiates a code review by creating a RB review request (or N requests if there are multiple commits). It also submits relevant metadata to Bugzilla so things are linked. The web service knows about the changesets, bug #'s, and review IDs. ReviewBoard can optionally know about additional metadata from this web service / tracking database.
3) Reviews are conducted on RB. Comments are left on RB. New changesets are pushed, reviews and bugs get updated automatically via #2.
4) Review is granted. RB notifies BZ. Autoland fields in BZ are updated automatically. (Perhaps there is a checkbox in RB for autoland.) Alternatively, RB notifies the review tracking web service and that service does everything. Just depends where you want the smarts to live.
5) Autoland does its thing

Is this not the flow you have proposed?
Thanks for the detailed response, Ehsan!

Let me go into some details on Review Board's use of commits and patches.

As of RB 2.0, there are two ways for clients to post a review request:

1) They generate and upload a patch to Review Board, which is then stored for later use.
2) They provide the ID of a commit in the repository to review. In this case, they don't generate or send a patch.

rbt post makes use of #1, whereas our new post-commit UI makes use of #2. Either method is provided through the API.

For #1, we validate and store the provided patch. It can be a patch reflecting a commit made, or some pending work that you didn't want to push to anything.

For #2, we internally generate and store a patch representing the changes made in that commit, based on what the repository gives us. It becomes an implementation detail.

When it comes time to present a side-by-side diff view, we take the file and revision information stored along with the patch and fetch the appropriate file/commit from the repository. That's the "before" version. The stored patch is then applied, generating the "after" version. It's equivalent to the changes made in the commit.

What you end up seeing in the diff viewer is not the uploaded/stored patch, but rather something we generate for the purposes of display only. An enhanced diff of the commit, with features not available in standard patches.

Right now, for #2, it does generate and store a patch. As part of my DVCS work, I'm looking into changing that to just store the commit IDs, and not have to store additional data. Either way, it's an implementation detail that should never matter to users. Users only know that they posted a commit ID for review.

You described some inherent problems with patches, like not knowing the commit a patch was made against. Patch files are indeed limited, which is why we don't fully depend on them. We don't share those limitations. When uploading plain patches through RBTools (or anything using its Python API), we transform the patch if needed, and send data through the API to supplement what's in the patch. In 2.0, that's becoming a lot more powerful, with the ability to post any custom data you want (file attributes, author information, branch, ROT13 of the file, a boolean indicating it's your favorite commit ever, etc.). A push-based proxy around Review Board could provide anything it needs, and just let Review Board do what it does internally.
(apologies to those who have heard this before)

I have previously used an Open Source, commit-based review tool called "Opera Critic" [1]. My experience with this was very good and although we cannot use it directly because it is very git-centric, I hope that any review solution we do adopt can achieve feature parity with Critic. I have a longer discussion of the key features in Critic at [2] and run a Critic instance at [3] (login via github) which is used for the W3C web-platform-tests testsuite and has been experimentally used with Servo. The tutorial there also explains the workflow in more detail.

[1] https://github.com/jensl/critic
[2] https://groups.google.com/d/msg/mozilla.tools.bmo/Qq6hlgRobaw/wgQVgR9NllMJ
[3] https://critic.hoppipolla.co.uk
From a (In reply to Gregory Szorc [:gps] from comment #41)
>
> CC mhoye (since he likes these kinds of discussions 

what the what

So: I'm not sure what I can add to this discussion from a tooling standpoint; I don't have any research to hand that speaks to one patch-review tool over another. From a community-engagement perspective, a tool that enforces a single org-wide coding standard and a process that acknowledges receipt of a patch immediately and gives the submitter a reasonable estimate of when it will get reviewed, both of those things are demonstrably big wins, but I don't think either of them falls into the ReviewBoard wheelhouse.

Comment 47

5 years ago
(In reply to Christian Hammond from comment #44)
> Thanks for the detailed response, Ehsan!

No worries, sorry for my delay in getting back to you!

> Let me go into some details on Review Board's use of commits and patches.
> 
> As of RB 2.0, there are two ways for clients to post a review request:
> 
> 1) They generate and upload a patch to Review Board, which is then stored
> for later use.
> 2) They provide the ID of a commit in the repository to review. In this
> case, they don't generate or send a patch.
> 
> rbt post makes use of #1, whereas our new post-commit UI makes use of #2.
> Either method is provided through the API.

Small question, since you mentioned post-commit UI, does the second way of uploading a review request make any assumption that the review actually happens post-commit (such as, not supporting the notion of addressing the review comments in another version of the patch, etc)?  I'm asking this because our reviews will still happen pre-commit (in respect to the main code repository).

> For #1, we validate and store the provided patch. It can be a patch
> reflecting a commit made, or some pending work that you didn't want to push
> to anything.
> 
> For #2, we internally generate and store a patch representing the changes
> made in that commit, based on what the repository gives us. It becomes an
> implementation detail.

What is the reason that you need to store the patch here instead of a URL to the remote repo on a sha1?  Is it just because of backwards compat with other places in the RB code base which expect to be able to work with patches?

> When it comes time to present a side-by-side diff view, we take the file and
> revision information stored along with the patch and fetch the appropriate
> file/commit from the repository. That's the "before" version. The stored
> patch is then applied, generating the "after" version. It's equivalent to
> the changes made in the commit.
> 
> What you end up seeing in the diff viewer is not the uploaded/stored patch,
> but rather something we generate for the purposes of display only. An
> enhanced diff of the commit, with features not available in standard patches.

So it would be exactly identical to the output of |git show| or |hg export| on the respective commit?

> Right now, for #2, it does generate and store a patch. As part of my DVCS
> work, I'm looking into changing that to just store the commit IDs, and not
> have to store additional data. Either way, it's an implementation detail
> that should never matter to users. Users only know that they posted a commit
> ID for review.

I'm a bit confused now.  Is the #2 workflow something that currently exists in RB or is that something that you and other RB hackers are planning to work on?

> You described some inherent problems with patches, like not knowing the
> commit a patch was made against. Patch files are indeed limited, which is
> why we don't fully depend on them. We don't share those limitations. When
> uploading plain patches through RBTools (or anything using its Python API),
> we transform the patch if needed, and send data through the API to
> supplement what's in the patch.

Well, the other issue with using patches in my opinion is ensuring that they indeed remain an implementation detail and do not leak into the observable behavior of the system.  So, for example, how does the interdiff generation work in the #2 workflow assuming that the two commits being interdiff-ed are not based on the same parent?  The current patch based interdiff implementation in Bugzilla for example is so bad that nobody really uses it.

> In 2.0, that's becoming a lot more powerful,
> with the ability to post any custom data you want (file attributes, author
> information, branch, ROT13 of the file, a boolean indicating it's your
> favorite commit ever, etc.).

I can't actually think of any reason why we would need to store any additional metadata.  I think the only thing that we need to have on the review tool side is the (repo, sha1, bug number) triple.

Comment 48

5 years ago
(In reply to Gregory Szorc [:gps] from comment #43)
> I've read your 10,000 ft overview and I can easily picture ReviewBoard as
> part of the solution.
> 
> The existing Bugzilla + RB workflow already implements some of these
> requirements! See bug 875562 for a demo - if you upload an attachment that
> contains a URL back to reviewboard.allizom.org, Bugzilla automatically sets
> the MIME type as a RB review and clicking the attachment redirects you to
> RB.

Yes, I'm aware of this.

> The big missing piece is linkage back to BZ when activity happens on RB.
> This is definitely possible with RB - I did it 5 years ago at another
> company: when a review was marked as complete, RB updated some fields in BZ
> automatically. We just need someone to write that hook for RB + BZ at
> Mozilla.

Yes, I'm sure that's possible.  But it's some work which is actually a decent chunk of the work required to do our own review tool.

> RB can initiate all the autoland things you described.

Yes, again, this is not the question of it being possible to modify RB to do that.

> I also take issue with the "comments synced back to Bugzilla" requirement. I
> hate, hate, hate review clutter getting in the way of multiple patch bugs.
> See http://gregoryszorc.com/blog/2014/01/07/on-multiple-patches-in-bugs/ for
> my rant on this.

Fixing the comment clutter problem is a larger issue than just review comments, since we currently lack the ability to do things like thread the comments, mark certain ones as important, tag some comments etc.  Some of these issues are things that Bugzilla/BMO developers are working on as far as I understand.  It seems to me like once we have a better story for managing comments overall, we already have a solution to this issue.

Also, it's important to note that while seeing review feedback as Bugzilla comments is annoying to you, other people rely on that as part of their workflows.

> I would strongly prefer a review system where individual
> patches are reviewed in relative isolation (you can group them, sure).

What I'm proposing gives you that, but actually RB doesn't provide you that without some modifications to make it aware of the notion of there being multiple review requests on going per bug, splitting up pieces of the work under review into multiple patches, squashing them, etc.

> I
> just don't want interleaved review activity, thank you. This is one of the
> reasons I prefer RB over Bugzilla - it decouples code review from issue
> tracking and activity. Bugs often have many followers and many interested
> parties. Much fewer people care about code review (often only two -
> sometimes a small handful).

If you're annoyed by receiving emails about those comments, then it seems like the right solution would be to do some kind of filtering in your email client.  It's true that not everybody is interested in every activity that happens on a bug but code reviews are just one example.

> I'd argue that as long as review comments are
> preserved, we shouldn't care too much about where a code review is
> occurring. (Requirement: code review history is preserved under a Mozilla
> owned and operated domain.)

Sure.

> I still feel we're dismissing a viable solution (RB) and are
> under-estimating the cost to build our own.

Please trust me that doing our own tool is my least favorite option as well.  But there are also costs to modify RB to make it do what we want, try to upstream as much of it as we can, and maintain the rest of the modifications downstream forever.  I think your argument would be accurate if RB did everything that we need out of the box, but that is not the case.

> Anyway, here's how I'd see RB fitting into your proposal:
> 
> 1) Changesets pushed to a repo
> 2) Web service call against review service initiates a code review by
> creating a RB review request (or N requests if there are multiple commits).
> It also submits relevant metadata to Bugzilla so things are linked. The web
> service knows about the changesets, bug #'s, and review IDs. ReviewBoard can
> optionally know about additional metadata from this web service / tracking
> database.
> 3) Reviews are conducted on RB. Comments are left on RB. New changesets are
> pushed, reviews and bugs get updated automatically via #2.
> 4) Review is granted. RB notifies BZ. Autoland fields in BZ are updated
> automatically. (Perhaps there is a checkbox in RB for autoland.)
> Alternatively, RB notifies the review tracking web service and that service
> does everything. Just depends where you want the smarts to live.
> 5) Autoland does its thing
> 
> Is this not the flow you have proposed?

Ignoring details such as RB not being "bug centric" etc, that's mostly true, but once we do all of this work, we have done much of the work required to write our own review tool.  Basically, most of the interesting bits of what we need are outside of the core of what a review tool does (store and display comments per lines of code).

Comment 49

5 years ago
(In reply to James Graham [:jgraham] from comment #45)
> I have previously used an Open Source, commit-based review tool called
> "Opera Critic" [1]. My experience with this was very good and although we
> cannot use it directly because it is very git-centric, I hope that any
> review solution we do adopt can achieve feature parity with Critic. I have a
> longer discussion of the key features in Critic at [2] and run a Critic
> instance at [3] (login via github) which is used for the W3C
> web-platform-tests testsuite and has been experimentally used with Servo.
> The tutorial there also explains the workflow in more detail.

I have looked at Critic before, and as you mentioned it's unfortunately very git-centric.  Do you see anything in https://github.com/ehsan/reviewtool which makes it impossible for us to extend this tool to do the things you mentioned in that thread?
(Reporter)

Comment 50

5 years ago
(In reply to :Ehsan Akhgari (needinfo? me!) (@ work week, reduced responsiveness) from comment #47)
> > In 2.0, that's becoming a lot more powerful,
> > with the ability to post any custom data you want (file attributes, author
> > information, branch, ROT13 of the file, a boolean indicating it's your
> > favorite commit ever, etc.).
> 
> I can't actually think of any reason why we would need to store any
> additional metadata.  I think the only thing that we need to have on the
> review tool side is the (repo, sha1, bug number) triple.

I can think of some:

* Results from automation
* Pushlog info (after it lands)
* Whether the client performed unit tests on the patch before upload
* Results of automated static analysis
* The first Nightly, Aurora, Beta, Release, etc build the patch appeared in (populated later)

The RB database could conceivably be used to drive part of autoland. Although that may not pan out since autoland appears to be mostly implemented through other means.
(In reply to :Ehsan Akhgari (needinfo? me!) (@ work week, reduced responsiveness) from comment #49)

> I have looked at Critic before, and as you mentioned it's unfortunately very
> git-centric.

Yes, I'm not claiming that we can use it directly, just that the workflow is very very good and should be a standard against which we measure our solution.

> Do you see anything in https://github.com/ehsan/reviewtool
> which makes it impossible for us to extend this tool to do the things you
> mentioned in that thread?

So, my general comment is that it is important to focus on the review tool rather than the way that we will integrate with existing systems to recreate today's workflows, because, as long as you design in extension points with hooks into the right parts of the workflow, integrating with other systems is something that you can add in once the core tool is working right. Moreover, once you have a great review tool you are likely to want different workflows than the ones you suffered with a poor review tool. For example the checkin-needed flag is mentioned there, but really that whole process should be automated and accessible from a "checkin now" button that appears once the review is accepted.

Now some more detailed feedback:

* url shows a list of all changesets pushed that triggered the hook and allow dev to set r? on each changeset pushed

There should be some mechanism to create a review directly from a push without going through the web UI at all. For example in critic pushing a branch with a name starting r/ will create a review of the last commit on that branch. This is super-convenient for the common case.

* lets developers annotate(append-only in the backend) lines in relevant changesets with comments (both before and after the change, in case someone needs to say something about the way the code used to work before the change.)

It also needs to be possible to make comments about the review in general and perhaps about specific files in a review but unrelated to a specific line. 

* can set overall review to "r+ accepted" or "r- rejected" or "r? cleared" or reassign r? to someone else

I'm not sure this is clear from the document, but in the common case the tool should pick the correct reviewer(s) rather than the requestee being responsible for manually picking a reviewer. Especially for new contributors, but really for anyone, the details of who owns what code is not something that you should have to learn in order to submit a change. Obviously it should be possible for people who know what they are doing to override the defaults, but this should not be a required step for everyone. It should also be possible for non-reviewers to make comments in the tool, and for multiple people to act as reviewers for a single change.

Also, I think that the concept of an overall status for the review is unnecessary. There are two orthogonal considerations to determine if a review is "done": a) Have all the changes been reviewed and b) Are there any outstanding issues from that review. When everything is reviewed and there are no remaining issues there is clearly nothing left to do so the review is accepted. Similarly when everything is reviewed but there are outstanding issues, it is clear to everyone what has to happen in order for the review to be accepted. Therefore it should be possible to mark parts of review branch as "reviewed" with the granularity of at least per file per commit.

* syntax highlight would be nice, but not necessary yet(might be able to reuse hg browsing ui or dxr)

Syntax highlight is essential.

* if the changesets are not transplanted cleanly, the author can rebase their work locally, push it again to the review tool, and set the checkin? flag on the rebased set of changes

It should be possible to review merge conflicts resulting from rebases.


I probably missed something, but it isn't clear to me how change authors are expected to update their changes in this tool. I assume they should push more commits to the review, rather than updating existing patches? This makes more sense because you don't have to reinvent interdiffs, and it makes it easy to track the progress of the review. Squashing can happen at the end of the review and could be provided by an in-place UI.
Just thought I'd add something new to the mix:

Over the weekend, smacleod and I hacked on a mechanism for creating Review Board review requests with a push-based work flow.

This only took us a few hours, and is mostly a proof-of-concept. There are some edge cases and concerns that we'll need to deal with, but we feel pretty good about what we came up with:

Demo video: https://vimeo.com/86448355
Code: https://github.com/mikeconley/rb-repo

Feedback welcome!
(In reply to Mike Conley (:mconley) from comment #52)
> Feedback welcome!

This rules. We'll have to make sure the url model, etc makes sense, but I think we should base our work in bug 961265 on this.
Blocks: 961265

Comment 54

5 years ago
Sorry for taking so long to respond, I'm mostly getting buried in email these days.

(In reply to James Graham [:jgraham] from comment #51)
> (In reply to :Ehsan Akhgari (needinfo? me!) (@ work week, reduced
> responsiveness) from comment #49)
> 
> > I have looked at Critic before, and as you mentioned it's unfortunately very
> > git-centric.
> 
> Yes, I'm not claiming that we can use it directly, just that the workflow is
> very very good and should be a standard against which we measure our
> solution.

Point taken (with the usual disclaimer that this is subjective etc etc. -- you know the drill.  :-)

> > Do you see anything in https://github.com/ehsan/reviewtool
> > which makes it impossible for us to extend this tool to do the things you
> > mentioned in that thread?
> 
> So, my general comment is that it is important to focus on the review tool
> rather than the way that we will integrate with existing systems to recreate
> today's workflows, because, as long as you design in extension points with
> hooks into the right parts of the workflow, integrating with other systems
> is something that you can add in once the core tool is working right.
> Moreover, once you have a great review tool you are likely to want different
> workflows than the ones you suffered with a poor review tool. For example
> the checkin-needed flag is mentioned there, but really that whole process
> should be automated and accessible from a "checkin now" button that appears
> once the review is accepted.

Yes it will all be automated but that work is mostly outside of the review tool itself (we call it the "Autoland" project), and we should ensure that we get the interactions of these systems right.

(Also note that I have been trying to use the terminology familiar to Mozilla hackers.)

> Now some more detailed feedback:
> 
> * url shows a list of all changesets pushed that triggered the hook and
> allow dev to set r? on each changeset pushed
> 
> There should be some mechanism to create a review directly from a push
> without going through the web UI at all. For example in critic pushing a
> branch with a name starting r/ will create a review of the last commit on
> that branch. This is super-convenient for the common case.
> 
> * lets developers annotate(append-only in the backend) lines in relevant
> changesets with comments (both before and after the change, in case someone
> needs to say something about the way the code used to work before the
> change.)
> 
> It also needs to be possible to make comments about the review in general
> and perhaps about specific files in a review but unrelated to a specific
> line. 
> 
> * can set overall review to "r+ accepted" or "r- rejected" or "r? cleared"
> or reassign r? to someone else

Yeah, these are all the standard of what everybody expects here.

> I'm not sure this is clear from the document, but in the common case the
> tool should pick the correct reviewer(s) rather than the requestee being
> responsible for manually picking a reviewer. Especially for new
> contributors, but really for anyone, the details of who owns what code is
> not something that you should have to learn in order to submit a change.
> Obviously it should be possible for people who know what they are doing to
> override the defaults, but this should not be a required step for everyone.

This is a Hard Problem.  In many cases picking the correct reviewer requires human judgement (and prior experience with the code in question.)  I agree that we should try to help people pick a reviewer as much as possible, but that's kind of outside of the scope of what we're mainly debating here.

FWIW we have a rudimentary version of this currently in Splinter which we can improve on.

> It should also be possible for non-reviewers to make comments in the tool,
> and for multiple people to act as reviewers for a single change.

Sure, again, this is what everybody would expect here.

> Also, I think that the concept of an overall status for the review is
> unnecessary. There are two orthogonal considerations to determine if a
> review is "done": a) Have all the changes been reviewed and b) Are there any
> outstanding issues from that review. When everything is reviewed and there
> are no remaining issues there is clearly nothing left to do so the review is
> accepted. Similarly when everything is reviewed but there are outstanding
> issues, it is clear to everyone what has to happen in order for the review
> to be accepted. Therefore it should be possible to mark parts of review
> branch as "reviewed" with the granularity of at least per file per commit.

It's not always easy to make that distinction the way we work.  Sometimes the last comment on a review is: "r=me with these comments addressed" which means the author of the patch has more work to do before getting their stuff landed.  Other times it is: "r=me if you file a follow-up bug for this issue", which means the author is ready to get their stuff landed as is.  Hence the need for an explicit "done" state.

> * syntax highlight would be nice, but not necessary yet(might be able to
> reuse hg browsing ui or dxr)
> 
> Syntax highlight is essential.

It is for an awesome review tool.  It's not for something to get us started on having an awesome review tool.  That is what I was addressing in that document.

> * if the changesets are not transplanted cleanly, the author can rebase
> their work locally, push it again to the review tool, and set the checkin?
> flag on the rebased set of changes
> 
> It should be possible to review merge conflicts resulting from rebases.

That should be possible with an interdiff of the pre/post-rebase versions.

> I probably missed something, but it isn't clear to me how change authors are
> expected to update their changes in this tool. I assume they should push
> more commits to the review, rather than updating existing patches?

Correct.

> This
> makes more sense because you don't have to reinvent interdiffs, and it makes
> it easy to track the progress of the review. Squashing can happen at the end
> of the review and could be provided by an in-place UI.

yes, this is exactly what I have in mind.

Comment 55

5 years ago
(In reply to Mike Conley (:mconley) from comment #52)
> Just thought I'd add something new to the mix:
> 
> Over the weekend, smacleod and I hacked on a mechanism for creating Review
> Board review requests with a push-based work flow.
> 
> This only took us a few hours, and is mostly a proof-of-concept. There are
> some edge cases and concerns that we'll need to deal with, but we feel
> pretty good about what we came up with:
> 
> Demo video: https://vimeo.com/86448355
> Code: https://github.com/mikeconley/rb-repo
> 
> Feedback welcome!

So Mike demoed this to me in person the other day and it blew my mind!  He even almost convinced me that we should proceed with RB.  I still have some doubts left and we are meeting tomorrow to discuss them and hopefully get to a decision.  More to come here soon!
(In reply to Mike Conley (:mconley) from comment #52)
> Just thought I'd add something new to the mix:
> 
> Over the weekend, smacleod and I hacked on a mechanism for creating Review
> Board review requests with a push-based work flow.
> 
> This only took us a few hours, and is mostly a proof-of-concept. There are
> some edge cases and concerns that we'll need to deal with, but we feel
> pretty good about what we came up with:
> 
> Demo video: https://vimeo.com/86448355
> Code: https://github.com/mikeconley/rb-repo
> 
> Feedback welcome!

This looks like a big improvement compared to the status quo, so that's great. But it still seems suboptimal in some ways. As far as I could tell one had to continually squash and force-push to update a branch, rather than pushing more commits to a branch. This has a number of significant disadvantages; it prevents very useful workflows (e.g. it makes it very difficult for more than one contributor to work on the same review request, which is *exceptionally* useful), it requires extra work from the patch author and makes it more difficult to track which bits of the patch have already been reviewed. If I have understood correctly, this requirement to maintain a single patch for each review request negates many of the key benefits of working in a branch based model.
(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #54)
> Yeah, these are all the standard of what everybody expects here.
> 
> > I'm not sure this is clear from the document, but in the common case the
> > tool should pick the correct reviewer(s) rather than the requestee being
> > responsible for manually picking a reviewer. Especially for new
> > contributors, but really for anyone, the details of who owns what code is
> > not something that you should have to learn in order to submit a change.
> > Obviously it should be possible for people who know what they are doing to
> > override the defaults, but this should not be a required step for everyone.
> 
> This is a Hard Problem.  In many cases picking the correct reviewer requires
> human judgement (and prior experience with the code in question.)  I agree
> that we should try to help people pick a reviewer as much as possible, but
> that's kind of outside of the scope of what we're mainly debating here.
> 
> FWIW we have a rudimentary version of this currently in Splinter which we
> can improve on.

It's only a Hard Problem if you insist that there is exactly one reviewer per review. Critic simply maintains a mapping of file:reviewers. Whenever a file is touched by a commit in a review anyone who is a reviewer for that file is included in the review (by default only as a reviewer for files they own). There are then manual overrides to either exclude yourself from a review which you don't intend do, to take full ownership of a particular review, or to allow someone to review changes in files they aren't normally a reviewer for. In practice this system works fine and avoids many of the problems of the single-owner model e.g. there is no need to try and guess who the least-burdened reviewer for a specific component is and people don't review changes in code that they don't actually understand.

> > Also, I think that the concept of an overall status for the review is
> > unnecessary. There are two orthogonal considerations to determine if a
> > review is "done": a) Have all the changes been reviewed and b) Are there any
> > outstanding issues from that review. When everything is reviewed and there
> > are no remaining issues there is clearly nothing left to do so the review is
> > accepted. Similarly when everything is reviewed but there are outstanding
> > issues, it is clear to everyone what has to happen in order for the review
> > to be accepted. Therefore it should be possible to mark parts of review
> > branch as "reviewed" with the granularity of at least per file per commit.
> 
> It's not always easy to make that distinction the way we work.  Sometimes
> the last comment on a review is: "r=me with these comments addressed" which
> means the author of the patch has more work to do before getting their stuff
> landed.  Other times it is: "r=me if you file a follow-up bug for this
> issue", which means the author is ready to get their stuff landed as is. 
> Hence the need for an explicit "done" state.

I think the problem here is the model. I *hate* getting r=me with changes type comments. If the changes are that trivial, they should also be easy enough to review. I assume that the reason we don't do this at the moment is that the overhead is so high. Once you have a system where adding minor fixups to the review is

vcs commit -m "fixup! Implement feature"
vcs push

and the reviewer can review just that commit, you don't have any need to allow unreviewed fixup changes into the tree anymore.

r="me with followup bug" in a critic-like model is "Issue: File a bug on X", which the author can address by filing the bug, adding a comment pointing to the bug, and manually resolving the issue, which will automatically set the review to "ACCEPTED" (assuming the code is all reviewed and there are no other outstanding issues). So I still think the critic model is strictly better than the one with a single "done" flag.

Comment 58

5 years ago
(In reply to James Graham [:jgraham] from comment #57)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailapocalypse) from comment #54)
> > Yeah, these are all the standard of what everybody expects here.
> > 
> > > I'm not sure this is clear from the document, but in the common case the
> > > tool should pick the correct reviewer(s) rather than the requestee being
> > > responsible for manually picking a reviewer. Especially for new
> > > contributors, but really for anyone, the details of who owns what code is
> > > not something that you should have to learn in order to submit a change.
> > > Obviously it should be possible for people who know what they are doing to
> > > override the defaults, but this should not be a required step for everyone.
> > 
> > This is a Hard Problem.  In many cases picking the correct reviewer requires
> > human judgement (and prior experience with the code in question.)  I agree
> > that we should try to help people pick a reviewer as much as possible, but
> > that's kind of outside of the scope of what we're mainly debating here.
> > 
> > FWIW we have a rudimentary version of this currently in Splinter which we
> > can improve on.
> 
> It's only a Hard Problem if you insist that there is exactly one reviewer
> per review.

Nope.  We regularly have multiple people review the same change.

> Critic simply maintains a mapping of file:reviewers. Whenever a
> file is touched by a commit in a review anyone who is a reviewer for that
> file is included in the review (by default only as a reviewer for files they
> own). There are then manual overrides to either exclude yourself from a
> review which you don't intend do, to take full ownership of a particular
> review, or to allow someone to review changes in files they aren't normally
> a reviewer for. In practice this system works fine and avoids many of the
> problems of the single-owner model

Well, note that there is a distinction between the right reviewer for a file and the right reviewer for a change.  They don't always map cleanly to each other.  I can definitely believe that we can improve on various things here, and we should try to do that, but there is nothing here which needs to block our current work.

> e.g. there is no need to try and guess
> who the least-burdened reviewer for a specific component is and people don't
> review changes in code that they don't actually understand.

Showing how many outstanding review requests the person has is definitely one of the things that the code review tool is in the best position to reflect in the UI.

> > > Also, I think that the concept of an overall status for the review is
> > > unnecessary. There are two orthogonal considerations to determine if a
> > > review is "done": a) Have all the changes been reviewed and b) Are there any
> > > outstanding issues from that review. When everything is reviewed and there
> > > are no remaining issues there is clearly nothing left to do so the review is
> > > accepted. Similarly when everything is reviewed but there are outstanding
> > > issues, it is clear to everyone what has to happen in order for the review
> > > to be accepted. Therefore it should be possible to mark parts of review
> > > branch as "reviewed" with the granularity of at least per file per commit.
> > 
> > It's not always easy to make that distinction the way we work.  Sometimes
> > the last comment on a review is: "r=me with these comments addressed" which
> > means the author of the patch has more work to do before getting their stuff
> > landed.  Other times it is: "r=me if you file a follow-up bug for this
> > issue", which means the author is ready to get their stuff landed as is. 
> > Hence the need for an explicit "done" state.
> 
> I think the problem here is the model. I *hate* getting r=me with changes
> type comments. If the changes are that trivial, they should also be easy
> enough to review. I assume that the reason we don't do this at the moment is
> that the overhead is so high. Once you have a system where adding minor
> fixups to the review is
> 
> vcs commit -m "fixup! Implement feature"
> vcs push
> 
> and the reviewer can review just that commit, you don't have any need to
> allow unreviewed fixup changes into the tree anymore.

I'm not sure if I understand why you (assuming as the change author) hate getting those types of r+'es...  But the reason why this is a common practice here is basically trust on part of the reviewer.  So when you get these from reviewers, it's a sign that they trust you with those changes.  The reviewer usually makes a judgment call about the nature of the comments he's making, and how much they trust the author of the change to be able to address them properly without another round of review.  For example, I typically r+ patches coming from a developer that I fully trust when the changes are trivial and there is only one sane way of doing them.  But if I receive the same patch from somebody I don't know before, I will have a harder time trusting them to address my request without breaking things, so in those cases I r- the patch, and require another review.

(Note that this is basically more of a culture question.  There is little objective merit to either ways of doing things.)

> r="me with followup bug" in a critic-like model is "Issue: File a bug on X",
> which the author can address by filing the bug, adding a comment pointing to
> the bug, and manually resolving the issue, which will automatically set the
> review to "ACCEPTED" (assuming the code is all reviewed and there are no
> other outstanding issues). So I still think the critic model is strictly
> better than the one with a single "done" flag.

Like I said, it's a culture issue.  We basically do the exact same thing through communicating in English rather than through software.  It's true that the kinds of things that your tools support can shape the culture people build around using them.  But my goal here is to stick to as much of the culture people are used to as possible.  Adopting new tools is always hard, and we don't want to make that harder than necessary by requiring people to change both their tools and their culture here.

Comment 59

5 years ago
(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #55)
> (In reply to Mike Conley (:mconley) from comment #52)
> > Just thought I'd add something new to the mix:
> > 
> > Over the weekend, smacleod and I hacked on a mechanism for creating Review
> > Board review requests with a push-based work flow.
> > 
> > This only took us a few hours, and is mostly a proof-of-concept. There are
> > some edge cases and concerns that we'll need to deal with, but we feel
> > pretty good about what we came up with:
> > 
> > Demo video: https://vimeo.com/86448355
> > Code: https://github.com/mikeconley/rb-repo
> > 
> > Feedback welcome!
> 
> So Mike demoed this to me in person the other day and it blew my mind!  He
> even almost convinced me that we should proceed with RB.  I still have some
> doubts left and we are meeting tomorrow to discuss them and hopefully get to
> a decision.  More to come here soon!

We had the meeting as planned and it went very well.  At this point I have no further reservations about using RB as our code review tool, and I think we should proceed with it.  Thanks a lot to everybody who helped here!  I very much appreciate it.  Here are the notes (well, the transcript) from the meeting, thanks to mconley: https://etherpad.mozilla.org/rb-talk
Excellent.  I will continue my work into putting Bugzilla integration into a proper extension (which I had shelved pending this general conversation/investigation into review tools) so that it's more maintainable, and we can talk more about what kind of further Bugzilla integration is desirable.
(Reporter)

Comment 61

5 years ago
What are people thinking for transition plans? Is now too soon to get people used to typing |mach review| (or equivalent) to submit code for review? It may not be push-based now, but it could certainly transition into that. I think we could maintain the same CLI API seamlessly. I think there are compelling reasons to start getting more people using ReviewBoard today, since it will be the ultimate solution.

I was thinking of hacking up a |mach review| or |mach postreview| command that publishes a RB patch and attaches a "redirect" attachment to the corresponding Bugzilla bug. Is that enough, or should we wait until Mozilla's RB instance is more official/featureful?
(In reply to Gregory Szorc [:gps] from comment #61)
> I was thinking of hacking up a |mach review| or |mach postreview| command
> that publishes a RB patch and attaches a "redirect" attachment to the
> corresponding Bugzilla bug. Is that enough, or should we wait until
> Mozilla's RB instance is more official/featureful?

I think that sounds like a fine start to get people going with it.
(In reply to Gregory Szorc [:gps] from comment #61)
> I was thinking of hacking up a |mach review| or |mach postreview| command
> that publishes a RB patch and attaches a "redirect" attachment to the
> corresponding Bugzilla bug.

make sure you just attach the reviewboard url as text/plain, not text/html with a meta refresh.
bmo will automatically do the redirection (bug 922226) and it then becomes a url we can report on.
Do we need to upgrade our Review Board install before it is particularly useful?  Or are we okay with what we have there for a little bit?  I ask because it's going to take me a little while to create a proper official Bugzilla extension (like a week or two, depending on other stuff).
Most of my complaints have been about the integration with review flags, which sounds like it's part of the bugzilla extension. As a reviewer, it's really hard to know from the current RB emails whether something is ready for a second review.
(In reply to Mark Côté ( :mcote ) from comment #64)
> Do we need to upgrade our Review Board install before it is particularly
> useful?  Or are we okay with what we have there for a little bit?  I ask
> because it's going to take me a little while to create a proper official
> Bugzilla extension (like a week or two, depending on other stuff).

My recommendation would be to move to the latest 2.0 beta, given how close
we are to getting 2.0 out. There's a lot of new abilities in there for
extensions, workflow integrations, and commit IDs.

I'm finishing up the last bit of what's either going to be beta 3 or RC 1,
and we've been heavily dogfooding it. There are some bugs in beta 2, but
nothing major that we've hit.

Comment 67

5 years ago
(In reply to comment #61)
> What are people thinking for transition plans? Is now too soon to get people
> used to typing |mach review| (or equivalent) to submit code for review? It may
> not be push-based now, but it could certainly transition into that. I think we
> could maintain the same CLI API seamlessly. I think there are compelling
> reasons to start getting more people using ReviewBoard today, since it will be
> the ultimate solution.

I'm still thinking about the workflow.  I'm not yet sure if it's a good idea to hide the push behind a mach command; I'm not yet sure what the thing we want in terms of submitting changes is.  Also, given the fact that the RB interfacing will probably change soon, it might make sense for us to hold off a bit here to know more about what we want to do.

The contractor is going to start their work very soon (tomorrow-ish) so this is very near in the future, so I hope it's OK for you to hold off for a bit.  Thanks!
mcote - assuming we want to move to RB 2.0 for the goodness therein, should we kick-off a security review for that version? Is that the way to move forward there?
Flags: needinfo?(mcote)
Yeah, security will probably want to have a look.  However I'd like to wait until I have the Bugzilla extension separated out so they can both be reviewed at the same time.  I'm making good progress on that when I have time; I expect I'll be done next week some time.
Flags: needinfo?(mcote)

Comment 70

5 years ago
(In reply to comment #69)
> Yeah, security will probably want to have a look.  However I'd like to wait
> until I have the Bugzilla extension separated out so they can both be reviewed
> at the same time.

Makes sense.  FWIW I do think we want RB 2.0 for the purposes of security review whenever we get to it.

> I'm making good progress on that when I have time; I expect
> I'll be done next week some time.

Thanks a lot!
The extension is at https://github.com/mozilla/rbbz.  It provides the same functionality as my fork (auth and user-search handled by Bugzilla).  It requires the latest from the Review Board release-2.0.x branch.

I filed a security review request as bug 978976.

We should also discuss a plan for further Bugzilla integration; I know there are some varied opinions (e.g. review comments mirrored to Bugzilla vs left solely in Review Board).

Comment 72

5 years ago
(In reply to comment #71)
> We should also discuss a plan for further Bugzilla integration; I know there
> are some varied opinions (e.g. review comments mirrored to Bugzilla vs left
> solely in Review Board).

Good point.  I'll ping you about this once we get some more stuff done on the review tool.  Is there anything you would like to know sooner than later on your end?
Not specifically; I'd just like to come up with a plan--what work needs to be done on Bugzilla or to the Bugzilla extension and in what order.  Bug 515210 has the (very old) original plan, and we have our own ideas, but of course we'd like the input of other interested developers (like yourself!).  We're planning Q2 goals soon, so I'd like to incorporate some of the plan work into them.

Comment 74

5 years ago
(In reply to comment #73)
> Not specifically; I'd just like to come up with a plan--what work needs to be
> done on Bugzilla or to the Bugzilla extension and in what order.  Bug 515210
> has the (very old) original plan, and we have our own ideas, but of course we'd
> like the input of other interested developers (like yourself!).  We're planning
> Q2 goals soon, so I'd like to incorporate some of the plan work into them.

Fair enough.  I'll review the existing materials and will get back to you.
Just a note here that bug 515210 has the current plan and is a tracker for individual bugs.
(Reporter)

Comment 76

5 years ago
RBTools 0.6 was released the other day. `mach rbt` will now install that version (instead of installing from a specific pre-release Git commit).
Component: Administration → General
(Reporter)

Comment 77

5 years ago
I think this bug has ran its course of usefulness.

Mozilla is deploying ReviewBoard with deep integrations into Bugzilla. A requirement is being able to `hg push` or `git push` to initiate code review. I can't imagine "make adding proper patches to ReviewBoard easier" much easier than that!
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.