refs: cleanup errno sideband ref related functions #1012
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There is an issue in commit 2ac4b96: |
|
There is an issue in commit 0b12744: |
|
There is an issue in commit 768e8b5: |
hanwen
force-pushed
the
einval-sideband
branch
2 times, most recently
from
08d1824
to
9e161ee
Compare
|
/submit |
|
Submitted as pull.1012.git.git.1619710329.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget-git[bot]](https://avatars.githubusercontent.com/in/46807?s=60&v=4){kind=small}
| @@ -617,11 +617,10 @@ typedef int reflog_expire_fn(struct ref_store *ref_store, | |||
| * properly-formatted or even safe reference name. NEITHER INPUT NOR | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Han-Wen Nienhuys <hanwen@google.com>
>
> A grep for EINVAL */*c reveals that no code inspects EINVAL after reading
> references.
We often use a pattern (which is common) like this:
if (some_func_in_ref_API(...) < 0) {
if (errno == ENOENT || errno == EISDIR)
... it is OK for the file to be missing ...
else
... error ...
}
If a piece of code currently sets EINVAL to errno manually when
signalling a failure by returning a negative value to communicate
with such a caller, we wouldn't see EINVAL mentioned, so such a grep
alone would not help us guarantee the correctness of an update to
stop assignment of EINVAL at all. The callers must be vetted more
carefully than "we are happy that nobody explicitly mentions EINVAL".
> The files ref backend does use EINVAL so parse_loose_ref_contents() can
> communicate to lock_raw_ref() about garbage following the hex SHA1, or a short
> read in files_read_raw_ref(), but the files backend does not call into
> refs_read_raw_ref(), so its EINVAL sideband error is unused.
This paragraph is confusing. It says EINVAL is used to signal
lock_raw_ref(), and it says EINVAL is not used by the same files
backend. Which is correct? If one part of the backend uses it, and
other parts don't, wouldn't the backend as a whole still use it?
Having said that, ...
> - * Return 0 on success. If the ref doesn't exist, set errno to ENOENT
> - * and return -1. If the ref exists but is neither a symbolic ref nor
> - * an object ID, it is broken; set REF_ISBROKEN in type, set errno to
> - * EINVAL, and return -1. If there is another error reading the ref,
> - * set errno appropriately and return -1.
... the mention (and requirement) of EINVAL seems redundant, as it
sounds sufficient for the caller to inspect 'type' to see if it is
REF_ISBOKEN. So it may be OK for the code that gives REF_ISBROKEN
to type *not* to set errno to EINVAL, as long as it won't leave it
as ENOENT (meaning, an unrelated system call failed earlier may have
set errno to ENOENT, and after having dealt with such an error, the
control may have reached to the codepath we are interested in
here---errno must be cleared to some value other than ENOENT, and
assigning EINVAL is as good as any).
That is because there is a codeflow like this:
if (files_read_raw_ref(...)) {
if (errno == ENOENT) {
... do various things ...
} else if (errno == EISDIR) {
... do different and various things ...
} else if (errno == EINVAL && (*type & REF_ISBROKEN)) {
... deal with broken ref ...
}
...
}
where errno is looked at.
> + * Return 0 on success. If the ref doesn't exist, set errno to ENOENT and return
> + * -1. If the ref exists but is neither a symbolic ref nor an object ID, it is
> + * broken; set REF_ISBROKEN in type, and return -1. If there is another error
> + * reading the ref, set errno appropriately and return -1.
So, this is not sufficient to let caller correctly and safely handle
errors. "set REF_ISBROKEN in type, set errno to something other
than ENOENT or EISDIR, and then return -1" is necessary, I would
think.
There was a problem hiding this comment.
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):
On Fri, Apr 30, 2021 at 4:38 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Han-Wen Nienhuys <hanwen@google.com>
> >
> > A grep for EINVAL */*c reveals that no code inspects EINVAL after reading
> > references.
>
> We often use a pattern (which is common) like this:
>
> if (some_func_in_ref_API(...) < 0) {
> if (errno == ENOENT || errno == EISDIR)
> ... it is OK for the file to be missing ...
> else
> ... error ...
> }
>
> If a piece of code currently sets EINVAL to errno manually when
> signalling a failure by returning a negative value to communicate
> with such a caller, we wouldn't see EINVAL mentioned, so such a grep
> alone would not help us guarantee the correctness of an update to
> stop assignment of EINVAL at all. The callers must be vetted more
> carefully than "we are happy that nobody explicitly mentions EINVAL".
Sure. I looked at the callers, and documented further what I looked
for. But how far should one go? It's a global variable, so
transitively, almost all of the code could be observing the EINVAL
value under very specific circumstances. But it would also be a
terrible, fragile coding style and use of undocumented behavior.
> > The files ref backend does use EINVAL so parse_loose_ref_contents() can
> > communicate to lock_raw_ref() about garbage following the hex SHA1, or a short
> > read in files_read_raw_ref(), but the files backend does not call into
> > refs_read_raw_ref(), so its EINVAL sideband error is unused.
>
> This paragraph is confusing. It says EINVAL is used to signal
> lock_raw_ref(), and it says EINVAL is not used by the same files
> backend. Which is correct? If one part of the backend uses it, and
> other parts don't, wouldn't the backend as a whole still use it?
I tried to clarify the message. files-backend.c makes assumptions
about the errno return for files_read_raw_ref, but it's not making
assumptions about the abstracted API in refs.h
> That is because there is a codeflow like this:
>
> if (files_read_raw_ref(...)) {
> if (errno == ENOENT) {
> ... do various things ...
> } else if (errno == EISDIR) {
> ... do different and various things ...
> } else if (errno == EINVAL && (*type & REF_ISBROKEN)) {
> ... deal with broken ref ...
> }
> ...
> }
as mentioned above, this isn't calling refs_read_raw_ref, so it's not
affected by this patch.
> > + * Return 0 on success. If the ref doesn't exist, set errno to ENOENT and return
> > + * -1. If the ref exists but is neither a symbolic ref nor an object ID, it is
> > + * broken; set REF_ISBROKEN in type, and return -1. If there is another error
> > + * reading the ref, set errno appropriately and return -1.
>
> So, this is not sufficient to let caller correctly and safely handle
> errors. "set REF_ISBROKEN in type, set errno to something other
> than ENOENT or EISDIR, and then return -1" is necessary, I would
> think.
I tweaked the comment. Note that the function has only a handful of
callers (and only one caller where this behavior is relevant), and
it's changed in a follow-on patch in this series. Is it worth the
effort to wordsmith this further?
--
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
| @@ -910,7 +910,6 @@ static int create_reflock(const char *path, void *cb) | |||
|
|
|||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
I 100% agree with you that errno is cumbersome to use and carries
far less information than we want (we do not learn what operation
failed on what path) over a long distance. It only is useful when
the callchain still knows what path was operated on.
But...
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
> For the copy/rename support, calls to lock_ref_oid_basic() in this file are
> followed by:
>
> * lock_ref_oid_basic (copy/rename rollback error path)
>
> * write_ref_to_lockfile (both in the rollback path and the success path of
> copy/rename)
>
> These calls do not inspect the incoming errno. As they perform I/O, they can
> clobber errno. For this reason, callers cannot reliably observe the errno that
> lock_ref_oid_basic() generated, so it is unsound for programmatic use.
In the latter part of the above, "callers" refers to the callers of
"the copy/rename support" (aka files_copy_or_rename_ref())?
Then I am not sure why "callers cannot reliably observe the errno
that lock_ref_oid_basic() generated" is a problem. They will see
the errno from the last system call that failed, if they care. So
their performing I/O is perfectly acceptable, too.
Hence, I am not sure what change the above justifies, if any.
If we can show that no caller of files_copy_or_rename_ref() uses
errno, it is a clear indication that lock_ref_oid_basic() is saving
and restoring errno for no good reason. I think that is what was
done for the other two callers below.
So I traced what happens after the copy-rename thing gets called.
refs_rename_ref(), rename_ref(), refs_copy_existing_ref() and
copy_existing_ref() (all in refs.c) should be the only callers of
the function. All callers in builtin/branch.c and builtin/remote.c
of these functions (by the way, refs_X() variants do not seem to be
called from anywhere---are they over-engineered?) just die() when
they signal a failure by returning non-zero, so I think it is safe
and much easier to understand to justify this change like so:
refs/files-backend.c::lock_ref_oid_basic() tries hard to signal
how it failed to its callers using errno. The three callers of
this file-scope static function are
* files_copy_or_rename_ref()
* files_create_symref()
* files_reflog_expire()
None of them looks at errno after seeing a negative return from
lock_ref_oid_basic() to make any decision, and no caller of
these three functions looks at errno after they signal a failure
by returning a negative value.
> For files_create_symref() and files_reflog_expire(), grepping over callers
> showed no callers inspecting errno.
Yes, this is a lot more relevant justification to allow these two
functions, and functions that are called _only_ _by_ these two
functions, stop worrying about errno.
There was a problem hiding this comment.
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):
On Fri, Apr 30, 2021 at 5:10 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> I 100% agree with you that errno is cumbersome to use and carries
> far less information than we want (we do not learn what operation
> failed on what path) over a long distance. It only is useful when
> the callchain still knows what path was operated on.
>
> But...
>
> "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > For the copy/rename support, calls to lock_ref_oid_basic() in this file are
> > followed by:
> >
> > * lock_ref_oid_basic (copy/rename rollback error path)
> >
> > * write_ref_to_lockfile (both in the rollback path and the success path of
> > copy/rename)
> >
> > These calls do not inspect the incoming errno. As they perform I/O, they can
> > clobber errno. For this reason, callers cannot reliably observe the errno that
> > lock_ref_oid_basic() generated, so it is unsound for programmatic use.
>
> In the latter part of the above, "callers" refers to the callers of
> "the copy/rename support" (aka files_copy_or_rename_ref())?
>
> Then I am not sure why "callers cannot reliably observe the errno
> that lock_ref_oid_basic() generated" is a problem. They will see
> the errno from the last system call that failed, if they care. So
> their performing I/O is perfectly acceptable, too.
>
> Hence, I am not sure what change the above justifies, if any.
>
> If we can show that no caller of files_copy_or_rename_ref() uses
> errno, it is a clear indication that lock_ref_oid_basic() is saving
> and restoring errno for no good reason. I think that is what was
> done for the other two callers below.
>
> So I traced what happens after the copy-rename thing gets called.
>
> refs_rename_ref(), rename_ref(), refs_copy_existing_ref() and
> copy_existing_ref() (all in refs.c) should be the only callers of
> the function. All callers in builtin/branch.c and builtin/remote.c
> of these functions (by the way, refs_X() variants do not seem to be
> called from anywhere---are they over-engineered?) just die() when
> they signal a failure by returning non-zero, so I think it is safe
> and much easier to understand to justify this change like so:
>
> refs/files-backend.c::lock_ref_oid_basic() tries hard to signal
> how it failed to its callers using errno. The three callers of
> this file-scope static function are
>
> * files_copy_or_rename_ref()
> * files_create_symref()
> * files_reflog_expire()
>
> None of them looks at errno after seeing a negative return from
> lock_ref_oid_basic() to make any decision, and no caller of
> these three functions looks at errno after they signal a failure
> by returning a negative value.
I stole your message here; hope that's OK. My original message tries
to convey that if you do
/* should return errno */
int a() { .. }
int b() {
result = a();
maybe_do_IO();
return result;
}
then callers of b() can't reason about the errno result of a(),
because they can't know if an error code was generated by
maybe_do_IO() or a(). This means that the errno result of a() is
useless. (This is assuming that b() doesn't inspect errno, which I
failed to mention.)
--
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
refs.c
Outdated
| @@ -1675,13 +1675,16 @@ int refs_read_raw_ref(struct ref_store *ref_store, | |||
| const char *refname, struct object_id *oid, | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Han-Wen Nienhuys <hanwen@google.com>
>
> read_raw_ref_fn needs to supply a credible errno for a number of cases. These
> are primarily:
>
> 1) The files backend calls read_raw_ref from lock_raw_ref, and uses the
> resulting error codes to create/remove directories as needed.
>
> 2) ENOENT should be translated in a zero OID, optionally with REF_ISBROKEN set,
> returning the last successfully resolved symref. This is necessary so
> read_raw_ref("HEAD") on an empty repo returns refs/heads/main (or the default branch
> du-jour), and we know on which branch to create the first commit.
>
> Make this information flow explicit by adding a failure_errno to the signature
> of read_raw_ref. All errnos from the files backend are still propagated
> unchanged, even though inspection suggests only ENOTDIR, EISDIR and ENOENT are
> relevant.
I like the general direction to move away from errno, which may make
sense only in the context of files backend (e.g. ENOENT would be
left in errno, only because the original "loose ref" implementation
used one file per ref, when a ref we wanted to see did not exist)
and having other backends use the same errno would not make much
sense, as it is not the goal to make other backends like reftable
emulate files backend.
I wonder if in the ideal world, we'd rather want to define our own
enum, not <errno.h>, that is specific to failure modes of ref API
functions and signal failures by returning these values (and the
enum includes 0 as a value to signal success, all other errors are
negative values).
What I am really getting at is if we need an extra "failure"
out-parameter-pointer in the internal API. I do not mind if it
helps the transition to the ideal world, but I offhand do not see if
we need result and failure as separate variables.
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
> refs.c | 7 +++++--
> refs/debug.c | 4 ++--
> refs/files-backend.c | 24 ++++++++++++------------
> refs/packed-backend.c | 8 ++++----
> refs/refs-internal.h | 16 +++++++++-------
> 5 files changed, 32 insertions(+), 27 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 261fd82beb98..43e2ad6b612a 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1675,13 +1675,16 @@ int refs_read_raw_ref(struct ref_store *ref_store,
> const char *refname, struct object_id *oid,
> struct strbuf *referent, unsigned int *type)
> {
> + int result, failure;
> if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
> return refs_read_special_head(ref_store, refname, oid, referent,
> type);
> }
>
> - return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
> - type);
> + result = ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
> + type, &failure);
> + errno = failure;
> + return result;
> }
>
> /* This function needs to return a meaningful errno on failure */
> diff --git a/refs/debug.c b/refs/debug.c
> index 922e64fa6ad9..887dbb14be6e 100644
> --- a/refs/debug.c
> +++ b/refs/debug.c
> @@ -238,14 +238,14 @@ debug_ref_iterator_begin(struct ref_store *ref_store, const char *prefix,
>
> static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname,
> struct object_id *oid, struct strbuf *referent,
> - unsigned int *type)
> + unsigned int *type, int *failure_errno)
> {
> struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
> int res = 0;
>
> oidcpy(oid, &null_oid);
> res = drefs->refs->be->read_raw_ref(drefs->refs, refname, oid, referent,
> - type);
> + type, failure_errno);
>
> if (res == 0) {
> trace_printf_key(&trace_refs, "read_raw_ref: %s: %s (=> %s) type %x: %d\n",
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index c9511da1d387..efe493ca1425 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -341,9 +341,9 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
> return refs->loose;
> }
>
> -static int files_read_raw_ref(struct ref_store *ref_store,
> - const char *refname, struct object_id *oid,
> - struct strbuf *referent, unsigned int *type)
> +static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
> + struct object_id *oid, struct strbuf *referent,
> + unsigned int *type, int *failure_errno)
> {
> struct files_ref_store *refs =
> files_downcast(ref_store, REF_STORE_READ, "read_raw_ref");
> @@ -354,7 +354,6 @@ static int files_read_raw_ref(struct ref_store *ref_store,
> struct stat st;
> int fd;
> int ret = -1;
> - int save_errno;
> int remaining_retries = 3;
>
> *type = 0;
> @@ -459,10 +458,10 @@ static int files_read_raw_ref(struct ref_store *ref_store,
> ret = parse_loose_ref_contents(buf, oid, referent, type);
>
> out:
> - save_errno = errno;
> + if (failure_errno)
> + *failure_errno = errno;
> strbuf_release(&sb_path);
> strbuf_release(&sb_contents);
> - errno = save_errno;
> return ret;
> }
>
> @@ -541,6 +540,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
> struct strbuf ref_file = STRBUF_INIT;
> int attempts_remaining = 3;
> int ret = TRANSACTION_GENERIC_ERROR;
> + int failure_errno = 0;
>
> assert(err);
> files_assert_main_repository(refs, "lock_raw_ref");
> @@ -629,9 +629,9 @@ static int lock_raw_ref(struct files_ref_store *refs,
> * fear that its value will change.
> */
>
> - if (files_read_raw_ref(&refs->base, refname,
> - &lock->old_oid, referent, type)) {
> - if (errno == ENOENT) {
> + if (files_read_raw_ref(&refs->base, refname, &lock->old_oid, referent,
> + type, &failure_errno)) {
> + if (failure_errno == ENOENT) {
> if (mustexist) {
> /* Garden variety missing reference. */
> strbuf_addf(err, "unable to resolve reference '%s'",
> @@ -655,7 +655,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
> * reference named "refs/foo/bar/baz".
> */
> }
> - } else if (errno == EISDIR) {
> + } else if (failure_errno == EISDIR) {
> /*
> * There is a directory in the way. It might have
> * contained references that have been deleted. If
> @@ -693,13 +693,13 @@ static int lock_raw_ref(struct files_ref_store *refs,
> goto error_return;
> }
> }
> - } else if (errno == EINVAL && (*type & REF_ISBROKEN)) {
> + } else if (failure_errno == EINVAL && (*type & REF_ISBROKEN)) {
> strbuf_addf(err, "unable to resolve reference '%s': "
> "reference broken", refname);
> goto error_return;
> } else {
> strbuf_addf(err, "unable to resolve reference '%s': %s",
> - refname, strerror(errno));
> + refname, strerror(failure_errno));
> goto error_return;
> }
>
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index dfecdbc1db60..a457f18e93c8 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -724,9 +724,9 @@ static struct snapshot *get_snapshot(struct packed_ref_store *refs)
> return refs->snapshot;
> }
>
> -static int packed_read_raw_ref(struct ref_store *ref_store,
> - const char *refname, struct object_id *oid,
> - struct strbuf *referent, unsigned int *type)
> +static int packed_read_raw_ref(struct ref_store *ref_store, const char *refname,
> + struct object_id *oid, struct strbuf *referent,
> + unsigned int *type, int *failure_errno)
> {
> struct packed_ref_store *refs =
> packed_downcast(ref_store, REF_STORE_READ, "read_raw_ref");
> @@ -739,7 +739,7 @@ static int packed_read_raw_ref(struct ref_store *ref_store,
>
> if (!rec) {
> /* refname is not a packed reference. */
> - errno = ENOENT;
> + *failure_errno = ENOENT;
> return -1;
> }
>
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index 29728a339fed..ac8a14086724 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -617,10 +617,12 @@ typedef int reflog_expire_fn(struct ref_store *ref_store,
> * properly-formatted or even safe reference name. NEITHER INPUT NOR
> * OUTPUT REFERENCE NAMES ARE VALIDATED WITHIN THIS FUNCTION.
> *
> - * Return 0 on success. If the ref doesn't exist, set errno to ENOENT and return
> - * -1. If the ref exists but is neither a symbolic ref nor an object ID, it is
> - * broken; set REF_ISBROKEN in type, and return -1. If there is another error
> - * reading the ref, set errno appropriately and return -1.
> + * Return 0 on success. If the ref doesn't exist, set failure_errno to ENOENT
> + * and return -1. If the ref exists but is neither a symbolic ref nor an object
> + * ID, it is broken; set REF_ISBROKEN in type, and return -1. For the files
> + * backend, EISDIR and ENOTDIR may be set if the ref name is a directory. If
> + * there is another error reading the ref, set failure_errno appropriately and
> + * return -1.
> *
> * Backend-specific flags might be set in type as well, regardless of
> * outcome.
> @@ -634,9 +636,9 @@ typedef int reflog_expire_fn(struct ref_store *ref_store,
> * - in all other cases, referent will be untouched, and therefore
> * refname will still be valid and unchanged.
> */
> -typedef int read_raw_ref_fn(struct ref_store *ref_store,
> - const char *refname, struct object_id *oid,
> - struct strbuf *referent, unsigned int *type);
> +typedef int read_raw_ref_fn(struct ref_store *ref_store, const char *refname,
> + struct object_id *oid, struct strbuf *referent,
> + unsigned int *type, int *failure_errno);
>
> struct ref_storage_be {
> struct ref_storage_be *next;
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Junio C Hamano <gitster@pobox.com> writes:
> I like the general direction to move away from errno, which may make
> sense only in the context of files backend (e.g. ENOENT would be
> left in errno, only because the original "loose ref" implementation
> used one file per ref, when a ref we wanted to see did not exist)
> and having other backends use the same errno would not make much
> sense, as it is not the goal to make other backends like reftable
> emulate files backend.
>
> I wonder if in the ideal world, we'd rather want to define our own
> enum, not <errno.h>, that is specific to failure modes of ref API
> functions and signal failures by returning these values (and the
> enum includes 0 as a value to signal success, all other errors are
> negative values).
>
> What I am really getting at is if we need an extra "failure"
> out-parameter-pointer in the internal API. I do not mind if it
> helps the transition to the ideal world, but I offhand do not see if
> we need result and failure as separate variables.
In any case, the change in the function signature helped me catch
that this series invalidates what has been queued on hn/reftable
without updating that topic to align with the new way to handle the
errno (it would have become a silent semantic failure if we instead
encoded the "failure" in the return value).
The following is what I am tentatively using for tonight's
integration when merging this and hn/reftable together to the 'seen'
branch.
Thanks.
refs/reftable-backend.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git c/refs/reftable-backend.c w/refs/reftable-backend.c
index 1aff21adb4..4385d2d6f5 100644
--- c/refs/reftable-backend.c
+++ w/refs/reftable-backend.c
@@ -52,7 +52,8 @@ static struct reftable_stack *stack_for(struct git_reftable_ref_store *store,
static int git_reftable_read_raw_ref(struct ref_store *ref_store,
const char *refname, struct object_id *oid,
struct strbuf *referent,
- unsigned int *type);
+ unsigned int *type,
+ int *failure_errno);
static void clear_reftable_log_record(struct reftable_log_record *log)
{
@@ -376,7 +377,8 @@ static int fixup_symrefs(struct ref_store *ref_store,
&old_oid, &referent,
/* mutate input, like
files-backend.c */
- &update->type);
+ &update->type,
+ &errno);
if (err < 0 && errno == ENOENT &&
is_null_oid(&update->old_oid)) {
err = 0;
@@ -1538,7 +1540,7 @@ static int reftable_error_to_errno(int err)
static int git_reftable_read_raw_ref(struct ref_store *ref_store,
const char *refname, struct object_id *oid,
struct strbuf *referent,
- unsigned int *type)
+ unsigned int *type, int *failure_errno)
{
struct git_reftable_ref_store *refs =
(struct git_reftable_ref_store *)ref_store;
@@ -1560,12 +1562,12 @@ static int git_reftable_read_raw_ref(struct ref_store *ref_store,
err = reftable_stack_read_ref(stack, refname, &ref);
if (err > 0) {
- errno = ENOENT;
+ *failure_errno = ENOENT;
err = -1;
goto done;
}
if (err < 0) {
- errno = reftable_error_to_errno(err);
+ *failure_errno = reftable_error_to_errno(err);
err = -1;
goto done;
}
@@ -1578,7 +1580,7 @@ static int git_reftable_read_raw_ref(struct ref_store *ref_store,
oidread(oid, reftable_ref_record_val1(&ref));
} else {
*type |= REF_ISBROKEN;
- errno = EINVAL;
+ *failure_errno = EINVAL;
err = -1;
}
done:
There was a problem hiding this comment.
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):
On Fri, Apr 30, 2021 at 8:02 AM Junio C Hamano <gitster@pobox.com> wrote:
> > I like the general direction to move away from errno, which may make
> > sense only in the context of files backend (e.g. ENOENT would be
> > left in errno, only because the original "loose ref" implementation
> > used one file per ref, when a ref we wanted to see did not exist)
> > and having other backends use the same errno would not make much
> > sense, as it is not the goal to make other backends like reftable
> > emulate files backend.
> >
> > I wonder if in the ideal world, we'd rather want to define our own
> > enum, not <errno.h>, that is specific to failure modes of ref API
> > functions and signal failures by returning these values (and the
> > enum includes 0 as a value to signal success, all other errors are
> > negative values).
I think it would be healthy to have a set of canonical error codes in
general, but I don't know if this will help here. The files and
packed backend want to communicate about very specific conditions
(ENOENT, EISDIR, ENOTDIR), which seem too low-level to put in a
generic error code.
> In any case, the change in the function signature helped me catch
> that this series invalidates what has been queued on hn/reftable
> without updating that topic to align with the new way to handle the
> errno (it would have become a silent semantic failure if we instead
> encoded the "failure" in the return value).
>
> The following is what I am tentatively using for tonight's
> integration when merging this and hn/reftable together to the 'seen'
> branch.
That sounds right. How should I post the next update to hn/reftable ?
(based on which branch?)
--
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
|
This branch is now known as |
|
This patch series was integrated into seen via cc3c3f1. |
|
This patch series was integrated into seen via d624e40. |
|
This patch series was integrated into seen via bd96b12. |
|
This patch series was integrated into seen via e86e6b9. |
|
This patch series was integrated into seen via 3e06514. |
|
This patch series was integrated into seen via 4775848. |
|
This patch series was integrated into seen via 9f11ab3. |
|
This patch series was integrated into seen via a7f0447. |
|
This patch series was integrated into seen via 2a64570. |
|
There was a status update in the "New Topics" section about the branch Futz with the way 'errno' is relied on in the refs API to carry the failure modes up the callchain. Waiting for reviews. |
|
This patch series was integrated into seen via 468459c. |
|
This patch series was integrated into seen via 2f05516. |
|
This patch series was integrated into seen via 01ad026. |
|
This patch series was integrated into seen via 4b81bbe. |
|
There was a status update in the "Cooking" section about the branch Futz with the way 'errno' is relied on in the refs API to carry the failure modes up the callchain. Waiting for reviews. |
|
There was a status update in the "Cooking" section about the branch Futz with the way 'errno' is relied on in the refs API to carry the failure modes up the callchain. Will cook in 'next'. |
|
On the Git mailing list, Jonathan Tan wrote (reply to this): |
|
On the Git mailing list, Han-Wen Nienhuys wrote (reply to this): |
|
There was a status update in the "Cooking" section about the branch Futz with the way 'errno' is relied on in the refs API to carry the failure modes up the callchain. Will cook in 'next'. |
|
There was a status update in the "Cooking" section about the branch Futz with the way 'errno' is relied on in the refs API to carry the failure modes up the callchain. Kicked out of 'next', to give ab/refs/files-cleanup a clean restart. |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): |
|
There was a status update in the "Cooking" section about the branch Futz with the way 'errno' is relied on in the refs API to carry the failure modes up the callchain. Kicked out of 'next', to give ab/refs/files-cleanup a clean restart. |
|
There was a status update in the "Cooking" section about the branch Futz with the way 'errno' is relied on in the refs API to carry the failure modes up the callchain. |
|
There was a status update in the "Cooking" section about the branch Futz with the way 'errno' is relied on in the refs API to carry the failure modes up the callchain. |
|
There was a status update in the "Cooking" section about the branch Futz with the way 'errno' is relied on in the refs API to carry the failure modes up the callchain. |
|
There was a status update in the "Cooking" section about the branch Futz with the way 'errno' is relied on in the refs API to carry the failure modes up the callchain. |
|
There was a status update in the "Cooking" section about the branch Futz with the way 'errno' is relied on in the refs API to carry the failure modes up the callchain. |
|
There was a status update in the "Cooking" section about the branch Futz with the way 'errno' is relied on in the refs API to carry the failure modes up the callchain. |
|
There was a status update in the "Cooking" section about the branch Futz with the way 'errno' is relied on in the refs API to carry the failure modes up the callchain. Will merge to 'next'? |
|
There was a status update in the "Cooking" section about the branch Futz with the way 'errno' is relied on in the refs API to carry the failure modes up the callchain. Will merge to 'next'? |
|
There was a status update in the "Cooking" section about the branch Futz with the way 'errno' is relied on in the refs API to carry the failure modes up the call chain. Will merge to 'master'. |
|
There was a status update in the "Cooking" section about the branch Futz with the way 'errno' is relied on in the refs API to carry the failure modes up the call chain. Will merge to 'master'. |
|
There was a status update in the "Cooking" section about the branch Futz with the way 'errno' is relied on in the refs API to carry the failure modes up the call chain. Will merge to 'master'. |
|
There was a status update in the "Graduated to 'master'" section about the branch Futz with the way 'errno' is relied on in the refs API to carry the failure modes up the call chain. |
|
merged into master. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
v5
cc: Han-Wen Nienhuys hanwen@google.com
cc: Jonathan Tan jonathantanmy@google.com
cc: Ævar Arnfjörð Bjarmason avarab@gmail.com