Make it easier to review stacks of diffs
Open, Needs TriagePublic

Description

This screenshot shows my diff queue. If I reviewed the diffs top down, I would be reviewing them in nearly the opposite order of the dependencies. To be clear the dependencies are

D461 -> D460 and D459
D456 -> D458

D458 and D459 have no dependencies.

Perhaps showing somewhere in this table the dependencies of a given diff, or the start of a stack would make this easier to review in the correct order.
Or adjusting the sort to incorporate dependency information instead of just updated/modified time.

I sure hope I followed the 2.5 pages worth of rules for writing a feature request...

I browsed the open differential tasks and didn't see anything related.

Just to be extra clear, I want differential to help me review stacks by pushing me towards reviewing the first diff in a stack before reviewing dependent diffs.

avivey added a subscriber: avivey.Sep 8 2016, 10:39 PM

something like

?

I know of that view, but it's at least two clicks and some scrolling away. It doesn't help you decide which diff to start reviewing at the differential homepage or in the diffs widget.

Do they actually have dependencies declared? Any idea why the screenshot doesn't show the "Open Dependencies" marker visible here?

cspeckmim added a subscriber: cspeckmim.EditedSep 8 2016, 11:27 PM

What about creating a new query in Diffusion which is the same as the default, but orders the revisions by Creation Date (Oldest First)? You can then use that query in your dashboard. Does that help for this scenario?

Nevermind

Oh, that hint only shows for accepted revisions right now.

I guess it's not particularly useful in this case anyway.

If revisions in other states displayed like in your screenshot @epriestley that would solve the first 90%.

The next 90% would be showing the first unreviewed diff in a stack

Hi @epriestley

I'd like to tackle this change. From my investigation it doesn't look too difficult.

I'll first fetch a bit more information here: https://secure.phabricator.com/diffusion/P/browse/master/src/applications/differential/query/DifferentialRevisionSearchEngine.php$229
And use that to render more information here: https://secure.phabricator.com/diffusion/P/browse/master/src/applications/differential/view/DifferentialRevisionListView.php;ff64c4e02b201aae432005490e23bd9c23285b8b$124-132

Before I start I want to make sure I'm on the same page about the end state.

I can see a few different cases that might warrant different treatments

  • dependency 'Needs Review'
  • dependency is 'Accepted'
  • dependency 'Needs Revision'
  • dependency is abandoned?

I think saying 'Blocked by DXXX' or 'Waiting on DXXX, DXYY' (for multiple direct dependencies) would work for the first 3 cases, if the visual clutter introduced is acceptable.
I'm not sure what happens in the 4th case, maybe it can be ignored.

A smaller change would be to just add 'Open Dependencies' to all of these cases without specifying which particular diff is the dependent.

What would you prefer?

Sorry, we aren't interested in maintaining any version of this change in the upstream at this time.

eadler added a project: Restricted Project.Sep 15 2016, 6:11 PM
This comment was removed by epriestley.
This comment was removed by epriestley.