On Thursday 15 July 2010 14:47:05 Thiago Macieira wrote: > Now, going forward, I'd like to keep the good things about this and improve > what's currently suboptimal. Obviously, I'd like the same workflow for all > contributions, regardless of whether they're Trolls or not. Let's go step > by step. I will post a reply to this email with each step and changing the > subject so as to organise the thread.
Like I said, currently, if the author is a Troll, the *author* pushes the commit to a staging area. This implies that each author must have push rights. Also, sometimes when writing the reviewer's name, the author gets the name wrong or forgets to write it. And if the author is not a Troll, then the reviewer needs to push. That's a difference in workflow. It also causes issues like the reviewer being responsible for breakages introduced by an external contributor. Worse, the staging areas only go forward, so the only way to fix an issue is to push more commits, including reverts. It's not possible to simply yank out a patchset. An advantage of this method is that the reviewer doesn't have to stop what he's doing to push a patch he's reviewed. The review can happen out-of-band (e.g., reply to an email). What are the alternatives? One possibility is obviously to have the reviewer apply and push the patch. This is how I believe the development of Git happens: the reviewer uses git am to apply the patch received by email, possibly with the -s option that automatically adds the "Signed-off-by" line to the commit message. The drawback here is that this requires the reviewer to either stop what he's doing and check out a clean branch to apply, or to remember to do it later. Or to review things in batches, when he's not busy. Some of our engineers aren't very proficient with Git, even 18 months after we've switched to using it full-time. The effort of checking out a clean branch is very hard for them. And this is not even addressing git am and its failure modes (no conflict resolution at all). Another drawback I can see is if someone replies that the patch is reviewed, but forgets to apply it. Someone scanning the mailing list would not notice that the commit never went in. We'd only notice it's missing when someone needed it, most likely the author. By the way, this complaint is also valid for ReviewBoard: saying Ship it! doesn't mean the patch is committed. My suggestion would be to have an automated system that picks up the reviewed patches and adds them to a list of pending commits, which in turn serves as input to the integration bot (the next step). Actually, what I'd like is for this to *be* the review system, with an email gateway like KDE's Bugzilla. So as soon as someone creates a new contribution, an email is sent to an ML. Any replies sent will be picked up by the review system and logged. When a reply with a "Ship It!" (or to [email protected], where NNNN is some kind of automatic identifier) is detected, the contribution is marked reviewed and eligible for integration. So we avoid the problem of pushing by having no one push: the bot will pick up all reviewed changes and apply them one by one. This also has the additional benefit of having a clean Git history (no unnecessary merges). -- Thiago Macieira - thiago.macieira (AT) nokia.com Senior Product Manager - Nokia, Qt Development Frameworks Sandakerveien 116, NO-0402 Oslo, Norway Please don't send me .pptx -- prefer .odp or .ppt
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ Opengov mailing list [email protected] http://lists.qt-labs.org/listinfo/opengov
