Page MenuHomePhabricator

Fix an issue where migrating files could prematurely destroy duplicates
ClosedPublic

Authored by epriestley on Aug 20 2014, 10:01 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 11, 7:30 AM
Unknown Object (File)
Fri, Mar 29, 8:05 AM
Unknown Object (File)
Fri, Mar 29, 8:00 AM
Unknown Object (File)
Jan 29 2024, 11:22 PM
Unknown Object (File)
Jan 6 2024, 6:15 AM
Unknown Object (File)
Dec 23 2023, 11:32 PM
Unknown Object (File)
Dec 1 2023, 9:12 PM
Unknown Object (File)
Nov 30 2023, 5:05 AM

Details

Summary

Fixes T5912. When migrating files, we try to clean up the old data. However, this code isn't aware of reference counting, and unconditionally destroys the old data.

For example, if you migrate files F1 and F2 and they have the same data, we'll delete the shared data when we migrate F1. Then you'll get an error when you migrate F2.

Since this only affects duplicate files, it primarily hits default profile pictures, which are the most numerous duplicate files on most installs.

Test Plan
  • Verified that the theory was correct by uploading two copies of a file and migrating the first one, before applying the patch. The second one's data was nuked and it couldn't be migrated.
  • Applied patch.
  • Uploaded two copies of a new file, migrated the first one (no data deletion), migrated the second one (data correctly deleted).
  • Uploaded two copies of another new file, bin/remove destory'd the first one (no data deletion), then did it to the second one (data correctly deleted).

Diff Detail

Repository
rP Phabricator
Branch
filemig
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 2293
Build 2297: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

epriestley retitled this revision from to Fix an issue where migrating files could prematurely destroy duplicates.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: btrahan.
btrahan edited edge metadata.

Ah, nice catch

This revision is now accepted and ready to land.Aug 20 2014, 10:23 PM
epriestley updated this revision to Diff 24827.

Closed by commit rP66fa59d04d7c (authored by @epriestley).