[PATCH] trailer: allow spaces in tokens

[PATCH] trailer: allow spaces in tokens

[Max Bernstein via GitGitGadget]

From: Max Bernstein <max@bernsteinbear.com>

The docs (for example, https://git-scm.com/docs/git-interpret-trailers)
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
that, so re-enable and add a test.

Signed-off-by: Max Bernstein <max@bernsteinbear.com>
---
    trailer: allow spaces in tokens

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1309%2Ftekknolagi%2Fmaint-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1309/tekknolagi/maint-v1
Pull-Request: https://github.com/git/git/pull/1309

 t/t7513-interpret-trailers.sh | 40 +++++++++++++++++++++++++++++++++++
 trailer.c                     |  7 +-----
 2 files changed, 41 insertions(+), 6 deletions(-)

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

[PATCH v2] trailer: allow spaces in tokens

[Max Bernstein via GitGitGadget]

From: Max Bernstein <max@bernsteinbear.com>

The docs (for example, https://git-scm.com/docs/git-interpret-trailers)
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
that, so re-enable and add a test.

Signed-off-by: Max Bernstein <max@bernsteinbear.com>
---
    trailer: allow spaces in tokens
    
    Changed since v1:
    
     * "Fixed" a format-patch test. I'm not sure if the new output is what
       everyone might expect for this particular case.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1309%2Ftekknolagi%2Fmaint-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1309/tekknolagi/maint-v2
Pull-Request: https://github.com/git/git/pull/1309

Range-diff vs v1:

 1:  404a6d1b193 ! 1:  4d490851ac2 trailer: allow spaces in tokens
     @@ Commit message
      
          Signed-off-by: Max Bernstein <max@bernsteinbear.com>
      
     + ## t/t4014-format-patch.sh ##
     +@@ t/t4014-format-patch.sh: test_expect_success 'signoff: not really a signoff' '
     + 	4:Subject: [PATCH] subject
     + 	8:
     + 	9:I want to mention about Signed-off-by: here.
     +-	10:
     +-	11:Signed-off-by: C O Mitter <committer@example.com>
     ++	10:Signed-off-by: C O Mitter <committer@example.com>
     + 	EOF
     + 	test_cmp expect actual
     + '
     +
       ## t/t7513-interpret-trailers.sh ##
      @@ t/t7513-interpret-trailers.sh: test_expect_success 'only-trailers omits non-trailer in middle of block' '
       	test_cmp expected actual


 t/t4014-format-patch.sh       |  3 +--
 t/t7513-interpret-trailers.sh | 40 +++++++++++++++++++++++++++++++++++
 trailer.c                     |  7 +-----
 3 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index fbec8ad2ef7..db95cb6ee66 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1544,8 +1544,7 @@ test_expect_success 'signoff: not really a signoff' '
 	4:Subject: [PATCH] subject
 	8:
 	9:I want to mention about Signed-off-by: here.
-	10:
-	11:Signed-off-by: C O Mitter <committer@example.com>
+	10:Signed-off-by: C O Mitter <committer@example.com>
 	EOF
 	test_cmp expect actual
 '
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
+		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
-- 
gitgitgadget

Re: [PATCH] trailer: allow spaces in tokens

[Junio C Hamano]

[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

Re: [PATCH v2] trailer: allow spaces in tokens

[Junio C Hamano]

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

> Range-diff vs v1:
>
>  1:  404a6d1b193 ! 1:  4d490851ac2 trailer: allow spaces in tokens
>      @@ Commit message
>       
>           Signed-off-by: Max Bernstein <max@bernsteinbear.com>
>       
>      + ## t/t4014-format-patch.sh ##
>      +@@ t/t4014-format-patch.sh: test_expect_success 'signoff: not really a signoff' '
>      + 	4:Subject: [PATCH] subject
>      + 	8:
>      + 	9:I want to mention about Signed-off-by: here.
>      +-	10:
>      +-	11:Signed-off-by: C O Mitter <committer@example.com>
>      ++	10:Signed-off-by: C O Mitter <committer@example.com>
>      + 	EOF
>      + 	test_cmp expect actual
>      + '

So, the updated code mistook the body of the message that is not a
sign-off, because there is a colon on the line, the line does not
begin with the colon, and everything before the colon is an alnum or
a whitespace, so squashed the paragraph break before the real
trailer block and the last line of the body and made it a body-less
commit log message?

This might be a good demonstration of why it is a mistaken design to
allow whitespaces, which may steer us toward fixing the documentation?

I dunno.  What do others think?

Re: [PATCH] trailer: allow spaces in tokens

[Jonathan Tan]

Junio C Hamano <gitster@pobox.com> writes:
> 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?

Rereading the relevant mailing list thread, I think it's a bit of both.
The sequence of events was as follows:
 - I made a patch set that changed sequencer.c (which, among other
   things, controls the behavior of "commit -s") to use
   interpret-trailers's functionality. This caused a change in the
   behavior of a "commit -s" unit test (the "I want to mention about
   Signed-off-by: here." one), because "commit -s" previously did not
   support spaces before the separator. [1]
 - You mentioned that perhaps we should tighten it back. [2]
 - I agreed and tightened it, but didn't change the documentation. [3]

[1] https://lore.kernel.org/git/a416ab9b-ff1f-9a71-3e58-60fd4f8a6b8e@google.com/
[2] https://lore.kernel.org/git/xmqqtwbroykt.fsf@gitster.mtv.corp.google.com/
[3] https://lore.kernel.org/git/cover.1478028700.git.jonathantanmy@google.com/

So the "real world issue" was a test in our test suite, and perhaps the
side effect was unintended in that we wanted to preserve the behavior of
"commit -s" but didn't think about other possible uses of
interpret-trailers.

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

The "I want to mention about Signed-off-by: here." test might be a
sufficient 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.

Yes, it would be fine if the paragraph was two or more lines long, but
not if it's a single line.

Re: [PATCH v2] trailer: allow spaces in tokens

[Jonathan Tan]

Junio C Hamano <gitster@pobox.com> writes:
> "Max Bernstein via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > Range-diff vs v1:
> >
> >  1:  404a6d1b193 ! 1:  4d490851ac2 trailer: allow spaces in tokens
> >      @@ Commit message
> >       
> >           Signed-off-by: Max Bernstein <max@bernsteinbear.com>
> >       
> >      + ## t/t4014-format-patch.sh ##
> >      +@@ t/t4014-format-patch.sh: test_expect_success 'signoff: not really a signoff' '
> >      + 	4:Subject: [PATCH] subject
> >      + 	8:
> >      + 	9:I want to mention about Signed-off-by: here.
> >      +-	10:
> >      +-	11:Signed-off-by: C O Mitter <committer@example.com>
> >      ++	10:Signed-off-by: C O Mitter <committer@example.com>
> >      + 	EOF
> >      + 	test_cmp expect actual
> >      + '
> 
> So, the updated code mistook the body of the message that is not a
> sign-off, because there is a colon on the line, the line does not
> begin with the colon, and everything before the colon is an alnum or
> a whitespace, so squashed the paragraph break before the real
> trailer block and the last line of the body and made it a body-less
> commit log message?
> 
> This might be a good demonstration of why it is a mistaken design to
> allow whitespaces, which may steer us toward fixing the documentation?
> 
> I dunno.  What do others think?

Yeah, this was the same issue we discussed a few years ago [1]. "git
commit -s" has never supported spaces before the colon (at least, it did
not support it before my patch set in [1] and does not support it now)
so it seems that having no spaces is already the established convention,
and I think that interpret-trailers should follow it. I would agree that
we should fix the documentation to match the current behavior.

[1] https://lore.kernel.org/git/a416ab9b-ff1f-9a71-3e58-60fd4f8a6b8e@google.com/

Re: [PATCH] trailer: allow spaces in tokens

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> The "I want to mention about Signed-off-by: here." test might be a
> sufficient 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.
>
> Yes, it would be fine if the paragraph was two or more lines long, but
> not if it's a single line.

OK, so it seems that we want to fix the documentation, and we have
sufficient test coverage (I didn't notice that Max needed to adjust
the tests to accomodate the new "loose" definition when I wrote the
message you were responding to).

THanks.

Re: [PATCH v2] trailer: allow spaces in tokens

[Maxwell Bernstein]

On Thu Aug 18, 2022 at 9:54 AM PDT, Junio C Hamano wrote:
> So, the updated code mistook the body of the message that is not a
> sign-off, because there is a colon on the line, the line does not
> begin with the colon, and everything before the colon is an alnum or
> a whitespace, so squashed the paragraph break before the real
> trailer block and the last line of the body and made it a body-less
> commit log message?

I agree that it is not ideal, and I'm not sure how to fix it. This commit
probably shouldn't go in as-is; I modified the test in order to demonstrate
this problem.

> This might be a good demonstration of why it is a mistaken design to
> allow whitespaces, which may steer us toward fixing the documentation?
>
> I dunno.  What do others think?

I think allowing whitespace is good at least for the Phabricator project, which
despite being dead, has a number of existing users and existing commits. It
unfortunately has a "Differential revision" trailer with whitespace.

Re: [PATCH v2] trailer: allow spaces in tokens

[Christian Couder]

On Thu, Aug 18, 2022 at 9:03 PM Maxwell Bernstein <tekk.nolagi@gmail.com> wrote:
>
> On Thu Aug 18, 2022 at 9:54 AM PDT, Junio C Hamano wrote:
> > So, the updated code mistook the body of the message that is not a
> > sign-off, because there is a colon on the line, the line does not
> > begin with the colon, and everything before the colon is an alnum or
> > a whitespace, so squashed the paragraph break before the real
> > trailer block and the last line of the body and made it a body-less
> > commit log message?
>
> I agree that it is not ideal, and I'm not sure how to fix it. This commit
> probably shouldn't go in as-is; I modified the test in order to demonstrate
> this problem.
>
> > This might be a good demonstration of why it is a mistaken design to
> > allow whitespaces, which may steer us toward fixing the documentation?
> >
> > I dunno.  What do others think?
>
> I think allowing whitespace is good at least for the Phabricator project, which
> despite being dead, has a number of existing users and existing commits. It
> unfortunately has a "Differential revision" trailer with whitespace.

I think allowing one white space before the separator is a good idea
for the following reasons.

First, if people use something like #, !, ~ or % as a separator, for
example in the case of a trailer like "Bug #43", it is very natural to
have a white space before the # separator.

Note that GitLab for example uses the ~N format, where N is a number,
for issues, !N for merge requests and %N for milestones. I think
Bugzilla and many other bug trackers use a #N format for bug numbers.

Also in some languages, like French, the typographical rules when
writing regular text is to have a space (technically it's supposed to
be a "non breaking space", see:
https://en.wikipedia.org/wiki/Non-breaking_space, but in practice
people use a regular space most of the time) before a colon. So it is
very natural for a number of people in the world to automatically add
a white space before a colon.

https://en.wikipedia.org/wiki/Colon_(punctuation)#Spacing says: "In
print, a thin space was traditionally placed before a colon and a
thick space after it. In modern English-language printing, no space is
placed before a colon and a single space is placed after it. In
French-language typing and printing, the traditional rules are
preserved. "

I think it would be very annoying for users to find out that a number
of otherwise proper trailers would not be taken into account only
because they have a white space before the colon. At least there
should be an option to allow that.

Re: [PATCH v2] trailer: allow spaces in tokens

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> I think allowing one white space before the separator is a good idea
> for the following reasons.
> ...
> I think it would be very annoying for users to find out that a number
> of otherwise proper trailers would not be taken into account only
> because they have a white space before the colon. At least there
> should be an option to allow that.

In short, you do not support Max's patch that allows arbitrary
number of white spaces anywhere but at the beginning of line,
but see a room for unrelated improvement from the current code,
namely, to allow exactly one optional space, immediately before
the separator and nowhere else.

It may be a reasonable thing to do that may not break too many
things.  I do not know if that is loose enough to satisify Max's
original wish, though.

Thanks.

Re: [PATCH v2] trailer: allow spaces in tokens

[Junio C Hamano]

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

> In short, you do not support Max's patch that allows arbitrary
> number of white spaces anywhere but at the beginning of line,
> but see a room for unrelated improvement from the current code,
> namely, to allow exactly one optional space, immediately before
> the separator and nowhere else.

Ah, no, sorry, I misread the situation.  It's not a room for
improvement.  It is very close to what the current code already
does, i.e. to allow optional spaces immediately before the
separator.  The difference is that the current code allows arbitrary
number of optional spaces, not zero or exactly one.

So the only thing we need to do is to update the documentation that
Max misread as allowing spaces at arbitrary place to reflect what
the code has been doing, i.e. an optional run of spaces is allowed
between the "token" and the "separator"?

Anybody wants to do the honors to produce such a patch, then?

Thanks.


    

Re: [PATCH v2] trailer: allow spaces in tokens

[Christian Couder]

On Fri, Aug 19, 2022 at 6:33 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > In short, you do not support Max's patch that allows arbitrary
> > number of white spaces anywhere but at the beginning of line,

I haven't closely followed the patches that might have tweaked the
original default behavior regarding whitespaces in tokens, but I think
in general it's not a good idea to change the default behavior for no
very good reason, because it could cause regressions in the way others
already use `git interpret-trailers`. It could have been Ok to accept
that during a few years after the original implementation was merged
in 2014, but these days people and tools (like GitLab for example)
rely on trailers and `git interpret-trailers` more and more.

So in general I think it's better to have a more cautious approach
now, like we usually have for other commands, and only allow new
options to tweak the default behavior. If one such option appears to
be very widely used, then we might decide later that it should become
the default behavior.

I understand that having commands with many options might not be
considered by some of us as good design, but if we don't like that,
then we could perhaps just have only one option, maybe called
something like "trailer.tokenFormat", or just "trailer.format", that
would contain a regex that every token, or trailer, should match.

It's a complex subject because there are different and competing
viewpoints. On one hand it can be annoying to have some trailers
ignored just because they don't match the default format in a very
small way. (And rewriting trailers could mean rewriting published
history which is not an easy thing to do.) On the other hand if we
develop something like "trailer.tokenFormat" that allows everything,
then we might consider that we should instead encourage people to do
the right thing and to use as much as possible regular trailers with a
strict format. So allowing new options to only tweak things in a small
way might be the right thing to do after all.

> > but see a room for unrelated improvement from the current code,
> > namely, to allow exactly one optional space, immediately before
> > the separator and nowhere else.
>
> Ah, no, sorry, I misread the situation.  It's not a room for
> improvement.  It is very close to what the current code already
> does, i.e. to allow optional spaces immediately before the
> separator.  The difference is that the current code allows arbitrary
> number of optional spaces, not zero or exactly one.

Yeah, I think it makes sense to not change this default behavior, but
maybe, if people don't like it for some reason, allow options to tweak
it.

> So the only thing we need to do is to update the documentation that
> Max misread as allowing spaces at arbitrary place to reflect what
> the code has been doing, i.e. an optional run of spaces is allowed
> between the "token" and the "separator"?

Yeah, I also think that the documentation should be very clear (and
accurate of course) about where whitespaces and how many of them are
allowed by default, and what can possibly be tweaked by options.

> Anybody wants to do the honors to produce such a patch, then?

Ok, I will give it a try soon.

Re: [PATCH v2] trailer: allow spaces in tokens

[Johannes Schindelin]

Hi Christian,

On Fri, 19 Aug 2022, Christian Couder wrote:

> On Fri, Aug 19, 2022 at 6:33 AM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Junio C Hamano <gitster@pobox.com> writes:
> >
> > > but see a room for unrelated improvement from the current code,
> > > namely, to allow exactly one optional space, immediately before the
> > > separator and nowhere else.
> >
> > Ah, no, sorry, I misread the situation.  It's not a room for
> > improvement.  It is very close to what the current code already does,
> > i.e. to allow optional spaces immediately before the separator.  The
> > difference is that the current code allows arbitrary number of
> > optional spaces, not zero or exactly one.
>
> Yeah, I think it makes sense to not change this default behavior, but
> maybe, if people don't like it for some reason, allow options to tweak
> it.

That sounds like a very good compromise to me.

Ciao,
Dscho

[PATCH] Documentation: clarify whitespace rules for trailers

[Christian Couder]

Commit e4319562bc (trailer: be stricter in parsing separators, 2016-11-02)
restricted whitespaces allowed by `git interpret-trailers` in the "token"
part of the trailers it reads. This commit didn't update the related
documentation in Documentation/git-interpret-trailers.txt though.

Also commit 60ef86a162 (trailer: support values folded to multiple lines,
2016-10-21) updated the documentation, but didn't make it clear how many
whitespace characters are allowed at the beginning of new lines in folded
values.

Let's fix both of these issues by rewriting the paragraph describing
what whitespaces are allowed by `git interpret-trailers` in the trailers
it reads.
---
 Documentation/git-interpret-trailers.txt | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 956a01d184..0e125d795b 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -60,10 +60,12 @@ non-whitespace lines before a line that starts with '---' (followed by a
 space or the end of the line). Such three minus signs start the patch
 part of the message. See also `--no-divider` below.
 
-When reading trailers, there can be whitespaces after the
-token, the separator and the value. There can also be whitespaces
-inside the token and the value. The value may be split over multiple lines with
-each subsequent line starting with whitespace, like the "folding" in RFC 822.
+When reading trailers, there can be no whitespace inside the token,
+and only one regular space or tab character between the token and the
+separator. There can be whitespaces before, inside or after the
+value. The value may be split over multiple lines with each subsequent
+line starting with at least one whitespace, like the "folding" in RFC
+822.
 
 Note that 'trailers' do not follow and are not intended to follow many
 rules for RFC 822 headers. For example they do not follow
-- 
2.37.0

Re: [PATCH] Documentation: clarify whitespace rules for trailers

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> Commit e4319562bc (trailer: be stricter in parsing separators, 2016-11-02)
> restricted whitespaces allowed by `git interpret-trailers` in the "token"
> part of the trailers it reads. This commit didn't update the related
> documentation in Documentation/git-interpret-trailers.txt though.

OK.

> Also commit 60ef86a162 (trailer: support values folded to multiple lines,
> 2016-10-21) updated the documentation, but didn't make it clear how many
> whitespace characters are allowed at the beginning of new lines in folded
> values.
>
> Let's fix both of these issues by rewriting the paragraph describing
> what whitespaces are allowed by `git interpret-trailers` in the trailers
> it reads.
> ---
>  Documentation/git-interpret-trailers.txt | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)

Missing sign-off.


> diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
> index 956a01d184..0e125d795b 100644
> --- a/Documentation/git-interpret-trailers.txt
> +++ b/Documentation/git-interpret-trailers.txt
> @@ -60,10 +60,12 @@ non-whitespace lines before a line that starts with '---' (followed by a
>  space or the end of the line). Such three minus signs start the patch
>  part of the message. See also `--no-divider` below.
>  
> -When reading trailers, there can be whitespaces after the
> -token, the separator and the value. There can also be whitespaces
> -inside the token and the value. The value may be split over multiple lines with
> -each subsequent line starting with whitespace, like the "folding" in RFC 822.
> +When reading trailers, there can be no whitespace inside the token,
> +and only one regular space or tab character between the token and the
> +separator.

That may have been the intent, but does it match the behaviour?

        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 == '-'))
                                continue;
                        if (c != line && (*c == ' ' || *c == '\t')) {
                                whitespace_found = 1;
                                continue;
                        }
                        break;
                }
                return -1;
        }

When parsing "X  :", first we encounter 'X', we haven't seen
whitespace, 'X' passes isalnum(), and we continue.  Then we
encounter ' ', we haven't seen whitespace but it is neither isalnum
or dash, so we go on without hitting the first continue.  We are not
at the beginning of the line, we are seeing a space, so we remember
the fact that we saw whitespace and continue.  Next we see another ' ',
we do not hit the first continue, we are not at the beginning of the
line, and we are looking at ' ', so we again continue.  Finally, we see
':' that is a separator and we return happily.

The code seems to be allowing zero or more space/tab before the
separator.

Stepping back and reading the original again, I think the original
was almost correct.  There can be whitespaces after the token.
There can be whitespaces after the separator.  There can be
whitespaces after the value.  The only thing it got wrong was that
it pretended to allow whitespaces inside the token, while in reality
we allow whitespaces inside the value but not inside the token.

So, a minimum fix would be to s/token and the value/value/; I do not
mind a more extensive rewriting if it improves clarity and correctness,
but "only one between the token and the separator" is not quite correct.
Besides, that phrasing gives a false impression that the whitespace is
mandatory, but you wanted to express "zero or one" optionality, no?

> There can be whitespaces before, inside or after the
> +value. The value may be split over multiple lines with each subsequent
> +line starting with at least one whitespace, like the "folding" in RFC
> +822.
>  
>  Note that 'trailers' do not follow and are not intended to follow many
>  rules for RFC 822 headers. For example they do not follow

Re: [PATCH] Documentation: clarify whitespace rules for trailers

[Christian Couder]

On Wed, Aug 24, 2022 at 8:13 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:

> > -When reading trailers, there can be whitespaces after the
> > -token, the separator and the value. There can also be whitespaces
> > -inside the token and the value. The value may be split over multiple lines with
> > -each subsequent line starting with whitespace, like the "folding" in RFC 822.
> > +When reading trailers, there can be no whitespace inside the token,
> > +and only one regular space or tab character between the token and the
> > +separator.
>
> That may have been the intent, but does it match the behaviour?
>
>         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 == '-'))
>                                 continue;
>                         if (c != line && (*c == ' ' || *c == '\t')) {
>                                 whitespace_found = 1;
>                                 continue;
>                         }
>                         break;
>                 }
>                 return -1;
>         }
>
> When parsing "X  :", first we encounter 'X', we haven't seen
> whitespace, 'X' passes isalnum(), and we continue.  Then we
> encounter ' ', we haven't seen whitespace but it is neither isalnum
> or dash, so we go on without hitting the first continue.  We are not
> at the beginning of the line, we are seeing a space, so we remember
> the fact that we saw whitespace and continue.  Next we see another ' ',
> we do not hit the first continue, we are not at the beginning of the
> line, and we are looking at ' ', so we again continue.  Finally, we see
> ':' that is a separator and we return happily.
>
> The code seems to be allowing zero or more space/tab before the
> separator.

Yeah, I agree. I misread the code. I will send a new version soon.

Thanks.

Re: [PATCH] Documentation: clarify whitespace rules for trailers

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

>> The code seems to be allowing zero or more space/tab before the
>> separator.
>
> Yeah, I agree. I misread the code. I will send a new version soon.

I recall I did suggest to tighten to "zero or one" some point in the
discussion, but we decided to correctly document what we do right
now, so let's do that first.

Thanks.

[PATCH v2] Documentation: clarify whitespace rules for trailers

[Christian Couder]

Commit e4319562bc (trailer: be stricter in parsing separators, 2016-11-02)
restricted whitespaces allowed by `git interpret-trailers` in the "token"
part of the trailers it reads. This commit didn't update the related
documentation in Documentation/git-interpret-trailers.txt though.

Also commit 60ef86a162 (trailer: support values folded to multiple lines,
2016-10-21) updated the documentation, but didn't make it clear how many
whitespace characters are allowed at the beginning of new lines in folded
values.

Let's fix both of these issues by rewriting the paragraph describing
what whitespaces are allowed by `git interpret-trailers` in the trailers
it reads.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
Changes since v1:

  - rebased on top of 6c8e4ee870 (The sixteenth batch, 2022-08-29),
  - added my "Signed-off-by: ...",
  - clarified that there can be no whitespace before the token,
  - clarified that "any number of regular space and tab characters are
    allowed between the token and the separator".

 Documentation/git-interpret-trailers.txt | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
index 956a01d184..6d6197cd0a 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -60,10 +60,12 @@ non-whitespace lines before a line that starts with '---' (followed by a
 space or the end of the line). Such three minus signs start the patch
 part of the message. See also `--no-divider` below.
 
-When reading trailers, there can be whitespaces after the
-token, the separator and the value. There can also be whitespaces
-inside the token and the value. The value may be split over multiple lines with
-each subsequent line starting with whitespace, like the "folding" in RFC 822.
+When reading trailers, there can be no whitespace before or inside the
+token, but any number of regular space and tab characters are allowed
+between the token and the separator. There can be whitespaces before,
+inside or after the value. The value may be split over multiple lines
+with each subsequent line starting with at least one whitespace, like
+the "folding" in RFC 822.
 
 Note that 'trailers' do not follow and are not intended to follow many
 rules for RFC 822 headers. For example they do not follow
-- 
2.37.2.383.g1c78c54f66

Re: [PATCH v2] Documentation: clarify whitespace rules for trailers

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

>   - rebased on top of 6c8e4ee870 (The sixteenth batch, 2022-08-29),
>   - added my "Signed-off-by: ...",
>   - clarified that there can be no whitespace before the token,
>   - clarified that "any number of regular space and tab characters are
>     allowed between the token and the separator".
>
>  Documentation/git-interpret-trailers.txt | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)

Looks good.  Thanks.  Will queue.

>
> diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt
> index 956a01d184..6d6197cd0a 100644
> --- a/Documentation/git-interpret-trailers.txt
> +++ b/Documentation/git-interpret-trailers.txt
> @@ -60,10 +60,12 @@ non-whitespace lines before a line that starts with '---' (followed by a
>  space or the end of the line). Such three minus signs start the patch
>  part of the message. See also `--no-divider` below.
>  
> -When reading trailers, there can be whitespaces after the
> -token, the separator and the value. There can also be whitespaces
> -inside the token and the value. The value may be split over multiple lines with
> -each subsequent line starting with whitespace, like the "folding" in RFC 822.
> +When reading trailers, there can be no whitespace before or inside the
> +token, but any number of regular space and tab characters are allowed
> +between the token and the separator. There can be whitespaces before,
> +inside or after the value. The value may be split over multiple lines
> +with each subsequent line starting with at least one whitespace, like
> +the "folding" in RFC 822.
>  
>  Note that 'trailers' do not follow and are not intended to follow many
>  rules for RFC 822 headers. For example they do not follow