Page MenuHomePhabricator

You should be able to merge your own patches
Closed, InvalidPublic

Description

Arcanist should be a tool not a process mandate. We self merge changes from time to time now and its fine. Mostly this comes up for deploys but is also a useful (expert) tool for small, new projects - especially when you are working alone and many weeks away from a first release. These are the realities as they stand now. If we want to change this then lets do that but I don't want us to change it simply because a tool forces it on us. I think it'd be more useful to switch to arcanist and _then_ have the discussion about whether or not we should turn off self merges.

Related Objects

StatusAssignedTask
ResolvedDzahn
ResolvedCmjohnson
ResolvedDzahn
Resolveddemon
Resolveddemon
ResolvedDanny_B
ResolvedPaladox
ResolvedPaladox
ResolvedNemo_bis
Resolveddemon
ResolvedPaladox
ResolvedKrenair
Resolvedmmodell
InvalidNone
DeclinedNone
Resolveddemon
InvalidNone
InvalidNone
ResolvedQgil
DeclinedNone
DuplicateNone
Resolvedgreg
Invalidmmodell

Event Timeline

Manybubbles raised the priority of this task from to Low.
Manybubbles updated the task description. (Show Details)
bd808 added a comment.May 21 2015, 3:11 PM

AFAIK differential doesn't actually do the merge. Instead once a patch has been uploaded with arc and is ready for merge the reviewer downloads the patch locally via arc, merges it into their local branch and then pushes the reviewed change back to the canonical git repo (which can be hosted anywhere and is not tied to the review process directly).

Yes, it seems that you push a commit to review, and anyone can specify a reviewer for it (you can also do this from the commit message). Then when one of those reviewers accepts it, a command is shown for someone with edit rights to land the commit.

Joe added a subscriber: Joe.May 21 2015, 3:14 PM

@bd808 so that would mean I'd be able to push changes to - say - operations/puppet in a hurry should I ever need to withouth going through the review process?

That would be great.

bd808 added a comment.May 21 2015, 3:17 PM
In T99905#1301570, @Joe wrote:

@bd808 so that would mean I'd be able to push changes to - say - operations/puppet in a hurry should I ever need to withouth going through the review process?
That would be great.

That's my understanding, yes. See the comment by @mmodell on D1#30. This makes sense actually because differential itself is completely agnostic of the the backing VCS in use. It is really just a structured discussion system tuned to the general process of code review. This is a stark contrast to Gerrit which is a highly opinionated git based review and control system.

So, OTOH, people would be able to push changes to git bypassing review...is there a way to restrict that? I remember a TWN bug about not letting extension commits bypass gerrit...

cscott added a subscriber: cscott.May 21 2015, 4:35 PM

For both OCG and the Collection extension it is very hard for me to find reviewers. And for Parsoid we often have a number of "jointly authored" patches where the self-review requirement gets very weak. Usually we go with "someone other than the last person who pushed a new version of the patch" but that's often the original author of the patch.

In both cases a *mandatory* ban on self-review would be very disruptive. I have no problem with making self-review a big red button that makes you click through "I know what I'm doing", or allowing certain projects to disable self-review, or to making self-review a "privilege" that only certain accounts were able to use. But blanket ban would be very disruptive.

I also don't like the fact that direct pushes can bypass review entirely. That option is present in gerrit but we *very rarely* use it (only when gerrit is hugely broken in some way, or to create new branches). I prefer to have all code changes documented in a consistent way. Even if it's a self-review, having the change in gerrit and +2'ed means that there's a paper trail and a mechanism for folks to be notified of the change and comment on it after the fact. I think that falling back to a cowboy-style direct-push-to-git would be a huge step back.

+1 to everything @cscott said.

Direct push should always be an option in the rare case of horrible horrible things.

That button to land the commit - is it just like the submit button in gerrit? Can we make a bot that does it when on Jenkins' behalf?

That button to land the commit - is it just like the submit button in gerrit? Can we make a bot that does it when on Jenkins' behalf?

I don't think you quite understand, Differential doesn't seem to be that clever:

Oh I see now. We'd have to write a bot to do the actual landing ourselves. Fair enough.

upstream is working on a UI for landing changes. It's currently disabled in the distributed source but it may already be usable (though currently not finished so not yet supported)

revi added a subscriber: revi.May 28 2015, 12:35 PM
mmodell closed this task as Invalid.Oct 22 2015, 10:23 PM
mmodell claimed this task.

this is not really a valid task. there is nothing preventing self-merge inherent to arcanist. It's perfectly possible to merge and push, regardless of whether we use arcanist/differential.

demon moved this task from To Triage to Done/Archive on the Gerrit-Migration board.Dec 1 2015, 6:45 PM