* [PATCH] trailer: allow spaces in tokens @ 2022-08-18 7:06 Max Bernstein via GitGitGadget 2022-08-18 7:54 ` [PATCH v2] " Max Bernstein via GitGitGadget 2022-08-18 16:48 ` [PATCH] trailer: allow spaces in tokens Junio C Hamano 0 siblings, 2 replies; 19+ messages in thread From: Max Bernstein via GitGitGadget @ 2022-08-18 7:06 UTC (permalink / raw) To: git; +Cc: Max Bernstein, Max Bernstein 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2] trailer: allow spaces in tokens 2022-08-18 7:06 [PATCH] trailer: allow spaces in tokens Max Bernstein via GitGitGadget @ 2022-08-18 7:54 ` Max Bernstein via GitGitGadget 2022-08-18 16:54 ` Junio C Hamano 2022-08-18 16:48 ` [PATCH] trailer: allow spaces in tokens Junio C Hamano 1 sibling, 1 reply; 19+ messages in thread From: Max Bernstein via GitGitGadget @ 2022-08-18 7:54 UTC (permalink / raw) To: git; +Cc: Max Bernstein, Max Bernstein 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2] trailer: allow spaces in tokens 2022-08-18 7:54 ` [PATCH v2] " Max Bernstein via GitGitGadget @ 2022-08-18 16:54 ` Junio C Hamano 2022-08-18 17:56 ` Jonathan Tan 2022-08-18 19:03 ` Maxwell Bernstein 0 siblings, 2 replies; 19+ messages in thread From: Junio C Hamano @ 2022-08-18 16:54 UTC (permalink / raw) To: Max Bernstein via GitGitGadget Cc: git, Max Bernstein, Max Bernstein, Christian Couder, Jonathan Tan "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? ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] trailer: allow spaces in tokens 2022-08-18 16:54 ` Junio C Hamano @ 2022-08-18 17:56 ` Jonathan Tan 2022-08-18 19:03 ` Maxwell Bernstein 1 sibling, 0 replies; 19+ messages in thread From: Jonathan Tan @ 2022-08-18 17:56 UTC (permalink / raw) To: Junio C Hamano Cc: Jonathan Tan, Max Bernstein via GitGitGadget, git, Max Bernstein, Max Bernstein, Christian Couder 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/ ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] trailer: allow spaces in tokens 2022-08-18 16:54 ` Junio C Hamano 2022-08-18 17:56 ` Jonathan Tan @ 2022-08-18 19:03 ` Maxwell Bernstein 2022-08-18 20:46 ` Christian Couder 1 sibling, 1 reply; 19+ messages in thread From: Maxwell Bernstein @ 2022-08-18 19:03 UTC (permalink / raw) To: Junio C Hamano, Max Bernstein via GitGitGadget Cc: git, Max Bernstein, Max Bernstein, Christian Couder, Jonathan Tan 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] trailer: allow spaces in tokens 2022-08-18 19:03 ` Maxwell Bernstein @ 2022-08-18 20:46 ` Christian Couder 2022-08-18 21:31 ` Junio C Hamano 0 siblings, 1 reply; 19+ messages in thread From: Christian Couder @ 2022-08-18 20:46 UTC (permalink / raw) To: Maxwell Bernstein Cc: Junio C Hamano, Max Bernstein via GitGitGadget, git, Max Bernstein, Max Bernstein, Jonathan Tan 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] trailer: allow spaces in tokens 2022-08-18 20:46 ` Christian Couder @ 2022-08-18 21:31 ` Junio C Hamano 2022-08-19 4:33 ` Junio C Hamano 0 siblings, 1 reply; 19+ messages in thread From: Junio C Hamano @ 2022-08-18 21:31 UTC (permalink / raw) To: Christian Couder Cc: Maxwell Bernstein, Max Bernstein via GitGitGadget, git, Max Bernstein, Max Bernstein, Jonathan Tan 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] trailer: allow spaces in tokens 2022-08-18 21:31 ` Junio C Hamano @ 2022-08-19 4:33 ` Junio C Hamano 2022-08-19 10:29 ` Christian Couder 0 siblings, 1 reply; 19+ messages in thread From: Junio C Hamano @ 2022-08-19 4:33 UTC (permalink / raw) To: Christian Couder Cc: Maxwell Bernstein, Max Bernstein via GitGitGadget, git, Max Bernstein, Max Bernstein, Jonathan Tan 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] trailer: allow spaces in tokens 2022-08-19 4:33 ` Junio C Hamano @ 2022-08-19 10:29 ` Christian Couder 2022-08-22 13:58 ` Johannes Schindelin 2022-08-23 14:06 ` [PATCH] Documentation: clarify whitespace rules for trailers Christian Couder 0 siblings, 2 replies; 19+ messages in thread From: Christian Couder @ 2022-08-19 10:29 UTC (permalink / raw) To: Junio C Hamano Cc: Maxwell Bernstein, Max Bernstein via GitGitGadget, git, Max Bernstein, Max Bernstein, Jonathan Tan 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] trailer: allow spaces in tokens 2022-08-19 10:29 ` Christian Couder @ 2022-08-22 13:58 ` Johannes Schindelin 2022-08-23 14:06 ` [PATCH] Documentation: clarify whitespace rules for trailers Christian Couder 1 sibling, 0 replies; 19+ messages in thread From: Johannes Schindelin @ 2022-08-22 13:58 UTC (permalink / raw) To: Christian Couder Cc: Junio C Hamano, Maxwell Bernstein, Max Bernstein via GitGitGadget, git, Max Bernstein, Max Bernstein, Jonathan Tan 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH] Documentation: clarify whitespace rules for trailers 2022-08-19 10:29 ` Christian Couder 2022-08-22 13:58 ` Johannes Schindelin @ 2022-08-23 14:06 ` Christian Couder 2022-08-24 18:13 ` Junio C Hamano 1 sibling, 1 reply; 19+ messages in thread From: Christian Couder @ 2022-08-23 14:06 UTC (permalink / raw) To: git Cc: Junio C Hamano, Maxwell Bernstein, Jonathan Tan, Johannes Schindelin, 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] Documentation: clarify whitespace rules for trailers 2022-08-23 14:06 ` [PATCH] Documentation: clarify whitespace rules for trailers Christian Couder @ 2022-08-24 18:13 ` Junio C Hamano 2022-08-25 11:59 ` Christian Couder 0 siblings, 1 reply; 19+ messages in thread From: Junio C Hamano @ 2022-08-24 18:13 UTC (permalink / raw) To: Christian Couder Cc: git, Maxwell Bernstein, Jonathan Tan, Johannes Schindelin 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Documentation: clarify whitespace rules for trailers 2022-08-24 18:13 ` Junio C Hamano @ 2022-08-25 11:59 ` Christian Couder 2022-08-25 16:20 ` Junio C Hamano 2022-08-30 10:50 ` [PATCH v2] " Christian Couder 0 siblings, 2 replies; 19+ messages in thread From: Christian Couder @ 2022-08-25 11:59 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Maxwell Bernstein, Jonathan Tan, Johannes Schindelin 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Documentation: clarify whitespace rules for trailers 2022-08-25 11:59 ` Christian Couder @ 2022-08-25 16:20 ` Junio C Hamano 2022-08-30 10:50 ` [PATCH v2] " Christian Couder 1 sibling, 0 replies; 19+ messages in thread From: Junio C Hamano @ 2022-08-25 16:20 UTC (permalink / raw) To: Christian Couder Cc: git, Maxwell Bernstein, Jonathan Tan, Johannes Schindelin 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2] Documentation: clarify whitespace rules for trailers 2022-08-25 11:59 ` Christian Couder 2022-08-25 16:20 ` Junio C Hamano @ 2022-08-30 10:50 ` Christian Couder 2022-08-30 17:20 ` Junio C Hamano 1 sibling, 1 reply; 19+ messages in thread From: Christian Couder @ 2022-08-30 10:50 UTC (permalink / raw) To: git Cc: Junio C Hamano, Maxwell Bernstein, Jonathan Tan, Johannes Schindelin, Christian Couder, 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2] Documentation: clarify whitespace rules for trailers 2022-08-30 10:50 ` [PATCH v2] " Christian Couder @ 2022-08-30 17:20 ` Junio C Hamano 0 siblings, 0 replies; 19+ messages in thread From: Junio C Hamano @ 2022-08-30 17:20 UTC (permalink / raw) To: Christian Couder Cc: git, Maxwell Bernstein, Jonathan Tan, Johannes Schindelin, Christian Couder 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] trailer: allow spaces in tokens 2022-08-18 7:06 [PATCH] trailer: allow spaces in tokens Max Bernstein via GitGitGadget 2022-08-18 7:54 ` [PATCH v2] " Max Bernstein via GitGitGadget @ 2022-08-18 16:48 ` Junio C Hamano 2022-08-18 17:52 ` Jonathan Tan 1 sibling, 1 reply; 19+ messages in thread From: Junio C Hamano @ 2022-08-18 16:48 UTC (permalink / raw) To: Max Bernstein via GitGitGadget Cc: git, Max Bernstein, Max Bernstein, Christian Couder, Jonathan Tan [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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] trailer: allow spaces in tokens 2022-08-18 16:48 ` [PATCH] trailer: allow spaces in tokens Junio C Hamano @ 2022-08-18 17:52 ` Jonathan Tan 2022-08-18 17:58 ` Junio C Hamano 0 siblings, 1 reply; 19+ messages in thread From: Jonathan Tan @ 2022-08-18 17:52 UTC (permalink / raw) To: Junio C Hamano Cc: Jonathan Tan, Max Bernstein via GitGitGadget, git, Max Bernstein, Max Bernstein, Christian Couder 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] trailer: allow spaces in tokens 2022-08-18 17:52 ` Jonathan Tan @ 2022-08-18 17:58 ` Junio C Hamano 0 siblings, 0 replies; 19+ messages in thread From: Junio C Hamano @ 2022-08-18 17:58 UTC (permalink / raw) To: Jonathan Tan Cc: Max Bernstein via GitGitGadget, git, Max Bernstein, Max Bernstein, Christian Couder 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2022-08-30 17:21 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-18 7:06 [PATCH] trailer: allow spaces in tokens Max Bernstein via GitGitGadget 2022-08-18 7:54 ` [PATCH v2] " Max Bernstein via GitGitGadget 2022-08-18 16:54 ` Junio C Hamano 2022-08-18 17:56 ` Jonathan Tan 2022-08-18 19:03 ` Maxwell Bernstein 2022-08-18 20:46 ` Christian Couder 2022-08-18 21:31 ` Junio C Hamano 2022-08-19 4:33 ` Junio C Hamano 2022-08-19 10:29 ` Christian Couder 2022-08-22 13:58 ` Johannes Schindelin 2022-08-23 14:06 ` [PATCH] Documentation: clarify whitespace rules for trailers Christian Couder 2022-08-24 18:13 ` Junio C Hamano 2022-08-25 11:59 ` Christian Couder 2022-08-25 16:20 ` Junio C Hamano 2022-08-30 10:50 ` [PATCH v2] " Christian Couder 2022-08-30 17:20 ` Junio C Hamano 2022-08-18 16:48 ` [PATCH] trailer: allow spaces in tokens Junio C Hamano 2022-08-18 17:52 ` Jonathan Tan 2022-08-18 17:58 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).