Revert "Remove tempfile from external diff creation"
What does this MR do?
Revert !35750 (merged)
We're seeing failures creating MR diffs correctly that are correlated with the deployment of this MR into gstg and gprd. #225839 (comment 374313754) for more details on that.
When the size of the external diff StringIO is 0, carrierwave, or our ObjectStorage concern, seems to behave differently. I think this may be because CarrierWave::SanitizedFile#empty? starts returning true in this case rather than false: https://github.com/carrierwaveuploader/carrierwave/blob/v1.3.1/lib/carrierwave/sanitized_file.rb#L141
When we use a tempfile, exists? returns true, so we say we're not empty. When we don't, exists? returns false, so we say we're empty.
I'm still not at the bottom of it, but I don't see a nice way to get around this, so a revert seems like the correct decision.
The upshot is that we never call store! on our uploader.
I'll post a follow-up MR with specs I developed while chasing this problem. We should be able to get those merged to prevent a future regression.
Screenshots
Does this MR meet the acceptance criteria?
Conformity
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
-
Label as security and @ mention @gitlab-com/gl-security/appsec -
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team