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

[Han-Wen Nienhuys <hanwen@google.com>]

On Fri, Apr 30, 2021 at 5:10 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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:
>
> > 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.

I stole your message here; hope that's OK. My original message tries
to convey that if you do

/* should return errno */
int a() { .. }

int b() {
   result = a();
   maybe_do_IO();
   return result;
}

then callers of b() can't reason about the errno result of a(),
because they can't know if an error code was generated by
maybe_do_IO() or a(). This means that the errno result of a() is
useless.  (This is assuming that b() doesn't inspect errno, which I
failed to mention.)

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado