Re: [PATCH] trailer: allow spaces in tokens
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 18, 2022, 16:48 UTC
- Message-ID
- <xmqqo7whqyzs.fsf@gitster.g>
- In-Reply-To
- <pull.1309.git.git.1660806376021.gitgitgadget@gmail.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.
Show 5 quoted lines
> 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.
Show 20 quoted lines
> 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.
Show 56 quoted lines
> + 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