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

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

On Tue, Jul 6, 2021 at 2:48 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
>
> 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)

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

> 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