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

From: Han-Wen Nienhuys <hanwen@google.com>
Date: Jun 9, 2021, 11:29 UTC
Message-ID: <CAFQ2z_NQSbshLSstkBM9OZ7XWvydSQ=x0GCHj0a8mf1OC4t2jw@mail.gmail.com>
In-Reply-To: <20210603021356.342665-1-jonathantanmy@google.com>

On Thu, Jun 3, 2021 at 4:14 AM Jonathan Tan <jonathantanmy@google.com> wrote:

Show 12 quoted lines

>
> > v3
> >
> >  * remove errno as an implicit communication mechanism from refs support
> >    completely.
>
> Looking at all the patches, it seems that this patch set is about
> functions in Git code that set errno themselves to indicate the category
> of failure encountered, and instead of setting errno, we want them to
> transmit that information through an out param instead. I notice that
> the cover letter talks about "cleanup" and "remove errno", but that
> could have been explained in greater detail, I think.

What is the right place to document these considerations? The data in the cover letter doesn't end up in the project history, so it seems like the wrong place.

Show 9 quoted lines

> As for whether it is a good idea overall, it could be said that errno is
> idiomatic in C and writing "if (myfunc()) { store_errno = errno; ..." is
> not much more difficult than "int store_errno; if (myfunc(&store_errno))
> { ...". But presumably Han-Wen thinks the opposite, since he wrote this
> patch set, and I'll defer to his opinion on this since he's working on
> the ref code and I'm not. Besides that, having the out param is more
> explicit (which might be better) and permits chaining of such functions
> (e.g. "if (myfunc(&store_errno) || myotherfunc(&store_errno))"). I'll
> review this patch set as if this is the approach we want to take.

it's certainly idiomatic and easy for writing. The problem is that it makes reading code much more difficult: since all of the code has access to errno, it's hard to tell which code depends on certain writes to errno. One signal of this are the extensive discussions in this series about how to argue (in the commit message) that a certain code change is safe.

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