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