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

On Tue, Jul 6, 2021 at 2:48 AM Ævar Arnfjörð Bjarmason [off-list ref] wrote:

On Mon, Jul 05 2021, Han-Wen Nienhuys via GitGitGadget wrote:

quoted

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)

quoted

 * address Ævar's comment; punt on clearing errno.
[...]

          Signed-off-by: Han-Wen Nienhuys [off-list ref]
          Reviewed-by: Jonathan Tan [off-list ref]
     +    Reviewed-by: Ævar Arnfjörð Bjarmason [off-list ref]

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