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