I 100% agree with you that errno is cumbersome to use and carries
far less information than we want (we do not learn what operation
failed on what path) over a long distance. It only is useful when
the callchain still knows what path was operated on.
But...
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> 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.
In the latter part of the above, "callers" refers to the callers of
"the copy/rename support" (aka files_copy_or_rename_ref())?
Then I am not sure why "callers cannot reliably observe the errno
that lock_ref_oid_basic() generated" is a problem. They will see
the errno from the last system call that failed, if they care. So
their performing I/O is perfectly acceptable, too.
Hence, I am not sure what change the above justifies, if any.
If we can show that no caller of files_copy_or_rename_ref() uses
errno, it is a clear indication that lock_ref_oid_basic() is saving
and restoring errno for no good reason. I think that is what was
done for the other two callers below.
So I traced what happens after the copy-rename thing gets called.
refs_rename_ref(), rename_ref(), refs_copy_existing_ref() and
copy_existing_ref() (all in refs.c) should be the only callers of
the function. All callers in builtin/branch.c and builtin/remote.c
of these functions (by the way, refs_X() variants do not seem to be
called from anywhere---are they over-engineered?) just die() when
they signal a failure by returning non-zero, so I think it is safe
and much easier to understand to justify this change like so:
refs/files-backend.c::lock_ref_oid_basic() tries hard to signal
how it failed to its callers using errno. The three callers of
this file-scope static function are
* files_copy_or_rename_ref()
* files_create_symref()
* files_reflog_expire()
None of them looks at errno after seeing a negative return from
lock_ref_oid_basic() to make any decision, and no caller of
these three functions looks at errno after they signal a failure
by returning a negative value.
> For files_create_symref() and files_reflog_expire(), grepping over callers
> showed no callers inspecting errno.
Yes, this is a lot more relevant justification to allow these two
functions, and functions that are called _only_ _by_ these two
functions, stop worrying about errno.
