Re: [PATCH] trailer: allow spaces in tokens

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

[jc: summoning an area expert and the developer whose commit is
blamed for the breakage by adding their addresses on Cc: list]

"Max Bernstein via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Max Bernstein <max@bernsteinbear.com>
>
> The docs (for example, https://git-scm.com/docs/git-interpret-trailers)

Refer to "git help interpret-trailers" instead.

> say that whitespace should be allowed inside tokens:
>
>> There can also be whitespaces inside the token and the value.
>
> The code since e4319562bc2834096fade432fd90c985b96476db does not allow

Refer to an existing commit like so

	e4319562 (trailer: be stricter in parsing separators, 2016-11-02)

> that, so re-enable and add a test.

Jonathan, do you remember if the "be stricter" was in a response to
specific real world issues, or is this "we no longer allow whitespace"
an unintended side effect of the change?

If the former, an equally valid "fix" to what Max reports here would
be to add such a real world issue or two as new tests to demonostrate
why allowing whitespaces was a mistake that hurts real-world workflow,
and fix the documentation.

I actually am suspecting it is the latter, only because we would have
added a testcase to show why whitespaces in the trailer label is a
bad idea in e4319562 if this was triggered by a real-world issue.

I would say that it would be a disaster, if we took any random
line with colon : in it in the middle of the commit message and
mistook it as a trailer (like the line above), but since we do not
allow paragraph breaks in the trailer block, as long as the message
has a valid trailer block, it might not be a huge issue.  I dunno.

> diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
> index 97f10905d23..47bf83003ef 100755
> --- a/t/t7513-interpret-trailers.sh
> +++ b/t/t7513-interpret-trailers.sh
> @@ -1481,6 +1481,46 @@ test_expect_success 'only-trailers omits non-trailer in middle of block' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'supports spaces inside token' '
> +	git config --unset trailer.sign.command &&
> +	cat >expected <<-\EOF &&
> +		Signed-off-by: nobody <nobody@nowhere>
> +		some other trailer: a value
> +		Signed-off-by: somebody <somebody@somewhere>
> +	EOF
> +	echo "wrote to expected" 1>&2 &&
> +	git interpret-trailers --only-trailers >actual <<-\EOF &&
> +		subject
> +
> +		it is important that the trailers below are signed-off-by

It also is important that if you add colon at the end of the above
line, it is *not* mistaken as a trailer with whitespace in token,
with an empty value.

> +		so that they meet the "25% trailers Git knows about" heuristic
> +
> +		Signed-off-by: nobody <nobody@nowhere>
> +		some other trailer: a value
> +		Signed-off-by: somebody <somebody@somewhere>
> +	EOF
> +	test_cmp expected actual
> +'
> +
> +test_expect_success 'does not support space at beginning of token' '
> +	cat >expected <<-\EOF &&
> +		Signed-off-by: nobody <nobody@nowhere> not a trailer: thing
> +		Signed-off-by: somebody <somebody@somewhere>
> +	EOF
> +	echo "wrote to expected" 1>&2 &&
> +	git interpret-trailers --only-trailers --unfold >actual <<-\EOF &&
> +		subject
> +
> +		it is important that the trailers below are signed-off-by
> +		so that they meet the "25% trailers Git knows about" heuristic
> +
> +		Signed-off-by: nobody <nobody@nowhere>
> +		 not a trailer: thing
> +		Signed-off-by: somebody <somebody@somewhere>
> +	EOF
> +	test_cmp expected actual
> +'
> +
>  test_expect_success 'only input' '
>  	git config trailer.sign.command "echo config-value" &&
>  	cat >expected <<-\EOF &&
> diff --git a/trailer.c b/trailer.c
> index d419c20735e..d02a9154512 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -618,17 +618,12 @@ static int token_matches_item(const char *tok, struct arg_item *item, size_t tok
>   */
>  static ssize_t find_separator(const char *line, const char *separators)
>  {
> -	int whitespace_found = 0;
>  	const char *c;
>  	for (c = line; *c; c++) {
>  		if (strchr(separators, *c))
>  			return c - line;
> -		if (!whitespace_found && (isalnum(*c) || *c == '-'))
> +		if (isalnum(*c) || *c == '-' || (c != line && (*c == ' ' || *c == '\t')))
>  			continue;
> -		if (c != line && (*c == ' ' || *c == '\t')) {
> -			whitespace_found = 1;
> -			continue;
> -		}
>  		break;
>  	}
>  	return -1;
>
> base-commit: ad60dddad72dfb8367bd695028b5b8dc6c33661b