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

Assigned To

None

Authored By

	QChris
	Apr 26 2014, 9:13 PM

Tags

Referenced Files

None

Subscribers

Jdforrester-WMF

• Mattflaschen-WMF

Description

Steps to Reproduce:

Upload a harmless commit to Phabricator.
Get someone to review and accept.
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
Search...

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.

	Status	Subtype	Assigned	Task
				· · ·
	Resolved		Qgil	T23 Identify features Gerrit users would miss in Phabricator
	Resolved		None	T135 Updating a commit in a Differential does not reset “Accepted” review flag
				· · ·

Event Timeline

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

• flimport added a subscriber: • chasemp.Oct 1 2014, 10:32 PM

• flimport added a subscriber: Qgil.Oct 2 2014, 9:47 PM

• flimport added a subscriber: QChris.Oct 2 2014, 9:55 PM

• flimport added a subscriber: • csteipp.Oct 6 2014, 8:00 PM

• flimport added a subscriber: Jdforrester-WMF.Oct 6 2014, 11:00 PM

• flimport added a subscriber: scfc.Oct 7 2014, 3:00 AM

• flimport added a subscriber: Milimetric.Oct 8 2014, 6:00 PM

• flimport added a subscriber: • Mattflaschen-WMF.Oct 8 2014, 11:00 PM

• Mattflaschen-WMF mentioned this in T164: Uploading a new Differential Revision does not reset “Accept” states in commit messages.Nov 7 2014, 5:19 AM

Negative24 mentioned this in T95987: Plan for small "todo" author owned tasks.Apr 14 2015, 1:31 PM