Page MenuHomePhabricator

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

Authored by epriestley on Apr 20 2022, 4:37 PM.



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

rP Phabricator
Lint Not Applicable
Tests Not Applicable

Event Timeline

epriestley created this revision.
cspeckmim added inline comments.

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.

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.