Re: [PATCH] sideband: skip unrecognized allowControlCharacters keywords

[Junio C Hamano <gitster@pobox.com>]

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