Cleanup: dead code removal + assorted low-risk correctness fixes#318
Merged
Conversation
c34af32 to
db97a0e
Compare
truncateLogs was wired into performHousekeeping but its reduceEach: returned a merged signal as a value rather than flattening it, so the inner defer block was never subscribed and the file-handle work never executed. The helper it would have called also opened the log with fileHandleForWritingToURL: (O_WRONLY) and then called readDataToEndOfFile, which would raise NSFileHandleOperationException — so making the chain actually run would have turned a no-op into a crash once a ShipIt log crossed 8 MB. Nobody has missed this since it landed, so delete it instead of fixing it.
The JSONFILE manifest transform (extract currentRelease, walk releases,
re-serialize updateTo) lived inside the
'if ([response isKindOfClass:NSHTTPURLResponse.class])' guard. For a
file:// updateRequest NSURLConnection returns a plain NSURLResponse, so
the transform was skipped and the raw manifest was passed straight to
-updateFromJSONData:, which expects the inner {url:...} shape and fails
with SQRLUpdaterErrorInvalidJSON.
Only the RELEASESERVER status-code checks actually need an
NSHTTPURLResponse; move the read-only-volume check and the JSONFILE
manifest transform outside the guard so they run for both transports.
IOPMAssertionCreateWithDescription only writes the out-param on success; on failure the stack uint32_t held garbage and the disposable block unconditionally passed it to IOPMAssertionRelease. Initialize to kIOPMNullAssertionID and skip the release when creation failed. Not unit-testable without faking the IOKit error path.
errno was already snapshotted into 'code' immediately after rmdir() failed and used for strerror(), but the NSError on the next line read live errno which can be clobbered by the intervening dictionary/string allocations. The parallel rename() error path already does this correctly. Diagnostic-only (the error is logged then swallowed).
In the default install path installItemToURL: renames the staged update bundle into place, after which the same staged URL is fed back into deleteOwnedBundleAtURL: for cleanup. removeItemAtURL: failed ENOENT, the error was logged and swallowed, and the chained then: that rmdirs the parent mkdtemp directory never ran. Net effect: one empty 0700 mkdtemp dir under NSTemporaryDirectory() per successful install. Treat NSFileNoSuchFileError as success so the parent rmdir still runs.
The fallback existed for 10.7-era NSDateFormatter where ZZZZZ didn't leniently accept '+HH:MM' offsets, and it carried a 'DD' (day-of-year) typo that would have produced wrong dates anyway. On the current 10.13 deployment target the primary ZZZZZ format already parses '+01:00', '-0700' and 'Z'. New tests cover those shapes explicitly so a regression in formatter leniency would surface.
db97a0e to
00a7b17
Compare
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Batch of small, independent cleanups found during a whole-repo audit. Each is an atomic commit; happy to split any out if preferred.
truncateLogs/backwardTruncateFile—reduceEach:was never followed byflatten, so the inner signal was emitted as a value and never subscribed; the rotation has been a no-op since it landed in 2016. The helper also opened the handle write-only then calledreadDataToEndOfFile, so wiring it up would have thrown rather than rotated.file://feed URLs in JSONFILE mode — the JSONFILE manifest transform sat insideif ([response isKindOfClass:NSHTTPURLResponse.class]), so a local-file feed (which the header docs explicitly mention for testing) skipped the transform and failed withSQRLUpdaterErrorInvalidJSON. Only the HTTP-status checks stay gated now.IOPMAssertionIDon create failure — initialize tokIOPMNullAssertionIDand guard the release. The out-param is documented as written on success only.IOPMAssertionCreateWithDescriptionis called directly inside a static C function with no injection point; making it fail under test would require refactoring the function signature. The fix is mechanical (init + guard).errnosnapshot when building rmdirNSError—code:errno→code:code, matching the parallelrename()error path that already does this.should report the rmdir errno…case belowdeleteOwnedBundleAtURL:now treatsENOENTfromremoveItemAtURL:as success so the chainedrmdirruns. Previously every successful install left one empty 0700 dir underNSTemporaryDirectory()plus a spurious "No such file" log line.-deleteOwnedBundleAtURL:happy path, already-gone path, non-empty parent reportsENOTEMPTY)releaseDatefallback parser — the fallback usedDD(day-of-year) instead ofdd, but the primaryZZZZZformat already accepts every shape it covered on the current deployment target.+0000/-07:00parse via the primary path)