Page MenuHomePhabricator

Don't show error operations after a successful land operation
ClosedPublic

Authored by epriestley on Oct 26 2015, 9:24 PM.
Tags
None
Referenced Files
F18814516: D14348.diff
Mon, Oct 20, 11:25 PM
F18789963: D14348.id34634.diff
Wed, Oct 15, 11:56 AM
F18774543: D14348.id.diff
Fri, Oct 10, 1:46 PM
F18770241: D14348.diff
Wed, Oct 8, 11:30 AM
F18735447: D14348.id.diff
Wed, Oct 1, 1:39 AM
F18701741: D14348.id34634.diff
Sep 27 2025, 8:51 PM
F18605860: D14348.id34635.diff
Sep 13 2025, 9:30 PM
F18533579: D14348.diff
Sep 7 2025, 9:52 AM
Subscribers
None

Details

Summary

Ref T182. When viewing a revision, if there are several error operations and then a success operation, we currently show the last error. This is misleading.

Instead, don't show anything if there's a success (this may require tuning eventually if you can land multiple times onto different branches or whatever, but should be reasonable for now).

Also make the table a little nicer, particularly for merge failure output.

Test Plan

Screen Shot 2015-10-26 at 2.21.22 PM.png (297×897 px, 45 KB)

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Don't show error operations after a successful land operation.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added reviewers: chad, Mnkras.
Mnkras edited edge metadata.
This revision is now accepted and ready to land.Oct 26 2015, 9:25 PM
This revision was automatically updated to reflect the committed changes.
src/applications/differential/controller/DifferentialRevisionViewController.php
1072

I just realized that its possible for $operation to not be set here, this should be moved up into the foreach loop.

src/applications/differential/controller/DifferentialRevisionViewController.php
1072

Operation should always be set here because $operations is guaranteed nonempty (per line 1056) so $operation will leak out of the loop on line 1064 with some value.

Relying on loops leaking variables is arguably bad form, of course.

src/applications/differential/controller/DifferentialRevisionViewController.php
1072

Very true,

You could save a few instructions by throwing this in the loop also :P then no more relying on leaking variables!