Page MenuHomePhabricator

Updating a commit in a Differential does not reset “Accepted” review flag
Closed, ResolvedPublic

Description

Steps to Reproduce:

  1. Upload a harmless commit to Phabricator.
  2. Get someone to review and accept.
  3. Upload a new revision to the Differential that has a harmfull part in it.

Expected outcome:
“Accept” vote should get removed.

Actual outcome:
New revision with harmfull part is still marked with “Accept” vote.

(See the timeline and first two Differentials in D8 for a contrete example)


When in the end doing arc patch D8, the harmfull part even has “Reviewed By: qchris2” in the commit message of the local checkout. :-(

Details

Reference
fl226

Related Objects

View Standalone Graph
This task is connected to more than 200 other tasks. Only direct parents and subtasks are shown here. Use View Standalone Graph to show more of the graph.

Event Timeline

flimport raised the priority of this task from to Low.Sep 12 2014, 1:31 AM
flimport set Reference to fl226.

epriestley wrote on 2014-04-26 21:15:14 (UTC)

You can configure this with differential.sticky-accept.

This discusses why the default behavior works the way it does:

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

qchris wrote on 2014-04-26 21:37:11 (UTC)

You can configure this with differential.sticky-accept.

The default for differential.sticky-accept makes my head hurt :-)

@ fab.wmflabs.org admins: Could we please adjust this parameter to not carry over “Accept” votes?

qgil wrote on 2014-04-28 15:37:07 (UTC)

Is it me or there is a bug in this configuration?

I have changed the setting to "Accepts are reset by updates", mirroring the behavior we have currently in Gerrit.

Now /config/group/differential/ says:

differential.sticky-accept

Customized
Should updating an accepted revision require re-review?
Current Value: false

true/false seem to be exchanged.

Anyway, @QChris please check whether the current setting behaves as you expect.

epriestley wrote on 2014-04-28 16:29:23 (UTC)

Thanks, fixed wording in HEAD (see https://secure.phabricator.com/D8884).

qchris wrote on 2014-05-01 08:45:29 (UTC)

Thanks, @Qgil.

Resetting of “Accept” state now works as expected.

qchris wrote on 2014-05-01 12:12:26 (UTC)

Resetting of “Accept” state now works as expected.

Oh no. It doesn't. :-(
It seems the “Accept” survives as “Reviewed By” in the commit message. See T270.

importing issue status

flimport closed this task as Resolved.Sep 12 2014, 1:32 AM