pack/set.go: close potentially-opened index files #9
Conversation
In [1], we matched upstream Git's behavior more closely by (1) first opening the corresponding *.idx, and _then_ opening the associated *.pack, skipping over any pairs which had missing/corrupt index files. In this commit, augment that behavior by closing a file descriptor we may have opened, but is otherwise deemed by os.Open to be unusable for one reason or another. This path is unlikely in practice, but is a safe change nonetheless. [1]: 561ed22 (pack/set: ignore packs without indices, 2018-11-29)
| // We have a pack (since it matched the regex), but the | ||
| // index is missing or unusable. Skip this pack and | ||
| // continue on with the next one, as Git does. | ||
| if idxf != nil { |
There was a problem hiding this comment.
I don't think we can take this branch, since (at least from my reading of the Go source for Unix and Windows), we are guaranteed that if err is not nil, then idxf is nil (and vice versa).
Having said that, I'm not opposed to being careful here if we think that might change in the future.
There was a problem hiding this comment.
Having said that, I'm not opposed to being careful here if we think that might change in the future.
This is what motivated that change here (though I should have mentioned so explicitly in my original patch, and I am sure that there are many places throughout the code that don't do this).
I agree with your reading of the implementation, but I am hesitant to drop this patch based on that alone. Since the documentation doesn't promise anything about the relation between the *os.File and error returned, I don't think that we can rely on the source alone.
In 1, we matched upstream Git's behavior more closely by (1) first
opening the corresponding *.idx, and then opening the associated
*.pack, skipping over any pairs which had missing/corrupt index files.
In this commit, augment that behavior by closing a file descriptor we
may have opened, but is otherwise deemed by os.Open to be unusable for
one reason or another.
This path is unlikely in practice, but is a safe change nonetheless.
/cc @git-lfs/core
/cc #8