Re: [PATCH v3 0/5] refs: cleanup errno sideband ref related functions

On Tue, Jul 6, 2021 at 4:37 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:

quoted

I've added a failure_errno argument parse_loose_ref_contents now.

Great, it's not clear if you picked up the full diff-on-top but I think
consistently using *failure_errno in files_read_raw_ref() instead of
"errno" and the "*failure_errno = errno" at the end also makes the code
much more readable (and allows for the removal of the "saved_errno" we
don't want).

It's hard to get right: there are many system calls, and in most cases
EISDIR/ENOTDIR have to be propagated to the caller. I tried doing
this, and promptly got a failing test related to dir/file conflicts.
By doing

  *failure_errno = errno

near the exit of files_read_raw_ref(), we can be sure there are no
changes in behavior.  If anyone wants to understand the files backend
in more depth, they're welcome to disentangle this in a follow-up
patch.

By using "errno" itself for the body of the function it needs to be
really carefully read to assure oneself that one of the functions it
calls doesn't make a syscall, and even if we're assured of that now if
one of them has a new syscall added in the future it might be clobbered
at a distance.

Yes, anyone adding a syscall to files_read_raw_ref has to be very careful.

-- 
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