[PATCH] sideband: skip unrecognized allowControlCharacters keywords

[PATCH] sideband: skip unrecognized allowControlCharacters keywords

[Thomas Bachem via GitGitGadget]

From: Thomas Bachem <mail@thomasbachem.com>

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 <mail@thomasbachem.com>
---
    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(-)

diff --git a/sideband.c b/sideband.c
index 374187402e..ed6840bf89 100644
--- a/sideband.c
+++ b/sideband.c
@@ -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;
 }
diff --git a/t/t5409-colorize-remote-messages.sh b/t/t5409-colorize-remote-messages.sh
index 5da8b0ad64..6117579eb1 100755
--- a/t/t5409-colorize-remote-messages.sh
+++ b/t/t5409-colorize-remote-messages.sh
@@ -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
-- 
gitgitgadget

Re: [PATCH] sideband: skip unrecognized allowControlCharacters keywords

[Tuomas Ahola]

"Thomas Bachem via GitGitGadget" <gitgitgadget@gmail.com> wrote:

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

Re: [PATCH] sideband: skip unrecognized allowControlCharacters keywords

[Thomas Bachem]

Tuomas Ahola <taahol@utu.fi> wrote:
>> + 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

Re: [PATCH] sideband: skip unrecognized allowControlCharacters keywords

[Junio C Hamano]

Tuomas Ahola <taahol@utu.fi> writes:

> "Thomas Bachem via GitGitGadget" <gitgitgadget@gmail.com> wrote:
>
>> +			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

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.

Re: [PATCH] sideband: skip unrecognized allowControlCharacters keywords

[Tuomas Ahola]

Junio C Hamano <gitster@pobox.com> wrote:

> Tuomas Ahola <taahol@utu.fi> writes:
> 
> > "Thomas Bachem via GitGitGadget" <gitgitgadget@gmail.com> wrote:
> >
> >> +			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
> 
> 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.

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