Fixed bingo get fails on Windows #155
Conversation
| @@ -200,7 +200,9 @@ func get(ctx context.Context, logger *log.Logger, c getConfig, rawTarget string) | |||
| if err != nil { | |||
| return errors.Wrapf(err, "found unparsable mod file %v. Uninstall it first via get %v@none or fix it manually.", e, name) | |||
| } | |||
| defer errcapture.Do(&err, mf.Close, "close") | |||
| if err := mf.Close(); err != nil { | |||
There was a problem hiding this comment.
Thanks! Can we ensure we don't use mf variable after Close? Here we can do easily with dpkg := *mf.DirectPackage() and then close and then append to target (:
| @@ -260,7 +262,9 @@ func get(ctx context.Context, logger *log.Logger, c getConfig, rawTarget string) | |||
| if err != nil { | |||
| return errors.Wrapf(err, "found unparsable mod file %v. Uninstall it first via get %v@none or fix it manually.", e, name) | |||
| } | |||
| defer errcapture.Do(&err, mf.Close, "close") | |||
| if err := mf.Close(); err != nil { | |||
There was a problem hiding this comment.
ditto. We can't use mf after Close even if by accident in this code we can.
| @@ -652,6 +663,12 @@ func getPackage(ctx context.Context, logger *log.Logger, c installPackageConfig, | |||
| return err | |||
| } | |||
|
|
|||
| // We have close it once here so the defer func does not need to close it again. | |||
| closed = true | |||
| if err := tmpModFile.Close(); err != nil { | |||
There was a problem hiding this comment.
Same here, it's not acceptable to use tmpModFile var that is closed. It might work by accident now, but will cause surprises later on.
Why not closing right before os.Rename (and saving tmpModFile fileapath to variable?)
@bwplotka Thanks for the suggestions! I've already made the changes, please take a look and let me know if there's anything else need to improve! 🙏🏽 |
This PR is to make sure opened files are closed immediately (instead of at defer) if we don't need it anymore.
On Windows an error occurs when we try to rename from/to an opened files. Calling
os.Rename(old, new)we should make sure to close botholdandnewfiles (ifnewis existed and opened).Fixes #127