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

From: Han-Wen Nienhuys <hanwen@google.com>
Date: Jul 6, 2021, 09:53 UTC
Message-ID: <CAFQ2z_N_J499X_wd4RGR7jobs3O3NvQuobJ9UcQijkNYN712dw@mail.gmail.com>
In-Reply-To: <87v95o5ku8.fsf@evledraar.gmail.com>

On Tue, Jul 6, 2021 at 2:48 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 7 quoted lines

>
>
> On Mon, Jul 05 2021, Han-Wen Nienhuys via GitGitGadget wrote:
>
> > v5
>
> v3?

I erroneously closed out the github PR I used for the first 2 versions of this change series. (https://github.com/git/git/pull/1011)

Show 17 quoted lines

> >  * address Ævar's comment; punt on clearing errno.
> > [...]
> >
> >           Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> >           Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
> >      +    Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>
> FWIW per Documentation/SubmittingPatches:
>
>     . `Reviewed-by:`, unlike the other tags, can only be offered by the
>       reviewer and means that she is completely satisfied that the patch
>       is ready for application.  It is usually offered only after a
>       detailed review.
>
> It's not that I'm hard to please, but I can honestly say that I don't
> quite understand some parts of what you're gonig for, so that trailer is
> probably premature :)

oh, ok. Removed them.

Does that mean you'd have to repost my patchseries just to add the reviewed-by header?

Show 13 quoted lines

> To rephrase my comment on the v2 to hopefully better get at the
> point/question I had.
>
> It wasn't that I don't get why you wouldn't save/restore errno in
> general.
>
> It's that the pattern of doing so seems backwards to me. I.e. surely the
> goal here should be to one function at a time, and from the bottom-up,
> figure out where we rely on "errno" and convert that to a
> "failure_errno".
>
> Instead not even files_read_raw_ref() resets "errno = 0" at the end, so
> the errno /there/ can propagate upwards, and in this v3 we're not clearing it at all.

I would really want errno to be an explicit output for more functions in refs.h, but I don't have the time to see that through, and it's a distraction from the reftable work, so I have changed the objective of the patch series.

I'm limiting myself to explicitly propagating errno values that are used beyond error reporting. The EINVAL value for parse_loose_ref_contents is a borderline case: it has an extra "if ()" body in lock_raw_ref, but it's for an error message.

This means that we should avoid clearing errno, because we'd otherwise have to track down all the places where it's being put into strerror(). The one case you found earlier is still there, and by not clearing errno, I can keep this series smaller by not also having to rework verify_lock/read_refs_full.

In general, it's frustrating to observe that the files backend is a complex beast that nobody understands, but in order to replace it with something more principled, I have to spend lots of time cleaning it up.

> Having dug a bit further, it seems what you're doing, whether it's
> intentional or not, is relying on the parse_loose_ref_contents() setting
> EINVAL, but you clobber your *failure_errno with it whether it returned
> -1 or not.

I've added a failure_errno argument parse_loose_ref_contents now.

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