trace2: prevent segfault on config collection where no value specified

PATCH Superseded Adam Murray via GitGitGadget 5 participants 11 messages Nov 7, 2024 – Jan 23, 2025

lore

Share & tools

lore

Adam Murray via GitGitGadget

author

2024-11-07 00:04 UTC #0 lore raw

PATCH 1/1 trace2: prevent segfault on config collection where no value specified

From: Adam Murray <[email protected]>

When TRACE2 analytics is enabled, a git config option that has no value causes a segfault.

Steps to Reproduce
GIT_TRACE2=true GIT_TRACE2_CONFIG_PARAMS=status.*
git -c status.relativePaths version
Expected Result
git version 2.46.0
Actual Result
zsh: segmentation fault GIT_TRACE2=true

This adds a null check to prevent the segfault and instead return the "empty config value" error.

Signed-off-by Adam Murray <[email protected]>

---
trace2: prevent segfault on config collection where no value specified

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1814%2Fad-murray%2Ffix-trace2-segfault-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1814/ad-murray/fix-trace2-segfault-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1814

 t/t0210-trace2-normal.sh | 8 ++++++++
 trace2.c                 | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

t/t0210-trace2-normal.sh +8 -0

index b9adc94aab4..4047ab562a4 100755
@@ -244,6 +244,14 @@ test_expect_success 'bug messages followed by BUG() are written to trace2' '
 	test_cmp expect actual
 '
 
+test_expect_success 'empty configuration values are handled' '
+  test_when_finished "rm trace2.normal actual expect" &&
+  echo >expect &&
+  GIT_TRACE2="$(pwd)/trace2.normal"  GIT_TRACE2_CONFIG_PARAMS=foo.empty \
+	  git -c foo.empty config foo.empty >actual &&
+	test_cmp expect actual
+'
+
 sane_unset GIT_TRACE2_BRIEF
 
 # Now test without environment variables and get all Trace2 settings

trace2.c +1 -1

index f894532d053..5df43478b8f 100644
@@ -759,7 +759,7 @@ void trace2_def_param_fl(const char *file, int line, const char *param,
 	int j;
 	const char *redacted;
 
-	if (!trace2_enabled)
+	if (!trace2_enabled || !value)
 		return;
 
 	redacted = redact_arg(value);

base-commit: 8f8d6eee531b3fa1a8ef14f169b0cb5035f7a772

Signature · 1 line

gitgitgadget

Jeff King

2024-11-07 02:01 UTC #1 lore raw

Quoted 11 lines · Adam Murray via GitGitGadget

When TRACE2 analytics is enabled, a git config option that has no value causes a segfault.
Steps to Reproduce
GIT_TRACE2=true GIT_TRACE2_CONFIG_PARAMS=status.*
git -c status.relativePaths version
Expected Result
git version 2.46.0
Actual Result
zsh: segmentation fault GIT_TRACE2=true

Quoted 2 lines

This adds a null check to prevent the segfault and instead return the "empty config value" error.

We definitely should deal with the NULL here, but I'm not sure that returning an error is correct. A value-less config like this is a synonym for "true". If the point of this code is to dump a trace of config settings, then by returning without printing anything, we're misleading the user.

I.e., doing this, with an explicit value for the config option:

GIT_TRACE2=true GIT_TRACE2_CONFIG_PARAMS=status.* git -c status.relativePaths=true version

should (and does) show:

20:48:11.662470 trace2.c:437 def_param scope:command status.relativepaths=true

If we swap that our for "-c status.relativePaths", then the outcome is the same: we've turned on that config option. But with your patch, the trace won't mention it at all.

trace2.c void trace2_def_param_fl(const char *file, int line, const char *param, · line 765

Quoted 13 lines

trace2.c +1 -1

index f894532d053..5df43478b8f 100644
@@ -759,7 +759,7 @@ void trace2_def_param_fl(const char *file, int line, const char *param,
 	int j;
 	const char *redacted;
 
-	if (!trace2_enabled)
+	if (!trace2_enabled || !value)
 		return;
 
 	redacted = redact_arg(value);

So here I think we need to either:

1. Just quietly substitute "true" for the value. For a bool, the two are equivalent, and this is probably an acceptable fiction for a trace to show. For a non-bool (e.g., something like "author.name"), though, it's an error, and the trace is somewhat misleading.

2. Put in some special marker for the NULL value. Something like "(null)" works, but it's ambiguous with a config of the same value (which obviously you wouldn't expect in normal use, but since the point of tracing is often to debug, I could see it being misleading).

All of this is made harder by the fact that there are multiple output targets. So you'd have to pass the NULL down to them and let them handle it. Something like:

trace2.c +2 -2

index 5df43478b8..e67edf4b1b 100644
@@ -759,10 +759,10 @@ void trace2_def_param_fl(const char *file, int line, const char *param,
 	int j;
 	const char *redacted;
 
-	if (!trace2_enabled || !value)
+	if (!trace2_enabled)
 		return;
 
-	redacted = redact_arg(value);
+	redacted = value ? redact_arg(value) : NULL;
 
 	for_each_wanted_builtin (j, tgt_j)
 		if (tgt_j->pfn_param_fl)

trace2/tr2_tgt_normal.c +3 -2

index baef48aa69..924736ab36 100644
@@ -307,8 +307,9 @@ static void fn_param_fl(const char *file, int line, const char *param,
 	enum config_scope scope = kvi->scope;
 	const char *scope_name = config_scope_name(scope);
 
-	strbuf_addf(&buf_payload, "def_param scope:%s %s=%s", scope_name, param,
-		    value);
+	strbuf_addf(&buf_payload, "def_param scope:%s %s", scope_name, param);
+	if (value)
+		strbuf_addf(&buf_payload, "=%s", value);
 	normal_io_write_fl(file, line, &buf_payload);
 	strbuf_release(&buf_payload);
 }

but you'd need to do the same for each target implementation.

-Peff

Junio C Hamano

2024-11-07 03:02 UTC #2 lore raw

Quoted 11 lines · Jeff King

I.e., doing this, with an explicit value for the config option:
GIT_TRACE2=true GIT_TRACE2_CONFIG_PARAMS=status.* git -c status.relativePaths=true version
should (and does) show:
20:48:11.662470 trace2.c:437 def_param scope:command status.relativepaths=true
If we swap that our for "-c status.relativePaths", then the outcome is the same: we've turned on that config option. But with your patch, the trace won't mention it at all.

which may be improvement, but ideally, the "valueless truth" case should be given differently, perhaps like

20:48:11.662470 trace2.c:437 def_param scope:command status.relativepaths

to allow showing what exactly the system has seen. After all, trace output is often used for debugging, and it is not unusual for a buggy code path to behave on explicit truth and valueless truth differently.

Quoted 22 lines

So here I think we need to either:
1. Just quietly substitute "true" for the value. For a bool, the two are equivalent, and this is probably an acceptable fiction for a trace to show. For a non-bool (e.g., something like "author.name"), though, it's an error, and the trace is somewhat misleading.
2. Put in some special marker for the NULL value. Something like "(null)" works, but it's ambiguous with a config of the same value (which obviously you wouldn't expect in normal use, but since the point of tracing is often to debug, I could see it being misleading).
All of this is made harder by the fact that there are multiple output targets. So you'd have to pass the NULL down to them and let them handle it. Something like:
...
trace2/tr2_tgt_normal.c +0 -0
index baef48aa69..924736ab36 100644

trace2/tr2_tgt_normal.c static void fn_param_fl(const char *file, int line, const char *param, · line 312

Quoted 8 lines

trace2/tr2_tgt_normal.c +3 -2

@@ -307,8 +307,9 @@ static void fn_param_fl(const char *file, int line, const char *param,
 	enum config_scope scope = kvi->scope;
 	const char *scope_name = config_scope_name(scope);
 
-	strbuf_addf(&buf_payload, "def_param scope:%s %s=%s", scope_name, param,
-		    value);
+	strbuf_addf(&buf_payload, "def_param scope:%s %s", scope_name, param);
+	if (value)
+		strbuf_addf(&buf_payload, "=%s", value);

Yes, exactly.

Quoted 5 lines

trace2/tr2_tgt_normal.c +0 -0
 	normal_io_write_fl(file, line, &buf_payload);
 	strbuf_release(&buf_payload);
 }
but you'd need to do the same for each target implementation.

Thanks.

Adam Murray via GitGitGadget

author

2025-01-10 07:28 UTC #3 lore raw

PATCH v2 1/1 trace2: prevent segfault on config collection where no value specified

In reply to Adam Murray via GitGitGadget

From: Adam Murray <[email protected]>

When TRACE2 analytics is enabled, a git config option that has no value causes a segfault.

Steps to Reproduce
GIT_TRACE2=true GIT_TRACE2_CONFIG_PARAMS=status.*
git -c status.relativePaths version
Expected Result
git version 2.46.0
Actual Result
zsh: segmentation fault GIT_TRACE2=true

This adds checks to prevent the segfault and instead return an empty value.

Signed-off-by Adam Murray <[email protected]>

---
trace2: prevent segfault on config collection where no value specified

cc Jeff King [email protected]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1814%2Fad-murray%2Ffix-trace2-segfault-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1814/ad-murray/fix-trace2-segfault-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1814

Range-diff vs v1:

1: 24ba9db7aa1 ! 1: fd7bed52dda trace2: prevent segfault on config collection where no value specified
@@ Commit message
Actual Result
zsh: segmentation fault GIT_TRACE2=true

- This adds a null check to prevent the segfault and instead return
- the "empty config value" error.
+ This adds checks to prevent the segfault and instead return
+ an empty value.

Signed-off-by Adam Murray <[email protected]>

@@ t/t0210-trace2-normal.sh: test_expect_success 'bug messages followed by BUG() ar

## trace2.c ##
@@ trace2.c: void trace2_def_param_fl(const char *file, int line, const char *param,
- int j;
- const char *redacted;
-
-- if (!trace2_enabled)
-+ if (!trace2_enabled || !value)
+ if (!trace2_enabled)
return;

- redacted = redact_arg(value);
+- redacted = redact_arg(value);
++ redacted = value ? redact_arg(value): NULL;
+
+ for_each_wanted_builtin (j, tgt_j)
+ if (tgt_j->pfn_param_fl)
+
+ ## trace2/tr2_tgt_event.c ##
+@@ trace2/tr2_tgt_event.c: static void fn_param_fl(const char *file, int line, const char *param,
+ event_fmt_prepare(event_name, file, line, NULL, &jw);
+ jw_object_string(&jw, "scope", scope_name);
+ jw_object_string(&jw, "param", param);
+- jw_object_string(&jw, "value", value);
++ if (value)
++ jw_object_string(&jw, "value", value);
+ jw_end(&jw);
+
+ tr2_dst_write_line(&tr2dst_event, &jw.json);
+
+ ## trace2/tr2_tgt_normal.c ##
+@@ trace2/tr2_tgt_normal.c: static void fn_param_fl(const char *file, int line, const char *param,
+ enum config_scope scope = kvi->scope;
+ const char *scope_name = config_scope_name(scope);
+
+- strbuf_addf(&buf_payload, "def_param scope:%s %s=%s", scope_name, param,
+- value);
++ strbuf_addf(&buf_payload, "def_param scope:%s %s", scope_name, param);
++ if (value)
++ strbuf_addf(&buf_payload, "=%s", value);
+ normal_io_write_fl(file, line, &buf_payload);
+ strbuf_release(&buf_payload);
+ }
+
+ ## trace2/tr2_tgt_perf.c ##
+@@ trace2/tr2_tgt_perf.c: static void fn_param_fl(const char *file, int line, const char *param,
+ struct strbuf scope_payload = STRBUF_INIT;
+ enum config_scope scope = kvi->scope;
+ const char *scope_name = config_scope_name(scope);
+-
+- strbuf_addf(&buf_payload, "%s:%s", param, value);
++ strbuf_addstr(&buf_payload, param);
++ if (value)
++ strbuf_addf(&buf_payload, ":%s", value);
+ strbuf_addf(&scope_payload, "%s:%s", "scope", scope_name);
+
+ perf_io_write_fl(file, line, event_name, NULL, NULL, NULL,

 t/t0210-trace2-normal.sh | 8 ++++++++
 trace2.c                 | 2 +-
 trace2/tr2_tgt_event.c   | 3 ++-
 trace2/tr2_tgt_normal.c  | 5 +++--
 trace2/tr2_tgt_perf.c    | 5 +++--
 5 files changed, 17 insertions(+), 6 deletions(-)

t/t0210-trace2-normal.sh +8 -0

index b9adc94aab4..4047ab562a4 100755
@@ -244,6 +244,14 @@ test_expect_success 'bug messages followed by BUG() are written to trace2' '
 	test_cmp expect actual
 '
 
+test_expect_success 'empty configuration values are handled' '
+  test_when_finished "rm trace2.normal actual expect" &&
+  echo >expect &&
+  GIT_TRACE2="$(pwd)/trace2.normal"  GIT_TRACE2_CONFIG_PARAMS=foo.empty \
+	  git -c foo.empty config foo.empty >actual &&
+	test_cmp expect actual
+'
+
 sane_unset GIT_TRACE2_BRIEF
 
 # Now test without environment variables and get all Trace2 settings

trace2.c +1 -1

index f894532d053..49e7d1db88f 100644
@@ -762,7 +762,7 @@ void trace2_def_param_fl(const char *file, int line, const char *param,
 	if (!trace2_enabled)
 		return;
 
-	redacted = redact_arg(value);
+	redacted = value ? redact_arg(value): NULL;
 
 	for_each_wanted_builtin (j, tgt_j)
 		if (tgt_j->pfn_param_fl)

trace2/tr2_tgt_event.c +2 -1

index 45b0850a5ec..8e09485c83c 100644
@@ -491,7 +491,8 @@ static void fn_param_fl(const char *file, int line, const char *param,
 	event_fmt_prepare(event_name, file, line, NULL, &jw);
 	jw_object_string(&jw, "scope", scope_name);
 	jw_object_string(&jw, "param", param);
-	jw_object_string(&jw, "value", value);
+	if (value)
+		jw_object_string(&jw, "value", value);
 	jw_end(&jw);
 
 	tr2_dst_write_line(&tr2dst_event, &jw.json);

trace2/tr2_tgt_normal.c +3 -2

index baef48aa698..924736ab360 100644
@@ -307,8 +307,9 @@ static void fn_param_fl(const char *file, int line, const char *param,
 	enum config_scope scope = kvi->scope;
 	const char *scope_name = config_scope_name(scope);
 
-	strbuf_addf(&buf_payload, "def_param scope:%s %s=%s", scope_name, param,
-		    value);
+	strbuf_addf(&buf_payload, "def_param scope:%s %s", scope_name, param);
+	if (value)
+		strbuf_addf(&buf_payload, "=%s", value);
 	normal_io_write_fl(file, line, &buf_payload);
 	strbuf_release(&buf_payload);
 }

trace2/tr2_tgt_perf.c +3 -2

index a6f9a8a193e..19ae7433ef8 100644
@@ -446,8 +446,9 @@ static void fn_param_fl(const char *file, int line, const char *param,
 	struct strbuf scope_payload = STRBUF_INIT;
 	enum config_scope scope = kvi->scope;
 	const char *scope_name = config_scope_name(scope);
-
-	strbuf_addf(&buf_payload, "%s:%s", param, value);
+	strbuf_addstr(&buf_payload, param);
+	if (value)
+		strbuf_addf(&buf_payload, ":%s", value);
 	strbuf_addf(&scope_payload, "%s:%s", "scope", scope_name);
 
 	perf_io_write_fl(file, line, event_name, NULL, NULL, NULL,

base-commit: 8f8d6eee531b3fa1a8ef14f169b0cb5035f7a772

Signature · 1 line

gitgitgadget

Junio C Hamano

2025-01-10 17:10 UTC #4 lore raw

Quoted 4 lines · Adam Murray via GitGitGadget

From: Adam Murray <[email protected]>
When TRACE2 analytics is enabled, a git config option that has no value causes a segfault.

We often call this "valueless true syntax". It may techincally correct to say "has no value", but it is more friendly to readers if you said "a configuration variable that is set to 'true' with the valueless true syntax".

Quoted 18 lines

trace2.c +1 -1

index f894532d053..49e7d1db88f 100644
@@ -762,7 +762,7 @@ void trace2_def_param_fl(const char *file, int line, const char *param,
 	if (!trace2_enabled)
 		return;
 
-	redacted = redact_arg(value);
+	redacted = value ? redact_arg(value): NULL;
 
 	for_each_wanted_builtin (j, tgt_j)
 		if (tgt_j->pfn_param_fl)

trace2/tr2_tgt_event.c +0 -0

index 45b0850a5ec..8e09485c83c 100644

trace2/tr2_tgt_event.c static void fn_param_fl(const char *file, int line, const char *param, · line 496

Quoted 7 lines

trace2/tr2_tgt_event.c +2 -1

@@ -491,7 +491,8 @@ static void fn_param_fl(const char *file, int line, const char *param,
 	event_fmt_prepare(event_name, file, line, NULL, &jw);
 	jw_object_string(&jw, "scope", scope_name);
 	jw_object_string(&jw, "param", param);
-	jw_object_string(&jw, "value", value);
+	if (value)
+		jw_object_string(&jw, "value", value);
 	jw_end(&jw);

OK, so for "valueless true", we do not get the "value" element in the json output, which makes sense. Don't we have documentation that explains what each element in the output means and when they are given? Shouldn't we update it?

trace2/tr2_tgt_event.c static void fn_param_fl(const char *file, int line, const char *param,

Quoted 5 lines

trace2/tr2_tgt_event.c +3 -2

-	strbuf_addf(&buf_payload, "def_param scope:%s %s=%s", scope_name, param,
-		    value);
+	strbuf_addf(&buf_payload, "def_param scope:%s %s", scope_name, param);
+	if (value)
+		strbuf_addf(&buf_payload, "=%s", value);

OK.

The input did not spell the "=value" part and said "[section] key" to mean that section.key is true, so we report that without "=value" part. This also makes sense.

trace2/tr2_tgt_perf.c static void fn_param_fl(const char *file, int line, const char *param, · line 451

Quoted 13 lines

trace2/tr2_tgt_perf.c +3 -2

index a6f9a8a193e..19ae7433ef8 100644
@@ -446,8 +446,9 @@ static void fn_param_fl(const char *file, int line, const char *param,
 	struct strbuf scope_payload = STRBUF_INIT;
 	enum config_scope scope = kvi->scope;
 	const char *scope_name = config_scope_name(scope);
-
-	strbuf_addf(&buf_payload, "%s:%s", param, value);
+	strbuf_addstr(&buf_payload, param);
+	if (value)
+		strbuf_addf(&buf_payload, ":%s", value);

I am not versed well enough in tgt-parf output format to tell if this makes sense. We'd need somebody else to review this part.

Thanks.

Johannes Schindelin

2025-01-22 10:11 UTC #5 lore raw

In reply to Adam Murray via GitGitGadget

Hi Adam,

Quoted 13 lines · Adam Murray via GitGitGadget

From: Adam Murray <[email protected]>
When TRACE2 analytics is enabled, a git config option that has no value causes a segfault.
Steps to Reproduce
GIT_TRACE2=true GIT_TRACE2_CONFIG_PARAMS=status.*
git -c status.relativePaths version
Expected Result
git version 2.46.0
Actual Result
zsh: segmentation fault GIT_TRACE2=true

Quoted 4 lines

This adds checks to prevent the segfault and instead return an empty value.
Signed-off-by Adam Murray <[email protected]>

This patch looks good to me!

Thank you for the fix,
Johannes

Quoted 174 lines

---
trace2: prevent segfault on config collection where no value specified
cc Jeff King [email protected]
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1814%2Fad-murray%2Ffix-trace2-segfault-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1814/ad-murray/fix-trace2-segfault-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1814
Range-diff vs v1:
1: 24ba9db7aa1 ! 1: fd7bed52dda trace2: prevent segfault on config collection where no value specified
@@ Commit message
Actual Result
zsh: segmentation fault GIT_TRACE2=true
- This adds a null check to prevent the segfault and instead return
- the "empty config value" error.
+ This adds checks to prevent the segfault and instead return
+ an empty value.
Signed-off-by Adam Murray <[email protected]>
@@ t/t0210-trace2-normal.sh: test_expect_success 'bug messages followed by BUG() ar
## trace2.c ##
@@ trace2.c: void trace2_def_param_fl(const char *file, int line, const char *param,
- int j;
- const char *redacted;
-
-- if (!trace2_enabled)
-+ if (!trace2_enabled || !value)
+ if (!trace2_enabled)
return;
- redacted = redact_arg(value);
+- redacted = redact_arg(value);
++ redacted = value ? redact_arg(value): NULL;
+
+ for_each_wanted_builtin (j, tgt_j)
+ if (tgt_j->pfn_param_fl)
+
+ ## trace2/tr2_tgt_event.c ##
+@@ trace2/tr2_tgt_event.c: static void fn_param_fl(const char *file, int line, const char *param,
+ event_fmt_prepare(event_name, file, line, NULL, &jw);
+ jw_object_string(&jw, "scope", scope_name);
+ jw_object_string(&jw, "param", param);
+- jw_object_string(&jw, "value", value);
++ if (value)
++ jw_object_string(&jw, "value", value);
+ jw_end(&jw);
+
+ tr2_dst_write_line(&tr2dst_event, &jw.json);
+
+ ## trace2/tr2_tgt_normal.c ##
+@@ trace2/tr2_tgt_normal.c: static void fn_param_fl(const char *file, int line, const char *param,
+ enum config_scope scope = kvi->scope;
+ const char *scope_name = config_scope_name(scope);
+
+- strbuf_addf(&buf_payload, "def_param scope:%s %s=%s", scope_name, param,
+- value);
++ strbuf_addf(&buf_payload, "def_param scope:%s %s", scope_name, param);
++ if (value)
++ strbuf_addf(&buf_payload, "=%s", value);
+ normal_io_write_fl(file, line, &buf_payload);
+ strbuf_release(&buf_payload);
+ }
+
+ ## trace2/tr2_tgt_perf.c ##
+@@ trace2/tr2_tgt_perf.c: static void fn_param_fl(const char *file, int line, const char *param,
+ struct strbuf scope_payload = STRBUF_INIT;
+ enum config_scope scope = kvi->scope;
+ const char *scope_name = config_scope_name(scope);
+-
+- strbuf_addf(&buf_payload, "%s:%s", param, value);
++ strbuf_addstr(&buf_payload, param);
++ if (value)
++ strbuf_addf(&buf_payload, ":%s", value);
+ strbuf_addf(&scope_payload, "%s:%s", "scope", scope_name);
+
+ perf_io_write_fl(file, line, event_name, NULL, NULL, NULL,
 t/t0210-trace2-normal.sh | 8 ++++++++
 trace2.c                 | 2 +-
 trace2/tr2_tgt_event.c   | 3 ++-
 trace2/tr2_tgt_normal.c  | 5 +++--
 trace2/tr2_tgt_perf.c    | 5 +++--
 5 files changed, 17 insertions(+), 6 deletions(-)
t/t0210-trace2-normal.sh +8 -0
index b9adc94aab4..4047ab562a4 100755
@@ -244,6 +244,14 @@ test_expect_success 'bug messages followed by BUG() are written to trace2' '
 	test_cmp expect actual
 '
 
+test_expect_success 'empty configuration values are handled' '
+  test_when_finished "rm trace2.normal actual expect" &&
+  echo >expect &&
+  GIT_TRACE2="$(pwd)/trace2.normal"  GIT_TRACE2_CONFIG_PARAMS=foo.empty \
+	  git -c foo.empty config foo.empty >actual &&
+	test_cmp expect actual
+'
+
 sane_unset GIT_TRACE2_BRIEF
 
 # Now test without environment variables and get all Trace2 settings
trace2.c +1 -1
index f894532d053..49e7d1db88f 100644
@@ -762,7 +762,7 @@ void trace2_def_param_fl(const char *file, int line, const char *param,
 	if (!trace2_enabled)
 		return;
 
-	redacted = redact_arg(value);
+	redacted = value ? redact_arg(value): NULL;
 
 	for_each_wanted_builtin (j, tgt_j)
 		if (tgt_j->pfn_param_fl)
trace2/tr2_tgt_event.c +2 -1
index 45b0850a5ec..8e09485c83c 100644
@@ -491,7 +491,8 @@ static void fn_param_fl(const char *file, int line, const char *param,
 	event_fmt_prepare(event_name, file, line, NULL, &jw);
 	jw_object_string(&jw, "scope", scope_name);
 	jw_object_string(&jw, "param", param);
-	jw_object_string(&jw, "value", value);
+	if (value)
+		jw_object_string(&jw, "value", value);
 	jw_end(&jw);
 
 	tr2_dst_write_line(&tr2dst_event, &jw.json);
trace2/tr2_tgt_normal.c +3 -2
index baef48aa698..924736ab360 100644
@@ -307,8 +307,9 @@ static void fn_param_fl(const char *file, int line, const char *param,
 	enum config_scope scope = kvi->scope;
 	const char *scope_name = config_scope_name(scope);
 
-	strbuf_addf(&buf_payload, "def_param scope:%s %s=%s", scope_name, param,
-		    value);
+	strbuf_addf(&buf_payload, "def_param scope:%s %s", scope_name, param);
+	if (value)
+		strbuf_addf(&buf_payload, "=%s", value);
 	normal_io_write_fl(file, line, &buf_payload);
 	strbuf_release(&buf_payload);
 }
trace2/tr2_tgt_perf.c +3 -2
index a6f9a8a193e..19ae7433ef8 100644
@@ -446,8 +446,9 @@ static void fn_param_fl(const char *file, int line, const char *param,
 	struct strbuf scope_payload = STRBUF_INIT;
 	enum config_scope scope = kvi->scope;
 	const char *scope_name = config_scope_name(scope);
-
-	strbuf_addf(&buf_payload, "%s:%s", param, value);
+	strbuf_addstr(&buf_payload, param);
+	if (value)
+		strbuf_addf(&buf_payload, ":%s", value);
 	strbuf_addf(&scope_payload, "%s:%s", "scope", scope_name);
 
 	perf_io_write_fl(file, line, event_name, NULL, NULL, NULL,
base-commit: 8f8d6eee531b3fa1a8ef14f169b0cb5035f7a772 --
gitgitgadget

Junio C Hamano

2025-01-22 18:12 UTC #6 lore raw

Quoted 2 lines · Johannes Schindelin

This patch looks good to me!

Thanks. As I punted on reviewing the tgt_perf part, it is very good to see somebody else step in to look it over.

Will queue.

Quoted 22 lines

trace2/tr2_tgt_perf.c +3 -2

index a6f9a8a193e..19ae7433ef8 100644
@@ -446,8 +446,9 @@ static void fn_param_fl(const char *file, int line, const char *param,
 	struct strbuf scope_payload = STRBUF_INIT;
 	enum config_scope scope = kvi->scope;
 	const char *scope_name = config_scope_name(scope);
-
-	strbuf_addf(&buf_payload, "%s:%s", param, value);
+	strbuf_addstr(&buf_payload, param);
+	if (value)
+		strbuf_addf(&buf_payload, ":%s", value);
 	strbuf_addf(&scope_payload, "%s:%s", "scope", scope_name);
 
 	perf_io_write_fl(file, line, event_name, NULL, NULL, NULL,

base-commit: 8f8d6eee531b3fa1a8ef14f169b0cb5035f7a772 --
gitgitgadget

Junio C Hamano

2025-01-22 18:18 UTC #7 lore raw

Quoted 9 lines · Junio C Hamano

Quoted 2 lines · Johannes Schindelin
This patch looks good to me!
Thanks. As I punted on reviewing the tgt_perf part, it is very good to see somebody else step in to look it over.
Will queue.

The test part used broken indentation and also the use of test_when_finished was careless, so I'll touch it up before queuing.

No need to resend.

Thanks.

D. Ben Knoble

2025-01-23 17:01 UTC #8 lore raw

Quoted 11 lines · Junio C Hamano

Quoted 8 lines · Junio C Hamano
Quoted 2 lines · Johannes Schindelin
This patch looks good to me!
Thanks. As I punted on reviewing the tgt_perf part, it is very good to see somebody else step in to look it over.

Quoted 9 lines

Quoted 1 line
Will queue.
The test part used broken indentation and also the use of test_when_finished was careless, so I'll touch it up before queuing.
No need to resend.
Thanks.

I was curious what changed, so I found 792a3850fa (trace2: prevent segfault on config collection with valueless true, 2025-01-10) and I noticed this:

- redacted = redact_arg(value);
+ redacted = value ? redact_arg(value): NULL;

I think I expected (based on Documentation/CodingGuidelines) a space before the ternary conditional's colon. Then again, "git grep '?.*[^ ]:' *.[ch]" finds a few other cases with the unspaced style.

Signature · 1 line

D. Ben Knoble

Junio C Hamano

2025-01-23 18:02 UTC #9 lore raw

trace2.c void trace2_def_param_fl(const char *file, int line, const char *param, · line 765

Quoted 6 lines · D. Ben Knoble

I was curious what changed, so I found 792a3850fa (trace2: prevent segfault on config collection with valueless true, 2025-01-10) and I noticed this:
trace2.c +1 -1
-       redacted = redact_arg(value);
+       redacted = value ? redact_arg(value): NULL;

Oh, I do not think I tweaked any of the code, other than fixing the new test that was not properly formatted. Mostly I touched up the log message.

Will touch it up. Thanks for a careful reading.

D. Ben Knoble

2025-01-23 21:15 UTC #10 lore raw

Quoted 7 lines · Junio C Hamano

Quoted 4 lines · D. Ben Knoble
I was curious what changed, so I found 792a3850fa (trace2: prevent segfault on config collection with valueless true, 2025-01-10) and I noticed this:

Quoted 6 lines

trace2.c void trace2_def_param_fl(const char *file, int line, const char *param, · line 765
Quoted 2 lines
trace2.c +1 -1
-       redacted = redact_arg(value);
+       redacted = value ? redact_arg(value): NULL;
Oh, I do not think I tweaked any of the code, other than fixing the new test that was not properly formatted. Mostly I touched up the log message.

I could have been clearer—I think this was in the original. I just happened to notice it because I was curious about what touch-ups you performed :)

Signature · 1 line

D. Ben Knoble