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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 30, 2021, 03:10 UTC
Message-ID: <xmqqbl9weabw.fsf@gitster.g>
In-Reply-To: <b2c72097e5e8985e7fdd8e3eee66cdf43d1b65c0.1619710329.git.gitgitgadget@gmail.com>

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@gmail.com> writes:

Show 11 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.

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.