Page MenuHomePhabricator

Consider ways to improve revert handling
Closed, ResolvedPublic

Description

(Discussion about revert handling comes up every so often. Collecting some of that discussion here.)

Currently, Phabricator treats reverts exactly like any other commit and does no special handling of them.

We can probably do better than this. One problem is that the current behavior is counterintuitive -- users often expect to reopen a revision and continue code review there after the changes are reverted, but we don't support a "reopen" operation right now.

Reopening is tricky, though, and adds enough complexity that it might not be the right approach to pursue. For example:

  • arc diff won't be able to automatically associate the revert-of-revert commit with the original revision.
  • The daemons likely need additional complexity to avoid, e.g., closing a reopened revision incorrectly.

And, generally, the current model works well enough at a low revert rate (Phabricator's is around 0.3% at the moment).

One simpler improvement might be to detect when a commit is a revert (by examining the commit message for, e.g., the git revert template) and note that on the revision, e.g. "reverted by rXnnnn".

Revisions and Commits

rP Phabricator
Abandoned
D12652
Restricted Differential Revision
Restricted Differential Revision
Restricted Differential Revision

Event Timeline

epriestley triaged this task as Wishlist priority.Aug 31 2012, 5:30 PM
epriestley added a subscriber: epriestley.
epriestley updated the task description. (Show Details)Sep 4 2012, 5:22 PM

I like the idea of just putting a note in the revision; no need for the added complexity of reopening diffs.

Would it be possible when starting a new diff from a revert to include the last/all the other patches from the previous diff? Just having the history of what changed (or changed since the revert) when doing a review would be enough.

btrahan added a project: Differential.
epriestley edited this Maniphest Task.Feb 28 2013, 8:20 PM
epriestley edited this Maniphest Task.
epriestley edited this Maniphest Task.Apr 3 2013, 1:54 PM

+@hchan

I'm going to build this to fill out the server-side of D5553; we also need it for Releeph revert handling (via T2714).

epriestley raised the priority of this task from Wishlist to Normal.May 6 2013, 5:32 PM
epriestley added a subscriber: hchan.
epriestley edited this Maniphest Task.May 6 2013, 6:01 PM
epriestley edited this Maniphest Task.May 6 2013, 7:34 PM
epriestley edited this Maniphest Task.May 6 2013, 7:54 PM
epriestley edited this Maniphest Task.May 7 2013, 1:05 AM
epriestley edited this Maniphest Task.May 7 2013, 1:34 AM
hchan added a comment.May 8 2013, 10:40 AM

Hi Evan/Nick,

I treid arc diffing on the arcanist repo and I got this error message:
Exception
[cURL/7] (https://secure.phabricator.com/api/conduit.connect)
<CURLE_COULDNT_CONNECT> The cURL library raised an error while making a
request. You may be able to find more information about this error (error
code: 7) on the cURL site:
http://curl.haxx.se/libcurl/c/libcurl-errors.html

Any idea? I'm currently out of the country, but I wouldn't suspect that's
the case.

Thanks,

Howard

Can you curl https://secure.phabricator.com/ from the same machine?

Related to this are commits that aren't reverts, but are fixes to regressions. Maybe whatever problem was initially introduced wasn't so horrible that it first had to be reverted -- it was easy enough to just fix it. A fix might link back to a revision that introduced the error. Addressing concerns from audits is related: if I author a revision to address an audit concern, it might be nice to formally link the two.

Concerns on my mind:

  1. Merging the bugfix into other release branches is easier if the bugfix is a direct child of the commit that introduced the bug. That is, port it forward, rather than port it back. See DaggyFixes.
  2. It's cool to know, if for no reason other than entertainment, how much stuff is broken. For example, the Twisted high scores gives you 1000 points for reviewing something, but docks you 600 points if it later turns out what you reviewed was broken.

I imagine with a bit of careful execution, #1 is possible with current tools. I think it would mean not directly using arc land, however. Some intelligence in the tool could make proper execution easier and less error-prone.

#2 simply isn't possible if there is nothing in Phabricator's schema recording that a thing was broken and had to be fixed.

One possibility here is to reopen a Maniphest task if a "fixes" commit is reverted. I'm not sure whether this behavior is desirable.

chad changed the visibility from "All Users" to "Public (No Login Required)".Jul 3 2015, 4:37 AM
eadler added a project: Restricted Project.Jul 7 2016, 5:10 PM
epriestley closed this task as Resolved.Jan 31 2018, 3:38 PM

I'm going to roll this forward into T13057.