| Message ID | 20201123190412.101265-1-jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] usage: add trace2 entry upon warning() | expand |
On 11/23/20 2:04 PM, Jonathan Tan wrote: > Emit a trace2 error event whenever warning() is called, just like when > die(), error(), or usage() is called. > > This helps debugging issues that would trigger warnings but not errors. > In particular, this might have helped debugging an issue I encountered > with commit graphs at $DAYJOB [1]. > > There is a tradeoff between including potentially relevant messages and > cluttering up the trace output produced. I think that warning() messages > should be included in traces, because by its nature, Git is used over > multiple invocations of the Git tool, and a failure (currently traced) > in a Git invocation might be caused by an unexpected interaction in a > previous Git invocation that only has a warning (currently untraced) as > a symptom - as is the case in [1]. > > [1] https://lore.kernel.org/git/20200629220744.1054093-1-jonathantanmy@google.com/ > > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> LGTM Thanks, Jeff
diff --git a/Documentation/technical/api-trace2.txt b/Documentation/technical/api-trace2.txt index 6b6085585d..c65ffafc48 100644 --- a/Documentation/technical/api-trace2.txt +++ b/Documentation/technical/api-trace2.txt @@ -466,7 +466,7 @@ completed.) `"error"`:: This event is emitted when one of the `error()`, `die()`, - or `usage()` functions are called. + `warning()`, or `usage()` functions are called. + ------------ { diff --git a/usage.c b/usage.c index 06665823a2..673edf5afd 100644 --- a/usage.c +++ b/usage.c @@ -81,6 +81,12 @@ static void error_builtin(const char *err, va_list params) static void warn_builtin(const char *warn, va_list params) { + /* + * We call this trace2 function first and expect it to va_copy 'params' + * before using it (because an 'ap' can only be walked once). + */ + trace2_cmd_error_va(err, params); + vreportf("warning: ", warn, params); }
Emit a trace2 error event whenever warning() is called, just like when die(), error(), or usage() is called. This helps debugging issues that would trigger warnings but not errors. In particular, this might have helped debugging an issue I encountered with commit graphs at $DAYJOB [1]. There is a tradeoff between including potentially relevant messages and cluttering up the trace output produced. I think that warning() messages should be included in traces, because by its nature, Git is used over multiple invocations of the Git tool, and a failure (currently traced) in a Git invocation might be caused by an unexpected interaction in a previous Git invocation that only has a warning (currently untraced) as a symptom - as is the case in [1]. [1] https://lore.kernel.org/git/20200629220744.1054093-1-jonathantanmy@google.com/ Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- The documentation (see the patch below) says that it is emitted when usage() is called, but I don't see it in the code (nor in practice). If not emitting traces upon usage() is intended (which is reasonable to me), I'll edit the documentation (and the commit message above). I marked this as RFC mostly because this is a perhaps subjective tradeoff that needs to be made. I think the tradeoff should be in favor of tracing warning() messages, and I have explained that in the commit message. --- Documentation/technical/api-trace2.txt | 2 +- usage.c | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-)