Skip to contentSkip to conversation

sideband: skip unrecognized allowControlCharacters keywords

PATCH In review Thomas Bachem via GitGitGadget 4 participants 6 messages Oct 16 – Nov 7

lore
Share & tools
lore

Thomas Bachem via GitGitGadget

author
#0 lore raw

PATCH 1/1 sideband: skip unrecognized allowControlCharacters keywords

From: Thomas Bachem <[email protected]>

Since 128914438a (sideband: add options to allow more control sequences to be passed through, 2026-03-05), the value of sideband.allowControlCharacters is a comma-separated list of keywords. When the parser meets a keyword it does not know, it warns but stays on that keyword, so the warning repeats forever.

As the setting is read whenever a transport is set up, a typo like "colour", a list written as "color, cursor" with a space, or a keyword that only a newer version of Git knows is enough to make every fetch, push, clone and ls-remote hang this way.

Move on to the next keyword after the warning, and name only the unrecognized keyword in it, not the rest of the list.

Assisted-by Claude Opus 5.5
Signed-off-by Thomas Bachem <[email protected]>

---
sideband: skip unrecognized allowControlCharacters keywords

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2261%2Fthomasbachem%2Fsideband-control-characters-loop-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2261/thomasbachem/sideband-control-characters-loop-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2261

 sideband.c                          | 19 ++++++++++++-------
 t/t5409-colorize-remote-messages.sh | 25 +++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 7 deletions(-)
sideband.c +12 -7
index 374187402e..ed6840bf89 100644
@@ -62,18 +62,23 @@ int sideband_allow_control_characters_config(const char *var, const char *value)
 
 	allow_control_characters = ALLOW_NO_CONTROL_CHARACTERS;
 	while (*value) {
-		if (skip_prefix_in_csv(value, "color", &value))
+		if (skip_prefix_in_csv(value, "color", &value)) {
 			allow_control_characters |= ALLOW_ANSI_COLOR_SEQUENCES;
-		else if (skip_prefix_in_csv(value, "cursor", &value))
+		} else if (skip_prefix_in_csv(value, "cursor", &value)) {
 			allow_control_characters |= ALLOW_ANSI_CURSOR_MOVEMENTS;
-		else if (skip_prefix_in_csv(value, "erase", &value))
+		} else if (skip_prefix_in_csv(value, "erase", &value)) {
 			allow_control_characters |= ALLOW_ANSI_ERASE;
-		else if (skip_prefix_in_csv(value, "true", &value))
+		} else if (skip_prefix_in_csv(value, "true", &value)) {
 			allow_control_characters = ALLOW_ALL_CONTROL_CHARACTERS;
-		else if (skip_prefix_in_csv(value, "false", &value))
+		} else if (skip_prefix_in_csv(value, "false", &value)) {
 			allow_control_characters = ALLOW_NO_CONTROL_CHARACTERS;
-		else
-			warning(_("unrecognized value for '%s': '%s'"), var, value);
+		} else {
+			const char *end = strchrnul(value, ',');
+
+			warning(_("unrecognized value for '%s': '%.*s'"),
+				var, (int)(end - value), value);
+			value = end + !!*end;
+		}
 	}
 	return 0;
 }
t/t5409-colorize-remote-messages.sh +25 -0
index 5da8b0ad64..6117579eb1 100755
@@ -190,4 +190,29 @@ test_expect_success 'allow all control sequences for a specific URL' '
 	test_grep ! "\\^\\[\\[K" decoded
 '
 
+test_expect_success 'unrecognized keywords in allowControlCharacters are skipped' '
+	write_script .git/eraser <<-\EOF &&
+	printf "error: Ohai!\\r\\033[K" >&2
+	exec "$@"
+	EOF
+	test_config_global uploadPack.packObjectsHook ./eraser &&
+	test_commit unrecognized-keyword &&
+
+	# Cap the output, so that a parser stuck on the unrecognized
+	# keyword fails the test instead of warning forever.
+	git -c sideband.allowControlCharacters=bogus,erase \
+		ls-remote . 2>&1 >/dev/null |
+	head -n 2 >warnings &&
+	test_line_count = 1 warnings &&
+	test_grep "allowcontrolcharacters.: .bogus.$" warnings &&
+
+	rm -rf throw-away &&
+	git -c sideband.allowControlCharacters=bogus,erase \
+		clone --no-local . throw-away 2>stderr &&
+	test_decode_color <stderr >color-decoded &&
+	test_decode_csi <color-decoded >decoded &&
+	test_grep "CSI \\[K" decoded &&
+	test_grep ! "\\^\\[\\[K" decoded
+'
+
 test_done

base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd

Signature · 1 line
gitgitgadget
#1 lore raw

sideband.c int sideband_allow_control_characters_config(const char *var, const char *value) · line 81

Quoted 4 lines · Thomas Bachem via GitGitGadget
sideband.c +4 -0
+			warning(_("unrecognized value for '%s': '%.*s'"),
+				var, (int)(end - value), value);
+			value = end + !!*end;
+		}

Reading CodingGuidelines, it seems that !! is a construct we would like to avoid:

- Some clever tricks, like using the !! operator with arithmetic constructs, can be extremely confusing to others. Avoid them, unless there is a compelling reason to use them.

--Tuomas

#2 lore raw
Quoted 4 lines · Tuomas Ahola
Quoted 1 line

+ value = end + !!*end;

Reading CodingGuidelines, it seems that !! is a construct we would like to avoid:

I wrote it this way to match skip_prefix_in_csv() a few lines above, which steps past the comma the same way:

*out = value + !!*value;

so the two places in this parser read alike. Happy to spell it out as a conditional in a v2 if that's preferred, though.

Thomas

#3 lore raw
In reply to Tuomas Ahola
Quoted 10 lines · Tuomas Ahola

sideband.c int sideband_allow_control_characters_config(const char *var, const char *value) · line 81

Quoted 4 lines · Thomas Bachem via GitGitGadget
sideband.c +4 -0
+			warning(_("unrecognized value for '%s': '%.*s'"),
+				var, (int)(end - value), value);
+			value = end + !!*end;
+		}

Reading CodingGuidelines, it seems that !! is a construct we would like to avoid:

Quoted 5 lines

- Some clever tricks, like using the !! operator with arithmetic constructs, can be extremely confusing to others. Avoid them, unless there is a compelling reason to use them.

--Tuomas

But here it is compelling. You want to normalize random byte into 0 or 1, depending on "is it 0 or non-zero?" criteria, and it certainly is easier to read than (*end ? 1 : 0).

Quite honestly, I do not think !! qualifies as "clever trick that is confusing" anymore. Perhaps putting the rule there is misguided.

Thanks.

#4 lore raw
Quoted 16 lines · Junio C Hamano
Quoted 14 lines · Tuomas Ahola

sideband.c int sideband_allow_control_characters_config(const char *var, const char *value) · line 81

Quoted 4 lines · Thomas Bachem via GitGitGadget
sideband.c +4 -0
+			warning(_("unrecognized value for '%s': '%.*s'"),
+				var, (int)(end - value), value);
+			value = end + !!*end;
+		}

Reading CodingGuidelines, it seems that !! is a construct we would like to avoid:

- Some clever tricks, like using the !! operator with arithmetic constructs, can be extremely confusing to others. Avoid them, unless there is a compelling reason to use them.

Quoted 6 lines
Quoted 1 line

--Tuomas

But here it is compelling. You want to normalize random byte into 0 or 1, depending on "is it 0 or non-zero?" criteria, and it certainly is easier to read than (*end ? 1 : 0).

Uh, of course. Sorry for my mental short-circuit.

Quoted 4 lines

Quite honestly, I do not think !! qualifies as "clever trick that is confusing" anymore. Perhaps putting the rule there is misguided.

Thanks.

Okay. I'll consider writing a patch to drop that from CodingGuidelines.

#5 lore raw
In reply to Thomas Bachem via GitGitGadget
Quoted 43 lines · Thomas Bachem via GitGitGadget

From: Thomas Bachem <[email protected]>

Since 128914438a (sideband: add options to allow more control sequences to be passed through, 2026-03-05), the value of sideband.allowControlCharacters is a comma-separated list of keywords. When the parser meets a keyword it does not know, it warns but stays on that keyword, so the warning repeats forever.

As the setting is read whenever a transport is set up, a typo like "colour", a list written as "color, cursor" with a space, or a keyword that only a newer version of Git knows is enough to make every fetch, push, clone and ls-remote hang this way.

Move on to the next keyword after the warning, and name only the unrecognized keyword in it, not the rest of the list.

Assisted-by Claude Opus 5.5
Signed-off-by Thomas Bachem <[email protected]>

---
sideband: skip unrecognized allowControlCharacters keywords

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2261%2Fthomasbachem%2Fsideband-control-characters-loop-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2261/thomasbachem/sideband-control-characters-loop-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2261

 sideband.c                          | 19 ++++++++++++-------
 t/t5409-colorize-remote-messages.sh | 25 +++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 7 deletions(-)
sideband.c +3 -0
index 374187402e..ed6840bf89 100644
@@ -62,18 +62,23 @@ int sideband_allow_control_characters_config(const char *var, const char *value)
 
 	allow_control_characters = ALLOW_NO_CONTROL_CHARACTERS;
 	while (*value) {
+		if (skip_prefix_in_csv(value, "color", &value)) {
 			allow_control_characters |= ALLOW_ANSI_COLOR_SEQUENCES;
+		} else if (skip_prefix_in_csv(value, "cursor", &value)) {
 			allow_control_characters |= ALLOW_ANSI_CURSOR_MOVEMENTS;
+		} else if (skip_prefix_in_csv(value, "erase", &value)) {
 			allow_control_characters |= ALLOW_ANSI_ERASE;

sideband.c int sideband_allow_control_characters_config(const char *var, const char *value) · line 80

Quoted 10 lines
sideband.c +8 -0
+		} else if (skip_prefix_in_csv(value, "true", &value)) {
 			allow_control_characters = ALLOW_ALL_CONTROL_CHARACTERS;
+		} else if (skip_prefix_in_csv(value, "false", &value)) {
 			allow_control_characters = ALLOW_NO_CONTROL_CHARACTERS;
+		} else {
+			const char *end = strchrnul(value, ',');
+
+			warning(_("unrecognized value for '%s': '%.*s'"),
+				var, (int)(end - value), value);
+			value = end + !!*end;

OK, so this assignment to "value" is the only thing that is new.

sideband.c int sideband_allow_control_characters_config(const char *var, const char *value) · line 81

Quoted 1 line
sideband.c +1 -0
+		}

Somehow the review thread seems to have been sidetracked and left the topic in "needs review' state. The above looks reasonable to me.

It might even make sense to allow whitespaces after the comma, but that would fall into "enhancement" category, and I would prefer to keep the change required to fix things to the minimum.

Quoted 14 lines
sideband.c +0 -0
 	}
 	return 0;
 }
t/t5409-colorize-remote-messages.sh +3 -0
index 5da8b0ad64..6117579eb1 100755
@@ -190,4 +190,29 @@ test_expect_success 'allow all control sequences for a specific URL' '
 	test_grep ! "\\^\\[\\[K" decoded
 '
 
+test_expect_success 'unrecognized keywords in allowControlCharacters are skipped' '
+	write_script .git/eraser <<-\EOF &&
+	printf "error: Ohai!\\r\\033[K" >&2

t/t5409-colorize-remote-messages.sh test_expect_success 'allow all control sequences for a specific URL' ' · line 205

Quoted 10 lines
t/t5409-colorize-remote-messages.sh +10 -0
+	exec "$@"
+	EOF
+	test_config_global uploadPack.packObjectsHook ./eraser &&
+	test_commit unrecognized-keyword &&
+
+	# Cap the output, so that a parser stuck on the unrecognized
+	# keyword fails the test instead of warning forever.
+	git -c sideband.allowControlCharacters=bogus,erase \
+		ls-remote . 2>&1 >/dev/null |
+	head -n 2 >warnings &&

A well thought out testing method. I like it.

t/t5409-colorize-remote-messages.sh test_expect_success 'allow all control sequences for a specific URL' ' · line 218

Quoted 13 lines
t/t5409-colorize-remote-messages.sh +12 -0
+	test_line_count = 1 warnings &&
+	test_grep "allowcontrolcharacters.: .bogus.$" warnings &&
+
+	rm -rf throw-away &&
+	git -c sideband.allowControlCharacters=bogus,erase \
+		clone --no-local . throw-away 2>stderr &&
+	test_decode_color <stderr >color-decoded &&
+	test_decode_csi <color-decoded >decoded &&
+	test_grep "CSI \\[K" decoded &&
+	test_grep ! "\\^\\[\\[K" decoded
+'
+
 test_done

Thanks. Let's mark the topic as ready for 'next'.