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

[Junio C Hamano <gitster@pobox.com>]

Jonathan Tan <jonathantanmy@google.com> writes:

> diff --git a/usage.c b/usage.c
> index 06665823a2..1868a24f7a 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(warn, params);
> +
>  	vreportf("warning: ", warn, params);

Without the comment, this would have been a huge red sign.  Given
what the trace2 function does, I think cmd_error_va() has to do the
va_copy()/va_end() dance somewhere in it, so from the caller's point
of view, not having to va_copy()/va_end() is convenient and nice,
but perhaps we can eventually (not today---we have the same issue
with die_builtin() and error_builtin()) move the comment to the
callee so that other people who are writing more callers do not have
to rediscover what to do with "can va_lsit be used more than once?"
issue.  Perhaps like (I am not demonstrating the removal of dups):

 trace2.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git c/trace2.h w/trace2.h
index b18bc5529c..10ba231122 100644
--- c/trace2.h
+++ w/trace2.h
@@ -116,6 +116,11 @@ int trace2_cmd_exit_fl(const char *file, int line, int code);
  * Emit an 'error' event.
  *
  * Write an error message to the TRACE2 targets.
+ *
+ * Note that the "va_list ap" the caller has is not touched, because
+ * it is fed to each of the TRACE2 targets, which must use va_copy()
+ * and use va_end() on the copy.  The caller can still use ap it uses
+ * to call this funciton and and call va_end() on it when it is done.
  */
 void trace2_cmd_error_va_fl(const char *file, int line, const char *fmt,
 			    va_list ap);