Re: [PATCH v2 2/8] refs/files-backend: stop setting errno from lock_ref_oid_basic

From: Han-Wen Nienhuys <hanwen@google.com>
Date: Jul 5, 2021, 14:16 UTC
Message-ID: <CAFQ2z_NpyJQLuM70MhJ8K1h2v3QXFuAZRjN=SvSsjnukNRJ8pw@mail.gmail.com>
In-Reply-To: <87k0mae0ga.fsf@evledraar.gmail.com>

On Thu, Jul 1, 2021 at 1:30 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 15 quoted lines

> >       }
> >       if (!resolved) {
> > -             last_errno = errno;
> > +             int last_errno = errno;
> >               if (last_errno != ENOTDIR ||
> >                   !refs_verify_refname_available(&refs->base, refname,
> >                                                  extras, skip, err))
>
> ...this particular change gives me some pause, because all the rest is
> about squirreling away our own errno for our own caller (which it turns
> out, we didn't need).
>
> But in this case we're only guarding against
> refs_verify_refname_available() possibly clobbering the errno we just
> got on !resolved in refs_verify_refname_available().

This comes from 5b2d8d6f2184381b76c13504a2f5ec8a62cd584e

   .. invoke verify_refname_available() to try
    to generate a more helpful error message.

    That function might not detect an error. For example, some
    non-reference file might be blocking the deletion of an
    otherwise-empty directory tree, or there might be a race with another
    process that just deleted the offending reference. In such cases,
    generate the strerror-based error message like before.

I think we want to keep this behavior, hence I think this should be kept as is.

I'll add a comment.

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