| Message ID | pull.1002.git.git.1618255954484.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | refs: print errno for read_raw_ref if GIT_TRACE_REFS is set | expand |
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> 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.
On Mon, Apr 12, 2021 at 11:45 PM Junio C Hamano <gitster@pobox.com> 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?
On Tue, Apr 13, 2021 at 1:58 PM Han-Wen Nienhuys <hanwen@google.com> wrote: > > On Mon, Apr 12, 2021 at 11:45 PM Junio C Hamano <gitster@pobox.com> 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 <hanwen@google.com> 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.
On Tue, Apr 13, 2021 at 10:08 PM Junio C Hamano <gitster@pobox.com> wrote: > > Han-Wen Nienhuys <hanwen@google.com> 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. <xmqq4kgbb2ic.fsf@gitster.g> but I don't know what is left to do. Oversight?
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; }