1507578 - Reopen landed review requests when patches get backed out

Status: RESOLVED FIXED

(Conduit :: General, enhancement, P2)

Description

[Gregory Szorc [:gps]]

Differential revisions are closed in Phabricator when they "land" in a tracked repo.

Lando refuses to land closed Differential revisions.

In order to re-land a backed out changeset using Lando, one needs to first re-open the revision in Phabricator. This is somewhat tolerable for single revisions. But if you have a series of say 10 revisions, manually iterating through all of them to re-open is overly tedious and wastes my time.

I can think of a few mitigations for this:

1. Allow Lando to land closed revisions. (We'll presumably need this anyway to handle uplifts.)

2. Automatically reopen a Phabricator revision when it is backed out. (Pulsebot could potentially do this.)

Comment 1

[Mark Côté [:mcote]]

Yes, this is probably one of the most annoying things about Phabricator-Lando right now.  See also bug 1489126, which is something that we reported to upstream but hasn't been planned out.  We may need to work around it as well, in which case it's related to this, so I'll set this as a blocker.

Comment 2

[Nick Alexander :nalexander [he/him]]

I just ran into this.  I'm concerned that nothing in the Lando-Phabricator-Differential tool stack seems to know "these N commits are all part of the same unit of work".  We see it here, where we need to manually open revisions; but we also see it in Lando's "land stack" operation, which doesn't recognize that the commit I'm focused on is #4 of 6 and that landing should land the entire 6-commit stack.

Making phabricator better would help, since there doesn't seem to be any reason that you can't operate on multiple revisions.  But the information architecture seems wrong: we're not capturing the actual unit of work to be done.  Am I mis-understanding the system?  Is this intentional?

Comment 3

[Mark Côté [:mcote]]

Phabricator is definitely not optimized for packaged multiple-commit units of work.  I've had long conversations with Phacility about this.  Frankly, no tools really are; Mozilla appears to be somewhat of an outlier here, for whatever reason.

Regardless, the child-parent relationship does a fairly good job of capturing this, for our needs.  The example you gave was a conscious decision in Lando as a way to quickly implement support for partial-stack landings.  For sure it is not the most intuitive way to do this; see bug 1490337 for a better solution we're working on.

So in sum, no, Phabricator doesn't make it terrible easy to operate on a stack of commits all together, but we'll be continuing to roll out better support in various ways.

Comment 6

[Steven MacLeod [:smacleod]]

Attachment: differential: re-open revisions backed out by a commit (Bug 1507578).

Differential revisions will now be re-opened by audit when diffusion
imports a backout commit. This feature was requested upstream[1] but
they do not want it in the core phabricator product, so we will use a
patch to make it work.

It is important that the differential.allow-reopen config entry be set
to true or commit importing will fail and repeatedly retry. This
shouldn't be a problem in Mozilla's production install as that is the
config value we use.

[1] https://admin.phacility.com/PHI1008

Comment 7

[David Lawrence [:dkl]]

This has landed. Closing.