Page MenuHomePhabricator
Create Task
Maniphest T11608

Make it easier to review stacks of diffs
Closed, WontfixPublic
Actions

Assigned To
epriestley
Authored By
starruler
Sep 8 2016, 10:24 PM
Tags
Referenced Files
F1817221: feels.png
Sep 13 2016, 8:57 PM
F1805290: Screen Shot 2016-09-08 at 4.25.21 PM.png
Sep 8 2016, 11:26 PM
F1805238: pasted_file
Sep 8 2016, 10:39 PM
F1805218: Screen Shot 2016-09-08 at 2.31.10 PM.png
Sep 8 2016, 10:24 PM
Subscribers
• ashtonwalles
avivey
• brucelee001
cspeckmim
eadler
epriestley
girtsf
View All 10 Subscribers

Description

Screen Shot 2016-09-08 at 2.31.10 PM.png (288×1 px, 52 KB)

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.

Event Timeline

starruler created this task.Sep 8 2016, 10:24 PM
Herald added a subscriber: eadler. · View Herald TranscriptSep 8 2016, 10:24 PM
starruler added a comment.Sep 8 2016, 10:26 PM

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

pasted_file (205×449 px, 22 KB)
?

starruler added a comment.Sep 8 2016, 10:44 PM

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.

epriestley added a subscriber: epriestley.Sep 8 2016, 11:26 PM

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

Screen Shot 2016-09-08 at 4.25.21 PM.png (141×431 px, 18 KB)

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

epriestley added a comment.Sep 8 2016, 11:28 PM

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

epriestley added a comment.Sep 8 2016, 11:29 PM

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

starruler added a comment.Sep 9 2016, 5:14 AM

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

fooishbar added a subscriber: fooishbar.Sep 9 2016, 10:27 AM
joshuaspence added a subscriber: joshuaspence.Sep 10 2016, 12:39 AM
starruler added a comment.Sep 13 2016, 7:01 AM

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?

epriestley added a comment.Sep 13 2016, 4:21 PM

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

starruler added a comment.Sep 13 2016, 8:57 PM

feels.png (2×2 px, 225 KB)

eadler added a project: Restricted Project.Sep 15 2016, 6:11 PM
• ashtonwalles added a subscriber: • ashtonwalles.Jan 24 2017, 7:44 AM
This comment was removed by epriestley.
• brucelee001 added a subscriber: • brucelee001.Apr 1 2017, 4:42 AM
This comment was removed by epriestley.
girtsf added a subscriber: girtsf.Jun 28 2017, 9:29 PM
fooishbar removed a subscriber: fooishbar.Sep 27 2017, 5:56 PM
• pasik added a subscriber: • pasik.Mar 9 2019, 8:26 PM
epriestley closed this task as Wontfix.Mar 15 2021, 5:14 PM
epriestley claimed this task.

This is very old, fairly vague, and doesn't have any clear modern customer interest. I'm open to implementing it, but would like a more clear/modern customer request before pursuing it.

Phabricator · Built by Phacility