Ref T13667. When a revision's diff has no changesets (usually because Diffusion performed an automatic update with an empty commit), the UI currently tries to "min()" an empty array and fatals.
Handle this case properly.
Details
Details
- Created a revision with a diff with no changesets ("git commit --allow-empty" + copy-paste into web UI).
- Viewed revision.
- Before: "min()" fatal.
- After: UI isn't perfect, but works without fataling.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- sub2
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 25649 Build 35477: arc lint + arc unit
Event Timeline
| src/infrastructure/diff/engine/PhabricatorInlineCommentAdjustmentEngine.php | ||
|---|---|---|
| 164 | Should this be null-checked or would PHP do some sort of coercion? From a quick glance through I couldn't be certain that this line would only be hit if $new was valid/present | |
| src/infrastructure/diff/engine/PhabricatorInlineCommentAdjustmentEngine.php | ||
|---|---|---|
| 164 | I thought it was unreachable when I was writing it, but I think you're right that it's not unreachable (or, at least, not obviously unreachable). PHP will type coerce, but I'll make this obviously-correct. The >= test is kind of bad anyway since it's depending on data storage behavior, and we can imagine changes where "ids always go up" might not be true, e.g. possibly "revert to an older version of this change" in Differential. | |
Comment Actions
Use a simpler "is new?" test, by just testing if the changeset ID is in the new list.
- Created a revision with a mixture of nonempty and empty changes.
- Added comments to nonempty changes.
- Interdiffed across various empty/nonempty changesets.
- Got sensible-seeming ghost comment behaviors.
- Empty diff still works fine.
I'm not completely sure these tests are actually equivalent, but couldn't find any behavioral changes, and this code would be more robust if it was not ID-order dependent anyway.