Subject: Re: [PATCH 2/8] refs/files-backend: stop setting errno from lock_ref_oid_basic

From: Junio C Hamano <[email-masked]>
Date: Fri, 30 Apr 2021 12:10:43 +0900

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" <[email-masked]> 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.