grailbio / base

A collection of Go utility packages used by GRAIL's tools
Apache License 2.0
89 stars 25 forks source link

Remove `newInfo` because it is dead code #20

Closed jcharum closed 4 years ago

jcharum commented 4 years ago
  1. startGetObjectRequest will only call newInfo if f.info == nil.
  2. startGetObjectRequest is only called by handleRead.
  3. handleRead is only called to handle a readRequest.
  4. readRequest is only issued in (*s3Reader).Read.
  5. The only way to get an *s3Reader is by (*s3File).Reader. It only does this if f.mode == readonly.
  6. The only way to get a readonly *s3File is by (*s3Impl).Open.
  7. (*s3Impl).Open will only return successfully if the handleStat sends a non-error response, via runRequest and statRequest.
  8. handleStat only sends a non-error response after populating f.info with the result of stat, when stat returns a nil error. stat only returns a nil error when the *s3Info it returns is non-nil.
  9. Therefore, f.info != nil in startGetObjectRequest, and it will not call newInfo.

I think the f.info == nil case became obsolete in 3cf2bd3a402b890b1e04641627c2044fb9fe986e, which introduced the statRequest on (*s3Impl).Open.

This was raised and originally reasoned about in https://github.com/grailbio/base/pull/16: https://github.com/grailbio/base/pull/16#issuecomment-622092442.

jcharum commented 4 years ago

This was removed in an internal commit at GRAIL and synced.