Page MenuHomePhabricator

Uploading a new Differential Revision does not reset “Accept” states in commit messages
Closed, DeclinedPublic

Description

  • Steps to reproduce (D22):
    • qchris creates a new local commit on top of master.
    • qchris arc diffs to generate Differential Revision 1, asking qchris2 for review.
    • qchris2 accepts Differential Revision 1
    • qchris arc diffs to generate Differential Revision 2
    • Phabricator marks qchris2's “Accept” as “Accepted Prior Diff”.
    • qchris runs arc patch D22 (which pulls Differential Revision 2 into branch “arcpatch-D22”)
  • Actual result:
    • Commit message of arcpatch-D22 commit says “Reviewed By: qchris2”.
  • Expected result:
    • Commit message of arcpatch-D22 commit does not say “Reviewed By: qchris2”, as qchris2 never reviewed Differential Revision 2.

Details

Reference
fl270

Event Timeline

flimport raised the priority of this task from to Normal.Sep 12 2014, 1:33 AM
flimport set Reference to fl270.
Qgil lowered the priority of this task from Normal to Low.Oct 9 2014, 9:23 PM
Qgil added a comment.Oct 10 2014, 12:05 PM

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.

scfc added a comment.Oct 13 2014, 12:03 AM

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.

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.

mmodell added a subscriber: mmodell.EditedMay 1 2015, 4:48 AM

"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?

Restricted Application added subscribers: Steinsplitter, Aklapper. · View Herald TranscriptSep 9 2015, 3:18 PM
greg closed this task as Declined.Oct 22 2015, 9:50 PM
greg claimed this task.
greg added a subscriber: greg.

"Reviewed by" isn't inaccurate, unless the entire changeset was drastically modified before submit. And that can only be done but someone with push rights. So it's an issue of trust.
What are we afraid of?

People being bad actors, I guess.

But my thought is: We trust people with commit rights. Period. If we don't trust people to review changes before they commit them then we need to revoke their commit rights.

From epriestly:

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.

I don't see a technical blocker here, but of course the new workflow in Differential should be documented (with this as a part of it).

Restricted Application added a project: User-greg. · View Herald TranscriptOct 22 2015, 9:50 PM

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.

greg set Security to None.
Restricted Application added a project: User-greg. · View Herald TranscriptOct 22 2015, 10:01 PM
scfc added a comment.Oct 23 2015, 3:07 PM

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.

demon moved this task from To Triage to Done/Archive on the Gerrit-Migration board.Dec 1 2015, 6:45 PM
scfc added a comment.Feb 27 2017, 11:03 PM

@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.

demon added a subscriber: demon.Feb 27 2017, 11:12 PM

+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?

scfc added a comment.Feb 27 2017, 11:52 PM

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.