refs: cleanup errno sideband ref related functions #1012

gitgitgadget-git · 2021-07-06T19:40:45Z

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> > > The EINVAL error from parse_loose_ref_contents is used in files-backend > to create a custom error message. > > Signed-off-by: Han-Wen Nienhuys <hanwen@google.com> > Reviewed-By: Ævar Arnfjörð Bjarmason <avarab@gmail.com> I think a -By in the trailer is spelled with lowercase, i.e. "Reviewed-by". > --- > refs.c | 8 +++++--- > refs/files-backend.c | 13 +++++++++---- > refs/refs-internal.h | 6 ++++-- > 3 files changed, 18 insertions(+), 9 deletions(-) > > diff --git a/refs.c b/refs.c > index 25d80e544d0..eca7310e7a4 100644 > --- a/refs.c > +++ b/refs.c > @@ -1653,7 +1653,8 @@ int for_each_fullref_in_prefixes(const char *namespace, > > static int refs_read_special_head(struct ref_store *ref_store, > const char *refname, struct object_id *oid, > - struct strbuf *referent, unsigned int *type) > + struct strbuf *referent, unsigned int *type, > + int *failure_errno) > { > struct strbuf full_path = STRBUF_INIT; > struct strbuf content = STRBUF_INIT; > @@ -1663,7 +1664,8 @@ static int refs_read_special_head(struct ref_store *ref_store, > if (strbuf_read_file(&content, full_path.buf, 0) < 0) > goto done; > > - result = parse_loose_ref_contents(content.buf, oid, referent, type); > + result = parse_loose_ref_contents(content.buf, oid, referent, type, > + failure_errno); > > done: > strbuf_release(&full_path); > @@ -1683,7 +1685,7 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname, > > if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) { > return refs_read_special_head(ref_store, refname, oid, referent, > - type); > + type, failure_errno); > } > > return ref_store->be->read_raw_ref(ref_store, refname, oid, referent, > diff --git a/refs/files-backend.c b/refs/files-backend.c > index f38c9703504..b43ec4c66cb 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -455,9 +455,13 @@ stat_ref: > strbuf_rtrim(&sb_contents); > buf = sb_contents.buf; > > - ret = parse_loose_ref_contents(buf, oid, referent, type); > - > + ret = parse_loose_ref_contents(buf, oid, referent, type, failure_errno); > + errno = *failure_errno; All of the above makes sense. > out: > + /* Collect all types of failures from errno. Many system calls in this > + * function can fail with ENOTDIR/EISDIR, and we want to collect all of > + * them. > + */ Style. > *failure_errno = errno; > strbuf_release(&sb_path); > strbuf_release(&sb_contents); > @@ -465,7 +469,8 @@ out: > } > > int parse_loose_ref_contents(const char *buf, struct object_id *oid, > - struct strbuf *referent, unsigned int *type) > + struct strbuf *referent, unsigned int *type, > + int *failure_errno) > { > const char *p; > if (skip_prefix(buf, "ref:", &buf)) { > @@ -484,7 +489,7 @@ int parse_loose_ref_contents(const char *buf, struct object_id *oid, > if (parse_oid_hex(buf, oid, &p) || > (*p != '\0' && !isspace(*p))) { > *type |= REF_ISBROKEN; > - errno = EINVAL; > + *failure_errno = EINVAL; > return -1; > } OK. > return 0; > diff --git a/refs/refs-internal.h b/refs/refs-internal.h > index 54f57c6a2df..bf581e70cf6 100644 > --- a/refs/refs-internal.h > +++ b/refs/refs-internal.h > @@ -689,10 +689,12 @@ struct ref_store { > }; > > /* > - * Parse contents of a loose ref file. > + * Parse contents of a loose ref file. *failure_errno maybe be set to EINVAL for > + * invalid contents. > */ OK. > int parse_loose_ref_contents(const char *buf, struct object_id *oid, > - struct strbuf *referent, unsigned int *type); > + struct strbuf *referent, unsigned int *type, > + int *failure_errno); > > /* > * Fill in the generic part of refs and add it to our collection of

On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):

On Tue, Jul 6, 2021 at 9:37 PM Junio C Hamano <gitster@pobox.com> wrote: > > "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Han-Wen Nienhuys <hanwen@google.com> > > > > The EINVAL error from parse_loose_ref_contents is used in files-backend > > to create a custom error message. > > > > Signed-off-by: Han-Wen Nienhuys <hanwen@google.com> > > Reviewed-By: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > > I think a -By in the trailer is spelled with lowercase, i.e. "Reviewed-by". .. All done. I also addressed your other fixup. -- 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

gitgitgadget-git · 2021-07-11T12:48:26Z

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):

On Wed, Jul 07 2021, Han-Wen Nienhuys via GitGitGadget wrote: > Subject: refs: explicitly return failure_errno from parse_loose_ref_contents > [...] > The EINVAL error from parse_loose_ref_contents is used in files-backend > to create a custom error message. Except as my https://lore.kernel.org/git/87v95o5ku8.fsf@evledraar.gmail.com/ shows, and if you remove this: > + ret = parse_loose_ref_contents(buf, oid, referent, type, failure_errno); > + errno = *failure_errno; We aren't explicitly returning things yet, instead we set it for some, but others still fail if we don't support that one caller relying on this "errno" and not "failure_errno" through the backdoor, why not simply convert that remaining caller?

gitgitgadget-git · 2021-07-01T12:20:41Z

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):

On Thu, Jun 10 2021, Han-Wen Nienhuys via GitGitGadget wrote: > From: Han-Wen Nienhuys <hanwen@google.com> > > This makes the errno output of refs_read_raw_ref explicit. > > Signed-off-by: Han-Wen Nienhuys <hanwen@google.com> > Reviewed-by: Jonathan Tan <jonathantanmy@google.com> > --- > refs.c | 29 ++++++++++++++--------------- > refs/files-backend.c | 8 ++++---- > refs/packed-backend.c | 10 ++++++---- > refs/refs-internal.h | 6 +++--- > 4 files changed, 27 insertions(+), 26 deletions(-) > > diff --git a/refs.c b/refs.c > index 64e2d55adcfb..ed2dde1c0c6d 100644 > --- a/refs.c > +++ b/refs.c > @@ -1671,21 +1671,19 @@ static int refs_read_special_head(struct ref_store *ref_store, > return result; > } > > -int refs_read_raw_ref(struct ref_store *ref_store, > - const char *refname, struct object_id *oid, > - struct strbuf *referent, unsigned int *type) > +int refs_read_raw_ref(struct ref_store *ref_store, const char *refname, > + struct object_id *oid, struct strbuf *referent, > + unsigned int *type, int *failure_errno) > { > - int result, failure; > + if (failure_errno) > + *failure_errno = 0; > if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) { > return refs_read_special_head(ref_store, refname, oid, referent, > type); > } > > - failure = 0; > - result = ref_store->be->read_raw_ref(ref_store, refname, oid, referent, > - type, &failure); > - errno = failure; > - return result; > + return ref_store->be->read_raw_ref(ref_store, refname, oid, referent, > + type, failure_errno); > } Ah, so here we drop the whole intermediate step of having this function not take a failure_errno itself. I think this would be better squashed into that earlier change. > /* This function needs to return a meaningful errno on failure */ > @@ -1726,9 +1724,10 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs, > > for (symref_count = 0; symref_count < SYMREF_MAXDEPTH; symref_count++) { > unsigned int read_flags = 0; > + int read_failure = 0; Let's call it failure_errno consistently if we end up keeping it. > - if (refs_read_raw_ref(refs, refname, > - oid, &sb_refname, &read_flags)) { > + if (refs_read_raw_ref(refs, refname, oid, &sb_refname, > + &read_flags, &read_failure)) { > *flags |= read_flags; > > /* In reading mode, refs must eventually resolve */ > @@ -1740,9 +1739,8 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs, > * may show errors besides ENOENT if there are > * similarly-named refs. > */ > - if (errno != ENOENT && > - errno != EISDIR && > - errno != ENOTDIR) > + if (read_failure != ENOENT && read_failure != EISDIR && > + read_failure != ENOTDIR) > return NULL; But ditto my previous comments, this seems like a whole dance to avoid reading errno directly in cases where doing so is actually OK. I.e. the "last_errno" pattern is for things that encounter an errno, do some other stuff (such as a printf) where they might get /another/ errno (or reset it), but in this case we can just document "these will set errno on failure". > oidclr(oid); > @@ -2254,7 +2252,8 @@ int refs_verify_refname_available(struct ref_store *refs, > if (skip && string_list_has_string(skip, dirname.buf)) > continue; > > - if (!refs_read_raw_ref(refs, dirname.buf, &oid, &referent, &type)) { > + if (!refs_read_raw_ref(refs, dirname.buf, &oid, &referent, > + &type, NULL)) { > strbuf_addf(err, _("'%s' exists; cannot create '%s'"), > dirname.buf, refname); > goto cleanup; And if we do care about errno at all, why would we not add it with a strerror() here instead of explicitly ignoring it? > diff --git a/refs/files-backend.c b/refs/files-backend.c > index 5a430aabf623..01c9bd0dbf04 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -383,8 +383,8 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname, > if (lstat(path, &st) < 0) { > if (errno != ENOENT) > goto out; > - if (refs_read_raw_ref(refs->packed_ref_store, refname, > - oid, referent, type)) { > + if (refs_read_raw_ref(refs->packed_ref_store, refname, oid, > + referent, type, NULL)) { > errno = ENOENT; > goto out; > } In this case that seems to make sense, since we substitute our own errno. Maybe I haven't read this all carefully enough, but why are we not caring about the errno refs_read_raw_ref() might return here? Is it because we might take the refs_read_special_head() codepath? In any case, I for one would appreciate a comment/commit message note about why we ignore the errno these "I'll give you an errno on failure" functions return in some cases, but not others. I think it's probably best to split those into another commit, i.e. do the "we just ferry errno around differently now (if we'll do that at all)" as one step, and "here we might get errno, but we like our own better" as another. > @@ -423,8 +423,8 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname, > * ref is supposed to be, there could still be a > * packed ref: > */ > - if (refs_read_raw_ref(refs->packed_ref_store, refname, > - oid, referent, type)) { > + if (refs_read_raw_ref(refs->packed_ref_store, refname, oid, > + referent, type, NULL)) { > errno = EISDIR; > goto out; Ditto. > diff --git a/refs/packed-backend.c b/refs/packed-backend.c > index a457f18e93c8..03353ce48869 100644 > --- a/refs/packed-backend.c > +++ b/refs/packed-backend.c > @@ -739,7 +739,8 @@ static int packed_read_raw_ref(struct ref_store *ref_store, const char *refname, > > if (!rec) { > /* refname is not a packed reference. */ > - *failure_errno = ENOENT; > + if (failure_errno) > + *failure_errno = ENOENT; > return -1; > } FWIW I'd think it's better in terms of code readability to not do this, and make callers who don't care about our errno explicitly provide a throwaway variable to show that they're not caring, but I don't have the full picture yet. I.e. make them do something like: int ret; int got_errno; ret = func(..., &got_errno); if (!ret) { if (!got_errno) BUG("error but no errno?"); /* We don't care, use our own */ got_errno = ENOSOMETHING ...

gitgitgadget-git · 2021-07-01T12:26:44Z

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):

On Thu, Jun 10 2021, Han-Wen Nienhuys via GitGitGadget wrote: > From: Han-Wen Nienhuys <hanwen@google.com> > > This is done in a separate commit, to pinpoint the precise cause should there be > regressions in error reporting. > > This is implemented by renaming the existing logic to a static function > refs_resolve_unsafe_implicit_errno(), minimizing the code diff. > > Signed-off-by: Han-Wen Nienhuys <hanwen@google.com> > Reviewed-by: Jonathan Tan <jonathantanmy@google.com> > --- > refs.c | 22 ++++++++++++++++------ > 1 file changed, 16 insertions(+), 6 deletions(-) > > diff --git a/refs.c b/refs.c > index ed2dde1c0c6d..191cbf5a330f 100644 > --- a/refs.c > +++ b/refs.c > @@ -1687,10 +1687,10 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname, > } > > /* This function needs to return a meaningful errno on failure */ > -const char *refs_resolve_ref_unsafe(struct ref_store *refs, > - const char *refname, > - int resolve_flags, > - struct object_id *oid, int *flags) > +static const char * The "let's make this static" seems like an unrelated change we should squash into the earlier "refs: use refs_resolve_ref_unsafe_with_errno() where needed", we stopped using it in the files backend then. No? > +refs_resolve_ref_unsafe_implicit_errno(struct ref_store *refs, > + const char *refname, int resolve_flags, > + struct object_id *oid, int *flags) > { > static struct strbuf sb_refname = STRBUF_INIT; > struct object_id unused_oid; > @@ -1779,14 +1779,24 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs, > return NULL; > } > > +const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname, > + int resolve_flags, struct object_id *oid, > + int *flags) > +{ > + const char *result = refs_resolve_ref_unsafe_implicit_errno( > + refs, refname, resolve_flags, oid, flags); > + errno = 0; > + return result; > +} > + > const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs, > const char *refname, > int resolve_flags, > struct object_id *oid, > int *flags, int *failure_errno) > { > - const char *result = refs_resolve_ref_unsafe(refs, refname, > - resolve_flags, oid, flags); > + const char *result = refs_resolve_ref_unsafe_implicit_errno( > + refs, refname, resolve_flags, oid, flags); > *failure_errno = errno; > return result; > } Per my earlier comments this whole thing again seems a bit backwards. We explicitly clear errno instead of telling the caller to care. Has this whole thing perhaps had the unstated aim that you were trying to distinguish the now-remaining refs_resolve_ref_unsafe() callers from the "I care about errno" by explicitly clearing out errno for them, thus ensuring that they need to use the wrapper function with "errno" in the name to get the errno they'd otherwise get?

gitgitgadget-git · 2021-07-01T13:09:46Z

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):

On Thu, Jun 10 2021, Han-Wen Nienhuys via GitGitGadget wrote: > From: Han-Wen Nienhuys <hanwen@google.com> > > The function refs_resolve_ref_unsafe_with_errno should produce an errno output. > Rather than taking the value from the errno (which might contain garbage > beforehand), explicitly propagate the failure_errno coming out of > refs_read_raw_ref(). > > Signed-off-by: Han-Wen Nienhuys <hanwen@google.com> > Reviewed-by: Jonathan Tan <jonathantanmy@google.com> > --- > refs.c | 38 ++++++++++++++------------------------ > 1 file changed, 14 insertions(+), 24 deletions(-) > > diff --git a/refs.c b/refs.c > index 191cbf5a330f..92c4796078bb 100644 > --- a/refs.c > +++ b/refs.c > @@ -1686,11 +1686,11 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname, > type, failure_errno); > } > > -/* This function needs to return a meaningful errno on failure */ > -static const char * > -refs_resolve_ref_unsafe_implicit_errno(struct ref_store *refs, > - const char *refname, int resolve_flags, > - struct object_id *oid, int *flags) > +const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs, > + const char *refname, > + int resolve_flags, > + struct object_id *oid, > + int *flags, int *failure_errno) > { At this point I'm lost. We just introduced this function two commits ago, and now it's gone again. I guess the "we should update the comment" from an earlier reply of mine is addressed in some way here... > static struct strbuf sb_refname = STRBUF_INIT; > struct object_id unused_oid; > @@ -1703,11 +1703,12 @@ refs_resolve_ref_unsafe_implicit_errno(struct ref_store *refs, As an aside I really wish our hunk title detection would spot the rename & use the new name... > flags = &unused_flags; > > *flags = 0; > + *failure_errno = 0; > > if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { > if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) || > !refname_is_safe(refname)) { > - errno = EINVAL; > + *failure_errno = EINVAL; > return NULL; > } > > @@ -1730,6 +1731,8 @@ refs_resolve_ref_unsafe_implicit_errno(struct ref_store *refs, > &read_flags, &read_failure)) { > *flags |= read_flags; > > + *failure_errno = read_failure; > + > /* In reading mode, refs must eventually resolve */ > if (resolve_flags & RESOLVE_REF_READING) > return NULL; > @@ -1767,7 +1770,7 @@ refs_resolve_ref_unsafe_implicit_errno(struct ref_store *refs, > if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { > if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) || > !refname_is_safe(refname)) { > - errno = EINVAL; > + *failure_errno = EINVAL; > return NULL; > } > > @@ -1775,7 +1778,7 @@ refs_resolve_ref_unsafe_implicit_errno(struct ref_store *refs, > } > } > > - errno = ELOOP; > + *failure_errno = ELOOP; > return NULL; > } > > @@ -1783,22 +1786,9 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname, > int resolve_flags, struct object_id *oid, > int *flags) > { > - const char *result = refs_resolve_ref_unsafe_implicit_errno( > - refs, refname, resolve_flags, oid, flags); > - errno = 0; > - return result; > -} > - > -const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs, > - const char *refname, > - int resolve_flags, > - struct object_id *oid, > - int *flags, int *failure_errno) > -{ > - const char *result = refs_resolve_ref_unsafe_implicit_errno( > - refs, refname, resolve_flags, oid, flags); > - *failure_errno = errno; > - return result; > + int ignore; > + return refs_resolve_ref_unsafe_with_errno(refs, refname, resolve_flags, > + oid, flags, &ignore); > } Okey, so here we have something like the "caller should explicitly ignore it" I suggested in <878s2qdy8y.fsf@evledraar.gmail.com>. We no longer set "errno" at all for the benefit of the callers of that function, but instead explicitly throw it away, our refs_resolve_ref_unsafe_with_errno() no longer sets errno. But this end-state seems to have resulted in introducing new bugs. A bunch of functions are thin wrappers for refs_resolve_ref_unsafe(). Previously they could inspect errno on a -1 return, now they can't. I didn't look at them all, but just the second one I looked at, refs_read_ref_full() has a verify_lock() caller which calls it, and that function then expects a meaningful errno that it in turn ferries up with "can't verify ref". It in turn is called by lock_ref_oid_basic(), it ferries up that strbuf with the error, which'll now have a meaningless strerror(0) in it. So it seems to me that the refactoring being done here is halfway done, and results in a buggy end-state. The below diff is my attempt to fix that up for just one caller, things thath call resolve_gitlink_ref() (it's literally the third caller out of N, so I'm not picking and choosing). In that case I believe that we don't actually have a regression because of your changes, but I think the changes below are the logical end-state of what you're starting here. I.e. we have a low-level function that may encounter an errno, and then callers removed from that via: index_path() -> resolve_gitlink_ref() -> refs_resolve_ref_unsafe() Where the common case for index_path() is to report the error we got, but it doesn't call die_errno() or error_errno() now, but really should, per the below diff (which you can consider to have by SOB if you want to pick it up). That passes all tests, except one where we actually fail because of what I'd argue is a better error message. i.e this in t3700-add.sh: + diff -u expect actual --- expect 2021-07-01 13:01:23.267001881 +0000 +++ actual 2021-07-01 13:01:23.267001881 +0000 @@ -1,2 +1,2 @@ -error: 'empty/' does not have a commit checked out +error: 'empty/' does not have a commit checked out: No such file or directory fatal: adding files failed In one sense I don't think you need to do this excercise of going all the way to the top of callers like index_path() that don't get an errno now, but in another sense I think you really do. By explicitly clearing "errno" as your end-state does and declaring that callers of refs_resolve_ref_unsafe() must be as happy to ignore "errno" as your own callers are, you're potentially introducing subtle bugs unless we go all the way to the top of the callstack and check if anything above us cares about errno, which we'll need due to do due to its global nature. I think I'd really only be confident that we've got them all if your chance was amended to something where we add such "int *maybe_errno" parameters to all upstream callers that may relay the <0 return value we got. diff --git a/builtin/update-index.c b/builtin/update-index.c index f1f16f2de52..54fcbc5be10 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -272,6 +272,7 @@ static int add_one_path(const struct cache_entry *old, const char *path, int len { int option; struct cache_entry *ce; + int ignore_errno; /* Was the old index entry already up-to-date? */ if (old && !ce_stage(old) && !ce_match_stat(old, st, 0)) @@ -285,7 +286,7 @@ static int add_one_path(const struct cache_entry *old, const char *path, int len ce->ce_mode = ce_mode_from_stat(old, st->st_mode); if (index_path(&the_index, &ce->oid, path, st, - info_only ? 0 : HASH_WRITE_OBJECT)) { + info_only ? 0 : HASH_WRITE_OBJECT, &ignore_errno)) { discard_cache_entry(ce); return -1; } @@ -325,14 +326,15 @@ static int process_directory(const char *path, int len, struct stat *st) { struct object_id oid; int pos = cache_name_pos(path, len); + int their_errno = 0; /* Exact match: file or existing gitlink */ if (pos >= 0) { const struct cache_entry *ce = active_cache[pos]; if (S_ISGITLINK(ce->ce_mode)) { - /* Do nothing to the index if there is no HEAD! */ - if (resolve_gitlink_ref(path, "HEAD", &oid) < 0) + int ignore_errno; + if (resolve_gitlink_ref(path, "HEAD", &oid, &ignore_errno) < 0) return 0; return add_one_path(ce, path, len, st); @@ -358,11 +360,16 @@ static int process_directory(const char *path, int len, struct stat *st) } /* No match - should we add it as a gitlink? */ - if (!resolve_gitlink_ref(path, "HEAD", &oid)) + if (resolve_gitlink_ref(path, "HEAD", &oid, &their_errno) >= 0) return add_one_path(NULL, path, len, st); /* Error out. */ - return error("%s: is a directory - add files inside instead", path); + if (their_errno) { + errno = their_errno; + return error_errno("%s: is a directory - add files inside instead", path); + } else { + return error("%s: is a directory - add files inside instead", path); + } } static int process_path(const char *path, struct stat *st, int stat_errno) diff --git a/cache.h b/cache.h index ba04ff8bd36..62e5ac335b8 100644 --- a/cache.h +++ b/cache.h @@ -879,7 +879,9 @@ int ie_modified(struct index_state *, const struct cache_entry *, struct stat *, #define HASH_FORMAT_CHECK 2 #define HASH_RENORMALIZE 4 int index_fd(struct index_state *istate, struct object_id *oid, int fd, struct stat *st, enum object_type type, const char *path, unsigned flags); -int index_path(struct index_state *istate, struct object_id *oid, const char *path, struct stat *st, unsigned flags); +int index_path(struct index_state *istate, struct object_id *oid, + const char *path, struct stat *st, unsigned flags, + int *maybe_errno); /* * Record to sd the data from st that we use to check whether a file diff --git a/combine-diff.c b/combine-diff.c index 7d925ce9ce4..7ac9bc6e943 100644 --- a/combine-diff.c +++ b/combine-diff.c @@ -1059,7 +1059,8 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent, elem->mode = canon_mode(st.st_mode); } else if (S_ISDIR(st.st_mode)) { struct object_id oid; - if (resolve_gitlink_ref(elem->path, "HEAD", &oid) < 0) + int ignore_errno; + if (resolve_gitlink_ref(elem->path, "HEAD", &oid, &ignore_errno) < 0) result = grab_blob(opt->repo, &elem->oid, elem->mode, &result_size, NULL, NULL); diff --git a/diff-lib.c b/diff-lib.c index c2ac9250fe9..4ae27851cb9 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -40,6 +40,7 @@ static int check_removed(const struct index_state *istate, const struct cache_en return 1; if (S_ISDIR(st->st_mode)) { struct object_id sub; + int ignore_errno; /* * If ce is already a gitlink, we can have a plain @@ -53,7 +54,7 @@ static int check_removed(const struct index_state *istate, const struct cache_en * a directory --- the blob was removed! */ if (!S_ISGITLINK(ce->ce_mode) && - resolve_gitlink_ref(ce->name, "HEAD", &sub)) + resolve_gitlink_ref(ce->name, "HEAD", &sub, &ignore_errno)) return 1; } return 0; diff --git a/diff.c b/diff.c index 52c791574b7..972ad0e57de 100644 --- a/diff.c +++ b/diff.c @@ -4426,6 +4426,7 @@ static void diff_fill_oid_info(struct diff_filespec *one, struct index_state *is { if (DIFF_FILE_VALID(one)) { if (!one->oid_valid) { + int maybe_errno = 0; struct stat st; if (one->is_stdin) { oidclr(&one->oid); @@ -4433,8 +4434,14 @@ static void diff_fill_oid_info(struct diff_filespec *one, struct index_state *is } if (lstat(one->path, &st) < 0) die_errno("stat '%s'", one->path); - if (index_path(istate, &one->oid, one->path, &st, 0)) - die("cannot hash %s", one->path); + if (index_path(istate, &one->oid, one->path, &st, 0, &maybe_errno)) { + if (maybe_errno) { + errno = maybe_errno; + die_errno("cannot hash %s", one->path); + } else { + die("cannot hash %s", one->path); + } + } } } else diff --git a/dir.c b/dir.c index ebe5ec046e0..95eb4ada718 100644 --- a/dir.c +++ b/dir.c @@ -2967,9 +2967,10 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up) int only_empty = (flag & REMOVE_DIR_EMPTY_ONLY); int keep_toplevel = (flag & REMOVE_DIR_KEEP_TOPLEVEL); struct object_id submodule_head; + int ignore_errno; if ((flag & REMOVE_DIR_KEEP_NESTED_GIT) && - !resolve_gitlink_ref(path->buf, "HEAD", &submodule_head)) { + resolve_gitlink_ref(path->buf, "HEAD", &submodule_head, &ignore_errno) < 0) { /* Do not descend and nuke a nested git work tree. */ if (kept_up) *kept_up = 1; diff --git a/notes-merge.c b/notes-merge.c index 46c1f7c7f11..f7e0091bd72 100644 --- a/notes-merge.c +++ b/notes-merge.c @@ -698,6 +698,7 @@ int notes_merge_commit(struct notes_merge_options *o, while ((e = readdir_skip_dot_and_dotdot(dir)) != NULL) { struct stat st; struct object_id obj_oid, blob_oid; + int maybe_errno = 0; if (get_oid_hex(e->d_name, &obj_oid)) { if (o->verbosity >= 3) @@ -710,8 +711,14 @@ int notes_merge_commit(struct notes_merge_options *o, /* write file as blob, and add to partial_tree */ if (stat(path.buf, &st)) die_errno("Failed to stat '%s'", path.buf); - if (index_path(o->repo->index, &blob_oid, path.buf, &st, HASH_WRITE_OBJECT)) - die("Failed to write blob object from '%s'", path.buf); + if (index_path(o->repo->index, &blob_oid, path.buf, &st, HASH_WRITE_OBJECT, &maybe_errno)) { + if (maybe_errno) { + errno = maybe_errno; + die_errno("Failed to write blob object from '%s'", path.buf); + } else { + die("Failed to write blob object from '%s'", path.buf); + } + } if (add_note(partial_tree, &obj_oid, &blob_oid, NULL)) die("Failed to add resolved note '%s' to notes tree", path.buf); diff --git a/object-file.c b/object-file.c index f233b440b22..0030854dcdf 100644 --- a/object-file.c +++ b/object-file.c @@ -2260,7 +2260,8 @@ int index_fd(struct index_state *istate, struct object_id *oid, } int index_path(struct index_state *istate, struct object_id *oid, - const char *path, struct stat *st, unsigned flags) + const char *path, struct stat *st, unsigned flags, + int *maybe_errno) { int fd; struct strbuf sb = STRBUF_INIT; @@ -2286,7 +2287,7 @@ int index_path(struct index_state *istate, struct object_id *oid, strbuf_release(&sb); break; case S_IFDIR: - return resolve_gitlink_ref(path, "HEAD", oid); + return resolve_gitlink_ref(path, "HEAD", oid, maybe_errno); default: return error(_("%s: unsupported file type"), path); } diff --git a/read-cache.c b/read-cache.c index 77961a38854..a40a326587f 100644 --- a/read-cache.c +++ b/read-cache.c @@ -256,6 +256,7 @@ static int ce_compare_link(const struct cache_entry *ce, size_t expected_size) static int ce_compare_gitlink(const struct cache_entry *ce) { struct object_id oid; + int ignore_errno; /* * We don't actually require that the .git directory @@ -265,7 +266,7 @@ static int ce_compare_gitlink(const struct cache_entry *ce) * * If so, we consider it always to match. */ - if (resolve_gitlink_ref(ce->name, "HEAD", &oid) < 0) + if (resolve_gitlink_ref(ce->name, "HEAD", &oid, &ignore_errno) < 0) return 0; return !oideq(&oid, &ce->oid); } @@ -748,8 +749,13 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st, namelen = strlen(path); if (S_ISDIR(st_mode)) { - if (resolve_gitlink_ref(path, "HEAD", &oid) < 0) - return error(_("'%s' does not have a commit checked out"), path); + int their_errno = 0; + if (resolve_gitlink_ref(path, "HEAD", &oid, &their_errno) < 0) { + if (their_errno) + return error_errno(_("'%s' does not have a commit checked out"), path); + else + return error(_("'%s' does not have a commit checked out"), path); + } while (namelen && path[namelen-1] == '/') namelen--; } @@ -799,9 +805,16 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st, } } if (!intent_only) { - if (index_path(istate, &ce->oid, path, st, hash_flags)) { + int maybe_errno = 0; + if (index_path(istate, &ce->oid, path, st, hash_flags, + &maybe_errno)) { discard_cache_entry(ce); - return error(_("unable to index file '%s'"), path); + if (maybe_errno) { + errno = maybe_errno; + return error_errno(_("unable to index file '%s'"), path); + } else { + return error(_("unable to index file '%s'"), path); + } } } else set_object_name_for_intent_to_add_entry(ce); diff --git a/refs.c b/refs.c index 92c4796078b..082b6b3d133 100644 --- a/refs.c +++ b/refs.c @@ -1807,7 +1807,7 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags, } int resolve_gitlink_ref(const char *submodule, const char *refname, - struct object_id *oid) + struct object_id *oid, int *maybe_errno) { struct ref_store *refs; int flags; @@ -1817,7 +1817,8 @@ int resolve_gitlink_ref(const char *submodule, const char *refname, if (!refs) return -1; - if (!refs_resolve_ref_unsafe(refs, refname, 0, oid, &flags) || + if (!refs_resolve_ref_unsafe_with_errno(refs, refname, 0, oid, &flags, + maybe_errno) || is_null_oid(oid)) return -1; return 0; diff --git a/refs.h b/refs.h index 48970dfc7e0..b0505b35698 100644 --- a/refs.h +++ b/refs.h @@ -133,10 +133,16 @@ int peel_iterated_oid(const struct object_id *base, struct object_id *peeled); * Resolve refname in the nested "gitlink" repository in the specified * submodule (which must be non-NULL). If the resolution is * successful, return 0 and set oid to the name of the object; - * otherwise, return a non-zero value. + * otherwise, return negative value. + * + * If we encounter an error we may have an errno to report from + * refs_resolve_ref_unsafe_with_errno(), or we might have hit another + * error. Set your `maybe_errno` to 0 beforehand, and check it after + * an error where we return a negative value to see if you should + * report the errno as well. */ int resolve_gitlink_ref(const char *submodule, const char *refname, - struct object_id *oid); + struct object_id *oid, int *maybe_errno); /* * Return true iff abbrev_name is a possible abbreviation for diff --git a/refs/files-backend.c b/refs/files-backend.c index 01c9bd0dbf0..9ed58c40de6 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -870,6 +870,7 @@ static int verify_lock(struct ref_store *ref_store, struct ref_lock *lock, &lock->old_oid, NULL)) { if (old_oid) { int save_errno = errno; + BUG("got here"); strbuf_addf(err, "can't verify ref '%s'", lock->ref_name); errno = save_errno; return -1; diff --git a/unpack-trees.c b/unpack-trees.c index f88a69f8e71..dd90787c56c 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1986,7 +1986,9 @@ static int verify_clean_subdirectory(const struct cache_entry *ce, if (S_ISGITLINK(ce->ce_mode)) { struct object_id oid; - int sub_head = resolve_gitlink_ref(ce->name, "HEAD", &oid); + int ignore_errno; + int sub_head = resolve_gitlink_ref(ce->name, "HEAD", &oid, + &ignore_errno); /* * If we are not going to update the submodule, then * we don't care.

On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):

On Thu, Jul 1, 2021 at 3:06 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > - const char *result = refs_resolve_ref_unsafe_implicit_errno( > > - refs, refname, resolve_flags, oid, flags); > > - *failure_errno = errno; > > - return result; > > + int ignore; > > + return refs_resolve_ref_unsafe_with_errno(refs, refname, resolve_flags, > > + oid, flags, &ignore); > > } > > Okey, so here we have something like the "caller should explicitly > ignore it" I suggested in <878s2qdy8y.fsf@evledraar.gmail.com>. We no > longer set "errno" at all for the benefit of the callers of that > function, but instead explicitly throw it away, our > refs_resolve_ref_unsafe_with_errno() no longer sets errno. > > But this end-state seems to have resulted in introducing new bugs. A > bunch of functions are thin wrappers for > refs_resolve_ref_unsafe(). Previously they could inspect errno on a -1 > return, now they can't. > > I didn't look at them all, but just the second one I looked at, > refs_read_ref_full() has a verify_lock() caller which calls it, and that It looks like you were lucky. I looked at the output of egrep -A7 -B7 --color -nH -e '(refs_resolve_refdup|resolve_refdup|refs_read_ref_full|read_ref_full|read_ref|refs_ref_exists|ref_exists|resolve_gitlink_ref|index_path)\(' *.[ch] */*.[ch] and the case you pointed out is the only one which inspects errno after calling resolve_ref_unsafe (transitively through the grepped functions.) > function then expects a meaningful errno that it in turn ferries up with > "can't verify ref". It in turn is called by lock_ref_oid_basic(), it > ferries up that strbuf with the error, which'll now have a meaningless > strerror(0) in it. > > So it seems to me that the refactoring being done here is halfway done, > [..] > of what you're starting here. I.e. we have a low-level function that may > encounter an errno, and then callers removed from that via: > > index_path() -> resolve_gitlink_ref() -> refs_resolve_ref_unsafe() >.. > Where the common case for index_path() is to report the error we got, > but it doesn't call die_errno() or error_errno() now, but really should, "but really should": Why? Why should all functions that resolve a ref print out system level errors? For example, that would entail large changes to sequencer.c which does "error" == "does not exist". Also, the errors you print out will be fundamentally limited. An error in resolving HEAD could be caused by a problem in any of the following files .git/HEAD .git/packed-refs .git/refs/ .git/refs/heads/ .git/refs/heads/main Since we have no way of reporting the filename involved, what is a user going to do with the error message ("file doesn't exist")? I think refs_verify_refname_available() provides a more realistic approach: create custom error messages for situations that a user is more likely to encounter. Also, for something like the reftable backend, there are error classes that have no direct errno counterpart. For example, if reftable finds corrupt zlib data, should it invent a bogus errno code so it can work with the errno reporting system? I'll take from this review that I should elide the part where errno is cleared, and leave it to someone else to figure out a more holistic strategy to error reporting. -- 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

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):

On Mon, Jul 05 2021, Han-Wen Nienhuys wrote: > On Thu, Jul 1, 2021 at 3:06 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: >> > - const char *result = refs_resolve_ref_unsafe_implicit_errno( >> > - refs, refname, resolve_flags, oid, flags); >> > - *failure_errno = errno; >> > - return result; >> > + int ignore; >> > + return refs_resolve_ref_unsafe_with_errno(refs, refname, resolve_flags, >> > + oid, flags, &ignore); >> > } >> >> Okey, so here we have something like the "caller should explicitly >> ignore it" I suggested in <878s2qdy8y.fsf@evledraar.gmail.com>. We no >> longer set "errno" at all for the benefit of the callers of that >> function, but instead explicitly throw it away, our >> refs_resolve_ref_unsafe_with_errno() no longer sets errno. >> >> But this end-state seems to have resulted in introducing new bugs. A >> bunch of functions are thin wrappers for >> refs_resolve_ref_unsafe(). Previously they could inspect errno on a -1 >> return, now they can't. >> >> I didn't look at them all, but just the second one I looked at, >> refs_read_ref_full() has a verify_lock() caller which calls it, and that > > It looks like you were lucky. I looked at the output of Maybe so... > egrep -A7 -B7 --color -nH -e > '(refs_resolve_refdup|resolve_refdup|refs_read_ref_full|read_ref_full|read_ref|refs_ref_exists|ref_exists|resolve_gitlink_ref|index_path)\(' > *.[ch] */*.[ch] FWIW this gives better results: git grep -E -W '\b(refs_resolve_refdup|resolve_refdup|refs_read_ref_full|read_ref_full|read_ref|refs_ref_exists|ref_exists|resolve_gitlink_ref|index_path)\(' > and the case you pointed out is the only one which inspects errno > after calling resolve_ref_unsafe (transitively through the grepped > functions.) Isn't the `errno == EISDIR` in files_copy_or_rename_ref() another one? It's just after calling refs_read_ref_full(). Yes, but aside from that it does look like I got lucky. I assumed this issue was more widespread. Phew! >> function then expects a meaningful errno that it in turn ferries up with >> "can't verify ref". It in turn is called by lock_ref_oid_basic(), it >> ferries up that strbuf with the error, which'll now have a meaningless >> strerror(0) in it. >> >> So it seems to me that the refactoring being done here is halfway done, >> [..] >> of what you're starting here. I.e. we have a low-level function that may >> encounter an errno, and then callers removed from that via: >> >> index_path() -> resolve_gitlink_ref() -> refs_resolve_ref_unsafe() >>.. >> Where the common case for index_path() is to report the error we got, >> but it doesn't call die_errno() or error_errno() now, but really should, > > "but really should": Why? > > Why should all functions that resolve a ref print out system level > errors? For example, that would entail large changes to sequencer.c > which does "error" == "does not exist". > > Also, the errors you print out will be fundamentally limited. An error > in resolving HEAD could be caused by a problem in any of the following > files > > .git/HEAD > .git/packed-refs > .git/refs/ > .git/refs/heads/ > .git/refs/heads/main > > Since we have no way of reporting the filename involved, what is a > user going to do with the error message ("file doesn't exist")? I > think refs_verify_refname_available() provides a more realistic > approach: create custom error messages for situations that a user is > more likely to encounter. Let me rephrase: "we really shouldn't", as in I agree that this API sucks and that refs_verify_refname_available() is a much better approach. But as long as errno was the abstraction we were passing around it seemed like e.g. callers like process_directory() should be getting the errno and using the *_errno() error functions, but of course it would be much better if they used a better error return flow. But having looked at the grep above with fresh eyes I think it's fine to leave it at the refs_resolve_ref_unsafe_with_errno() boundary. The error messages are clear enough, and we're trying to phase out errno anyway. I was more paranoid about things that would actually do equality checks against the errno, as it appearsfiles_copy_or_rename_ref() is still doing. > Also, for something like the reftable backend, there are error classes > that have no direct errno counterpart. For example, if reftable finds > corrupt zlib data, should it invent a bogus errno code so it can work > with the errno reporting system? No, that would be crazy. We should be moving away from errno entirely. My concern is only that we might be introducing subtle bugs further up the callstack now that we clear "errno" ... > I'll take from this review that I should elide the part where errno is > cleared, and leave it to someone else to figure out a more holistic > strategy to error reporting. ...or if we don't clear errno introducing those sorts of subtle bugs when we use reftable instead of the files backend. I.e. no, we really should be clearing errno, if not in this series then in some other pre-reftable series. To not do so would be kicking this particular can down the road, and leaving those bugs for reftable users to find. Which given that I've found a few cases with no test coverage...

On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):

On Mon, Jul 5, 2021 at 9:31 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > and the case you pointed out is the only one which inspects errno > > after calling resolve_ref_unsafe (transitively through the grepped > > functions.) > > Isn't the `errno == EISDIR` in files_copy_or_rename_ref() another one? > It's just after calling refs_read_ref_full(). no. It's handling the EISDIR for refs_delete_ref. > > I'll take from this review that I should elide the part where errno is > > cleared, and leave it to someone else to figure out a more holistic > > strategy to error reporting. > > ...or if we don't clear errno introducing those sorts of subtle bugs > when we use reftable instead of the files backend. I.e. no, we really > should be clearing errno, if not in this series then in some other > pre-reftable series. > > To not do so would be kicking this particular can down the road, and > leaving those bugs for reftable users to find. Which given that I've > found a few cases with no test coverage... too late :-) I've already kicked the can down the road by not clearing errno in the current version of the series. -- 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

gitgitgadget-git · 2021-07-06T19:40:46Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes: > -int refs_read_raw_ref(struct ref_store *ref_store, > - const char *refname, struct object_id *oid, > - struct strbuf *referent, unsigned int *type) > +int refs_read_raw_ref(struct ref_store *ref_store, const char *refname, > + struct object_id *oid, struct strbuf *referent, > + unsigned int *type, int *failure_errno) > { > - int result; > - int failure_errno; > + int ignore; > + if (failure_errno) > + *failure_errno = 0; > + else > + failure_errno = &ignore; > + Same comment as v3(v5). Squashable fix-up follows. diff --git a/refs.c b/refs.c index eca7310e7a..aaea9b34c6 100644 --- a/refs.c +++ b/refs.c @@ -1677,11 +1677,11 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname, struct object_id *oid, struct strbuf *referent, unsigned int *type, int *failure_errno) { - int ignore; - if (failure_errno) - *failure_errno = 0; - else - failure_errno = &ignore; + int unused_errno; + + if (!failure_errno) + failure_errno = &unused_errno; + *failure_errno = 0; if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) { return refs_read_special_head(ref_store, refname, oid, referent,

gitgitgadget-git · 2021-07-11T12:03:23Z

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):

On Wed, Jul 07 2021, Han-Wen Nienhuys via GitGitGadget wrote: > From: Han-Wen Nienhuys <hanwen@google.com> > > This lets us use the explicit errno output parameter in refs_resolve_ref_unsafe. > [...] > +const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs, This and the subsequent commit don't compile for me, because this lacks a prototype that you finally add in 6/6.

On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):

On Sun, Jul 11, 2021 at 2:00 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > > On Wed, Jul 07 2021, Han-Wen Nienhuys via GitGitGadget wrote: > > > From: Han-Wen Nienhuys <hanwen@google.com> > > > > This lets us use the explicit errno output parameter in refs_resolve_ref_unsafe. > > [...] > > +const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs, > > This and the subsequent commit don't compile for me, because this lacks > a prototype that you finally add in 6/6. Whoops! thanks for correcting. -- 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

gitgitgadget-git · 2021-07-01T12:09:00Z

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):

On Thu, Jun 10 2021, Han-Wen Nienhuys via GitGitGadget wrote: > From: Han-Wen Nienhuys <hanwen@google.com> > > lock_ref_oid_basic() in files-backend.c is the only caller of refs_resolve_ref() > that needs error information to make logic decisions. > > Signed-off-by: Han-Wen Nienhuys <hanwen@google.com> > Reviewed-by: Jonathan Tan <jonathantanmy@google.com> > --- > refs/files-backend.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > > diff --git a/refs/files-backend.c b/refs/files-backend.c > index 8f969c8f711f..5a430aabf623 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -924,6 +924,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, > int mustexist = (old_oid && !is_null_oid(old_oid)); > int resolve_flags = RESOLVE_REF_NO_RECURSE; > int resolved; > + int resolve_errno = 0; > > files_assert_main_repository(refs, "lock_ref_oid_basic"); > assert(err); > @@ -936,10 +937,11 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, > resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME; > > files_ref_path(refs, &ref_file, refname); > - resolved = !!refs_resolve_ref_unsafe(&refs->base, > - refname, resolve_flags, > - &lock->old_oid, type); > - if (!resolved && errno == EISDIR) { > + resolved = !!refs_resolve_ref_unsafe_with_errno(&refs->base, refname, > + resolve_flags, > + &lock->old_oid, type, > + &resolve_errno); > + if (!resolved && resolve_errno == EISDIR) { > /* > * we are trying to lock foo but we used to > * have foo/bar which now does not exist; > @@ -959,12 +961,11 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, > &lock->old_oid, type); > } > if (!resolved) { > - int last_errno = errno; > - if (last_errno != ENOTDIR || > - !refs_verify_refname_available(&refs->base, refname, > - extras, skip, err)) > + if (resolve_errno != ENOTDIR || > + !refs_verify_refname_available(&refs->base, refname, extras, > + skip, err)) > strbuf_addf(err, "unable to resolve reference '%s': %s", > - refname, strerror(last_errno)); > + refname, strerror(resolve_errno)); > > goto error_return; > } So, having read 4/8 and this I wonder why refs_resolve_ref_unsafe_with_errno() is needed at all. It's just a wrapper that sets errno to a variable you give it, but we could just document that tha caller should check errno. So far I haven't seen anything that suggests the below diff-on-top wouldn't be OK (and all tests pass with it). It steps on the toes of some of my earlier suggestions, but I'm doing these one-at-a-time. In any case the comment I adjusted seems like something you should adjust to. It looks like a TODO for having the sort of function you've just implemented in refs_resolve_ref_unsafe_with_errno(). diff --git a/refs.c b/refs.c index 64e2d55adcf..a07d852fcdc 100644 --- a/refs.c +++ b/refs.c @@ -1688,7 +1688,10 @@ int refs_read_raw_ref(struct ref_store *ref_store, return result; } -/* This function needs to return a meaningful errno on failure */ +/* + * This function clears errno at the beginning. If it fails the errno + * will be meaningful. + */ const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname, int resolve_flags, @@ -1698,6 +1701,7 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs, struct object_id unused_oid; int unused_flags; int symref_count; + errno = 0; if (!oid) oid = &unused_oid; @@ -1781,18 +1785,6 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs, return NULL; } -const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs, - const char *refname, - int resolve_flags, - struct object_id *oid, - int *flags, int *failure_errno) -{ - const char *result = refs_resolve_ref_unsafe(refs, refname, - resolve_flags, oid, flags); - *failure_errno = errno; - return result; -} - /* backend functions */ int refs_init_db(struct strbuf *err) { diff --git a/refs/files-backend.c b/refs/files-backend.c index 5a430aabf62..5a400e55cbf 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -924,7 +924,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, int mustexist = (old_oid && !is_null_oid(old_oid)); int resolve_flags = RESOLVE_REF_NO_RECURSE; int resolved; - int resolve_errno = 0; files_assert_main_repository(refs, "lock_ref_oid_basic"); assert(err); @@ -937,11 +936,10 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME; files_ref_path(refs, &ref_file, refname); - resolved = !!refs_resolve_ref_unsafe_with_errno(&refs->base, refname, - resolve_flags, - &lock->old_oid, type, - &resolve_errno); - if (!resolved && resolve_errno == EISDIR) { + resolved = !!refs_resolve_ref_unsafe(&refs->base, refname, + resolve_flags, + &lock->old_oid, type); + if (!resolved && errno == EISDIR) { /* * we are trying to lock foo but we used to * have foo/bar which now does not exist; @@ -961,11 +959,11 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, &lock->old_oid, type); } if (!resolved) { - if (resolve_errno != ENOTDIR || + if (errno != ENOTDIR || !refs_verify_refname_available(&refs->base, refname, extras, skip, err)) strbuf_addf(err, "unable to resolve reference '%s': %s", - refname, strerror(resolve_errno)); + refname, strerror(errno)); goto error_return; }

gitgitgadget-git · 2021-04-30T03:13:13Z

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.

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

gitgitgadget-git · 2021-07-01T11:32:33Z

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):

On Thu, Jun 10 2021, Han-Wen Nienhuys via GitGitGadget wrote: > From: Han-Wen Nienhuys <hanwen@google.com> > > refs/files-backend.c::lock_ref_oid_basic() tries to signal how it failed > to its callers using errno. > > It is safe to stop setting errno here, because the 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. In particular, > > * files_copy_or_rename_ref() - here, calls are followed by error() > (which performs I/O) or write_ref_to_lockfile() (which calls > parse_object() which may perform I/O) > > * files_create_symref() - here, calls are followed by error() or > create_symref_locked() (which performs I/O and does not inspect > errno) > > * files_reflog_expire() - here, calls are followed by error() or > refs_reflog_exists() (which calls a function in a vtable that is not > documented to use and/or preserve errno) > > Signed-off-by: Han-Wen Nienhuys <hanwen@google.com> > Reviewed-by: Jonathan Tan <jonathantanmy@google.com> > --- > refs/files-backend.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) This all looks good and well justified after the last commit (where we even mentioned refs_verify_refname_available() explicitly), but... > diff --git a/refs/files-backend.c b/refs/files-backend.c > index 677b7e4cdd2d..6aa0f5c41dd3 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -910,7 +910,6 @@ static int create_reflock(const char *path, void *cb) > > /* > * Locks a ref returning the lock on success and NULL on failure. > - * On failure errno is set to something meaningful. > */ > static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, > const char *refname, > @@ -922,7 +921,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, > { > struct strbuf ref_file = STRBUF_INIT; > struct ref_lock *lock; > - int last_errno = 0; > int mustexist = (old_oid && !is_null_oid(old_oid)); > int resolve_flags = RESOLVE_REF_NO_RECURSE; > int resolved; > @@ -949,7 +947,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, > * to remain. > */ > if (remove_empty_directories(&ref_file)) { > - last_errno = errno; > if (!refs_verify_refname_available( > &refs->base, > refname, extras, skip, err)) > @@ -962,7 +959,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, > &lock->old_oid, type); > } > if (!resolved) { > - last_errno = errno; > + int last_errno = errno; > if (last_errno != ENOTDIR || > !refs_verify_refname_available(&refs->base, refname, > extras, skip, err)) ...this particular change gives me some pause, because all the rest is about squirreling away our own errno for our own caller (which it turns out, we didn't need). But in this case we're only guarding against refs_verify_refname_available() possibly clobbering the errno we just got on !resolved in refs_verify_refname_available(). So instead I'd expect either this on top: diff --git a/refs/files-backend.c b/refs/files-backend.c index 6aa0f5c41dd..28aa4932529 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -959,12 +959,11 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, &lock->old_oid, type); } if (!resolved) { - int last_errno = errno; - if (last_errno != ENOTDIR || + if (errno != ENOTDIR || !refs_verify_refname_available(&refs->base, refname, extras, skip, err)) strbuf_addf(err, "unable to resolve reference '%s': %s", - refname, strerror(last_errno)); + refname, strerror(errno)); goto error_return; } Or, if we are actually worried about the errno being reset as we report it: diff --git a/refs/files-backend.c b/refs/files-backend.c index 6aa0f5c41dd..8ee6af61f1a 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -960,11 +960,20 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, } if (!resolved) { int last_errno = errno; + errno = 0; if (last_errno != ENOTDIR || !refs_verify_refname_available(&refs->base, refname, - extras, skip, err)) - strbuf_addf(err, "unable to resolve reference '%s': %s", - refname, strerror(last_errno)); + extras, skip, err)) { + if (errno) + strbuf_addf(err, "unable to resolve reference '%s': '%s', " + "also got '%s when reporting the error!", + refname, strerror(last_errno), + strerror(errno)); + else + strbuf_addf(err, "unable to resolve reference '%s': %s", + refname, strerror(errno)); + + } goto error_return; } I think in the end it doesn't matter much, we hit our primary error, so we're only potentially losing another error on our way out the door. It's more about clarity, the "last_errno" pattern signals "I'm about to call something that'll reset the errno I care about", but it's not clear if that's actually the case here, or if this is just leftover boilerplate. In any case running the tests with my second hunk with just a: if (errno) BUG("lost it?"); else ... Passes all our tests. I don't think it should be the scope of this series to give this code 100% test coverage, but (looking ahead) there's no mention of /test/ anywhere in the commit messages/comments. I think even if we keep your "last_errno" as-is here it would be useful to at least say something like: /* * Just paranoia, we probably won't lose errno in * refs_verify_refname_available(). */ int last_errno = errno; ...

On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):

On Thu, Jul 1, 2021 at 1:30 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > } > > if (!resolved) { > > - last_errno = errno; > > + int last_errno = errno; > > if (last_errno != ENOTDIR || > > !refs_verify_refname_available(&refs->base, refname, > > extras, skip, err)) > > ...this particular change gives me some pause, because all the rest is > about squirreling away our own errno for our own caller (which it turns > out, we didn't need). > > But in this case we're only guarding against > refs_verify_refname_available() possibly clobbering the errno we just > got on !resolved in refs_verify_refname_available(). This comes from 5b2d8d6f2184381b76c13504a2f5ec8a62cd584e .. invoke verify_refname_available() to try to generate a more helpful error message. That function might not detect an error. For example, some non-reference file might be blocking the deletion of an otherwise-empty directory tree, or there might be a race with another process that just deleted the offending reference. In such cases, generate the strerror-based error message like before. I think we want to keep this behavior, hence I think this should be kept as is. I'll add a comment. -- 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

gitgitgadget-git · 2021-07-11T11:50:52Z

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):

On Wed, Jul 07 2021, Han-Wen Nienhuys via GitGitGadget wrote: > /* > * Locks a ref returning the lock on success and NULL on failure. > - * On failure errno is set to something meaningful. > */ > static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, > const char *refname, > @@ -922,7 +921,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, > { > struct strbuf ref_file = STRBUF_INIT; > struct ref_lock *lock; > - int last_errno = 0; > int mustexist = (old_oid && !is_null_oid(old_oid)); > int resolve_flags = RESOLVE_REF_NO_RECURSE; > int resolved; > @@ -949,7 +947,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, > * to remain. > */ > if (remove_empty_directories(&ref_file)) { > - last_errno = errno; > if (!refs_verify_refname_available( > &refs->base, > refname, extras, skip, err)) > @@ -962,10 +959,13 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, > &lock->old_oid, type); > } > if (!resolved) { > - last_errno = errno; > + int last_errno = errno; > if (last_errno != ENOTDIR || > - !refs_verify_refname_available(&refs->base, refname, > - extras, skip, err)) > + /* in case of D/F conflict, try to generate a better error > + * message. If that fails, fall back to strerror(ENOTDIR). > + */ > + !refs_verify_refname_available(&refs->base, refname, extras, > + skip, err)) > strbuf_addf(err, "unable to resolve reference '%s': %s", > refname, strerror(last_errno)); I don't think it's some dealbreaker and we can move on, but just FWIW I think what I mentioned ending in your https://lore.kernel.org/git/CAFQ2z_NpyJQLuM70MhJ8K1h2v3QXFuAZRjN=SvSsjnukNRJ8pw@mail.gmail.com/ is still outstanding. I.e. you added the comment, which is just says what the error emitting looks like, that's all well & good. But what I was pointing out that it didn't make sense to do any "last_errno" here at all anymore. You pointed to 5b2d8d6f218 (lock_ref_sha1_basic(): improve diagnostics for ref D/F conflicts, 2015-05-11), we started setting "last_errno" there, but that was *not* to avoid clobbering between the !resolved and the strbuf_add(strerror(last_errno)) here, but rather to carry the "last_errno" forward to the end of this lock_ref_oid_basic(), because other things (after this hunk) might reset/clear errno. Anyway, as noted there it doesn't actually matter, just reviewing & looking if there's any loose ends, and for future source spelunking for anyone reading this thread. I.e. something like what I mentioned in https://lore.kernel.org/git/87k0mae0ga.fsf@evledraar.gmail.com/ could be squashed in, or better yet (probably) this: diff --git a/refs/files-backend.c b/refs/files-backend.c index 83ddfb3b627..f0ce0aac857 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -958,17 +958,15 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, refname, resolve_flags, &lock->old_oid, type); } - if (!resolved) { - int last_errno = errno; - if (last_errno != ENOTDIR || - /* in case of D/F conflict, try to generate a better error - * message. If that fails, fall back to strerror(ENOTDIR). - */ - !refs_verify_refname_available(&refs->base, refname, extras, - skip, err)) - strbuf_addf(err, "unable to resolve reference '%s': %s", - refname, strerror(last_errno)); - + if (!resolved && + (errno != ENOTDIR || + /* in case of D/F conflict, try to generate a better error + * message. If that fails, fall back to strerror(ENOTDIR). + */ + !refs_verify_refname_available(&refs->base, refname, extras, + skip, err))) { + strbuf_addf(err, "unable to resolve reference '%s': %s", + refname, strerror(errno)); goto error_return; }

On the Git mailing list, Han-Wen Nienhuys wrote (reply to this):

On Sun, Jul 11, 2021 at 1:48 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > > On Wed, Jul 07 2021, Han-Wen Nienhuys via GitGitGadget wrote: > > > /* > > * Locks a ref returning the lock on success and NULL on failure. > > - * On failure errno is set to something meaningful. > > */ > > static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, > > const char *refname, > > @@ -922,7 +921,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, > > { > > struct strbuf ref_file = STRBUF_INIT; > > struct ref_lock *lock; > > - int last_errno = 0; > > int mustexist = (old_oid && !is_null_oid(old_oid)); > > int resolve_flags = RESOLVE_REF_NO_RECURSE; > > int resolved; > > @@ -949,7 +947,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, > > * to remain. > > */ > > if (remove_empty_directories(&ref_file)) { > > - last_errno = errno; > > if (!refs_verify_refname_available( > > &refs->base, > > refname, extras, skip, err)) > > @@ -962,10 +959,13 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, > > &lock->old_oid, type); > > } > > if (!resolved) { > > - last_errno = errno; > > + int last_errno = errno; > > if (last_errno != ENOTDIR || > > - !refs_verify_refname_available(&refs->base, refname, > > - extras, skip, err)) > > + /* in case of D/F conflict, try to generate a better error > > + * message. If that fails, fall back to strerror(ENOTDIR). > > + */ > > + !refs_verify_refname_available(&refs->base, refname, extras, > > + skip, err)) > > strbuf_addf(err, "unable to resolve reference '%s': %s", > > refname, strerror(last_errno)); > > I don't think it's some dealbreaker and we can move on, but just FWIW I > think what I mentioned ending in your > https://lore.kernel.org/git/CAFQ2z_NpyJQLuM70MhJ8K1h2v3QXFuAZRjN=SvSsjnukNRJ8pw@mail.gmail.com/ > is still outstanding. > > I.e. you added the comment, which is just says what the error emitting > looks like, that's all well & good. > > But what I was pointing out that it didn't make sense to do any > "last_errno" here at all anymore. You pointed to 5b2d8d6f218 > (lock_ref_sha1_basic(): improve diagnostics for ref D/F conflicts, > 2015-05-11), we started setting "last_errno" there, but that was *not* > to avoid clobbering between the !resolved and the > strbuf_add(strerror(last_errno)) here, but rather to carry the > "last_errno" forward to the end of this lock_ref_oid_basic(), because > other things (after this hunk) might reset/clear errno. I disagree. In your suggested change > + if (!resolved && > + (errno != ENOTDIR || > + /* in case of D/F conflict, try to generate a better error > + * message. If that fails, fall back to strerror(ENOTDIR). > + */ > + !refs_verify_refname_available(&refs->base, refname, extras, > + skip, err))) { > + strbuf_addf(err, "unable to resolve reference '%s': %s", > + refname, strerror(errno)); > goto error_return; the refs_verify_refname_available() call happens only if errno==ENOTDIR. The call might clobber the ENOTDIR errno and then fail. Then we'd be printing the last errno that refs_verify_refname_available() saw, which may be different from ENOTDIR. Other than that, for clarity's sake, it's always better to avoid the use of global errno if we can. -- 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

-Original file line number
+Diff line change
@@ Expand Up @@
     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());
     	errno = 0;
     	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",
@@ Expand Down @@

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refs: cleanup errno sideband ref related functions #1012

Diff view

Diff view

There are no files selected for viewing

gitgitgadget-git bot Jul 6, 2021

gitgitgadget-git bot Jul 7, 2021

gitgitgadget-git bot Jul 11, 2021

gitgitgadget-git bot Jul 1, 2021

gitgitgadget-git bot Jul 1, 2021

gitgitgadget-git bot Jul 1, 2021

gitgitgadget-git bot Jul 5, 2021

gitgitgadget-git bot Jul 5, 2021

gitgitgadget-git bot Jul 5, 2021

gitgitgadget-git bot Jul 6, 2021

gitgitgadget-git bot Jul 11, 2021

gitgitgadget-git bot Jul 13, 2021

gitgitgadget-git bot Jul 1, 2021

gitgitgadget-git bot Apr 30, 2021

gitgitgadget-git bot May 19, 2021

gitgitgadget-git bot Jul 1, 2021

gitgitgadget-git bot Jul 5, 2021

gitgitgadget-git bot Jul 11, 2021

gitgitgadget-git bot Jul 13, 2021

refs: cleanup errno sideband ref related functions #1012

refs: cleanup errno sideband ref related functions #1012

Diff view

Diff view

There are no files selected for viewing

gitgitgadget-git bot Jul 6, 2021

Choose a reason for hiding this comment

gitgitgadget-git bot Jul 7, 2021

Choose a reason for hiding this comment

gitgitgadget-git bot Jul 11, 2021

Choose a reason for hiding this comment

gitgitgadget-git bot Jul 1, 2021

Choose a reason for hiding this comment

gitgitgadget-git bot Jul 1, 2021

Choose a reason for hiding this comment

gitgitgadget-git bot Jul 1, 2021

Choose a reason for hiding this comment

gitgitgadget-git bot Jul 5, 2021

Choose a reason for hiding this comment

gitgitgadget-git bot Jul 5, 2021

Choose a reason for hiding this comment

gitgitgadget-git bot Jul 5, 2021

Choose a reason for hiding this comment

gitgitgadget-git bot Jul 6, 2021

Choose a reason for hiding this comment

gitgitgadget-git bot Jul 11, 2021

Choose a reason for hiding this comment

gitgitgadget-git bot Jul 13, 2021

Choose a reason for hiding this comment

gitgitgadget-git bot Jul 1, 2021

Choose a reason for hiding this comment

gitgitgadget-git bot Apr 30, 2021

Choose a reason for hiding this comment

gitgitgadget-git bot May 19, 2021

Choose a reason for hiding this comment

gitgitgadget-git bot Jul 1, 2021

Choose a reason for hiding this comment

gitgitgadget-git bot Jul 5, 2021

Choose a reason for hiding this comment

gitgitgadget-git bot Jul 11, 2021

Choose a reason for hiding this comment

gitgitgadget-git bot Jul 13, 2021

Choose a reason for hiding this comment