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
F12847544: D21760.id51877.diff
Fri, Mar 29, 2:24 AM
Unknown Object (File)
Wed, Mar 13, 1:16 AM
Unknown Object (File)
Mon, Mar 11, 12:19 PM
Unknown Object (File)
Wed, Mar 6, 9:07 PM
Unknown Object (File)
Feb 7 2024, 11:31 PM
Unknown Object (File)
Feb 4 2024, 1:42 AM
Unknown Object (File)
Feb 4 2024, 1:37 AM
Unknown Object (File)
Jan 15 2024, 2:19 PM
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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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.