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

Re: [PATCH] sideband: skip unrecognized allowControlCharacters keywords

[Junio C Hamano]

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

> 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)) {
>  			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;
> +		} 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.

> +		}

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.

>  	}
>  	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 &&

A well thought out testing method.  I like it.

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