Page MenuHomePhabricator

Fix an issue where we may "min()" an empty array when viewing a revision with no changesets
ClosedPublic

Authored by epriestley on Apr 20 2022, 4:37 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jul 2, 2:18 AM
Unknown Object (File)
Fri, Jul 1, 7:41 AM
Unknown Object (File)
Fri, Jul 1, 7:41 AM
Unknown Object (File)
Fri, Jul 1, 7:41 AM
Unknown Object (File)
Fri, Jul 1, 7:40 AM
Unknown Object (File)
Wed, Jun 29, 9:45 PM
Unknown Object (File)
Wed, Jun 29, 9:12 AM
Unknown Object (File)
Sun, Jun 26, 3:36 AM
Subscribers

Details

Summary

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.

Test Plan
  • 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

Repository
rP Phabricator
Branch
sub2
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 25649
Build 35477: arc lint + arc unit

Event Timeline

epriestley created this revision.
cspeckmim added inline comments.
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

epriestley added inline comments.
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.

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.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 20 2022, 8:04 PM
This revision was automatically updated to reflect the committed changes.