chore: defer file closes

[Haroka-74]

What?

Replaced manual Close() calls on every error path in the binary copy block of runUpdateCLI with defer, and report close errors on out via the named return value.

Why?

The previous manual Close() calls on every return path were fragile and silently discarded flush errors on out.

Closes #1056

[Haroka-74]

@andrinoff

I need to change this if err := os.Rename(tmpNew, execPath); err != nil { to if err = os.Rename(tmpNew, execPath); err != nil {.

Am I right?

[andrinoff]

@andrinoff

I need to change this if err := os.Rename(tmpNew, execPath); err != nil { to if err = os.Rename(tmpNew, execPath); err != nil {.

Am I right?

I dont think so, although there could be a Windows-specific issue.
But it could be non-important and I couldn't get it to fail

[Haroka-74]

Anyway, I'll change it for consistency.

[andrinoff]

i'll check it out later (possibly tomorrow) @Haroka-74

[andrinoff]

@Haroka-74 fix the comments

[andrinoff]

Not sure what is the reason for this implementation? this is heavily unnecessary

[andrinoff]

if removed from the above, this has to be a declaration (:=)

[Haroka-74]

Hey @andrinoff, just to clarify why the named return is necessary.

The original issue asked for two things:

• Use defer for both files
• Report close errors (especially on out, where a close error is the only signal that pending data was actually flushed)

To satisfy point 2, the defer needs a way to propagate the close error back to the caller. The only way to do that from a defer is via a named return:

func runUpdateCLI() (err error) {
    // ...
    defer func() {
        if cerr := out.Close(); cerr != nil && err == nil {
            err = fmt.Errorf("could not flush new binary to disk: %w", cerr)
        }
    }()
    // ...
}

Without the named return, the defer has no way to surface the close error to the caller — it would just be silently dropped, which is exactly what the issue was complaining about.

Let me know what you think, happy to discuss if you have a different approach in mind!

[andrinoff]

An IIFE removes the need for a named return entirely. Each file gets its own defer scoped to the closure, and errors are returned through a normal local variable:

example:

var copyErr error
func() {
    defer in.Close()
    defer func() {
        if cerr := out.Close(); copyErr == nil && cerr != nil {
            copyErr = fmt.Errorf("could not flush new binary to disk: %w", cerr)
        }
    }()
    if _, copyErr = io.Copy(out, in); copyErr != nil {
        copyErr = fmt.Errorf("could not write new binary to disk: %w", copyErr)
    }
}()
if copyErr != nil {
    return copyErr
}

[andrinoff]

lgtm

[andrinoff]

i'll merge the PR, it is good enough and pretty clean, though i do not like the implementation entirely

[andrinoff]

/approve

[floatpanebot]

Approved on behalf of @andrinoff via /approve command.

[Haroka-74]

I can change the implementation if you’d like.

[andrinoff]

I can change the implementation if you’d like.

already merged and releasing, if the issue comes up, i'll link this PR