Re: [PATCH v3] usage: add trace2 entry upon warning()

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 24, 2020, 22:15 UTC
Message-ID: <xmqqzh36h04o.fsf@gitster.c.googlers.com>
In-Reply-To: <20201124200527.890563-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 21 quoted lines

> 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>
> ---
> Thanks, Junio. That comment looks good. Here is the version with Junio's
> suggested comment included, for everyone's reference.

Heh, I meant it as more of a #leftoverbit, not directly applicable to this particular patch, but would be a good follow-up topic, as I would have expected that die/warn/error should lose their own comments where they call trace2_cmd_error_va() in the same patch that adds the comment for callers near the function.

Let's use v2 if the difference between v2 and v3 is only the addition of the comment before trace2_cmd_error_va() function decl to help the callers.