mbox series

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

Message ID pull.1012.git.git.1619710329.gitgitgadget@gmail.com (mailing list archive)
Headers show
Series refs: cleanup errno sideband ref related functions | expand

Message

Elijah Newren via GitGitGadget April 29, 2021, 3:32 p.m. UTC
v3

 * remove errno as an implicit communication mechanism from refs support
   completely.

Han-Wen Nienhuys (8):
  refs: remove EINVAL specification from the errno sideband in
    read_raw_ref_fn
  refs/files-backend: stop setting errno from lock_ref_oid_basic
  refs: make errno output explicit for read_raw_ref_fn
  refs: make errno output explicit for refs_resolve_ref_unsafe
  refs: add failure_errno to refs_read_raw_ref() signature
  refs: clear errno return in refs_resolve_ref_unsafe()
  refs: stop setting EINVAL and ELOOP in symref resolution
  refs: explicitly propagate errno from refs_read_raw_ref

 refs.c                | 46 ++++++++++++++++++++--------------
 refs.h                |  1 +
 refs/debug.c          |  4 +--
 refs/files-backend.c  | 58 +++++++++++++++++++------------------------
 refs/packed-backend.c | 16 ++++++------
 refs/refs-internal.h  | 31 +++++++++++++++--------
 6 files changed, 86 insertions(+), 70 deletions(-)


base-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1012%2Fhanwen%2Feinval-sideband-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1012/hanwen/einval-sideband-v1
Pull-Request: https://github.com/git/git/pull/1012

Comments

Jonathan Tan June 3, 2021, 2:13 a.m. UTC | #1
> 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.

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.
Han-Wen Nienhuys June 9, 2021, 11:29 a.m. UTC | #2
On Thu, Jun 3, 2021 at 4:14 AM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> > 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.

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