git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Han-Wen Nienhuys <hanwen@google.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: Han-Wen Nienhuys via GitGitGadget <gitgitgadget@gmail.com>,
	git <git@vger.kernel.org>, Han-Wen Nienhuys <hanwenn@gmail.com>
Subject: Re: [PATCH 0/8] refs: cleanup errno sideband ref related functions
Date: Wed, 9 Jun 2021 13:29:32 +0200	[thread overview]
Message-ID: <CAFQ2z_NQSbshLSstkBM9OZ7XWvydSQ=x0GCHj0a8mf1OC4t2jw@mail.gmail.com> (raw)
In-Reply-To: <20210603021356.342665-1-jonathantanmy@google.com>

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.


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

  reply	other threads:[~2021-06-09 11:29 UTC|newest]

Thread overview: 148+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-29 15:32 Han-Wen Nienhuys via GitGitGadget
2021-04-29 15:32 ` [PATCH 1/8] refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
2021-04-30  2:38   ` Junio C Hamano
2021-05-19 12:25     ` Han-Wen Nienhuys
2021-06-03  2:19   ` Jonathan Tan
2021-06-09 11:28     ` Han-Wen Nienhuys
2021-04-29 15:32 ` [PATCH 2/8] refs/files-backend: stop setting errno from lock_ref_oid_basic Han-Wen Nienhuys via GitGitGadget
2021-04-30  3:10   ` Junio C Hamano
2021-05-19 12:29     ` Han-Wen Nienhuys
2021-06-03  2:33   ` Jonathan Tan
2021-06-10 10:02     ` Han-Wen Nienhuys
2021-04-29 15:32 ` [PATCH 3/8] refs: make errno output explicit for read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
2021-04-30  3:34   ` Junio C Hamano
2021-04-30  6:02     ` Junio C Hamano
2021-05-19 12:33       ` Han-Wen Nienhuys
2021-06-03  2:37   ` Jonathan Tan
2021-06-10 10:05     ` Han-Wen Nienhuys
2021-04-29 15:32 ` [PATCH 4/8] refs: make errno output explicit for refs_resolve_ref_unsafe Han-Wen Nienhuys via GitGitGadget
2021-06-03  2:51   ` Jonathan Tan
2021-06-10 11:27     ` Han-Wen Nienhuys
2021-04-29 15:32 ` [PATCH 5/8] refs: add failure_errno to refs_read_raw_ref() signature Han-Wen Nienhuys via GitGitGadget
2021-04-29 15:32 ` [PATCH 6/8] refs: clear errno return in refs_resolve_ref_unsafe() Han-Wen Nienhuys via GitGitGadget
2021-06-03  2:53   ` Jonathan Tan
2021-06-10 11:45     ` Han-Wen Nienhuys
2021-04-29 15:32 ` [PATCH 7/8] refs: stop setting EINVAL and ELOOP in symref resolution Han-Wen Nienhuys via GitGitGadget
2021-06-03  2:55   ` Jonathan Tan
2021-06-10 11:58     ` Han-Wen Nienhuys
2021-04-29 15:32 ` [PATCH 8/8] refs: explicitly propagate errno from refs_read_raw_ref Han-Wen Nienhuys via GitGitGadget
2021-06-03  2:13 ` [PATCH 0/8] refs: cleanup errno sideband ref related functions Jonathan Tan
2021-06-09 11:29   ` Han-Wen Nienhuys [this message]
2021-06-10 12:57 ` [PATCH v2 " Han-Wen Nienhuys via GitGitGadget
2021-06-10 12:57   ` [PATCH v2 1/8] refs: remove EINVAL errno output from specification of read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
2021-06-10 12:57   ` [PATCH v2 2/8] refs/files-backend: stop setting errno from lock_ref_oid_basic Han-Wen Nienhuys via GitGitGadget
2021-07-01 11:13     ` Ævar Arnfjörð Bjarmason
2021-07-05 14:16       ` Han-Wen Nienhuys
2021-06-10 12:57   ` [PATCH v2 3/8] refs: make errno output explicit for read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
2021-07-01 11:34     ` Ævar Arnfjörð Bjarmason
2021-07-05 14:34       ` Han-Wen Nienhuys
2021-06-10 12:57   ` [PATCH v2 4/8] refs: make errno output explicit for refs_resolve_ref_unsafe Han-Wen Nienhuys via GitGitGadget
2021-07-01 11:56     ` Ævar Arnfjörð Bjarmason
2021-06-10 12:57   ` [PATCH v2 5/8] refs: use refs_resolve_ref_unsafe_with_errno() where needed Han-Wen Nienhuys via GitGitGadget
2021-07-01 11:58     ` Ævar Arnfjörð Bjarmason
2021-06-10 12:57   ` [PATCH v2 6/8] refs: add failure_errno to refs_read_raw_ref() signature Han-Wen Nienhuys via GitGitGadget
2021-07-01 12:06     ` Ævar Arnfjörð Bjarmason
2021-06-10 12:57   ` [PATCH v2 7/8] refs: clear errno return in refs_resolve_ref_unsafe() Han-Wen Nienhuys via GitGitGadget
2021-07-01 12:19     ` Ævar Arnfjörð Bjarmason
2021-06-10 12:57   ` [PATCH v2 8/8] refs: explicitly propagate errno from refs_read_raw_ref Han-Wen Nienhuys via GitGitGadget
2021-07-01 12:26     ` Ævar Arnfjörð Bjarmason
2021-07-05 16:09       ` Han-Wen Nienhuys
2021-07-05 19:08         ` Ævar Arnfjörð Bjarmason
2021-07-05 19:39           ` Han-Wen Nienhuys
2021-06-14 10:10   ` [PATCH v2 0/8] refs: cleanup errno sideband ref related functions Han-Wen Nienhuys
2021-07-05 20:56   ` [PATCH v3 0/5] " Han-Wen Nienhuys via GitGitGadget
2021-07-05 20:56     ` [PATCH v3 1/5] refs: remove EINVAL errno output from specification of read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
2021-07-05 20:56     ` [PATCH v3 2/5] refs/files-backend: stop setting errno from lock_ref_oid_basic Han-Wen Nienhuys via GitGitGadget
2021-07-05 20:56     ` [PATCH v3 3/5] refs: make errno output explicit for read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
2021-07-05 20:56     ` [PATCH v3 4/5] refs: add failure_errno to refs_read_raw_ref() signature Han-Wen Nienhuys via GitGitGadget
2021-07-06 19:28       ` Junio C Hamano
2021-07-05 20:56     ` [PATCH v3 5/5] refs: make errno output explicit for refs_resolve_ref_unsafe Han-Wen Nienhuys via GitGitGadget
2021-07-06  0:38     ` [PATCH v3 0/5] refs: cleanup errno sideband ref related functions Ævar Arnfjörð Bjarmason
2021-07-06  9:53       ` Han-Wen Nienhuys
2021-07-06 14:27         ` Ævar Arnfjörð Bjarmason
2021-07-06 18:36           ` Han-Wen Nienhuys
2021-07-06 18:55     ` [PATCH v4 0/6] " Han-Wen Nienhuys via GitGitGadget
2021-07-06 18:55       ` [PATCH v4 1/6] refs: remove EINVAL errno output from specification of read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
2021-07-06 18:55       ` [PATCH v4 2/6] refs/files-backend: stop setting errno from lock_ref_oid_basic Han-Wen Nienhuys via GitGitGadget
2021-07-06 18:55       ` [PATCH v4 3/6] refs: make errno output explicit for read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
2021-07-06 18:55       ` [PATCH v4 4/6] refs: add failure_errno to refs_read_raw_ref() signature Han-Wen Nienhuys via GitGitGadget
2021-07-06 19:39         ` Junio C Hamano
2021-07-06 18:55       ` [PATCH v4 5/6] refs: explicitly return failure_errno from parse_loose_ref_contents Han-Wen Nienhuys via GitGitGadget
2021-07-06 19:37         ` Junio C Hamano
2021-07-07  8:20           ` Han-Wen Nienhuys
2021-07-06 18:55       ` [PATCH v4 6/6] refs: make errno output explicit for refs_resolve_ref_unsafe Han-Wen Nienhuys via GitGitGadget
2021-07-07 19:07       ` [PATCH v5 0/6] refs: cleanup errno sideband ref related functions Han-Wen Nienhuys via GitGitGadget
2021-07-07 19:07         ` [PATCH v5 1/6] refs: remove EINVAL errno output from specification of read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
2021-07-07 19:07         ` [PATCH v5 2/6] refs/files-backend: stop setting errno from lock_ref_oid_basic Han-Wen Nienhuys via GitGitGadget
2021-07-11 11:38           ` Ævar Arnfjörð Bjarmason
2021-07-13  8:00             ` Han-Wen Nienhuys
2021-07-07 19:07         ` [PATCH v5 3/6] refs: make errno output explicit for read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
2021-07-11 11:52           ` Ævar Arnfjörð Bjarmason
2021-07-07 19:07         ` [PATCH v5 4/6] refs: add failure_errno to refs_read_raw_ref() signature Han-Wen Nienhuys via GitGitGadget
2021-07-11 11:59           ` Ævar Arnfjörð Bjarmason
2021-07-13  8:02             ` Han-Wen Nienhuys
2021-07-07 19:07         ` [PATCH v5 5/6] refs: explicitly return failure_errno from parse_loose_ref_contents Han-Wen Nienhuys via GitGitGadget
2021-07-11 12:41           ` Ævar Arnfjörð Bjarmason
2021-07-07 19:07         ` [PATCH v5 6/6] refs: make errno output explicit for refs_resolve_ref_unsafe Han-Wen Nienhuys via GitGitGadget
2021-07-07 20:44         ` [PATCH v5 0/6] refs: cleanup errno sideband ref related functions Junio C Hamano
2021-07-11 16:30         ` [PATCH v6? 00/17] refs API: get rid of errno setting entirely Ævar Arnfjörð Bjarmason
2021-07-11 16:30           ` [PATCH v6? 01/17] refs: remove EINVAL errno output from specification of read_raw_ref_fn Ævar Arnfjörð Bjarmason
2021-07-11 16:30           ` [PATCH v6? 02/17] refs/files-backend: stop setting errno from lock_ref_oid_basic Ævar Arnfjörð Bjarmason
2021-07-11 16:30           ` [PATCH v6? 03/17] refs: make errno output explicit for read_raw_ref_fn Ævar Arnfjörð Bjarmason
2021-07-11 16:30           ` [PATCH v6? 04/17] refs: add failure_errno to refs_read_raw_ref() signature Ævar Arnfjörð Bjarmason
2021-07-11 16:30           ` [PATCH v6? 05/17] refs: explicitly return failure_errno from parse_loose_ref_contents Ævar Arnfjörð Bjarmason
2021-07-11 16:30           ` [PATCH v6? 06/17] refs: make errno output explicit for refs_resolve_ref_unsafe Ævar Arnfjörð Bjarmason
2021-07-11 16:30           ` [PATCH v6? 07/17] refs: make errno ignoring explicit in lock_ref_oid_basic() Ævar Arnfjörð Bjarmason
2021-07-11 16:30           ` [PATCH v6? 08/17] refs file-backend.c: stop setting "EBUSY" in verify_lock() Ævar Arnfjörð Bjarmason
2021-07-13  8:08             ` Han-Wen Nienhuys
2021-07-11 16:30           ` [PATCH v6? 09/17] refs file-backend.c: deal with errno directly " Ævar Arnfjörð Bjarmason
2021-07-11 16:30           ` [PATCH v6? 10/17] refs API: remove refs_read_ref_full() wrapper Ævar Arnfjörð Bjarmason
2021-07-11 16:30           ` [PATCH v6? 11/17] refs API: make resolve_gitlink_ref() not set errno Ævar Arnfjörð Bjarmason
2021-07-11 16:30           ` [PATCH v6? 12/17] refs API: make refs_resolve_ref_unsafe() static Ævar Arnfjörð Bjarmason
2021-07-11 16:30           ` [PATCH v6? 13/17] refs API: make refs_resolve_refdup() not set errno Ævar Arnfjörð Bjarmason
2021-07-11 16:30           ` [PATCH v6? 14/17] refs API: make refs_ref_exists() " Ævar Arnfjörð Bjarmason
2021-07-11 16:30           ` [PATCH v6? 15/17] refs API: make resolve_ref_unsafe() " Ævar Arnfjörð Bjarmason
2021-07-13  8:13             ` Han-Wen Nienhuys
2021-07-14  8:32               ` Ævar Arnfjörð Bjarmason
2021-07-11 16:30           ` [PATCH v6? 16/17] refs API: make expand_ref() and repo_dwim_log() " Ævar Arnfjörð Bjarmason
2021-07-11 16:30           ` [PATCH v6? 17/17] refs API: don't leak "errno" in run_transaction_hook() Ævar Arnfjörð Bjarmason
2021-07-13  8:28           ` [PATCH v6? 00/17] refs API: get rid of errno setting entirely Han-Wen Nienhuys
2021-07-13 18:26             ` Ævar Arnfjörð Bjarmason
2021-07-14  8:38               ` Ævar Arnfjörð Bjarmason
2021-07-14  8:43                 ` Han-Wen Nienhuys
2021-07-14 11:43           ` [PATCH v7 0/6] refs: cleanup errno sideband ref related functions Ævar Arnfjörð Bjarmason
2021-07-14 11:43             ` [PATCH v7 1/6] refs: remove EINVAL errno output from specification of read_raw_ref_fn Ævar Arnfjörð Bjarmason
2021-07-14 11:43             ` [PATCH v7 2/6] refs/files-backend: stop setting errno from lock_ref_oid_basic Ævar Arnfjörð Bjarmason
2021-07-14 11:43             ` [PATCH v7 3/6] refs: make errno output explicit for read_raw_ref_fn Ævar Arnfjörð Bjarmason
2021-07-14 11:43             ` [PATCH v7 4/6] refs: add failure_errno to refs_read_raw_ref() signature Ævar Arnfjörð Bjarmason
2021-07-14 11:43             ` [PATCH v7 5/6] refs: explicitly return failure_errno from parse_loose_ref_contents Ævar Arnfjörð Bjarmason
2021-07-14 11:43             ` [PATCH v7 6/6] refs: make errno output explicit for refs_resolve_ref_unsafe Ævar Arnfjörð Bjarmason
2021-07-16 14:22             ` [PATCH v8 0/7] refs: cleanup errno sideband ref related functions Ævar Arnfjörð Bjarmason
2021-07-16 14:22               ` [PATCH v8 1/7] refs file backend: move raceproof_create_file() here Ævar Arnfjörð Bjarmason
2021-07-16 14:22               ` [PATCH v8 2/7] refs: remove EINVAL errno output from specification of read_raw_ref_fn Ævar Arnfjörð Bjarmason
2021-07-16 14:22               ` [PATCH v8 3/7] refs/files-backend: stop setting errno from lock_ref_oid_basic Ævar Arnfjörð Bjarmason
2021-07-16 14:22               ` [PATCH v8 4/7] refs: make errno output explicit for read_raw_ref_fn Ævar Arnfjörð Bjarmason
2021-07-16 14:22               ` [PATCH v8 5/7] refs: add failure_errno to refs_read_raw_ref() signature Ævar Arnfjörð Bjarmason
2021-07-16 14:22               ` [PATCH v8 6/7] refs: explicitly return failure_errno from parse_loose_ref_contents Ævar Arnfjörð Bjarmason
2021-07-16 14:22               ` [PATCH v8 7/7] refs: make errno output explicit for refs_resolve_ref_unsafe Ævar Arnfjörð Bjarmason
2021-07-20 10:33               ` [PATCH v9 0/7] refs: cleanup errno sideband ref related functions Ævar Arnfjörð Bjarmason
2021-07-20 10:33                 ` [PATCH v9 1/7] refs file backend: move raceproof_create_file() here Ævar Arnfjörð Bjarmason
2021-07-20 10:33                 ` [PATCH v9 2/7] refs: remove EINVAL errno output from specification of read_raw_ref_fn Ævar Arnfjörð Bjarmason
2021-07-20 10:33                 ` [PATCH v9 3/7] refs/files-backend: stop setting errno from lock_ref_oid_basic Ævar Arnfjörð Bjarmason
2021-07-20 10:33                 ` [PATCH v9 4/7] refs: make errno output explicit for read_raw_ref_fn Ævar Arnfjörð Bjarmason
2021-08-16 13:00                   ` Han-Wen Nienhuys
2021-07-20 10:33                 ` [PATCH v9 5/7] refs: add failure_errno to refs_read_raw_ref() signature Ævar Arnfjörð Bjarmason
2021-07-20 10:33                 ` [PATCH v9 6/7] refs: explicitly return failure_errno from parse_loose_ref_contents Ævar Arnfjörð Bjarmason
2021-08-13 20:54                   ` Jonathan Tan
2021-07-20 10:33                 ` [PATCH v9 7/7] refs: make errno output explicit for refs_resolve_ref_unsafe Ævar Arnfjörð Bjarmason
2021-07-26 23:49                 ` [PATCH v9 0/7] refs: cleanup errno sideband ref related functions Ævar Arnfjörð Bjarmason
2021-07-27  0:18                   ` Junio C Hamano
2021-08-23 11:52                 ` [PATCH v10 0/8] " Ævar Arnfjörð Bjarmason
2021-08-23 11:52                   ` [PATCH v10 1/8] refs file backend: move raceproof_create_file() here Ævar Arnfjörð Bjarmason
2021-08-23 11:52                   ` [PATCH v10 2/8] refs: remove EINVAL errno output from specification of read_raw_ref_fn Ævar Arnfjörð Bjarmason
2021-08-23 11:52                   ` [PATCH v10 3/8] refs/files-backend: stop setting errno from lock_ref_oid_basic Ævar Arnfjörð Bjarmason
2021-08-23 11:52                   ` [PATCH v10 4/8] refs: make errno output explicit for read_raw_ref_fn Ævar Arnfjörð Bjarmason
2021-08-23 11:52                   ` [PATCH v10 5/8] refs: add failure_errno to refs_read_raw_ref() signature Ævar Arnfjörð Bjarmason
2021-08-23 11:52                   ` [PATCH v10 6/8] branch tests: test for errno propagating on failing read Ævar Arnfjörð Bjarmason
2021-08-23 11:52                   ` [PATCH v10 7/8] refs: explicitly return failure_errno from parse_loose_ref_contents Ævar Arnfjörð Bjarmason
2021-08-23 11:52                   ` [PATCH v10 8/8] refs: make errno output explicit for refs_resolve_ref_unsafe Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAFQ2z_NQSbshLSstkBM9OZ7XWvydSQ=x0GCHj0a8mf1OC4t2jw@mail.gmail.com' \
    --to=hanwen@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=hanwenn@gmail.com \
    --cc=jonathantanmy@google.com \
    --subject='Re: [PATCH 0/8] refs: cleanup errno sideband ref related functions' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).