git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [RFC PATCH] usage: add trace2 entry upon warning()
@ 2020-11-23 19:04 Jonathan Tan
  2020-11-23 20:45 ` [RFC PATCH v2] " Jonathan Tan
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Jonathan Tan @ 2020-11-23 19:04 UTC (permalink / raw)
  To: git; +Cc: Jonathan Tan, git

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(-)

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);
 }
 
-- 
2.29.2.454.gaff20da3a2-goog


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [RFC PATCH v2] usage: add trace2 entry upon warning()
  2020-11-23 19:04 [RFC PATCH] usage: add trace2 entry upon warning() Jonathan Tan
@ 2020-11-23 20:45 ` Jonathan Tan
  2020-11-23 22:30   ` Junio C Hamano
  2020-11-24 20:05 ` [PATCH v3] " Jonathan Tan
  2020-11-30 16:16 ` [RFC PATCH] " Jeff Hostetler
  2 siblings, 1 reply; 6+ messages in thread
From: Jonathan Tan @ 2020-11-23 20:45 UTC (permalink / raw)
  To: git; +Cc: Jonathan Tan, git

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>
---
Whoops...I ran a compile for the first version but I sent the email out
before looking at the results. The first version had a compile error,
but this one should be fine.
---
 Documentation/technical/api-trace2.txt | 2 +-
 usage.c                                | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

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..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);
 }
 
-- 
2.29.2.454.gaff20da3a2-goog


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH v2] usage: add trace2 entry upon warning()
  2020-11-23 20:45 ` [RFC PATCH v2] " Jonathan Tan
@ 2020-11-23 22:30   ` Junio C Hamano
  0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2020-11-23 22:30 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: git, git

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);

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3] usage: add trace2 entry upon warning()
  2020-11-23 19:04 [RFC PATCH] usage: add trace2 entry upon warning() Jonathan Tan
  2020-11-23 20:45 ` [RFC PATCH v2] " Jonathan Tan
@ 2020-11-24 20:05 ` Jonathan Tan
  2020-11-24 22:15   ` Junio C Hamano
  2020-11-30 16:16 ` [RFC PATCH] " Jeff Hostetler
  2 siblings, 1 reply; 6+ messages in thread
From: Jonathan Tan @ 2020-11-24 20:05 UTC (permalink / raw)
  To: git; +Cc: Jonathan Tan, gitster

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.
---
 Documentation/technical/api-trace2.txt | 2 +-
 trace2.h                               | 5 +++++
 usage.c                                | 6 ++++++
 3 files changed, 12 insertions(+), 1 deletion(-)

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/trace2.h b/trace2.h
index b18bc5529c..2b5c5c4ba0 100644
--- a/trace2.h
+++ b/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 function 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);
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);
 }
 
-- 
2.29.2.454.gaff20da3a2-goog


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3] usage: add trace2 entry upon warning()
  2020-11-24 20:05 ` [PATCH v3] " Jonathan Tan
@ 2020-11-24 22:15   ` Junio C Hamano
  0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2020-11-24 22:15 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: git

Jonathan Tan <jonathantanmy@google.com> writes:

> 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.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH] usage: add trace2 entry upon warning()
  2020-11-23 19:04 [RFC PATCH] usage: add trace2 entry upon warning() Jonathan Tan
  2020-11-23 20:45 ` [RFC PATCH v2] " Jonathan Tan
  2020-11-24 20:05 ` [PATCH v3] " Jonathan Tan
@ 2020-11-30 16:16 ` Jeff Hostetler
  2 siblings, 0 replies; 6+ messages in thread
From: Jeff Hostetler @ 2020-11-30 16:16 UTC (permalink / raw)
  To: Jonathan Tan, git



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


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-11-30 16:19 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-23 19:04 [RFC PATCH] usage: add trace2 entry upon warning() Jonathan Tan
2020-11-23 20:45 ` [RFC PATCH v2] " Jonathan Tan
2020-11-23 22:30   ` Junio C Hamano
2020-11-24 20:05 ` [PATCH v3] " Jonathan Tan
2020-11-24 22:15   ` Junio C Hamano
2020-11-30 16:16 ` [RFC PATCH] " Jeff Hostetler

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).