Re: [PATCH 2/8] refs/files-backend: stop setting errno from lock_ref_oid_basic
Show 5 quoted lines
> From: Han-Wen Nienhuys <hanwen@google.com>
>
> Errno is a global variable written by almost all system calls, and therefore it
> is hard to reason about its state. It's also useless for user-visible errors, as
> it leaves no place to report the offending file and/or syscall.
I don't think this paragraph is useful.
Show 14 quoted lines
> For the copy/rename support, calls to lock_ref_oid_basic() in this file are
> followed by:
>
> * lock_ref_oid_basic (copy/rename rollback error path)
>
> * write_ref_to_lockfile (both in the rollback path and the success path of
> copy/rename)
>
> These calls do not inspect the incoming errno. As they perform I/O, they can
> clobber errno. For this reason, callers cannot reliably observe the errno that
> lock_ref_oid_basic() generated, so it is unsound for programmatic use.
>
> For files_create_symref() and files_reflog_expire(), grepping over callers
> showed no callers inspecting errno.
This is probably written more clearly as follows:
No call to the static function lock_ref_oid_basic() is immediately
followed by an errno check, so stopping setting errno is safe. But as a
sanity check, lock_ref_oid_basic() is used in these functions:
- files_copy_or_rename_ref() - here, calls are followed by error() (which
performs I/O) or write_ref_to_lockfile() (which calls parse_object() which
may perform I/O)
- files_create_symref() - here, calls are followed by error() or
create_symref_locked() (which performs I/O and does not inspect
errno)
- files_reflog_expire() - here, calls are followed by error() or
refs_reflog_exists() (which calls a function in a vtable that is not
documented to use and/or preserve errno)
So it is safe to stop setting errno in lock_ref_oid_basic().
The diff itself looks good.