Feedback welcome. We need to decide whether this is indeed a problem that Differential can't prevent today, and if this is the case whether we should fix it for T560: Proof of concept of code review in Phabricator.

https://secure.phabricator.com/book/phabricator/article/differential_faq/#why-does-an-accepted-rev

In T164#10378, @chasemp wrote:

https://secure.phabricator.com/book/phabricator/article/differential_faq/#why-does-an-accepted-rev

From there:

If authors are being jerks about this (making sweeping changes as soon as they get an accept), solve the problem socially by telling them to stop being jerks. Unless you've configured additional layers of enforcement, there's nothing stopping them from silently changing the code before pushing it, anyway.

Is this "silently changing the code before pushing it" problem covered by this task or another?

In T164#10904, @scfc wrote:

Is this "silently changing the code before pushing it" problem covered by this task or another?

AIUI the plan was for push to be restricted to people we now call "+2ers", so this wouldn't be an issue. However, maybe I'm mistaken?

In T164#10241, @Qgil wrote:

Feedback welcome. We need to decide whether this is indeed a problem that Differential can't prevent today, and if this is the case whether we should fix it for T560: Proof of concept of code review in Phabricator.

We know we have the option to do it Gerrit's way (set differential.sticky-accept to false). So we just need (before launch day) to decide which approach we want. There is no technical blocker here.

In T164#11150, @Jdforrester-WMF wrote:

AIUI the plan was for push to be restricted to people we now call "+2ers", so this wouldn't be an issue. However, maybe I'm mistaken?

We want all code merged to be "Accepted" (what we now called +2'ed). If that's not possible, at least anyone pushing or landing should have +2. Pushing bypassing Differential should be strongly discouraged (absent special circumstances) the same way bypassing Gerrit is.

This possibility ("changing code after Accepted, before merged") can only happen often if there is a noticeable gap before landing. Eventually, when we configure Jenkins (probably the first projects will not use Jenkins) perhaps it could auto-land stuff that is Accepted and has passing tests, as it auto-merges stuff in Gerrit.

At T135#1950, @QChris said even with the Gerrit-like differential.sticky-accept setting, "It seems the “Accept” survives as “Reviewed By” in the commit message."

This needs to be tested/confirmed.

In T164#19760, @Mattflaschen wrote:

At T135#1950, @QChris said even with the Gerrit-like differential.sticky-accept setting, "It seems the “Accept” survives as “Reviewed By” in the commit message."

This needs to be tested/confirmed.

I can confirm that this does happen. Tested with https://phab-02.wmflabs.org/D4.

"Reviewed by" still accurate, unless the entire changeset was drastically modified before submit. And that can only be done by someone with push rights. So it's an issue of trust.

What are we afraid of?

Upstream phabricator has recently added support for doing the merges in differential. It's not quite baked yet but it should be almost stable and soon will be a usable feature.

The problem I see is that currently from time to time someone accidentally screws up rebases or does not implement the review recommendations correctly on the first try. With our current Gerrit configuration, the responsibility for this is clear. With a configuration differential.sticky-accept = true, there is no incentive to review someone else's code as "Accepted" unless you yourself are going to commit it to the repository. If "review" becomes "glance over it and trust the author to do the right thing", then its benefits for the code quality are gone. Code review is based on the experience that even trusted, well-educated programmers make silly mistakes.

@scfc: I agree with you that everyone makes silly mistakes and code review is helpful at catching those.

I don't understand your argument about incentive though. How would sticky-accept affect my incentive to review code?

Also note: with phabricator's audit and owners tools it's possible to flag certain changes that get pushed, so that they get a final chance for post-commit review instead of going unnoticed.

@mmodell: When you review code, you can do so informally: "Yeah, looks alright, bro." +1/+2 are formalized statements, and just like formalized statements in other spheres, they have more weight: "I'm so confident that this change should be merged that I ticked this box." So you incur more cost (effort to test a change thoroughly and risk of loss of reputation), and that cost must be limited and offset by an incentive, in the case of code review usually the mutuality and the ability to search for open/ready-to-merge/need-work changes. An important factor there, like in other spheres, is that the form you signed can't be changed after the fact. If the form is not immutable, there is no incentive to sign it, so you end up (at most) with informal communication that can't be searched, queried, categorized, etc.

+1 proves nothing beyond your ability to click something with a mouse. I take almost no stock in seeing +1s on a patch without any comments associated with it.

In T164#3059644, @demon wrote:

+1 proves nothing beyond your ability to click something with a mouse. I take almost no stock in seeing +1s on a patch without any comments associated with it.

I feel like there is probably a better forum for this conversation, but not knowing where that would be, I'll continue it here:

That seems like an important distinction. If one review has comments but no rating, and another has comments AND a +1, would the +1 add value?

IMHO yes: Words are ambiguous and sometimes hard to parse, "+1"s are "binary".

Yes, +1/-1 with a comment is useful because it lets me know (before reading) if the comments will be mostly positive or mostly negative. It's a review score without additional context that's useless to me.