Skip to content

pack/set.go: close potentially-opened index files #9

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ttaylorr merged 1 commit into from
Jan 23, 2019
Merged

pack/set.go: close potentially-opened index files #9

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions pack/set.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ func NewSet(db string) (*Set, error) {
// 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 {
Copy link
Member

@bk2204 bk2204 Jan 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ttaylorr ttaylorr Jan 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@bk2204 bk2204 Jan 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I think that's fair.

// In the unlikely event that we did open a
// file, close it, but discard any error in
// doing so.
idxf.Close()
}
continue
}

Expand Down