deploy: Don't leak file descriptor passed for local rebases

[cgwalters]

Came up in #2210

[cgwalters]

(Only compile tested)

[lucab]

I've just read the fd-stealing internals behind this and I'm somehow amazed. It seems to be using errno for error-signaling, is it ok to just ignore errors or is some logging customary? Other than that, LGTM.

[jlebon]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jlebon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [cgwalters,jlebon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jlebon]

I've just read the fd-stealing internals behind this and I'm somehow amazed. It seems to be using errno for error-signaling, is it ok to just ignore errors or is some logging customary?

Yeah... we don't do any logging right now. You're right we probably should in general log something in case it helps to debug some higher-level issues (at least in write contexts, if not in read contexts like this one).

[cgwalters]

It seems to be using errno for error-signaling

That's how libc works right?

is it ok to just ignore errors or is some logging customary?

Ah well...see, close() is special. In fact, notice Rust today only exposes close() via impl Drop for File {} (i.e. implicitly)...and doesn't by default check errors either:
https://github.com/rust-lang/rust/blob/71f8d0c8f1060bbe74100f29cc6f2da63d697c28/library/std/src/sys/unix/fd.rs#L274

This is...mostly OK as long as you don't have any buffering involved. But IMO it is an evil trap today in Rust how one can create a BufWriter() and not be required to call flush() explicitly and have that happen at drop where you can't check errors.

[cgwalters]

See also https://lkml.org/lkml/2005/9/11/49 and https://lwn.net/Articles/576478/