# [PATCH] refs: print errno for read_raw_ref if GIT_TRACE_REFS is set 7 messages from 2021-04-12 to 2021-04-23. Participants: Han-Wen Nienhuys via GitGitGadget, Junio C Hamano, Han-Wen Nienhuys. Thread: https://gitlist.dev/t/55483 ## Han-Wen Nienhuys via GitGitGadget, 2021-04-12 19:32 Subject: [PATCH] refs: print errno for read_raw_ref if GIT_TRACE_REFS is set Message-ID: URL: https://gitlist.dev/e/pull.1002.git.git.1618255954484.gitgitgadget%40gmail.com ``` From: Han-Wen Nienhuys The ref backend API uses errno as a sideband error channel. Signed-off-by: Han-Wen Nienhuys --- refs: print errno for read_raw_ref if GIT_TRACE_REFS is set The ref backend API uses errno as a sideband error channel. Signed-off-by: Han-Wen Nienhuys hanwen@google.com Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1002%2Fhanwen%2Ferrno-debug-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1002/hanwen/errno-debug-v1 Pull-Request: https://github.com/git/git/pull/1002 refs/debug.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/refs/debug.c b/refs/debug.c index 922e64fa6ad9..576bf98e74ae 100644 --- a/refs/debug.c +++ b/refs/debug.c @@ -244,6 +244,7 @@ static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname, int res = 0; oidcpy(oid, &null_oid); + errno = 0; res = drefs->refs->be->read_raw_ref(drefs->refs, refname, oid, referent, type); @@ -251,7 +252,9 @@ static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname, trace_printf_key(&trace_refs, "read_raw_ref: %s: %s (=> %s) type %x: %d\n", refname, oid_to_hex(oid), referent->buf, *type, res); } else { - trace_printf_key(&trace_refs, "read_raw_ref: %s: %d\n", refname, res); + trace_printf_key(&trace_refs, + "read_raw_ref: %s: %d (errno %d)\n", refname, + res, errno); } return res; } base-commit: 89b43f80a514aee58b662ad606e6352e03eaeee4 -- gitgitgadget ``` ## Junio C Hamano, 2021-04-12 21:45 Subject: Re: [PATCH] refs: print errno for read_raw_ref if GIT_TRACE_REFS is set Message-ID: URL: https://gitlist.dev/e/xmqq4kgbb2ic.fsf%40gitster.g In-Reply-To: ``` "Han-Wen Nienhuys via GitGitGadget" writes: > diff --git a/refs/debug.c b/refs/debug.c > index 922e64fa6ad9..576bf98e74ae 100644 > --- a/refs/debug.c > +++ b/refs/debug.c > @@ -244,6 +244,7 @@ static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname, > int res = 0; > > oidcpy(oid, &null_oid); > + errno = 0; > res = drefs->refs->be->read_raw_ref(drefs->refs, refname, oid, referent, > type); > > @@ -251,7 +252,9 @@ static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname, > trace_printf_key(&trace_refs, "read_raw_ref: %s: %s (=> %s) type %x: %d\n", > refname, oid_to_hex(oid), referent->buf, *type, res); > } else { > - trace_printf_key(&trace_refs, "read_raw_ref: %s: %d\n", refname, res); > + trace_printf_key(&trace_refs, > + "read_raw_ref: %s: %d (errno %d)\n", refname, > + res, errno); > } OK. Between trace_printf_key() and the return of the call to read_raw_ref() method of the ref backend is only an "if (res == 0)" condition and I do not see anything that might clobber errno. I do wonder if we want strerror(errno) instead of the number, but I can go either way (it's just a trace output). Thanks, will queue. ``` ## Han-Wen Nienhuys, 2021-04-13 11:58 Subject: Re: [PATCH] refs: print errno for read_raw_ref if GIT_TRACE_REFS is set Message-ID: URL: https://gitlist.dev/e/CAFQ2z_MnZOfo4Bt6SsRJnL%2BF70r9wr_jYSf2KGvcvyX-8PfVRQ%40mail.gmail.com In-Reply-To: ``` On Mon, Apr 12, 2021 at 11:45 PM Junio C Hamano wrote: > > @@ -251,7 +252,9 @@ static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname, > > trace_printf_key(&trace_refs, "read_raw_ref: %s: %s (=> %s) type %x: %d\n", > > refname, oid_to_hex(oid), referent->buf, *type, res); > > } else { > > - trace_printf_key(&trace_refs, "read_raw_ref: %s: %d\n", refname, res); > > + trace_printf_key(&trace_refs, > > + "read_raw_ref: %s: %d (errno %d)\n", refname, > > + res, errno); > > } > > OK. Between trace_printf_key() and the return of the call to > read_raw_ref() method of the ref backend is only an "if (res == 0)" > condition and I do not see anything that might clobber errno. I don't want to bet on that. Let me send a second round. > I do wonder if we want strerror(errno) instead of the number, but I > can go either way (it's just a trace output). For tracing, it would be most useful if we got the actual symbol (eg. ENOENT). Is there a way to get at that? -- 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 ``` ## Han-Wen Nienhuys, 2021-04-13 12:02 Subject: Re: [PATCH] refs: print errno for read_raw_ref if GIT_TRACE_REFS is set Message-ID: URL: https://gitlist.dev/e/CAFQ2z_OMUdi4D3Jg3rsPr_Q-VeRptdiEOiYOL2kmyuBuvZLCjg%40mail.gmail.com In-Reply-To: ``` On Tue, Apr 13, 2021 at 1:58 PM Han-Wen Nienhuys wrote: > > On Mon, Apr 12, 2021 at 11:45 PM Junio C Hamano wrote: > > > @@ -251,7 +252,9 @@ static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname, > > > trace_printf_key(&trace_refs, "read_raw_ref: %s: %s (=> %s) type %x: %d\n", > > > refname, oid_to_hex(oid), referent->buf, *type, res); > > > } else { > > > - trace_printf_key(&trace_refs, "read_raw_ref: %s: %d\n", refname, res); > > > + trace_printf_key(&trace_refs, > > > + "read_raw_ref: %s: %d (errno %d)\n", refname, > > > + res, errno); > > > } > > > > OK. Between trace_printf_key() and the return of the call to > > read_raw_ref() method of the ref backend is only an "if (res == 0)" > > condition and I do not see anything that might clobber errno. > > I don't want to bet on that. Let me send a second round. oh, ugh. In email, there are two calls, but one is prefixed with '-'. Nevertheless, a bit of paranoia doesn't hurt here. -- 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 ``` ## Han-Wen Nienhuys via GitGitGadget, 2021-04-13 12:10 Subject: [PATCH v2] refs: print errno for read_raw_ref if GIT_TRACE_REFS is set Message-ID: URL: https://gitlist.dev/e/pull.1002.v2.git.git.1618315838582.gitgitgadget%40gmail.com In-Reply-To: ``` From: Han-Wen Nienhuys The ref backend API uses errno as a sideband error channel. Signed-off-by: Han-Wen Nienhuys --- refs: print errno for read_raw_ref if GIT_TRACE_REFS is set The ref backend API uses errno as a sideband error channel. Signed-off-by: Han-Wen Nienhuys hanwen@google.com Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1002%2Fhanwen%2Ferrno-debug-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1002/hanwen/errno-debug-v2 Pull-Request: https://github.com/git/git/pull/1002 Range-diff vs v1: 1: 9b150c5563f9 ! 1: 1e9a8990e7f2 refs: print errno for read_raw_ref if GIT_TRACE_REFS is set @@ Commit message ## refs/debug.c ## @@ refs/debug.c: static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname, + { + struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store; int res = 0; ++ int saved_errno = 0; oidcpy(oid, &null_oid); + errno = 0; res = drefs->refs->be->read_raw_ref(drefs->refs, refname, oid, referent, type); ++ saved_errno = errno; -@@ refs/debug.c: static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname, + if (res == 0) { trace_printf_key(&trace_refs, "read_raw_ref: %s: %s (=> %s) type %x: %d\n", refname, oid_to_hex(oid), referent->buf, *type, res); } else { - trace_printf_key(&trace_refs, "read_raw_ref: %s: %d\n", refname, res); + trace_printf_key(&trace_refs, + "read_raw_ref: %s: %d (errno %d)\n", refname, -+ res, errno); ++ res, saved_errno); } return res; } refs/debug.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/refs/debug.c b/refs/debug.c index 922e64fa6ad9..286987b72166 100644 --- a/refs/debug.c +++ b/refs/debug.c @@ -242,16 +242,21 @@ static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname, { struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store; int res = 0; + int saved_errno = 0; oidcpy(oid, &null_oid); + errno = 0; res = drefs->refs->be->read_raw_ref(drefs->refs, refname, oid, referent, type); + saved_errno = errno; if (res == 0) { trace_printf_key(&trace_refs, "read_raw_ref: %s: %s (=> %s) type %x: %d\n", refname, oid_to_hex(oid), referent->buf, *type, res); } else { - trace_printf_key(&trace_refs, "read_raw_ref: %s: %d\n", refname, res); + trace_printf_key(&trace_refs, + "read_raw_ref: %s: %d (errno %d)\n", refname, + res, saved_errno); } return res; } base-commit: 89b43f80a514aee58b662ad606e6352e03eaeee4 -- gitgitgadget ``` ## Junio C Hamano, 2021-04-13 20:08 Subject: Re: [PATCH] refs: print errno for read_raw_ref if GIT_TRACE_REFS is set Message-ID: URL: https://gitlist.dev/e/xmqqy2dmkkvo.fsf%40gitster.g In-Reply-To: ``` Han-Wen Nienhuys writes: >> I do wonder if we want strerror(errno) instead of the number, but I >> can go either way (it's just a trace output). > > For tracing, it would be most useful if we got the actual symbol (eg. > ENOENT). Is there a way to get at that? I do not think there is. And that is the reason why we see everywhere calls to die_errno(), error(..., strerror(errno)), etc. As this is interpolated into trace with untranslated string, I suspect that strerror() is not a good match, so let's live with %d for now. Thanks. ``` ## Han-Wen Nienhuys, 2021-04-23 08:34 Subject: Re: [PATCH] refs: print errno for read_raw_ref if GIT_TRACE_REFS is set Message-ID: URL: https://gitlist.dev/e/CAFQ2z_PhkOEnKNFZ29R5x9rttO%2B3POT8Tdx9stcQ__oNG%3DwQsA%40mail.gmail.com In-Reply-To: ``` On Tue, Apr 13, 2021 at 10:08 PM Junio C Hamano wrote: > > Han-Wen Nienhuys writes: > > >> I do wonder if we want strerror(errno) instead of the number, but I > >> can go either way (it's just a trace output). > > > > For tracing, it would be most useful if we got the actual symbol (eg. > > ENOENT). Is there a way to get at that? > > I do not think there is. > > And that is the reason why we see everywhere calls to die_errno(), > error(..., strerror(errno)), etc. > > As this is interpolated into trace with untranslated string, > I suspect that strerror() is not a good match, so let's live with %d > for now. Great! This topic is marked as Waiting for reviews to conclude. cf. but I don't know what is left to do. Oversight? -- 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 ```