{"thread":{"id":"52956","subject":"[PATCH 0/3] Teach ref-filter API to correctly handle CRLF in messages","startedAt":"2020-03-08T18:29:49Z","lastAt":"2020-10-29T12:48:37Z","messageCount":38,"participants":["Philippe Blain via GitGitGadget","Junio C Hamano","Philippe Blain","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"392951","messageId":"pull.576.git.1583692184.gitgitgadget@gmail.com","threadId":"52956","inReplyTo":null,"subject":"[PATCH 0/3] Teach ref-filter API to correctly handle CRLF in messages","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-08T18:29:41Z","receivedAt":"2020-03-08T18:29:49Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"The ref-filter API does not correctly handle commit or tag messages that use\nCRLF as the line terminator. Such messages can be created with the--verbatim \noption of git commit and git tag, or by using git commit-tree directly.\n\nThis impacts the output of git branch -v, and git branch, git tag and git\nfor-each-ref when used with a --format argument containing the atoms \n%(contents:subject) or %(contents:body).\n\nThe first patch in this series adds a new test library, \nt/lib-crlf-messages.sh, that contains functions to test this behaviour for\ncommands using either the ref-filter or the pretty API to display messages. \n\nThe second patch fixes the bug in ref-filter.c and adds corresponding tests.\n\nFinally, the third patch adds a test that checks the behaviour of git log in\nthe presence of CRLF in messages, to prevent futur regressions.\n\nPhilippe Blain (3):\n t: add lib-crlf-messages.sh for messages containing CRLF\n ref-filter: teach the API to correctly handle CRLF\n log: add tests for messages containing CRLF to t4202\n\n ref-filter.c | 19 +++++++++--\n t/lib-crlf-messages.sh | 73 ++++++++++++++++++++++++++++++++++++++++\n t/t3203-branch-output.sh | 26 +++++++++++---\n t/t4202-log.sh | 24 +++++++++++++\n t/t6300-for-each-ref.sh | 5 +++\n t/t7004-tag.sh | 7 ++++\n 6 files changed, 147 insertions(+), 7 deletions(-)\n create mode 100644 t/lib-crlf-messages.sh\n\n\nbase-commit: 076cbdcd739aeb33c1be87b73aebae5e43d7bcc5\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-576%2Fphil-blain%2Ffix-branch-verbose-crlf-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-576/phil-blain/fix-branch-verbose-crlf-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/576\n-- \ngitgitgadget\n"},{"id":"392952","messageId":"3e5b8ace7b8965ae8ef4f93f20fb7b268acf90df.1583692184.git.gitgitgadget@gmail.com","threadId":"52956","inReplyTo":"pull.576.git.1583692184.gitgitgadget@gmail.com","subject":"[PATCH 3/3] log: add tests for messages containing CRLF to t4202","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-08T18:29:44Z","receivedAt":"2020-03-08T18:29:51Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nA previous commit fixed a bug in the ref-filter API causing messages\ncontaining CRLF to be incorrectly parsed and displayed.\n\nAdd a test to also check that `git log` correctly handles such messages,\nto prevent futur regressions.\n\nSigned-off-by: Philippe Blain \n---\n t/t4202-log.sh | 24 ++++++++++++++++++++++++\n 1 file changed, 24 insertions(+)\n\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 4694b6d0ce7..1e6149372c5 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -6,6 +6,7 @@ test_description='git log'\n . \"$TEST_DIRECTORY/lib-gpg.sh\"\n . \"$TEST_DIRECTORY/lib-terminal.sh\"\n . \"$TEST_DIRECTORY/lib-log-graph.sh\"\n+. \"$TEST_DIRECTORY/lib-crlf-messages.sh\"\n \n test_cmp_graph () {\n \tlib_test_cmp_graph --format=%s \"$@\"\n@@ -105,6 +106,29 @@ test_expect_success 'oneline' '\n \ttest_cmp expect actual\n '\n \n+test_create_crlf_refs\n+\n+test_expect_success 'oneline with CRLF messages' '\n+\techo \"002093e Subject first line\" >expect &&\n+\tgit log --oneline -1 crlf >actual-branch &&\n+\tgit log --oneline -1 tag-crlf >actual-tag &&\n+\ttest_cmp expect actual-branch &&\n+\ttest_cmp expect actual-tag &&\n+\techo \"6f814b0 Subject first line\" >expect &&\n+\tgit log --oneline -1 crlf-empty-lines-after-subject >actual-branch &&\n+\tgit log --oneline -1 tag-crlf-empty-lines-after-subject >actual-tag &&\n+\ttest_cmp expect actual-branch &&\n+\ttest_cmp expect actual-tag &&\n+\techo \"8c58a85 Subject first line Subject second line\" >expect &&\n+\tgit log --oneline -1 crlf-two-line-subject >actual-branch &&\n+\tgit log --oneline -1 tag-crlf-two-line-subject >actual-tag &&\n+\ttest_cmp expect actual-branch &&\n+\ttest_cmp expect actual-tag\n+'\n+test_crlf_subject_body_and_contents log --all --reverse --grep Subject\n+\n+test_cleanup_crlf_refs\n+\n test_expect_success 'diff-filter=A' '\n \n \tgit log --no-renames --pretty=\"format:%s\" --diff-filter=A HEAD > actual &&\n-- \ngitgitgadget\n"},{"id":"392953","messageId":"b25211380355f7f95113b42734df0bb8b2b9e78f.1583692184.git.gitgitgadget@gmail.com","threadId":"52956","inReplyTo":"pull.576.git.1583692184.gitgitgadget@gmail.com","subject":"[PATCH 1/3] t: add lib-crlf-messages.sh for messages containing CRLF","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-08T18:29:42Z","receivedAt":"2020-03-08T18:29:52Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nA following commit will fix a bug in the ref-filter API that causes\ncommit and tag messages containing CRLF to be incorrectly parsed and\ndisplayed.\n\nAdd a test library (t/lib-crlf-messages.sh) that creates refs with such\ncommit messages, so that we can easily test that this bug does not\nappear in other commands in the future.\n\nThe function test_crlf_subject_body_and_contents can be used to test\nthat the `--format` option of `branch`, `tag`, `for-each-ref` and\n`log` correctly displays the subject, body and raw body of commits and\ntag messages.\n\nThe commits are created using `commit-tree` such that the current branch\nin the test repository is not affected when `test_create_crlf_refs` is\ncalled in a test. This is to done so that the CRLF tests can be inserted\nanywhere in a test script where it makes sense to do so, without having\nto potentially modify further test that depend on output that would be\nmodified if the current branch gained new commits.\n\nSigned-off-by: Philippe Blain \n---\n t/lib-crlf-messages.sh | 73 ++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 73 insertions(+)\n create mode 100644 t/lib-crlf-messages.sh\n\ndiff --git a/t/lib-crlf-messages.sh b/t/lib-crlf-messages.sh\nnew file mode 100644\nindex 00000000000..64d2ad12019\n--- /dev/null\n+++ b/t/lib-crlf-messages.sh\n@@ -0,0 +1,73 @@\n+# Setup refs with commit and tag messages containing CRLF\n+\n+create_crlf_ref () {\n+\tmessage=\"$1\" &&\n+\tsubject=\"$2\" &&\n+\tbody=\"$3\" &&\n+\tbranch=\"$4\" &&\n+\tprintf \"${message}\" >.crlf-message-${branch}.txt &&\n+\tprintf \"${subject}\" >.crlf-subject-${branch}.txt &&\n+\tprintf \"${body}\" >.crlf-body-${branch}.txt &&\n+ test_tick &&\n+\thash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&\n+\tgit branch ${branch} ${hash} &&\n+\tgit tag tag-${branch} ${branch} -F .crlf-message-${branch}.txt --cleanup=verbatim\n+}\n+\n+create_crlf_refs () {\n+\tmessage=\"Subject first line\\r\\n\\r\\nBody first line\\r\\nBody second line\\r\\n\" &&\n+\tbody=\"Body first line\\r\\nBody second line\\r\\n\" &&\n+\tsubject=\"Subject first line\" &&\n+\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"crlf\" &&\n+\tmessage=\"Subject first line\\r\\n\\r\\n\\r\\nBody first line\\r\\nBody second line\\r\\n\" &&\n+\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"crlf-empty-lines-after-subject\" &&\n+\tmessage=\"Subject first line\\r\\nSubject second line\\r\\n\\r\\nBody first line\\r\\nBody second line\\r\\n\" &&\n+\tsubject=\"Subject first line Subject second line\" &&\n+\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"crlf-two-line-subject\"\n+}\n+\n+test_create_crlf_refs () {\n+\ttest_expect_success 'setup refs with CRLF commit messages' '\n+\t\tcreate_crlf_refs\n+\t'\n+}\n+\n+cleanup_crlf_refs () {\n+\tfor branch in crlf crlf-empty-lines-after-subject crlf-two-line-subject; do\n+\t\tgit branch -D ${branch} &&\n+\t\tgit tag -d tag-${branch} &&\n+\t\trm .crlf-message-${branch}.txt &&\n+\t\trm .crlf-subject-${branch}.txt &&\n+\t\trm .crlf-body-${branch}.txt\n+\tdone\n+}\n+\n+test_cleanup_crlf_refs () {\n+\ttest_expect_success 'clenup refs with CRLF commit messages' '\n+\t\tcleanup_crlf_refs\n+\t'\n+}\n+\n+test_crlf_subject_body_and_contents() {\n+\tcommand_and_args=\"$@\" &&\n+\tcommand=$1 &&\n+\tif [ ${command} = \"branch\" ] || [ ${command} = \"for-each-ref\" ] || [ ${command} = \"tag\" ]; then\n+\t\tatoms=\"(contents:subject) (contents:body) (contents)\"\n+\telif [ ${command} = \"log\" ]; then\n+\t\tatoms=\"s b B\"\n+\tfi &&\n+\tfiles=\"subject body message\" &&\n+\twhile [ -n \"${atoms}\" ]; do\n+\t\tset ${atoms} && atom=$1 && shift && atoms=\"$*\" &&\n+\t\tset ${files} &&\tfile=$1 && shift && files=\"$*\" &&\n+\t\ttest_expect_success \"${command}: --format='%${atom}' works with CRLF input\" \"\n+\t\t\trm -f expect &&\n+\t\t\tfor ref in crlf crlf-empty-lines-after-subject crlf-two-line-subject; do\n+\t\t\t\tcat .crlf-${file}-\\\"\\${ref}\\\".txt >>expect &&\n+\t\t\t\tprintf \\\"\\n\\\" >>expect\n+\t\t\tdone &&\n+\t\t\tgit $command_and_args --format=\\\"%${atom}\\\" >actual &&\n+\t\t\ttest_cmp expect actual\n+\t\t\"\n+\tdone\n+}\n-- \ngitgitgadget\n\n"},{"id":"392954","messageId":"c68bc2b37889bb2394dfd618cd3f03c1a7d85280.1583692184.git.gitgitgadget@gmail.com","threadId":"52956","inReplyTo":"pull.576.git.1583692184.gitgitgadget@gmail.com","subject":"[PATCH 2/3] ref-filter: teach the API to correctly handle CRLF","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-08T18:29:43Z","receivedAt":"2020-03-08T18:29:53Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nThe ref-filter API does not correctly handle commit or tag messages that\nuse CRLF as the line terminator. Such messages can be created with the\n`--verbatim` option of `git commit` and `git tag`, or by using `git\ncommit-tree` directly.\n\nThis impacts the output of `git branch -v`, and `git branch`, `git\ntag` and `git for-each-ref` when used with a `--format` argument\ncontaining the atoms `%(contents:subject)` or `%(contents:body)`.\n\nFix this bug in ref-filter by improving the logic in `copy_subject` and\n`find_subpos`.\n\nAdd tests for `branch`, `tag` and `for-each-ref` using\nlib-crlf-messages.sh. The 'make commits' test at the beginning of\nt3203-branch-output.sh needs to be modified since it did not use\n`test_tick` and thus the commit hashes were not reproducible. For\nsimplicity, use `test_commit` as the content and name of the files\ncreated in this setup test are irrelevant to the rest of the test\nscript.\n\nSigned-off-by: Philippe Blain \n---\n ref-filter.c | 19 +++++++++++++++++--\n t/t3203-branch-output.sh | 26 +++++++++++++++++++++-----\n t/t6300-for-each-ref.sh | 5 +++++\n t/t7004-tag.sh | 7 +++++++\n 4 files changed, 50 insertions(+), 7 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 79bb5206783..537cc4de42c 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1050,10 +1050,18 @@ static char *copy_subject(const char *buf, unsigned long len)\n {\n \tchar *r = xmemdupz(buf, len);\n \tint i;\n+\tstruct strbuf sb = STRBUF_INIT;\n \n-\tfor (i = 0; i < len; i++)\n+\tstrbuf_attach(&sb, r, len, len + 1);\n+\tfor (i = 0; i < sb.len; i++) {\n \t\tif (r[i] == '\\n')\n \t\t\tr[i] = ' ';\n+\t\tif (r[i] == '\\r') {\n+\t\t\tstrbuf_remove(&sb, i, 1);\n+\t\t\ti -= 1;\n+\t\t}\n+\t}\n+\tstrbuf_detach(&sb, NULL);\n \n \treturn r;\n }\n@@ -1184,15 +1192,22 @@ static void find_subpos(const char *buf,\n \t\teol = strchrnul(buf, '\\n');\n \t\tif (*eol)\n \t\t\teol++;\n+\t\t/* protect against messages that might contain \\r\\n */\n+\t\tif (*eol == '\\r')\n+\t\t\teol++;\n \t\tbuf = eol;\n \t}\n \t*sublen = buf - *sub;\n \t/* drop trailing newline, if present */\n \tif (*sublen && (*sub)[*sublen - 1] == '\\n')\n \t\t*sublen -= 1;\n+\t/* protect against commit messages that might contain \\r\\n */\n+\telse if (*sublen && (*sub)[*sublen - 1] == '\\r')\n+\t\t*sublen -= 3; /* drop '\\r\\n\\r' */\n \n \t/* skip any empty lines */\n-\twhile (*buf == '\\n')\n+\t/* and protect against commit messages that might contain \\r\\n */\n+\twhile (*buf == '\\n' || *buf == '\\r')\n \t\tbuf++;\n \t*body = buf;\n \t*bodylen = strlen(buf);\ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nindex 71818b90f00..1235089619c 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3203-branch-output.sh\n@@ -3,13 +3,11 @@\n test_description='git branch display tests'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n+. \"$TEST_DIRECTORY\"/lib-crlf-messages.sh\n \n test_expect_success 'make commits' '\n-\techo content >file &&\n-\tgit add file &&\n-\tgit commit -m one &&\n-\techo content >>file &&\n-\tgit commit -a -m two\n+\ttest_commit one &&\n+\ttest_commit two\n '\n \n test_expect_success 'make branches' '\n@@ -95,6 +93,24 @@ test_expect_success 'git branch --ignore-case --list -v pattern shows branch sum\n \tawk \"{print \\$NF}\" actual &&\n \ttest_cmp expect actual\n '\n+test_create_crlf_refs\n+\n+test_expect_success 'git branch -v works with CRLF input' '\n+\tcat >expect <<-EOF &&\n+\t branch-one 139b20d two\n+\t branch-two d79ce16 one\n+\t crlf 2113b0e Subject first line\n+\t crlf-empty-lines-after-subject 0a9530d Subject first line\n+\t crlf-two-line-subject f9ded1f Subject first line Subject second line\n+\t* master 139b20d two\n+\tEOF\n+\tgit branch -v >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_crlf_subject_body_and_contents branch --list crlf*\n+\n+test_cleanup_crlf_refs\n \n test_expect_success 'git branch -v pattern does not show branch summaries' '\n \ttest_must_fail git branch -v branch*\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 9c910ce7467..467924de3df 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -8,6 +8,7 @@ test_description='for-each-ref test'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-gpg.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n+. \"$TEST_DIRECTORY\"/lib-crlf-messages.sh\n \n # Mon Jul 3 23:18:43 2006 +0000\n datestamp=1151968723\n@@ -888,4 +889,8 @@ test_expect_success 'for-each-ref --ignore-case ignores case' '\n \ttest_cmp expect actual\n '\n \n+test_create_crlf_refs\n+\n+test_crlf_subject_body_and_contents for-each-ref refs/heads/crlf*\n+\n test_done\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 6db92bd3ba6..25f465f34ea 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -10,6 +10,7 @@ Tests for operations with tags.'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-gpg.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n+. \"$TEST_DIRECTORY\"/lib-crlf-messages.sh\n \n # creating and listing lightweight tags:\n \n@@ -1969,6 +1970,12 @@ test_expect_success '--format should list tags as per format given' '\n \ttest_cmp expect actual\n '\n \n+test_create_crlf_refs\n+\n+test_crlf_subject_body_and_contents tag --list tag-crlf*\n+\n+test_cleanup_crlf_refs\n+\n test_expect_success \"set up color tests\" '\n \techo \"v1.0\" >expect.color &&\n \techo \"v1.0\" >expect.bare &&\n-- \ngitgitgadget\n\n"},{"id":"392965","messageId":"xmqqimjd1rx2.fsf@gitster-ct.c.googlers.com","threadId":"52956","inReplyTo":"pull.576.git.1583692184.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] Teach ref-filter API to correctly handle CRLF in messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-09T15:14:49Z","receivedAt":"2020-03-09T15:14:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philippe Blain via GitGitGadget\" writes:\n\n> The ref-filter API does not correctly handle commit or tag messages that use\n> CRLF as the line terminator. Such messages can be created with the--verbatim \n> option of git commit and git tag, or by using git commit-tree directly.\n>\n> This impacts the output of git branch -v, and git branch, git tag and git\n> for-each-ref when used with a --format argument containing the atoms \n> %(contents:subject) or %(contents:body).\n\nWhat is missing from the above is the definition of \"correct\".\nWithout saying what you consider in the current behaviour is\n\"incorrect\" and why, the first sentence does not give the right\ninformation to readers.\n\nLet me speculate why \"such messages\" are created by end users using\nthe --verbatim option. They probably have unusual message that\nneeds to have whitespaces and characters that are stripped without\nthe option at the end of the lines in the message, and they\nexplicitly ask Git not to lose them with the option. Perhaps CR may\nbe among those that they want to retain.\n\nSo \"git for-each-ref --format='%(contents:body)'\" should retain\nthese CRs and whitespaces at the end of these lines, but you are\nreporting an incorrect behaviour, so perhaps somehow we lose them?\n\nThat does indeed sound like a bug. We should keep them in the\noutput if the user took pains to deliberately place them in the\ncommit. Unless the user tells us to clean/hide them with an option,\nthat is.\n\nBut as you did not say what your definition of correctness is in the\nabove two paragraphs, it is not clear to readers if that is the bug\nyou are trying to address, or even if such a bug exists in the first\nplace.\n\n(haven't read the remainder of the cover or the patches yet).\n"},{"id":"392976","messageId":"63755050-10A5-4A46-9BB3-8207E055692C@gmail.com","threadId":"52956","inReplyTo":"xmqqimjd1rx2.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/3] Teach ref-filter API to correctly handle CRLF in messages","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-03-10T02:19:20Z","receivedAt":"2020-03-10T02:19:29Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Junio,\n\n> Le 9 mars 2020 à 11:14, Junio C Hamano a écrit :\n> \n> \"Philippe Blain via GitGitGadget\" writes:\n> \n>> The ref-filter API does not correctly handle commit or tag messages that use\n>> CRLF as the line terminator. Such messages can be created with the--verbatim \n>> option of git commit and git tag, or by using git commit-tree directly.\n>> \n>> This impacts the output of git branch -v, and git branch, git tag and git\n>> for-each-ref when used with a --format argument containing the atoms \n>> %(contents:subject) or %(contents:body).\n> \n> What is missing from the above is the definition of \"correct\".\n> Without saying what you consider in the current behaviour is\n> \"incorrect\" and why, the first sentence does not give the right\n> information to readers.\n\nYou are right, I'll send a v2 shortly with an updated cover letter and commit message.\n\n> Let me speculate why \"such messages\" are created by end users using\n> the --verbatim option. They probably have unusual message that\n> needs to have whitespaces and characters that are stripped without\n> the option at the end of the lines in the message, and they\n> explicitly ask Git not to lose them with the option. Perhaps CR may\n> be among those that they want to retain.\n\nThat's possible. In my case what made me discover the bug is our old version of GitLab at work,\nin which messages for merge commit created using the web UI contain CRLF [1].\n\n[1] https://gitlab.com/gitlab-org/gitlab-foss/-/issues/31671\n\n\n"},{"id":"392977","messageId":"pull.576.v2.git.1583807093.gitgitgadget@gmail.com","threadId":"52956","inReplyTo":"pull.576.git.1583692184.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] Teach ref-filter API to correctly handle CRLF in messages","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-10T02:24:50Z","receivedAt":"2020-03-10T02:24:58Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"The ref-filter API does not correctly handle commit or tag messages that use\nCRLF as the line terminator. Such messages can be created with the--verbatim \noption of git commit and git tag, or by using git commit-tree directly.\n\nThis impacts the output git branch, git tag and git for-each-refwhen used\nwith a --format argument containing the atoms%(contents:subject) or \n%(contents:body), as well as the output ofgit branch --verbose, which uses \n%(contents:subject) internally.\n\nThe function find_subpos in ref-filter.c looks for two consecutive '\\n' to\nfind the end of the subject line, a sequence which is absent in messages\nusing CRLF. This results in the whole message being parsed as the subject\nline (%(contents:subject)), and the body of the message (%(contents:body))\nbeing empty.\n\nMoreover, in copy_subject, '\\n' is replaced by space, but '\\r' is untouched,\nresulting in the escape sequence '^M' being output verbatim in most terminal\nemulators:\n\n$ git branch --verbose\n* crlf 2113b0e Subject first line^M ^M Body first line^M Body second line\n\nThis bug is a regression for git branch --verbose, which bisects down to\n949af06 (branch: use ref-filter printing APIs, 2017-01-10).\n\nThe first patch in this series adds a new test library, \nt/lib-crlf-messages.sh, that contains functions to test this behaviour for\ncommands using either the ref-filter or the pretty API to display messages. \n\nThe second patch fixes the bug in ref-filter.c and adds corresponding tests.\n\nFinally, the third patch adds a test that checks the behaviour of git log in\nthe presence of CRLF in messages, to prevent futur regressions.\n\nchanges since v1:\n\n * improved the cover letter and the commit message of the 2nd patch to\n actually describe the bug this series is fixing\n\n\n----------------------------------------------------------------------------\n\nv1:\n\nThe ref-filter API does not correctly handle commit or tag messages that use\nCRLF as the line terminator. Such messages can be created with the--verbatim \noption of git commit and git tag, or by using git commit-tree directly.\n\nThis impacts the output of git branch -v, and git branch, git tag and git\nfor-each-ref when used with a --format argument containing the atoms \n%(contents:subject) or %(contents:body).\n\nThe first patch in this series adds a new test library, \nt/lib-crlf-messages.sh, that contains functions to test this behaviour for\ncommands using either the ref-filter or the pretty API to display messages. \n\nThe second patch fixes the bug in ref-filter.c and adds corresponding tests.\n\nFinally, the third patch adds a test that checks the behaviour of git log in\nthe presence of CRLF in messages, to prevent futur regressions.\n\nPhilippe Blain (3):\n t: add lib-crlf-messages.sh for messages containing CRLF\n ref-filter: fix the API to correctly handle CRLF\n log: add tests for messages containing CRLF to t4202\n\n ref-filter.c | 19 +++++++++--\n t/lib-crlf-messages.sh | 73 ++++++++++++++++++++++++++++++++++++++++\n t/t3203-branch-output.sh | 26 +++++++++++---\n t/t4202-log.sh | 24 +++++++++++++\n t/t6300-for-each-ref.sh | 5 +++\n t/t7004-tag.sh | 7 ++++\n 6 files changed, 147 insertions(+), 7 deletions(-)\n create mode 100644 t/lib-crlf-messages.sh\n\n\nbase-commit: 076cbdcd739aeb33c1be87b73aebae5e43d7bcc5\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-576%2Fphil-blain%2Ffix-branch-verbose-crlf-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-576/phil-blain/fix-branch-verbose-crlf-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/576\n\nRange-diff vs v1:\n\n 1: b2521138035 = 1: b2521138035 t: add lib-crlf-messages.sh for messages containing CRLF\n 2: c68bc2b3788 ! 2: aab1f45ba97 ref-filter: teach the API to correctly handle CRLF\n @@ -1,26 +1,49 @@\n Author: Philippe Blain \n \n - ref-filter: teach the API to correctly handle CRLF\n + ref-filter: fix the API to correctly handle CRLF\n \n The ref-filter API does not correctly handle commit or tag messages that\n use CRLF as the line terminator. Such messages can be created with the\n `--verbatim` option of `git commit` and `git tag`, or by using `git\n commit-tree` directly.\n \n - This impacts the output of `git branch -v`, and `git branch`, `git\n - tag` and `git for-each-ref` when used with a `--format` argument\n - containing the atoms `%(contents:subject)` or `%(contents:body)`.\n + This impacts the output `git branch`, `git tag` and `git for-each-ref`\n + when used with a `--format` argument containing the atoms\n + `%(contents:subject)` or `%(contents:body)`, as well as the output of\n + `git branch --verbose`, which uses `%(contents:subject)` internally.\n \n - Fix this bug in ref-filter by improving the logic in `copy_subject` and\n - `find_subpos`.\n + The function find_subpos in ref-filter.c looks for two consecutive '\\n'\n + to find the end of the subject line, a sequence which is absent in\n + messages using CRLF. This results in the whole message being parsed as\n + the subject line (`%(contents:subject)`), and the body of the message\n + (`%(contents:body)`) being empty.\n +\n + Moreover, in copy_subject, '\\n' is replaced by space, but '\\r' is\n + untouched, resulting in the escape sequence '^M' being output verbatim\n + in most terminal emulators:\n +\n + $ git branch --verbose\n + * crlf 2113b0e Subject first line^M ^M Body first line^M Body second line\n +\n + This bug is a regression for `git branch --verbose`, which\n + bisects down to 949af0684c (branch: use ref-filter printing APIs,\n + 2017-01-10).\n +\n + Fix this bug in ref-filter by hardening the logic in `copy_subject` and\n + `find_subpos` to correctly parse messages containing CRFL.\n \n Add tests for `branch`, `tag` and `for-each-ref` using\n - lib-crlf-messages.sh. The 'make commits' test at the beginning of\n - t3203-branch-output.sh needs to be modified since it did not use\n - `test_tick` and thus the commit hashes were not reproducible. For\n - simplicity, use `test_commit` as the content and name of the files\n - created in this setup test are irrelevant to the rest of the test\n - script.\n + lib-crlf-messages.sh.\n +\n + The 'make commits' test at the beginning of t3203-branch-output.sh needs\n + to be modified since it did not use `test_tick` and thus the commit\n + hashes were not reproducible. For simplicity, use `test_commit` as the\n + content and name of the files created in this setup test are irrelevant\n + to the rest of the test script.\n +\n + `test_cleanup_crlf_refs` is used in t3203-branch-output.sh and\n + t7004-tag.sh to avoid having to modify the expected output in later\n + tests.\n \n Signed-off-by: Philippe Blain \n \n 3: 3e5b8ace7b8 = 3: c84092e457c log: add tests for messages containing CRLF to t4202\n\n-- \ngitgitgadget\n"},{"id":"392978","messageId":"b25211380355f7f95113b42734df0bb8b2b9e78f.1583807093.git.gitgitgadget@gmail.com","threadId":"52956","inReplyTo":"pull.576.v2.git.1583807093.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] t: add lib-crlf-messages.sh for messages containing CRLF","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-10T02:24:51Z","receivedAt":"2020-03-10T02:25:00Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nA following commit will fix a bug in the ref-filter API that causes\ncommit and tag messages containing CRLF to be incorrectly parsed and\ndisplayed.\n\nAdd a test library (t/lib-crlf-messages.sh) that creates refs with such\ncommit messages, so that we can easily test that this bug does not\nappear in other commands in the future.\n\nThe function test_crlf_subject_body_and_contents can be used to test\nthat the `--format` option of `branch`, `tag`, `for-each-ref` and\n`log` correctly displays the subject, body and raw body of commits and\ntag messages.\n\nThe commits are created using `commit-tree` such that the current branch\nin the test repository is not affected when `test_create_crlf_refs` is\ncalled in a test. This is to done so that the CRLF tests can be inserted\nanywhere in a test script where it makes sense to do so, without having\nto potentially modify further test that depend on output that would be\nmodified if the current branch gained new commits.\n\nSigned-off-by: Philippe Blain \n---\n t/lib-crlf-messages.sh | 73 ++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 73 insertions(+)\n create mode 100644 t/lib-crlf-messages.sh\n\ndiff --git a/t/lib-crlf-messages.sh b/t/lib-crlf-messages.sh\nnew file mode 100644\nindex 00000000000..64d2ad12019\n--- /dev/null\n+++ b/t/lib-crlf-messages.sh\n@@ -0,0 +1,73 @@\n+# Setup refs with commit and tag messages containing CRLF\n+\n+create_crlf_ref () {\n+\tmessage=\"$1\" &&\n+\tsubject=\"$2\" &&\n+\tbody=\"$3\" &&\n+\tbranch=\"$4\" &&\n+\tprintf \"${message}\" >.crlf-message-${branch}.txt &&\n+\tprintf \"${subject}\" >.crlf-subject-${branch}.txt &&\n+\tprintf \"${body}\" >.crlf-body-${branch}.txt &&\n+ test_tick &&\n+\thash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&\n+\tgit branch ${branch} ${hash} &&\n+\tgit tag tag-${branch} ${branch} -F .crlf-message-${branch}.txt --cleanup=verbatim\n+}\n+\n+create_crlf_refs () {\n+\tmessage=\"Subject first line\\r\\n\\r\\nBody first line\\r\\nBody second line\\r\\n\" &&\n+\tbody=\"Body first line\\r\\nBody second line\\r\\n\" &&\n+\tsubject=\"Subject first line\" &&\n+\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"crlf\" &&\n+\tmessage=\"Subject first line\\r\\n\\r\\n\\r\\nBody first line\\r\\nBody second line\\r\\n\" &&\n+\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"crlf-empty-lines-after-subject\" &&\n+\tmessage=\"Subject first line\\r\\nSubject second line\\r\\n\\r\\nBody first line\\r\\nBody second line\\r\\n\" &&\n+\tsubject=\"Subject first line Subject second line\" &&\n+\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"crlf-two-line-subject\"\n+}\n+\n+test_create_crlf_refs () {\n+\ttest_expect_success 'setup refs with CRLF commit messages' '\n+\t\tcreate_crlf_refs\n+\t'\n+}\n+\n+cleanup_crlf_refs () {\n+\tfor branch in crlf crlf-empty-lines-after-subject crlf-two-line-subject; do\n+\t\tgit branch -D ${branch} &&\n+\t\tgit tag -d tag-${branch} &&\n+\t\trm .crlf-message-${branch}.txt &&\n+\t\trm .crlf-subject-${branch}.txt &&\n+\t\trm .crlf-body-${branch}.txt\n+\tdone\n+}\n+\n+test_cleanup_crlf_refs () {\n+\ttest_expect_success 'clenup refs with CRLF commit messages' '\n+\t\tcleanup_crlf_refs\n+\t'\n+}\n+\n+test_crlf_subject_body_and_contents() {\n+\tcommand_and_args=\"$@\" &&\n+\tcommand=$1 &&\n+\tif [ ${command} = \"branch\" ] || [ ${command} = \"for-each-ref\" ] || [ ${command} = \"tag\" ]; then\n+\t\tatoms=\"(contents:subject) (contents:body) (contents)\"\n+\telif [ ${command} = \"log\" ]; then\n+\t\tatoms=\"s b B\"\n+\tfi &&\n+\tfiles=\"subject body message\" &&\n+\twhile [ -n \"${atoms}\" ]; do\n+\t\tset ${atoms} && atom=$1 && shift && atoms=\"$*\" &&\n+\t\tset ${files} &&\tfile=$1 && shift && files=\"$*\" &&\n+\t\ttest_expect_success \"${command}: --format='%${atom}' works with CRLF input\" \"\n+\t\t\trm -f expect &&\n+\t\t\tfor ref in crlf crlf-empty-lines-after-subject crlf-two-line-subject; do\n+\t\t\t\tcat .crlf-${file}-\\\"\\${ref}\\\".txt >>expect &&\n+\t\t\t\tprintf \\\"\\n\\\" >>expect\n+\t\t\tdone &&\n+\t\t\tgit $command_and_args --format=\\\"%${atom}\\\" >actual &&\n+\t\t\ttest_cmp expect actual\n+\t\t\"\n+\tdone\n+}\n-- \ngitgitgadget\n\n"},{"id":"392979","messageId":"aab1f45ba976d088a8c68573a21ed2458915d6a6.1583807093.git.gitgitgadget@gmail.com","threadId":"52956","inReplyTo":"pull.576.v2.git.1583807093.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] ref-filter: fix the API to correctly handle CRLF","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-10T02:24:52Z","receivedAt":"2020-03-10T02:25:01Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nThe ref-filter API does not correctly handle commit or tag messages that\nuse CRLF as the line terminator. Such messages can be created with the\n`--verbatim` option of `git commit` and `git tag`, or by using `git\ncommit-tree` directly.\n\nThis impacts the output `git branch`, `git tag` and `git for-each-ref`\nwhen used with a `--format` argument containing the atoms\n`%(contents:subject)` or `%(contents:body)`, as well as the output of\n`git branch --verbose`, which uses `%(contents:subject)` internally.\n\nThe function find_subpos in ref-filter.c looks for two consecutive '\\n'\nto find the end of the subject line, a sequence which is absent in\nmessages using CRLF. This results in the whole message being parsed as\nthe subject line (`%(contents:subject)`), and the body of the message\n(`%(contents:body)`) being empty.\n\nMoreover, in copy_subject, '\\n' is replaced by space, but '\\r' is\nuntouched, resulting in the escape sequence '^M' being output verbatim\nin most terminal emulators:\n\n $ git branch --verbose\n * crlf 2113b0e Subject first line^M ^M Body first line^M Body second line\n\nThis bug is a regression for `git branch --verbose`, which\nbisects down to 949af0684c (branch: use ref-filter printing APIs,\n2017-01-10).\n\nFix this bug in ref-filter by hardening the logic in `copy_subject` and\n`find_subpos` to correctly parse messages containing CRFL.\n\nAdd tests for `branch`, `tag` and `for-each-ref` using\nlib-crlf-messages.sh.\n\nThe 'make commits' test at the beginning of t3203-branch-output.sh needs\nto be modified since it did not use `test_tick` and thus the commit\nhashes were not reproducible. For simplicity, use `test_commit` as the\ncontent and name of the files created in this setup test are irrelevant\nto the rest of the test script.\n\n`test_cleanup_crlf_refs` is used in t3203-branch-output.sh and\nt7004-tag.sh to avoid having to modify the expected output in later\ntests.\n\nSigned-off-by: Philippe Blain \n---\n ref-filter.c | 19 +++++++++++++++++--\n t/t3203-branch-output.sh | 26 +++++++++++++++++++++-----\n t/t6300-for-each-ref.sh | 5 +++++\n t/t7004-tag.sh | 7 +++++++\n 4 files changed, 50 insertions(+), 7 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 79bb5206783..537cc4de42c 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1050,10 +1050,18 @@ static char *copy_subject(const char *buf, unsigned long len)\n {\n \tchar *r = xmemdupz(buf, len);\n \tint i;\n+\tstruct strbuf sb = STRBUF_INIT;\n \n-\tfor (i = 0; i < len; i++)\n+\tstrbuf_attach(&sb, r, len, len + 1);\n+\tfor (i = 0; i < sb.len; i++) {\n \t\tif (r[i] == '\\n')\n \t\t\tr[i] = ' ';\n+\t\tif (r[i] == '\\r') {\n+\t\t\tstrbuf_remove(&sb, i, 1);\n+\t\t\ti -= 1;\n+\t\t}\n+\t}\n+\tstrbuf_detach(&sb, NULL);\n \n \treturn r;\n }\n@@ -1184,15 +1192,22 @@ static void find_subpos(const char *buf,\n \t\teol = strchrnul(buf, '\\n');\n \t\tif (*eol)\n \t\t\teol++;\n+\t\t/* protect against messages that might contain \\r\\n */\n+\t\tif (*eol == '\\r')\n+\t\t\teol++;\n \t\tbuf = eol;\n \t}\n \t*sublen = buf - *sub;\n \t/* drop trailing newline, if present */\n \tif (*sublen && (*sub)[*sublen - 1] == '\\n')\n \t\t*sublen -= 1;\n+\t/* protect against commit messages that might contain \\r\\n */\n+\telse if (*sublen && (*sub)[*sublen - 1] == '\\r')\n+\t\t*sublen -= 3; /* drop '\\r\\n\\r' */\n \n \t/* skip any empty lines */\n-\twhile (*buf == '\\n')\n+\t/* and protect against commit messages that might contain \\r\\n */\n+\twhile (*buf == '\\n' || *buf == '\\r')\n \t\tbuf++;\n \t*body = buf;\n \t*bodylen = strlen(buf);\ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nindex 71818b90f00..1235089619c 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3203-branch-output.sh\n@@ -3,13 +3,11 @@\n test_description='git branch display tests'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n+. \"$TEST_DIRECTORY\"/lib-crlf-messages.sh\n \n test_expect_success 'make commits' '\n-\techo content >file &&\n-\tgit add file &&\n-\tgit commit -m one &&\n-\techo content >>file &&\n-\tgit commit -a -m two\n+\ttest_commit one &&\n+\ttest_commit two\n '\n \n test_expect_success 'make branches' '\n@@ -95,6 +93,24 @@ test_expect_success 'git branch --ignore-case --list -v pattern shows branch sum\n \tawk \"{print \\$NF}\" actual &&\n \ttest_cmp expect actual\n '\n+test_create_crlf_refs\n+\n+test_expect_success 'git branch -v works with CRLF input' '\n+\tcat >expect <<-EOF &&\n+\t branch-one 139b20d two\n+\t branch-two d79ce16 one\n+\t crlf 2113b0e Subject first line\n+\t crlf-empty-lines-after-subject 0a9530d Subject first line\n+\t crlf-two-line-subject f9ded1f Subject first line Subject second line\n+\t* master 139b20d two\n+\tEOF\n+\tgit branch -v >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_crlf_subject_body_and_contents branch --list crlf*\n+\n+test_cleanup_crlf_refs\n \n test_expect_success 'git branch -v pattern does not show branch summaries' '\n \ttest_must_fail git branch -v branch*\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 9c910ce7467..467924de3df 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -8,6 +8,7 @@ test_description='for-each-ref test'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-gpg.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n+. \"$TEST_DIRECTORY\"/lib-crlf-messages.sh\n \n # Mon Jul 3 23:18:43 2006 +0000\n datestamp=1151968723\n@@ -888,4 +889,8 @@ test_expect_success 'for-each-ref --ignore-case ignores case' '\n \ttest_cmp expect actual\n '\n \n+test_create_crlf_refs\n+\n+test_crlf_subject_body_and_contents for-each-ref refs/heads/crlf*\n+\n test_done\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 6db92bd3ba6..25f465f34ea 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -10,6 +10,7 @@ Tests for operations with tags.'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-gpg.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n+. \"$TEST_DIRECTORY\"/lib-crlf-messages.sh\n \n # creating and listing lightweight tags:\n \n@@ -1969,6 +1970,12 @@ test_expect_success '--format should list tags as per format given' '\n \ttest_cmp expect actual\n '\n \n+test_create_crlf_refs\n+\n+test_crlf_subject_body_and_contents tag --list tag-crlf*\n+\n+test_cleanup_crlf_refs\n+\n test_expect_success \"set up color tests\" '\n \techo \"v1.0\" >expect.color &&\n \techo \"v1.0\" >expect.bare &&\n-- \ngitgitgadget\n\n"},{"id":"392980","messageId":"c84092e457c873e18548b647e2c672b737abcb24.1583807093.git.gitgitgadget@gmail.com","threadId":"52956","inReplyTo":"pull.576.v2.git.1583807093.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] log: add tests for messages containing CRLF to t4202","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-10T02:24:53Z","receivedAt":"2020-03-10T02:25:02Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nA previous commit fixed a bug in the ref-filter API causing messages\ncontaining CRLF to be incorrectly parsed and displayed.\n\nAdd a test to also check that `git log` correctly handles such messages,\nto prevent futur regressions.\n\nSigned-off-by: Philippe Blain \n---\n t/t4202-log.sh | 24 ++++++++++++++++++++++++\n 1 file changed, 24 insertions(+)\n\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 4694b6d0ce7..1e6149372c5 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -6,6 +6,7 @@ test_description='git log'\n . \"$TEST_DIRECTORY/lib-gpg.sh\"\n . \"$TEST_DIRECTORY/lib-terminal.sh\"\n . \"$TEST_DIRECTORY/lib-log-graph.sh\"\n+. \"$TEST_DIRECTORY/lib-crlf-messages.sh\"\n \n test_cmp_graph () {\n \tlib_test_cmp_graph --format=%s \"$@\"\n@@ -105,6 +106,29 @@ test_expect_success 'oneline' '\n \ttest_cmp expect actual\n '\n \n+test_create_crlf_refs\n+\n+test_expect_success 'oneline with CRLF messages' '\n+\techo \"002093e Subject first line\" >expect &&\n+\tgit log --oneline -1 crlf >actual-branch &&\n+\tgit log --oneline -1 tag-crlf >actual-tag &&\n+\ttest_cmp expect actual-branch &&\n+\ttest_cmp expect actual-tag &&\n+\techo \"6f814b0 Subject first line\" >expect &&\n+\tgit log --oneline -1 crlf-empty-lines-after-subject >actual-branch &&\n+\tgit log --oneline -1 tag-crlf-empty-lines-after-subject >actual-tag &&\n+\ttest_cmp expect actual-branch &&\n+\ttest_cmp expect actual-tag &&\n+\techo \"8c58a85 Subject first line Subject second line\" >expect &&\n+\tgit log --oneline -1 crlf-two-line-subject >actual-branch &&\n+\tgit log --oneline -1 tag-crlf-two-line-subject >actual-tag &&\n+\ttest_cmp expect actual-branch &&\n+\ttest_cmp expect actual-tag\n+'\n+test_crlf_subject_body_and_contents log --all --reverse --grep Subject\n+\n+test_cleanup_crlf_refs\n+\n test_expect_success 'diff-filter=A' '\n \n \tgit log --no-renames --pretty=\"format:%s\" --diff-filter=A HEAD > actual &&\n-- \ngitgitgadget\n"},{"id":"392981","messageId":"xmqqr1y0evi0.fsf@gitster.c.googlers.com","threadId":"52956","inReplyTo":"pull.576.v2.git.1583807093.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/3] Teach ref-filter API to correctly handle CRLF in messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-10T03:31:19Z","receivedAt":"2020-03-10T03:31:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philippe Blain via GitGitGadget\" writes:\n\n> The function find_subpos in ref-filter.c looks for two consecutive '\\n' to\n> find the end of the subject line, a sequence which is absent in messages\n> using CRLF.\n\nThis is crucial, and makes it obvious why a change is needed.\nThanks for clearly writing it out.\n"},{"id":"393007","messageId":"xmqqo8t4jf71.fsf@gitster.c.googlers.com","threadId":"52956","inReplyTo":"pull.576.v2.git.1583807093.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/3] Teach ref-filter API to correctly handle CRLF in messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-10T17:24:34Z","receivedAt":"2020-03-10T17:24:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philippe Blain via GitGitGadget\" writes:\n\n> The function find_subpos in ref-filter.c looks for two consecutive '\\n' to\n> find the end of the subject line, a sequence which is absent in messages\n> using CRLF. This results in the whole message being parsed as the subject\n> line (%(contents:subject)), and the body of the message (%(contents:body))\n> being empty.\n\nTo be honest, I suspect that it is not a bug in the parser that\nparsed out %(contents:subject), but a user error that left the log\nmessage in CRLF endings ;-).\n\nSo \"correctly handle CRLF\" is probably a tad unfair to those who\nwrote the current ref-filter code; a description that is more fair\nto them is probably along the lines of \"handle malformed log\nmessages more gracefully\", I would think.\n\n> Moreover, in copy_subject, '\\n' is replaced by space, but '\\r' is untouched,\n> resulting in the escape sequence '^M' being output verbatim in most terminal\n> emulators:\n>\n> $ git branch --verbose\n> * crlf 2113b0e Subject first line^M ^M Body first line^M Body second line\n>\n> This bug is a regression for git branch --verbose, which bisects down to\n> 949af06 (branch: use ref-filter printing APIs, 2017-01-10).\n\nI am not sure where you want to go with this. Whether it is shown\nin the ^X notation (and some terminals even reverse color to\nhighlight them), or it is shown literally (i.e. causing the next\nbyte to overwrite the same line starting from the left-edge), you\nwould be annoyed either way, no? I suspect that the latter would\nannoy you even more. Isn't what \"most terminal emulators\" do,\ni.e. to show it in the ^X notation instead of emitting it literally,\na good thing? IOW, \"resulting in ...\" is not correctly telling us\nwhat you think is wrong---you don't have to blame terminals.\n\nIt is not limited to CR, and is not limited to control characters at\nthe end of the lines, no? If you had \"\\a\" (or \"\\r\") in the middle\nof the title, either the current or the old code would ring a bell\n(or cause the next character to appear at the end of the same line)\nor when piped to \"less\" you'd see \"^G\" (or \"^M\") in the liddle of\nthe line.\n\nThe old code used pretty.c::pretty_print_commit() mechanism;\npretty.c::format_subject() uses pretty.c::is_blank_line() to trim\nwhitespaces at the right end while trying to notice where the first\nparagraph break is, so any whitespace at the end of first paragraph\nbreak is removed, and each end of line got replaced by a SP, but it\ndid not do anything special to control characters in the middle of\nthe lines (and it didn't do anything to the control characters in\nthe middle of the line, either). So while the old code happened to\ncleanse CR at the end of the lines, it wasn't doing enough.\n\nI think fixing _that_ is (and should be) outside the scope of this\nseries, of course.\n\n> 2: c68bc2b3788 ! 2: aab1f45ba97 ref-filter: teach the API to correctly handle CRLF\n> @@ -1,26 +1,49 @@\n> Author: Philippe Blain \n> \n> - ref-filter: teach the API to correctly handle CRLF\n> + ref-filter: fix the API to correctly handle CRLF\n\nAPI is not changed (i.e. the callers do not have to do anything\nspecial); only the implementation.\n\n\tref-filter: handle CR at the end of the lines more gracefully\n\nperhaps?\n\n> The ref-filter API does not correctly handle commit or tag messages that\n> use CRLF as the line terminator. Such messages can be created with the\n> `--verbatim` option of `git commit` and `git tag`, or by using `git\n> commit-tree` directly.\n> \n> + This impacts the output `git branch`, `git tag` and `git for-each-ref`\n> + when used with a `--format` argument containing the atoms\n> + `%(contents:subject)` or `%(contents:body)`, as well as the output of\n> + `git branch --verbose`, which uses `%(contents:subject)` internally.\n\nIn other words...\n\n\tWhen a commit or a tag object uses CRLF line endings, the\n\tref-filter machinery does not identify the end of the first\n\tparagraph as intended by the writer, because it only looks\n\tfor two consecutive LFs and CR-LF-CR-LF does not look like a\n\tblank line that separates paragraphs to it. \"git branch\",\n\t\"git tag\" and \"git for-each-ref\" all rely on the messages\n\tsplit correctly into \"%(contents:subject)\" and\n\t\"%(contents:body)\" placeholders and ends up showing\n\teverything as the subject.\n\nNow based on what I hinted in the far-above part, there can be two\nvalid solutions here.\n\n * recognize CRLF as a valid line ending, but still retain ^M in the\n message. The replacement for \"%(contents:subject)\" would still\n end with \"^M\", and we add LF to it, which makes the resulting\n output end with CRLF and all is well. This will keep \"\\a\" and\n \"\\r\" in the middle of the line in the output.\n\n * strip CR and any control character other than LF from everywhere.\n This will cleanse \"\\a\" and \"\\r\" in the middle of, or anywhere on,\n the line, so that \"%(contents:subject)\", \"%(contents:body)\" and\n \"%(contents)\" all are \"clean\".\n\nI am not offhand sure which one is better (I haven't read the patch\nto see which one you chose to implement).\n\n> + The function find_subpos in ref-filter.c looks for two consecutive '\\n'\n> + to find the end of the subject line, a sequence which is absent in\n> + messages using CRLF. This results in the whole message being parsed as\n> + the subject line (`%(contents:subject)`), and the body of the message\n> + (`%(contents:body)`) being empty.\n\n\n\n> + Moreover, in copy_subject, '\\n' is replaced by space, but '\\r' is\n> + untouched, resulting in the escape sequence '^M' being output verbatim\n> + in most terminal emulators:\n> ...\n> + This bug is a regression for `git branch --verbose`, which\n> + bisects down to 949af0684c (branch: use ref-filter printing APIs,\n> + 2017-01-10).\n> +\n> + Fix this bug in ref-filter by hardening the logic in `copy_subject` and\n> + `find_subpos` to correctly parse messages containing CRFL.\n\nThe above few lines may need revising (based on what I said to the\ncover); --- even if they don't, CRFL here needs to become CRLF ;-)\n\nThanks for working on this.\n"},{"id":"393012","messageId":"xmqqk13sjdz8.fsf@gitster.c.googlers.com","threadId":"52956","inReplyTo":"aab1f45ba976d088a8c68573a21ed2458915d6a6.1583807093.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] ref-filter: fix the API to correctly handle CRLF","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-10T17:50:51Z","receivedAt":"2020-03-10T17:51:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philippe Blain via GitGitGadget\" writes:\n\n> From: Philippe Blain \n>\n> The ref-filter API does not correctly handle commit or tag messages that\n> ...\n\n(I won't repeat myself here; see 0/3)\n\n> Signed-off-by: Philippe Blain \n> ---\n> ref-filter.c | 19 +++++++++++++++++--\n> t/t3203-branch-output.sh | 26 +++++++++++++++++++++-----\n> t/t6300-for-each-ref.sh | 5 +++++\n> t/t7004-tag.sh | 7 +++++++\n> 4 files changed, 50 insertions(+), 7 deletions(-)\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 79bb5206783..537cc4de42c 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1050,10 +1050,18 @@ static char *copy_subject(const char *buf, unsigned long len)\n> {\n> \tchar *r = xmemdupz(buf, len);\n> \tint i;\n> +\tstruct strbuf sb = STRBUF_INIT;\n> \n> -\tfor (i = 0; i < len; i++)\n> +\tstrbuf_attach(&sb, r, len, len + 1);\n> +\tfor (i = 0; i < sb.len; i++) {\n> \t\tif (r[i] == '\\n')\n> \t\t\tr[i] = ' ';\n> +\t\tif (r[i] == '\\r') {\n> +\t\t\tstrbuf_remove(&sb, i, 1);\n> +\t\t\ti -= 1;\n> +\t\t}\n> +\t}\n> +\tstrbuf_detach(&sb, NULL);\n> \treturn r;\n> }\n\nSo, the chosen solution is to only remove CR and do so only in the\nfirst paragraph. Even if the second and subsequent paragraphs use\nCRLF line endings, those CRs are retained. Also, a lone CR in the\nfirst paragraph that is not part of CRLF end-of-line marker is lost,\nbut other control characters like \"\\a\" are retained. That sounds\nlike an almost \"minimum\" change but not quite.\n\nThe way strbuf is used in the implementation is a bit curious and\nrisky. Currently we do not realloc to shrink a strbuf, but when we\nstart doing so, this code would break because you are relying on the\nfact that just before calling strbuf_detach(), sb.buf happens to be\nstill the same as r.\n\nAs the point of the function is \"we want to return a copy of what is\nin buf[0..len] but the input is a (possibly multi-line) paragraph,\nand we want a single line 'title', so replace end-of-line with a\nSP\", a minimal translation that is more faithful to the intended\nmeaning of the function would be:\n\n\tstatic char *copy_subject(const char *buf, unsigned long len)\n\t{\n\t\tstruct strbuf sb = STRBUF_INIT;\n\n\t\tfor (i = 0; i < len; i++) {\n \tif (buf[i] == '\\r' &&\n\t\t\t i + 1 < len && buf[i + 1] == '\\n')\n continue; /* ignore CR in CRLF */\n\n\t\t\tif (buf[i] == '\\n')\n\t\t\t\tstrbuf_addch(&sb, ' ');\n\t\t\telse\n\t\t\t\tstrbuf_addch(&sb, buf[i]);\n\t\t}\n return strbuf_detach(&sb, NULL);\n\t}\n\nperhaps? This retains CR in the middle if exists just like BEL in\nthe middle of the line, and uses strbuf in a safe way, I think.\n\n> @@ -1184,15 +1192,22 @@ static void find_subpos(const char *buf,\n> \t\teol = strchrnul(buf, '\\n');\n> \t\tif (*eol)\n> \t\t\teol++;\n> +\t\t/* protect against messages that might contain \\r\\n */\n> +\t\tif (*eol == '\\r')\n> +\t\t\teol++;\n\nThis is quite convoluted. You found a LF and then are hoping to see\nif the byte after LF is CR (i.e. you are looking for LFCR, not\nCRLF).\n\n> \t\tbuf = eol;\n> \t}\n> \t*sublen = buf - *sub;\n> \t/* drop trailing newline, if present */\n> \tif (*sublen && (*sub)[*sublen - 1] == '\\n')\n> \t\t*sublen -= 1;\n> +\t/* protect against commit messages that might contain \\r\\n */\n> +\telse if (*sublen && (*sub)[*sublen - 1] == '\\r')\n> +\t\t*sublen -= 3; /* drop '\\r\\n\\r' */\n\nYeek. To find CR-LF-CR-LF, you look for CR-LF-CR? You only checked\nthat the previous byte is NOT LF (because you are in else-if, so the\nprevious if must have failed) and you have at least one previous byte\nthat is CR. What gives us OK that *sublen is sufficiently long that\nwe can safely subtract 3 from it (we only checked that it is not 0;\nwho says it is 3 or more in this code???) and the two bytes before\nthe CR we are looking at here are CRLF???\n"},{"id":"407357","messageId":"11d044a4f7feccdf20da6364a1f9bbe934e9981f.1602526169.git.gitgitgadget@gmail.com","threadId":"52956","inReplyTo":"pull.576.v3.git.1602526169.gitgitgadget@gmail.com","subject":"[PATCH v3 2/3] ref-filter: handle CRLF at end-of-line more gracefully","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-12T18:09:28Z","receivedAt":"2020-10-12T18:09:39Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nThe ref-filter code does not correctly handle commit or tag messages that\nuse CRLF as the line terminator. Such messages can be created with the\n`--verbatim` option of `git commit` and `git tag`, or by using `git\ncommit-tree` directly.\n\nThe function `find_subpos` in ref-filter.c looks for two consecutive\nLFs to find the end of the subject line, a sequence which is absent in\nmessages using CRLF. This results in the whole message being parsed as\nthe subject line (`%(contents:subject)`), and the body of the message\n(`%(contents:body)`) being empty.\n\nMoreover, in `copy_subject`, which wants to return the subject as a\nsingle line, '\\n' is replaced by space, but '\\r' is\nuntouched.\n\nThis impacts the output of `git branch`, `git tag` and `git\nfor-each-ref`.\n\nThis bug is a regression for `git branch --verbose`, which\nbisects down to 949af0684c (branch: use ref-filter printing APIs,\n2017-01-10).\n\nFix this bug in ref-filter by hardening the logic in `copy_subject` and\n`find_subpos` to correctly parse messages containing CRLF.\n\nAdd tests for `branch`, `tag` and `for-each-ref` using\nlib-crlf-messages.sh.\n\nThe 'make commits' test at the beginning of t3203-branch-output.sh does\nnot use `test_tick` and thus the commit hashes are not reproducible. For\nsimplicity, use `test_commit` to create the commits, as the content and\nname of the files created in this setup test are irrelevant to the rest\nof the test script.\n\nUse `test_cleanup_crlf_refs` in t3203-branch-output.sh and t7004-tag.sh\nto avoid having to modify the expected output in later tests.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Philippe Blain \n---\n ref-filter.c | 36 +++++++++++++++++++++---------------\n t/t3203-branch-output.sh | 31 ++++++++++++++++++++++++++-----\n t/t6300-for-each-ref.sh | 5 +++++\n t/t7004-tag.sh | 7 +++++++\n 4 files changed, 59 insertions(+), 20 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex c62f6b4822..92d8ca5340 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1097,14 +1097,18 @@ static const char *copy_email(const char *buf, struct used_atom *atom)\n \n static char *copy_subject(const char *buf, unsigned long len)\n {\n-\tchar *r = xmemdupz(buf, len);\n-\tint i;\n+\tstruct strbuf sb = STRBUF_INIT;\n \n-\tfor (i = 0; i < len; i++)\n-\t\tif (r[i] == '\\n')\n-\t\t\tr[i] = ' ';\n+\tfor (int i = 0; i < len; i++) {\n+\t\tif (buf[i] == '\\r' && i + 1 < len && buf[i + 1] == '\\n')\n+\t\t\tcontinue; /* ignore CR in CRLF */\n \n-\treturn r;\n+\t\tif (buf[i] == '\\n')\n+\t\t\tstrbuf_addch(&sb, ' ');\n+\t\telse\n+\t\t\tstrbuf_addch(&sb, buf[i]);\n+\t}\n+\treturn strbuf_detach(&sb, NULL);\n }\n \n static void grab_date(const char *buf, struct atom_value *v, const char *atomname)\n@@ -1228,20 +1232,22 @@ static void find_subpos(const char *buf,\n \n \t/* subject is first non-empty line */\n \t*sub = buf;\n-\t/* subject goes to first empty line */\n-\twhile (buf < *sig && *buf && *buf != '\\n') {\n-\t\teol = strchrnul(buf, '\\n');\n-\t\tif (*eol)\n-\t\t\teol++;\n-\t\tbuf = eol;\n-\t}\n+\t/* subject goes to first empty line before signature begins */\n+\tif ((eol = strstr(*sub, \"\\n\\n\"))) {\n+\t\teol = eol < *sig ? eol : *sig;\n+\t/* check if message uses CRLF */\n+\t} else if (! (eol = strstr(*sub, \"\\r\\n\\r\\n\"))) {\n+\t\t/* treat whole message as subject */\n+\t\teol = strrchr(*sub, '\\0');\n+\t}\n+\tbuf = eol;\n \t*sublen = buf - *sub;\n \t/* drop trailing newline, if present */\n-\tif (*sublen && (*sub)[*sublen - 1] == '\\n')\n+\twhile (*sublen && ((*sub)[*sublen - 1] == '\\n' || (*sub)[*sublen - 1] == '\\r'))\n \t\t*sublen -= 1;\n \n \t/* skip any empty lines */\n-\twhile (*buf == '\\n')\n+\twhile (*buf == '\\n' || *buf == '\\r')\n \t\tbuf++;\n \t*body = buf;\n \t*bodylen = strlen(buf);\ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nindex 71818b90f0..c06eca774f 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3203-branch-output.sh\n@@ -3,13 +3,11 @@\n test_description='git branch display tests'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n+. \"$TEST_DIRECTORY\"/lib-crlf-messages.sh\n \n test_expect_success 'make commits' '\n-\techo content >file &&\n-\tgit add file &&\n-\tgit commit -m one &&\n-\techo content >>file &&\n-\tgit commit -a -m two\n+\ttest_commit one &&\n+\ttest_commit two\n '\n \n test_expect_success 'make branches' '\n@@ -95,6 +93,29 @@ test_expect_success 'git branch --ignore-case --list -v pattern shows branch sum\n \tawk \"{print \\$NF}\" actual &&\n \ttest_cmp expect actual\n '\n+test_create_crlf_refs\n+\n+test_expect_success 'git branch -v works with CRLF input' '\n+\tcat >expect <<-EOF &&\n+\t two\n+\t one\n+\t Subject first line\n+\t Subject first line\n+\t Subject first line Subject second line\n+\t Subject first line Subject second line\n+\t Subject first line Subject second line\n+\t Subject first line Subject second line\n+\tEOF\n+\tgit branch -v >tmp &&\n+\t# Remove first two columns, and the line for the currently checked out branch\n+\tcurrent=$(git branch --show-current) &&\n+\tgrep -v $current actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_crlf_subject_body_and_contents branch --list crlf*\n+\n+test_cleanup_crlf_refs\n \n test_expect_success 'git branch -v pattern does not show branch summaries' '\n \ttest_must_fail git branch -v branch*\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex b359023189..c30940cf7a 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -8,6 +8,7 @@ test_description='for-each-ref test'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-gpg.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n+. \"$TEST_DIRECTORY\"/lib-crlf-messages.sh\n \n # Mon Jul 3 23:18:43 2006 +0000\n datestamp=1151968723\n@@ -1030,4 +1031,8 @@ test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' '\n \ttest_cmp expect actual\n '\n \n+test_create_crlf_refs\n+\n+test_crlf_subject_body_and_contents for-each-ref refs/heads/crlf*\n+\n test_done\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 05f411c821..cda735dab4 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -10,6 +10,7 @@ Tests for operations with tags.'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-gpg.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n+. \"$TEST_DIRECTORY\"/lib-crlf-messages.sh\n \n # creating and listing lightweight tags:\n \n@@ -1970,6 +1971,12 @@ test_expect_success '--format should list tags as per format given' '\n \ttest_cmp expect actual\n '\n \n+test_create_crlf_refs\n+\n+test_crlf_subject_body_and_contents tag --list tag-crlf*\n+\n+test_cleanup_crlf_refs\n+\n test_expect_success \"set up color tests\" '\n \techo \"v1.0\" >expect.color &&\n \techo \"v1.0\" >expect.bare &&\n-- \ngitgitgadget\n\n"},{"id":"407360","messageId":"pull.576.v3.git.1602526169.gitgitgadget@gmail.com","threadId":"52956","inReplyTo":"pull.576.v2.git.1583807093.gitgitgadget@gmail.com","subject":"[PATCH v3 0/3] ref-filter: handle CRLF at end-of-line more gracefully","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-12T18:09:26Z","receivedAt":"2020-10-12T18:09:39Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Changes since v2:\n\n * in response to Junio's comments, improved the logic in 'find_subpos'. I\n tried to also simplify the code by using 'str*' functions, to make it\n more obvious what the code is actually looking for in the message. \n * took Junio's suggestion for 'copy_subject'\n * in response to Junio's comments, tweaked the commit message for patch 2/3 \n * refactored 'lib-crlf-messages.sh' to reduce duplication (patch 1/3)\n * added tests for 'git show' in patch 3/3 \n * removed commit hashes in tests to make them hash-agnostic\n\n\n----------------------------------------------------------------------------\n\nThe ref-filter code does not correctly handle commit or tag messages that\nuse CRLF as the line terminator. Such messages can be created with the\n--verbatim option of git commit and git tag, or by using git commit-tree \ndirectly.\n\nThe function find_subpos in ref-filter.c looks for two consecutive '\\n' to\nfind the end of the subject line, a sequence which is absent in messages\nusing CRLF. This results in the whole message being parsed as the subject\nline (%(contents:subject)), and the body of the message (%(contents:body))\nbeing empty.\n\nMoreover, in copy_subject, '\\n' is replaced by space, but '\\r' is untouched.\n\nThis bug is a regression for git branch --verbose, which bisects down to\n949af06 (branch: use ref-filter printing APIs, 2017-01-10).\n\nThe first patch in this series adds a new test library, \nt/lib-crlf-messages.sh, that contains functions to test this behaviour for\ncommands using either the ref-filter or the pretty API to display messages. \n\nThe second patch fixes the bug in ref-filter.c and adds corresponding tests.\n\nFinally, the third patch adds a test that checks the behaviour of git log \nandgit show in the presence of CRLF in messages, to prevent futur\nregressions.\n\n\n----------------------------------------------------------------------------\n\nv2:\n\nThe ref-filter API does not correctly handle commit or tag messages that use\nCRLF as the line terminator. Such messages can be created with the--verbatim \noption of git commit and git tag, or by using git commit-tree directly.\n\nThis impacts the output git branch, git tag and git for-each-refwhen used\nwith a --format argument containing the atoms%(contents:subject) or \n%(contents:body), as well as the output ofgit branch --verbose, which uses \n%(contents:subject) internally.\n\nThe function find_subpos in ref-filter.c looks for two consecutive '\\n' to\nfind the end of the subject line, a sequence which is absent in messages\nusing CRLF. This results in the whole message being parsed as the subject\nline (%(contents:subject)), and the body of the message (%(contents:body))\nbeing empty.\n\nMoreover, in copy_subject, '\\n' is replaced by space, but '\\r' is untouched,\nresulting in the escape sequence '^M' being output verbatim in most terminal\nemulators:\n\n$ git branch --verbose\n* crlf 2113b0e Subject first line^M ^M Body first line^M Body second line\n\nThis bug is a regression for git branch --verbose, which bisects down to\n949af06 (branch: use ref-filter printing APIs, 2017-01-10).\n\nThe first patch in this series adds a new test library, \nt/lib-crlf-messages.sh, that contains functions to test this behaviour for\ncommands using either the ref-filter or the pretty API to display messages. \n\nThe second patch fixes the bug in ref-filter.c and adds corresponding tests.\n\nFinally, the third patch adds a test that checks the behaviour of git log in\nthe presence of CRLF in messages, to prevent futur regressions.\n\nchanges since v1:\n\n * improved the cover letter and the commit message of the 2nd patch to\n actually describe the bug this series is fixing\n\n\n----------------------------------------------------------------------------\n\nv1:\n\nThe ref-filter API does not correctly handle commit or tag messages that use\nCRLF as the line terminator. Such messages can be created with the--verbatim \noption of git commit and git tag, or by using git commit-tree directly.\n\nThis impacts the output of git branch -v, and git branch, git tag and git\nfor-each-ref when used with a --format argument containing the atoms \n%(contents:subject) or %(contents:body).\n\nThe first patch in this series adds a new test library, \nt/lib-crlf-messages.sh, that contains functions to test this behaviour for\ncommands using either the ref-filter or the pretty API to display messages. \n\nThe second patch fixes the bug in ref-filter.c and adds corresponding tests.\n\nFinally, the third patch adds a test that checks the behaviour of git log in\nthe presence of CRLF in messages, to prevent futur regressions.\n\nPhilippe Blain (3):\n t: add lib-crlf-messages.sh for messages containing CRLF\n ref-filter: handle CRLF at end-of-line more gracefully\n log, show: add tests for messages containing CRLF\n\n ref-filter.c | 36 +++++++++-------\n t/lib-crlf-messages.sh | 90 ++++++++++++++++++++++++++++++++++++++++\n t/t3203-branch-output.sh | 31 +++++++++++---\n t/t4202-log.sh | 18 ++++++++\n t/t6300-for-each-ref.sh | 5 +++\n t/t7004-tag.sh | 7 ++++\n t/t7007-show.sh | 5 +++\n 7 files changed, 172 insertions(+), 20 deletions(-)\n create mode 100644 t/lib-crlf-messages.sh\n\n\nbase-commit: d4a392452e292ff924e79ec8458611c0f679d6d4\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-576%2Fphil-blain%2Ffix-branch-verbose-crlf-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-576/phil-blain/fix-branch-verbose-crlf-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/576\n\nRange-diff vs v2:\n\n 1: b252113803 ! 1: f17d182c3b t: add lib-crlf-messages.sh for messages containing CRLF\n @@ Commit message\n commit messages, so that we can easily test that this bug does not\n appear in other commands in the future.\n \n - The function test_crlf_subject_body_and_contents can be used to test\n + The function `test_crlf_subject_body_and_contents` can be used to test\n that the `--format` option of `branch`, `tag`, `for-each-ref` and\n - `log` correctly displays the subject, body and raw body of commits and\n + `log` correctly displays the subject, body and raw content of commits and\n tag messages.\n \n The commits are created using `commit-tree` such that the current branch\n in the test repository is not affected when `test_create_crlf_refs` is\n - called in a test. This is to done so that the CRLF tests can be inserted\n - anywhere in a test script where it makes sense to do so, without having\n - to potentially modify further test that depend on output that would be\n + called in a test. This is done so that the CRLF tests can be inserted\n + anywhere in a test script, where it makes sense to do so, without having\n + to potentially modify further tests that depend on output that would be\n modified if the current branch gained new commits.\n \n Signed-off-by: Philippe Blain \n @@ t/lib-crlf-messages.sh (new)\n @@\n +# Setup refs with commit and tag messages containing CRLF\n +\n ++LIB_CRLF_BRANCHES=\"\"\n ++\n +create_crlf_ref () {\n +\tmessage=\"$1\" &&\n +\tsubject=\"$2\" &&\n @@ t/lib-crlf-messages.sh (new)\n +\tprintf \"${message}\" >.crlf-message-${branch}.txt &&\n +\tprintf \"${subject}\" >.crlf-subject-${branch}.txt &&\n +\tprintf \"${body}\" >.crlf-body-${branch}.txt &&\n -+ test_tick &&\n ++\tLIB_CRLF_BRANCHES=\"${LIB_CRLF_BRANCHES} ${branch}\"\n ++\ttest_tick &&\n +\thash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&\n +\tgit branch ${branch} ${hash} &&\n +\tgit tag tag-${branch} ${branch} -F .crlf-message-${branch}.txt --cleanup=verbatim\n @@ t/lib-crlf-messages.sh (new)\n +\tmessage=\"Subject first line\\r\\n\\r\\nBody first line\\r\\nBody second line\\r\\n\" &&\n +\tbody=\"Body first line\\r\\nBody second line\\r\\n\" &&\n +\tsubject=\"Subject first line\" &&\n -+\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"crlf\" &&\n ++\tbranch=\"crlf\" &&\n ++\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\" &&\n +\tmessage=\"Subject first line\\r\\n\\r\\n\\r\\nBody first line\\r\\nBody second line\\r\\n\" &&\n -+\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"crlf-empty-lines-after-subject\" &&\n ++\tbranch=\"crlf-empty-lines-after-subject\" &&\n ++\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\" &&\n +\tmessage=\"Subject first line\\r\\nSubject second line\\r\\n\\r\\nBody first line\\r\\nBody second line\\r\\n\" &&\n +\tsubject=\"Subject first line Subject second line\" &&\n -+\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"crlf-two-line-subject\"\n ++\tbranch=\"crlf-two-line-subject\" &&\n ++\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\" &&\n ++\tmessage=\"Subject first line\\r\\nSubject second line\" &&\n ++\tsubject=\"Subject first line Subject second line\" &&\n ++\tbody=\"\" &&\n ++\tbranch=\"crlf-two-line-subject-no-body\" &&\n ++\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\" &&\n ++\tmessage=\"Subject first line\\r\\nSubject second line\\r\\n\" &&\n ++\tbranch=\"crlf-two-line-subject-no-body-trailing-newline\" &&\n ++\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\" &&\n ++\tmessage=\"Subject first line\\r\\nSubject second line\\r\\n\\r\" &&\n ++\tbranch=\"crlf-two-line-subject-no-body-trailing-newline2\" &&\n ++\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\"\n +}\n +\n +test_create_crlf_refs () {\n @@ t/lib-crlf-messages.sh (new)\n +}\n +\n +cleanup_crlf_refs () {\n -+\tfor branch in crlf crlf-empty-lines-after-subject crlf-two-line-subject; do\n ++\tfor branch in ${LIB_CRLF_BRANCHES}; do\n +\t\tgit branch -D ${branch} &&\n +\t\tgit tag -d tag-${branch} &&\n +\t\trm .crlf-message-${branch}.txt &&\n @@ t/lib-crlf-messages.sh (new)\n +}\n +\n +test_cleanup_crlf_refs () {\n -+\ttest_expect_success 'clenup refs with CRLF commit messages' '\n ++\ttest_expect_success 'cleanup refs with CRLF commit messages' '\n +\t\tcleanup_crlf_refs\n +\t'\n +}\n @@ t/lib-crlf-messages.sh (new)\n +\tcommand=$1 &&\n +\tif [ ${command} = \"branch\" ] || [ ${command} = \"for-each-ref\" ] || [ ${command} = \"tag\" ]; then\n +\t\tatoms=\"(contents:subject) (contents:body) (contents)\"\n -+\telif [ ${command} = \"log\" ]; then\n ++\telif [ ${command} = \"log\" ] || [ ${command} = \"show\" ]; then\n +\t\tatoms=\"s b B\"\n +\tfi &&\n +\tfiles=\"subject body message\" &&\n @@ t/lib-crlf-messages.sh (new)\n +\t\tset ${files} &&\tfile=$1 && shift && files=\"$*\" &&\n +\t\ttest_expect_success \"${command}: --format='%${atom}' works with CRLF input\" \"\n +\t\t\trm -f expect &&\n -+\t\t\tfor ref in crlf crlf-empty-lines-after-subject crlf-two-line-subject; do\n ++\t\t\tfor ref in ${LIB_CRLF_BRANCHES}; do\n +\t\t\t\tcat .crlf-${file}-\\\"\\${ref}\\\".txt >>expect &&\n +\t\t\t\tprintf \\\"\\n\\\" >>expect\n +\t\t\tdone &&\n 2: aab1f45ba9 ! 2: 11d044a4f7 ref-filter: fix the API to correctly handle CRLF\n @@ Metadata\n Author: Philippe Blain \n \n ## Commit message ##\n - ref-filter: fix the API to correctly handle CRLF\n + ref-filter: handle CRLF at end-of-line more gracefully\n \n - The ref-filter API does not correctly handle commit or tag messages that\n + The ref-filter code does not correctly handle commit or tag messages that\n use CRLF as the line terminator. Such messages can be created with the\n `--verbatim` option of `git commit` and `git tag`, or by using `git\n commit-tree` directly.\n \n - This impacts the output `git branch`, `git tag` and `git for-each-ref`\n - when used with a `--format` argument containing the atoms\n - `%(contents:subject)` or `%(contents:body)`, as well as the output of\n - `git branch --verbose`, which uses `%(contents:subject)` internally.\n -\n - The function find_subpos in ref-filter.c looks for two consecutive '\\n'\n - to find the end of the subject line, a sequence which is absent in\n + The function `find_subpos` in ref-filter.c looks for two consecutive\n + LFs to find the end of the subject line, a sequence which is absent in\n messages using CRLF. This results in the whole message being parsed as\n the subject line (`%(contents:subject)`), and the body of the message\n (`%(contents:body)`) being empty.\n \n - Moreover, in copy_subject, '\\n' is replaced by space, but '\\r' is\n - untouched, resulting in the escape sequence '^M' being output verbatim\n - in most terminal emulators:\n + Moreover, in `copy_subject`, which wants to return the subject as a\n + single line, '\\n' is replaced by space, but '\\r' is\n + untouched.\n \n - $ git branch --verbose\n - * crlf 2113b0e Subject first line^M ^M Body first line^M Body second line\n + This impacts the output of `git branch`, `git tag` and `git\n + for-each-ref`.\n \n This bug is a regression for `git branch --verbose`, which\n bisects down to 949af0684c (branch: use ref-filter printing APIs,\n 2017-01-10).\n \n Fix this bug in ref-filter by hardening the logic in `copy_subject` and\n - `find_subpos` to correctly parse messages containing CRFL.\n + `find_subpos` to correctly parse messages containing CRLF.\n \n Add tests for `branch`, `tag` and `for-each-ref` using\n lib-crlf-messages.sh.\n \n - The 'make commits' test at the beginning of t3203-branch-output.sh needs\n - to be modified since it did not use `test_tick` and thus the commit\n - hashes were not reproducible. For simplicity, use `test_commit` as the\n - content and name of the files created in this setup test are irrelevant\n - to the rest of the test script.\n + The 'make commits' test at the beginning of t3203-branch-output.sh does\n + not use `test_tick` and thus the commit hashes are not reproducible. For\n + simplicity, use `test_commit` to create the commits, as the content and\n + name of the files created in this setup test are irrelevant to the rest\n + of the test script.\n \n - `test_cleanup_crlf_refs` is used in t3203-branch-output.sh and\n - t7004-tag.sh to avoid having to modify the expected output in later\n - tests.\n + Use `test_cleanup_crlf_refs` in t3203-branch-output.sh and t7004-tag.sh\n + to avoid having to modify the expected output in later tests.\n \n + Helped-by: Junio C Hamano \n Signed-off-by: Philippe Blain \n \n ## ref-filter.c ##\n -@@ ref-filter.c: static char *copy_subject(const char *buf, unsigned long len)\n +@@ ref-filter.c: static const char *copy_email(const char *buf, struct used_atom *atom)\n + \n + static char *copy_subject(const char *buf, unsigned long len)\n {\n - \tchar *r = xmemdupz(buf, len);\n - \tint i;\n +-\tchar *r = xmemdupz(buf, len);\n +-\tint i;\n +\tstruct strbuf sb = STRBUF_INIT;\n \n -\tfor (i = 0; i < len; i++)\n -+\tstrbuf_attach(&sb, r, len, len + 1);\n -+\tfor (i = 0; i < sb.len; i++) {\n - \t\tif (r[i] == '\\n')\n - \t\t\tr[i] = ' ';\n -+\t\tif (r[i] == '\\r') {\n -+\t\t\tstrbuf_remove(&sb, i, 1);\n -+\t\t\ti -= 1;\n -+\t\t}\n -+\t}\n -+\tstrbuf_detach(&sb, NULL);\n +-\t\tif (r[i] == '\\n')\n +-\t\t\tr[i] = ' ';\n ++\tfor (int i = 0; i < len; i++) {\n ++\t\tif (buf[i] == '\\r' && i + 1 < len && buf[i + 1] == '\\n')\n ++\t\t\tcontinue; /* ignore CR in CRLF */\n \n - \treturn r;\n +-\treturn r;\n ++\t\tif (buf[i] == '\\n')\n ++\t\t\tstrbuf_addch(&sb, ' ');\n ++\t\telse\n ++\t\t\tstrbuf_addch(&sb, buf[i]);\n ++\t}\n ++\treturn strbuf_detach(&sb, NULL);\n }\n + \n + static void grab_date(const char *buf, struct atom_value *v, const char *atomname)\n @@ ref-filter.c: static void find_subpos(const char *buf,\n - \t\teol = strchrnul(buf, '\\n');\n - \t\tif (*eol)\n - \t\t\teol++;\n -+\t\t/* protect against messages that might contain \\r\\n */\n -+\t\tif (*eol == '\\r')\n -+\t\t\teol++;\n - \t\tbuf = eol;\n - \t}\n + \n + \t/* subject is first non-empty line */\n + \t*sub = buf;\n +-\t/* subject goes to first empty line */\n +-\twhile (buf < *sig && *buf && *buf != '\\n') {\n +-\t\teol = strchrnul(buf, '\\n');\n +-\t\tif (*eol)\n +-\t\t\teol++;\n +-\t\tbuf = eol;\n +-\t}\n ++\t/* subject goes to first empty line before signature begins */\n ++\tif ((eol = strstr(*sub, \"\\n\\n\"))) {\n ++\t\teol = eol < *sig ? eol : *sig;\n ++\t/* check if message uses CRLF */\n ++\t} else if (! (eol = strstr(*sub, \"\\r\\n\\r\\n\"))) {\n ++\t\t/* treat whole message as subject */\n ++\t\teol = strrchr(*sub, '\\0');\n ++\t}\n ++\tbuf = eol;\n \t*sublen = buf - *sub;\n \t/* drop trailing newline, if present */\n - \tif (*sublen && (*sub)[*sublen - 1] == '\\n')\n +-\tif (*sublen && (*sub)[*sublen - 1] == '\\n')\n ++\twhile (*sublen && ((*sub)[*sublen - 1] == '\\n' || (*sub)[*sublen - 1] == '\\r'))\n \t\t*sublen -= 1;\n -+\t/* protect against commit messages that might contain \\r\\n */\n -+\telse if (*sublen && (*sub)[*sublen - 1] == '\\r')\n -+\t\t*sublen -= 3; /* drop '\\r\\n\\r' */\n \n \t/* skip any empty lines */\n -\twhile (*buf == '\\n')\n -+\t/* and protect against commit messages that might contain \\r\\n */\n +\twhile (*buf == '\\n' || *buf == '\\r')\n \t\tbuf++;\n \t*body = buf;\n @@ t/t3203-branch-output.sh: test_expect_success 'git branch --ignore-case --list -\n +\n +test_expect_success 'git branch -v works with CRLF input' '\n +\tcat >expect <<-EOF &&\n -+\t branch-one 139b20d two\n -+\t branch-two d79ce16 one\n -+\t crlf 2113b0e Subject first line\n -+\t crlf-empty-lines-after-subject 0a9530d Subject first line\n -+\t crlf-two-line-subject f9ded1f Subject first line Subject second line\n -+\t* master 139b20d two\n ++\t two\n ++\t one\n ++\t Subject first line\n ++\t Subject first line\n ++\t Subject first line Subject second line\n ++\t Subject first line Subject second line\n ++\t Subject first line Subject second line\n ++\t Subject first line Subject second line\n +\tEOF\n -+\tgit branch -v >actual &&\n ++\tgit branch -v >tmp &&\n ++\t# Remove first two columns, and the line for the currently checked out branch\n ++\tcurrent=$(git branch --show-current) &&\n ++\tgrep -v $current actual &&\n +\ttest_cmp expect actual\n +'\n +\n @@ t/t6300-for-each-ref.sh: test_description='for-each-ref test'\n \n # Mon Jul 3 23:18:43 2006 +0000\n datestamp=1151968723\n -@@ t/t6300-for-each-ref.sh: test_expect_success 'for-each-ref --ignore-case ignores case' '\n +@@ t/t6300-for-each-ref.sh: test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' '\n \ttest_cmp expect actual\n '\n \n 3: c84092e457 ! 3: 59957d1391 log: add tests for messages containing CRLF to t4202\n @@ Metadata\n Author: Philippe Blain \n \n ## Commit message ##\n - log: add tests for messages containing CRLF to t4202\n + log, show: add tests for messages containing CRLF\n \n - A previous commit fixed a bug in the ref-filter API causing messages\n + A previous commit fixed a bug in ref-filter.c causing messages\n containing CRLF to be incorrectly parsed and displayed.\n \n - Add a test to also check that `git log` correctly handles such messages,\n - to prevent futur regressions.\n + Add tests to also check that `git log` and `git show` correctly handle\n + such messages, to prevent futur regressions if these commands are\n + refactored to use the ref-filter API.\n +\n + To prevent having to modify expected output in further tests, use\n + 'test_cleanup_crlf_refs' in t4202 to clean-up after the added tests.\n \n Signed-off-by: Philippe Blain \n \n @@ t/t4202-log.sh: test_expect_success 'oneline' '\n +test_create_crlf_refs\n +\n +test_expect_success 'oneline with CRLF messages' '\n -+\techo \"002093e Subject first line\" >expect &&\n -+\tgit log --oneline -1 crlf >actual-branch &&\n -+\tgit log --oneline -1 tag-crlf >actual-tag &&\n -+\ttest_cmp expect actual-branch &&\n -+\ttest_cmp expect actual-tag &&\n -+\techo \"6f814b0 Subject first line\" >expect &&\n -+\tgit log --oneline -1 crlf-empty-lines-after-subject >actual-branch &&\n -+\tgit log --oneline -1 tag-crlf-empty-lines-after-subject >actual-tag &&\n -+\ttest_cmp expect actual-branch &&\n -+\ttest_cmp expect actual-tag &&\n -+\techo \"8c58a85 Subject first line Subject second line\" >expect &&\n -+\tgit log --oneline -1 crlf-two-line-subject >actual-branch &&\n -+\tgit log --oneline -1 tag-crlf-two-line-subject >actual-tag &&\n -+\ttest_cmp expect actual-branch &&\n -+\ttest_cmp expect actual-tag\n ++\tfor branch in $LIB_CLRF_BRANCHES; do\n ++\t\tcat .crlf-subject-${branch}.txt >expect &&\n ++\t\tgit log --oneline -1 ${branch} >tmp-branch &&\n ++\t\tgit log --oneline -1 tag-${branch} >tmp-tag &&\n ++\t\tawk \"{print \\$NF}\" actual-branch &&\n ++\t\tawk \"{print \\$NF}\" actual-tag &&\n ++\t\ttest_cmp expect actual-branch &&\n ++\t\ttest_cmp expect actual-tag\n ++\tdone\n +'\n +test_crlf_subject_body_and_contents log --all --reverse --grep Subject\n +\n @@ t/t4202-log.sh: test_expect_success 'oneline' '\n test_expect_success 'diff-filter=A' '\n \n \tgit log --no-renames --pretty=\"format:%s\" --diff-filter=A HEAD > actual &&\n +\n + ## t/t7007-show.sh ##\n +@@\n + test_description='git show'\n + \n + . ./test-lib.sh\n ++. \"$TEST_DIRECTORY/lib-crlf-messages.sh\"\n + \n + test_expect_success setup '\n + \techo hello world >foo &&\n +@@ t/t7007-show.sh: test_expect_success 'show --graph is forbidden' '\n + test_must_fail git show --graph HEAD\n + '\n + \n ++test_create_crlf_refs\n ++\n ++test_crlf_subject_body_and_contents show $LIB_CRLF_BRANCHES\n ++\n + test_done\n\n-- \ngitgitgadget\n"},{"id":"407359","messageId":"f17d182c3bf5e758490441801423cdb0da17060d.1602526169.git.gitgitgadget@gmail.com","threadId":"52956","inReplyTo":"pull.576.v3.git.1602526169.gitgitgadget@gmail.com","subject":"[PATCH v3 1/3] t: add lib-crlf-messages.sh for messages containing CRLF","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-12T18:09:27Z","receivedAt":"2020-10-12T18:09:40Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nA following commit will fix a bug in the ref-filter API that causes\ncommit and tag messages containing CRLF to be incorrectly parsed and\ndisplayed.\n\nAdd a test library (t/lib-crlf-messages.sh) that creates refs with such\ncommit messages, so that we can easily test that this bug does not\nappear in other commands in the future.\n\nThe function `test_crlf_subject_body_and_contents` can be used to test\nthat the `--format` option of `branch`, `tag`, `for-each-ref` and\n`log` correctly displays the subject, body and raw content of commits and\ntag messages.\n\nThe commits are created using `commit-tree` such that the current branch\nin the test repository is not affected when `test_create_crlf_refs` is\ncalled in a test. This is done so that the CRLF tests can be inserted\nanywhere in a test script, where it makes sense to do so, without having\nto potentially modify further tests that depend on output that would be\nmodified if the current branch gained new commits.\n\nSigned-off-by: Philippe Blain \n---\n t/lib-crlf-messages.sh | 90 ++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 90 insertions(+)\n create mode 100644 t/lib-crlf-messages.sh\n\ndiff --git a/t/lib-crlf-messages.sh b/t/lib-crlf-messages.sh\nnew file mode 100644\nindex 0000000000..10a2b57280\n--- /dev/null\n+++ b/t/lib-crlf-messages.sh\n@@ -0,0 +1,90 @@\n+# Setup refs with commit and tag messages containing CRLF\n+\n+LIB_CRLF_BRANCHES=\"\"\n+\n+create_crlf_ref () {\n+\tmessage=\"$1\" &&\n+\tsubject=\"$2\" &&\n+\tbody=\"$3\" &&\n+\tbranch=\"$4\" &&\n+\tprintf \"${message}\" >.crlf-message-${branch}.txt &&\n+\tprintf \"${subject}\" >.crlf-subject-${branch}.txt &&\n+\tprintf \"${body}\" >.crlf-body-${branch}.txt &&\n+\tLIB_CRLF_BRANCHES=\"${LIB_CRLF_BRANCHES} ${branch}\"\n+\ttest_tick &&\n+\thash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&\n+\tgit branch ${branch} ${hash} &&\n+\tgit tag tag-${branch} ${branch} -F .crlf-message-${branch}.txt --cleanup=verbatim\n+}\n+\n+create_crlf_refs () {\n+\tmessage=\"Subject first line\\r\\n\\r\\nBody first line\\r\\nBody second line\\r\\n\" &&\n+\tbody=\"Body first line\\r\\nBody second line\\r\\n\" &&\n+\tsubject=\"Subject first line\" &&\n+\tbranch=\"crlf\" &&\n+\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\" &&\n+\tmessage=\"Subject first line\\r\\n\\r\\n\\r\\nBody first line\\r\\nBody second line\\r\\n\" &&\n+\tbranch=\"crlf-empty-lines-after-subject\" &&\n+\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\" &&\n+\tmessage=\"Subject first line\\r\\nSubject second line\\r\\n\\r\\nBody first line\\r\\nBody second line\\r\\n\" &&\n+\tsubject=\"Subject first line Subject second line\" &&\n+\tbranch=\"crlf-two-line-subject\" &&\n+\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\" &&\n+\tmessage=\"Subject first line\\r\\nSubject second line\" &&\n+\tsubject=\"Subject first line Subject second line\" &&\n+\tbody=\"\" &&\n+\tbranch=\"crlf-two-line-subject-no-body\" &&\n+\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\" &&\n+\tmessage=\"Subject first line\\r\\nSubject second line\\r\\n\" &&\n+\tbranch=\"crlf-two-line-subject-no-body-trailing-newline\" &&\n+\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\" &&\n+\tmessage=\"Subject first line\\r\\nSubject second line\\r\\n\\r\" &&\n+\tbranch=\"crlf-two-line-subject-no-body-trailing-newline2\" &&\n+\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\"\n+}\n+\n+test_create_crlf_refs () {\n+\ttest_expect_success 'setup refs with CRLF commit messages' '\n+\t\tcreate_crlf_refs\n+\t'\n+}\n+\n+cleanup_crlf_refs () {\n+\tfor branch in ${LIB_CRLF_BRANCHES}; do\n+\t\tgit branch -D ${branch} &&\n+\t\tgit tag -d tag-${branch} &&\n+\t\trm .crlf-message-${branch}.txt &&\n+\t\trm .crlf-subject-${branch}.txt &&\n+\t\trm .crlf-body-${branch}.txt\n+\tdone\n+}\n+\n+test_cleanup_crlf_refs () {\n+\ttest_expect_success 'cleanup refs with CRLF commit messages' '\n+\t\tcleanup_crlf_refs\n+\t'\n+}\n+\n+test_crlf_subject_body_and_contents() {\n+\tcommand_and_args=\"$@\" &&\n+\tcommand=$1 &&\n+\tif [ ${command} = \"branch\" ] || [ ${command} = \"for-each-ref\" ] || [ ${command} = \"tag\" ]; then\n+\t\tatoms=\"(contents:subject) (contents:body) (contents)\"\n+\telif [ ${command} = \"log\" ] || [ ${command} = \"show\" ]; then\n+\t\tatoms=\"s b B\"\n+\tfi &&\n+\tfiles=\"subject body message\" &&\n+\twhile [ -n \"${atoms}\" ]; do\n+\t\tset ${atoms} && atom=$1 && shift && atoms=\"$*\" &&\n+\t\tset ${files} &&\tfile=$1 && shift && files=\"$*\" &&\n+\t\ttest_expect_success \"${command}: --format='%${atom}' works with CRLF input\" \"\n+\t\t\trm -f expect &&\n+\t\t\tfor ref in ${LIB_CRLF_BRANCHES}; do\n+\t\t\t\tcat .crlf-${file}-\\\"\\${ref}\\\".txt >>expect &&\n+\t\t\t\tprintf \\\"\\n\\\" >>expect\n+\t\t\tdone &&\n+\t\t\tgit $command_and_args --format=\\\"%${atom}\\\" >actual &&\n+\t\t\ttest_cmp expect actual\n+\t\t\"\n+\tdone\n+}\n-- \ngitgitgadget\n\n"},{"id":"407358","messageId":"59957d139135bc33dd1de97a60d02890dbfce7d5.1602526169.git.gitgitgadget@gmail.com","threadId":"52956","inReplyTo":"pull.576.v3.git.1602526169.gitgitgadget@gmail.com","subject":"[PATCH v3 3/3] log, show: add tests for messages containing CRLF","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-12T18:09:29Z","receivedAt":"2020-10-12T18:09:41Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nA previous commit fixed a bug in ref-filter.c causing messages\ncontaining CRLF to be incorrectly parsed and displayed.\n\nAdd tests to also check that `git log` and `git show` correctly handle\nsuch messages, to prevent futur regressions if these commands are\nrefactored to use the ref-filter API.\n\nTo prevent having to modify expected output in further tests, use\n'test_cleanup_crlf_refs' in t4202 to clean-up after the added tests.\n\nSigned-off-by: Philippe Blain \n---\n t/t4202-log.sh | 18 ++++++++++++++++++\n t/t7007-show.sh | 5 +++++\n 2 files changed, 23 insertions(+)\n\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 56d34ed465..d4942a6f92 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -6,6 +6,7 @@ test_description='git log'\n . \"$TEST_DIRECTORY/lib-gpg.sh\"\n . \"$TEST_DIRECTORY/lib-terminal.sh\"\n . \"$TEST_DIRECTORY/lib-log-graph.sh\"\n+. \"$TEST_DIRECTORY/lib-crlf-messages.sh\"\n \n test_cmp_graph () {\n \tlib_test_cmp_graph --format=%s \"$@\"\n@@ -105,6 +106,23 @@ test_expect_success 'oneline' '\n \ttest_cmp expect actual\n '\n \n+test_create_crlf_refs\n+\n+test_expect_success 'oneline with CRLF messages' '\n+\tfor branch in $LIB_CLRF_BRANCHES; do\n+\t\tcat .crlf-subject-${branch}.txt >expect &&\n+\t\tgit log --oneline -1 ${branch} >tmp-branch &&\n+\t\tgit log --oneline -1 tag-${branch} >tmp-tag &&\n+\t\tawk \"{print \\$NF}\" actual-branch &&\n+\t\tawk \"{print \\$NF}\" actual-tag &&\n+\t\ttest_cmp expect actual-branch &&\n+\t\ttest_cmp expect actual-tag\n+\tdone\n+'\n+test_crlf_subject_body_and_contents log --all --reverse --grep Subject\n+\n+test_cleanup_crlf_refs\n+\n test_expect_success 'diff-filter=A' '\n \n \tgit log --no-renames --pretty=\"format:%s\" --diff-filter=A HEAD > actual &&\ndiff --git a/t/t7007-show.sh b/t/t7007-show.sh\nindex 42d3db6246..5ffe852829 100755\n--- a/t/t7007-show.sh\n+++ b/t/t7007-show.sh\n@@ -3,6 +3,7 @@\n test_description='git show'\n \n . ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-crlf-messages.sh\"\n \n test_expect_success setup '\n \techo hello world >foo &&\n@@ -128,4 +129,8 @@ test_expect_success 'show --graph is forbidden' '\n test_must_fail git show --graph HEAD\n '\n \n+test_create_crlf_refs\n+\n+test_crlf_subject_body_and_contents show $LIB_CRLF_BRANCHES\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"407398","messageId":"xmqqlfgbm6el.fsf@gitster.c.googlers.com","threadId":"52956","inReplyTo":"f17d182c3bf5e758490441801423cdb0da17060d.1602526169.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/3] t: add lib-crlf-messages.sh for messages containing CRLF","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-12T22:22:10Z","receivedAt":"2020-10-12T22:22:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philippe Blain via GitGitGadget\" writes:\n\n> From: Philippe Blain \n>\n> A following commit will fix a bug in the ref-filter API that causes\n> commit and tag messages containing CRLF to be incorrectly parsed and\n> displayed.\n>\n> Add a test library (t/lib-crlf-messages.sh) that creates refs with such\n> commit messages, so that we can easily test that this bug does not\n> appear in other commands in the future.\n> ...\n> The function `test_crlf_subject_body_and_contents` can be used to test\n> that the `--format` option of `branch`, `tag`, `for-each-ref` and\n> `log` correctly displays the subject, body and raw content of commits and\n> tag messages.\n\nI am not sure about the wisdom of this arrangement. Surely you do\nnot want to write duplicated set-up for (existing) test scripts for\nfor-each-ref, branch and tag subcommands, assuming that these test\nscripts are separated for subcommands they test.\n\nBut you can have a single test script, that is differentiated from\nall other test scripts by what it tests: having to deal with commits\nthat use CRLF. Then we do not have to add dot-includable test\nlibrary that lets various tests to create these same funny commits.\nInstead, we can just do these as normal set-up step(s) for that\nsingle test scripts, and then in that test scripts, verify what is\nshown by various commands that share the underlying ref-filter\nmachinery. No?\n\n\n> diff --git a/t/lib-crlf-messages.sh b/t/lib-crlf-messages.sh\n> new file mode 100644\n> index 0000000000..10a2b57280\n> --- /dev/null\n> +++ b/t/lib-crlf-messages.sh\n> @@ -0,0 +1,90 @@\n> +# Setup refs with commit and tag messages containing CRLF\n> +\n> +LIB_CRLF_BRANCHES=\"\"\n> +\n> +create_crlf_ref () {\n> +\tmessage=\"$1\" &&\n> +\tsubject=\"$2\" &&\n> +\tbody=\"$3\" &&\n> +\tbranch=\"$4\" &&\n> +\tprintf \"${message}\" >.crlf-message-${branch}.txt &&\n> +\tprintf \"${subject}\" >.crlf-subject-${branch}.txt &&\n> +\tprintf \"${body}\" >.crlf-body-${branch}.txt &&\n> +\tLIB_CRLF_BRANCHES=\"${LIB_CRLF_BRANCHES} ${branch}\"\n> +\ttest_tick &&\n> +\thash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&\n> +\tgit branch ${branch} ${hash} &&\n> +\tgit tag tag-${branch} ${branch} -F .crlf-message-${branch}.txt --cleanup=verbatim\n> +}\n> +\n> +create_crlf_refs () {\n> +\tmessage=\"Subject first line\\r\\n\\r\\nBody first line\\r\\nBody second line\\r\\n\" &&\n> +\tbody=\"Body first line\\r\\nBody second line\\r\\n\" &&\n> +\tsubject=\"Subject first line\" &&\n> +\tbranch=\"crlf\" &&\n> +\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\" &&\n> +\tmessage=\"Subject first line\\r\\n\\r\\n\\r\\nBody first line\\r\\nBody second line\\r\\n\" &&\n> +\tbranch=\"crlf-empty-lines-after-subject\" &&\n> +\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\" &&\n> +\tmessage=\"Subject first line\\r\\nSubject second line\\r\\n\\r\\nBody first line\\r\\nBody second line\\r\\n\" &&\n> +\tsubject=\"Subject first line Subject second line\" &&\n> +\tbranch=\"crlf-two-line-subject\" &&\n> +\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\" &&\n> +\tmessage=\"Subject first line\\r\\nSubject second line\" &&\n> +\tsubject=\"Subject first line Subject second line\" &&\n> +\tbody=\"\" &&\n> +\tbranch=\"crlf-two-line-subject-no-body\" &&\n> +\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\" &&\n> +\tmessage=\"Subject first line\\r\\nSubject second line\\r\\n\" &&\n> +\tbranch=\"crlf-two-line-subject-no-body-trailing-newline\" &&\n> +\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\" &&\n> +\tmessage=\"Subject first line\\r\\nSubject second line\\r\\n\\r\" &&\n> +\tbranch=\"crlf-two-line-subject-no-body-trailing-newline2\" &&\n> +\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\"\n> +}\n> +\n> +test_create_crlf_refs () {\n> +\ttest_expect_success 'setup refs with CRLF commit messages' '\n> +\t\tcreate_crlf_refs\n> +\t'\n> +}\n> +\n> +cleanup_crlf_refs () {\n> +\tfor branch in ${LIB_CRLF_BRANCHES}; do\n> +\t\tgit branch -D ${branch} &&\n> +\t\tgit tag -d tag-${branch} &&\n> +\t\trm .crlf-message-${branch}.txt &&\n> +\t\trm .crlf-subject-${branch}.txt &&\n> +\t\trm .crlf-body-${branch}.txt\n> +\tdone\n> +}\n> +\n> +test_cleanup_crlf_refs () {\n> +\ttest_expect_success 'cleanup refs with CRLF commit messages' '\n> +\t\tcleanup_crlf_refs\n> +\t'\n> +}\n> +\n> +test_crlf_subject_body_and_contents() {\n\nIt does not excempt a script from being subject to the coding\nguidelines to be a test library.\n\n> +\tcommand_and_args=\"$@\" &&\n> +\tcommand=$1 &&\n> +\tif [ ${command} = \"branch\" ] || [ ${command} = \"for-each-ref\" ] || [ ${command} = \"tag\" ]; then\n> +\t\tatoms=\"(contents:subject) (contents:body) (contents)\"\n> +\telif [ ${command} = \"log\" ] || [ ${command} = \"show\" ]; then\n> +\t\tatoms=\"s b B\"\n> +\tfi &&\n\nThis is the part that made me react to the organization. Even\nthough this helper \"library\" pretends to be generic, it needs to\nactually know exactly what subcommands are going to be tested with\nthe helper. It is probably easier to read and understand if these\nhelper functions are defined in the same script as the one that\ntests these various commands but for one specific aspect of these\ncommands (i.e. how the log message with funny line ending convention\nare split into subject and body).\n\n> +\tfiles=\"subject body message\" &&\n> +\twhile [ -n \"${atoms}\" ]; do\n> +\t\tset ${atoms} && atom=$1 && shift && atoms=\"$*\" &&\n> +\t\tset ${files} &&\tfile=$1 && shift && files=\"$*\" &&\n> +\t\ttest_expect_success \"${command}: --format='%${atom}' works with CRLF input\" \"\n> +\t\t\trm -f expect &&\n> +\t\t\tfor ref in ${LIB_CRLF_BRANCHES}; do\n> +\t\t\t\tcat .crlf-${file}-\\\"\\${ref}\\\".txt >>expect &&\n> +\t\t\t\tprintf \\\"\\n\\\" >>expect\n> +\t\t\tdone &&\n> +\t\t\tgit $command_and_args --format=\\\"%${atom}\\\" >actual &&\n> +\t\t\ttest_cmp expect actual\n> +\t\t\"\n> +\tdone\n> +}\n"},{"id":"407399","messageId":"xmqqh7qzm6ai.fsf@gitster.c.googlers.com","threadId":"52956","inReplyTo":"11d044a4f7feccdf20da6364a1f9bbe934e9981f.1602526169.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/3] ref-filter: handle CRLF at end-of-line more gracefully","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-12T22:24:37Z","receivedAt":"2020-10-12T22:24:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philippe Blain via GitGitGadget\" writes:\n\n> -\tfor (i = 0; i < len; i++)\n> -\t\tif (r[i] == '\\n')\n> -\t\t\tr[i] = ' ';\n> +\tfor (int i = 0; i < len; i++) {\n\nWe do not allow this in our codebase (yet).\n\ncf. Documentation/CodingGuidelines\n\n - Declaring a variable in the for loop \"for (int i = 0; i < 10; i++)\"\n is still not allowed in this codebase.\n\n> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\n> index 71818b90f0..c06eca774f 100755\n> --- a/t/t3203-branch-output.sh\n> +++ b/t/t3203-branch-output.sh\n> @@ -3,13 +3,11 @@\n> test_description='git branch display tests'\n> . ./test-lib.sh\n> . \"$TEST_DIRECTORY\"/lib-terminal.sh\n> +. \"$TEST_DIRECTORY\"/lib-crlf-messages.sh\n> \n> test_expect_success 'make commits' '\n> -\techo content >file &&\n> -\tgit add file &&\n> -\tgit commit -m one &&\n> -\techo content >>file &&\n> -\tgit commit -a -m two\n> +\ttest_commit one &&\n> +\ttest_commit two\n> '\n\nWhat does this change have to do with the topic at hand?\n"},{"id":"407401","messageId":"20201012224706.GA4318@flurp.local","threadId":"52956","inReplyTo":"f17d182c3bf5e758490441801423cdb0da17060d.1602526169.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/3] t: add lib-crlf-messages.sh for messages containing CRLF","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-10-12T22:47:06Z","receivedAt":"2020-10-12T22:47:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Oct 12, 2020 at 06:09:27PM +0000, Philippe Blain via GitGitGadget wrote:\n> Add a test library (t/lib-crlf-messages.sh) that creates refs with such\n> commit messages, so that we can easily test that this bug does not\n> appear in other commands in the future.\n\nIn addition to Junio's review comments...\n\n> \n> Signed-off-by: Philippe Blain \n> ---\n> diff --git a/t/lib-crlf-messages.sh b/t/lib-crlf-messages.sh\n> @@ -0,0 +1,90 @@\n> +create_crlf_ref () {\n> +\tmessage=\"$1\" &&\n> +\tsubject=\"$2\" &&\n> +\tbody=\"$3\" &&\n> +\tbranch=\"$4\" &&\n> +\tprintf \"${message}\" >.crlf-message-${branch}.txt &&\n> +\tprintf \"${subject}\" >.crlf-subject-${branch}.txt &&\n> +\tprintf \"${body}\" >.crlf-body-${branch}.txt &&\n> +\tLIB_CRLF_BRANCHES=\"${LIB_CRLF_BRANCHES} ${branch}\"\n\nBroken &&-chain.\n\n> +\ttest_tick &&\n> +\thash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&\n> +\tgit branch ${branch} ${hash} &&\n> +\tgit tag tag-${branch} ${branch} -F .crlf-message-${branch}.txt --cleanup=verbatim\n> +}\n> +\n> +create_crlf_refs () {\n> +\tmessage=\"Subject first line\\r\\n\\r\\nBody first line\\r\\nBody second line\\r\\n\" &&\n> +\tbody=\"Body first line\\r\\nBody second line\\r\\n\" &&\n> +\tsubject=\"Subject first line\" &&\n> +\tbranch=\"crlf\" &&\n> +\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\" &&\n\nThis is somewhat onerous to digest and compose. Have you considered\nmaking it more automated and easier to read? Perhaps something like\nthis:\n\n create_crlf_ref () {\n branch=$1\n cat >.crlf-message-$branch.txt &&\n sed -n \"1,/^$/p\" <.crlf-message-$branch.txt | sed \"/^$/d\" | append_cr >.crlf-subject-$branch.txt &&\n sed -n \"/^$/,\\$p\" <.crlf-message-$branch.txt | sed \"1d\" | append_cr >.crlf-body-$branch.txt &&\n ...\n }\n\n create_crlf_refs () {\n create_crlf_ref crlf <<-\\EOF\n Subject first line\n\n Body first line\n Body second line\n EOF\n ...\n }\n\n> +test_create_crlf_refs () {\n> +\ttest_expect_success 'setup refs with CRLF commit messages' '\n> +\t\tcreate_crlf_refs\n> +\t'\n> +}\n\nThis almost seems like an unnecessary indirection since callers could\njust as easily do this on their own, like this:\n\n test_expect_success 'setup refs with CRLF commit messages' '\n create_crlf_refs\n '\n\nwhich isn't very burdensome. However, I suppose doing it this way\ngives consistent test titles between scripts, so not necessarily a\nstrong objection on my part.\n\n> +cleanup_crlf_refs () {\n> +\tfor branch in ${LIB_CRLF_BRANCHES}; do\n\nOur style is to place 'do' on its own line:\n\n for branch in $LIB_CRLF_BRANCHES\n do\n ...\n\nThis would be a syntax error if LIB_CRLF_BRANCHES is empty for some\nreason, but I suppose we don't really have to worry about it here(?).\n\n> +\t\tgit branch -D ${branch} &&\n> +\t\tgit tag -d tag-${branch} &&\n> +\t\trm .crlf-message-${branch}.txt &&\n> +\t\trm .crlf-subject-${branch}.txt &&\n> +\t\trm .crlf-body-${branch}.txt\n> +\tdone\n> +}\n> +\n> +test_cleanup_crlf_refs () {\n> +\ttest_expect_success 'cleanup refs with CRLF commit messages' '\n> +\t\tcleanup_crlf_refs\n> +\t'\n> +}\n> +\n> +test_crlf_subject_body_and_contents() {\n> +\tcommand_and_args=\"$@\" &&\n> +\tcommand=$1 &&\n> +\tif [ ${command} = \"branch\" ] || [ ${command} = \"for-each-ref\" ] || [ ${command} = \"tag\" ]; then\n> +\t\tatoms=\"(contents:subject) (contents:body) (contents)\"\n> +\telif [ ${command} = \"log\" ] || [ ${command} = \"show\" ]; then\n> +\t\tatoms=\"s b B\"\n> +\tfi &&\n\nStyle:\n\n if test \"$command\" = \"branch\" || test ...\n then\n ...\n elif test ...\n then\n ...\n fi &&\n\n> +\tfiles=\"subject body message\" &&\n> +\twhile [ -n \"${atoms}\" ]; do\n\nToo many spaces after 'while'.\n\nStyle:\n\n while tests -n \"...\"\n do\n ...\n\n> +\t\tset ${atoms} && atom=$1 && shift && atoms=\"$*\" &&\n> +\t\tset ${files} &&\tfile=$1 && shift && files=\"$*\" &&\n> +\t\ttest_expect_success \"${command}: --format='%${atom}' works with CRLF input\" \"\n> +\t\t\trm -f expect &&\n> +\t\t\tfor ref in ${LIB_CRLF_BRANCHES}; do\n\nStyle.\n\n> +\t\t\t\tcat .crlf-${file}-\\\"\\${ref}\\\".txt >>expect &&\n> +\t\t\t\tprintf \\\"\\n\\\" >>expect\n> +\t\t\tdone &&\n> +\t\t\tgit $command_and_args --format=\\\"%${atom}\\\" >actual &&\n> +\t\t\ttest_cmp expect actual\n> +\t\t\"\n> +\tdone\n> +}\n"},{"id":"407503","messageId":"1902E2D2-5265-48BC-877F-21B6C2D201B1@gmail.com","threadId":"52956","inReplyTo":"xmqqh7qzm6ai.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 2/3] ref-filter: handle CRLF at end-of-line more gracefully","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-10-14T13:09:59Z","receivedAt":"2020-10-14T13:10:08Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Junio,\n\n> Le 12 oct. 2020 à 18:24, Junio C Hamano a écrit :\n> \n> \"Philippe Blain via GitGitGadget\" writes:\n> \n>> -\tfor (i = 0; i < len; i++)\n>> -\t\tif (r[i] == '\\n')\n>> -\t\t\tr[i] = ' ';\n>> +\tfor (int i = 0; i < len; i++) {\n> \n> We do not allow this in our codebase (yet).\n> \n> cf. Documentation/CodingGuidelines\n> \n> - Declaring a variable in the for loop \"for (int i = 0; i < 10; i++)\"\n> is still not allowed in this codebase.\n\nIndeed. Will fix (and re-read the guidelines).\n\n> \n>> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\n>> index 71818b90f0..c06eca774f 100755\n>> --- a/t/t3203-branch-output.sh\n>> +++ b/t/t3203-branch-output.sh\n>> @@ -3,13 +3,11 @@\n>> test_description='git branch display tests'\n>> . ./test-lib.sh\n>> . \"$TEST_DIRECTORY\"/lib-terminal.sh\n>> +. \"$TEST_DIRECTORY\"/lib-crlf-messages.sh\n>> \n>> test_expect_success 'make commits' '\n>> -\techo content >file &&\n>> -\tgit add file &&\n>> -\tgit commit -m one &&\n>> -\techo content >>file &&\n>> -\tgit commit -a -m two\n>> +\ttest_commit one &&\n>> +\ttest_commit two\n>> '\n> \n> What does this change have to do with the topic at hand?\n\nIn the previous iteration, the expected output of one of the tests\nI was adding had commit hashes, \nso the change above was necessary to make those hashes reproducible.\nHowever in this series I removed the hashes from the expected output\nbecause it would break the \"linux-clang\" CI job which runs with SHA-256.\n\nMaybe we should add a note in t/README specifically saying that raw commit\nhashes should not appear in expected output, but use $(git rev-parse \"${ref}\")\ninstead ? Could this be a preparatory patch, or would deserve a separate topic ?\n\n"},{"id":"407504","messageId":"CC87B74C-5EC2-4129-82A5-2CD0C81188B9@gmail.com","threadId":"52956","inReplyTo":"20201012224706.GA4318@flurp.local","subject":"Re: [PATCH v3 1/3] t: add lib-crlf-messages.sh for messages containing CRLF","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-10-14T13:20:02Z","receivedAt":"2020-10-14T13:20:09Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Eric,\n\n> Le 12 oct. 2020 à 18:47, Eric Sunshine a écrit :\n> \n> On Mon, Oct 12, 2020 at 06:09:27PM +0000, Philippe Blain via GitGitGadget wrote:\n>> Add a test library (t/lib-crlf-messages.sh) that creates refs with such\n>> commit messages, so that we can easily test that this bug does not\n>> appear in other commands in the future.\n> \n> In addition to Junio's review comments...\n> \n>> \n>> Signed-off-by: Philippe Blain \n>> ---\n>> diff --git a/t/lib-crlf-messages.sh b/t/lib-crlf-messages.sh\n>> @@ -0,0 +1,90 @@\n>> +create_crlf_ref () {\n>> +\tmessage=\"$1\" &&\n>> +\tsubject=\"$2\" &&\n>> +\tbody=\"$3\" &&\n>> +\tbranch=\"$4\" &&\n>> +\tprintf \"${message}\" >.crlf-message-${branch}.txt &&\n>> +\tprintf \"${subject}\" >.crlf-subject-${branch}.txt &&\n>> +\tprintf \"${body}\" >.crlf-body-${branch}.txt &&\n>> +\tLIB_CRLF_BRANCHES=\"${LIB_CRLF_BRANCHES} ${branch}\"\n> \n> Broken &&-chain.\n> \n>> +\ttest_tick &&\n>> +\thash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&\n>> +\tgit branch ${branch} ${hash} &&\n>> +\tgit tag tag-${branch} ${branch} -F .crlf-message-${branch}.txt --cleanup=verbatim\n>> +}\n>> +\n>> +create_crlf_refs () {\n>> +\tmessage=\"Subject first line\\r\\n\\r\\nBody first line\\r\\nBody second line\\r\\n\" &&\n>> +\tbody=\"Body first line\\r\\nBody second line\\r\\n\" &&\n>> +\tsubject=\"Subject first line\" &&\n>> +\tbranch=\"crlf\" &&\n>> +\tcreate_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\" &&\n> \n> This is somewhat onerous to digest and compose. Have you considered\n> making it more automated and easier to read? Perhaps something like\n> this:\n> \n> create_crlf_ref () {\n> branch=$1\n> cat >.crlf-message-$branch.txt &&\n> sed -n \"1,/^$/p\" <.crlf-message-$branch.txt | sed \"/^$/d\" | append_cr >.crlf-subject-$branch.txt &&\n> sed -n \"/^$/,\\$p\" <.crlf-message-$branch.txt | sed \"1d\" | append_cr >.crlf-body-$branch.txt &&\n> ...\n> }\n> \n> create_crlf_refs () {\n> create_crlf_ref crlf <<-\\EOF\n> Subject first line\n> \n> Body first line\n> Body second line\n> EOF\n> ...\n> }\n\nI did not try to do that because I did not think of it. \nHowever, I think it's clearer using printf, this way '\\n' and '\\r'\nappear clearly on all platforms, whatever editor is in use\nand whatever settings this editor is using to hide or not hide\ncontrol characters.\n\n> \n>> +test_create_crlf_refs () {\n>> +\ttest_expect_success 'setup refs with CRLF commit messages' '\n>> +\t\tcreate_crlf_refs\n>> +\t'\n>> +}\n> \n> This almost seems like an unnecessary indirection since callers could\n> just as easily do this on their own, like this:\n> \n> test_expect_success 'setup refs with CRLF commit messages' '\n> create_crlf_refs\n> '\n> \n> which isn't very burdensome. However, I suppose doing it this way\n> gives consistent test titles between scripts, so not necessarily a\n> strong objection on my part.\n\nYes, that was the reason. Given Junio's comments I'll surely\nrefactor his library into a script, so there won't be a need for\nthis indirection.\n\n> \n>> +cleanup_crlf_refs () {\n>> +\tfor branch in ${LIB_CRLF_BRANCHES}; do\n> \n> Our style is to place 'do' on its own line:\n> \n> for branch in $LIB_CRLF_BRANCHES\n> do\n> ...\n> \n> This would be a syntax error if LIB_CRLF_BRANCHES is empty for some\n> reason, but I suppose we don't really have to worry about it here(?).\n> \n>> +\t\tgit branch -D ${branch} &&\n>> +\t\tgit tag -d tag-${branch} &&\n>> +\t\trm .crlf-message-${branch}.txt &&\n>> +\t\trm .crlf-subject-${branch}.txt &&\n>> +\t\trm .crlf-body-${branch}.txt\n>> +\tdone\n>> +}\n>> +\n>> +test_cleanup_crlf_refs () {\n>> +\ttest_expect_success 'cleanup refs with CRLF commit messages' '\n>> +\t\tcleanup_crlf_refs\n>> +\t'\n>> +}\n>> +\n>> +test_crlf_subject_body_and_contents() {\n>> +\tcommand_and_args=\"$@\" &&\n>> +\tcommand=$1 &&\n>> +\tif [ ${command} = \"branch\" ] || [ ${command} = \"for-each-ref\" ] || [ ${command} = \"tag\" ]; then\n>> +\t\tatoms=\"(contents:subject) (contents:body) (contents)\"\n>> +\telif [ ${command} = \"log\" ] || [ ${command} = \"show\" ]; then\n>> +\t\tatoms=\"s b B\"\n>> +\tfi &&\n> \n> Style:\n> \n> if test \"$command\" = \"branch\" || test ...\n> then\n> ...\n> elif test ...\n> then\n> ...\n> fi &&\n> \n>> +\tfiles=\"subject body message\" &&\n>> +\twhile [ -n \"${atoms}\" ]; do\n> \n> Too many spaces after 'while'.\n> \n> Style:\n> \n> while tests -n \"...\"\n> do\n> ...\n> \n>> +\t\tset ${atoms} && atom=$1 && shift && atoms=\"$*\" &&\n>> +\t\tset ${files} &&\tfile=$1 && shift && files=\"$*\" &&\n>> +\t\ttest_expect_success \"${command}: --format='%${atom}' works with CRLF input\" \"\n>> +\t\t\trm -f expect &&\n>> +\t\t\tfor ref in ${LIB_CRLF_BRANCHES}; do\n> \n> Style.\n> \n>> +\t\t\t\tcat .crlf-${file}-\\\"\\${ref}\\\".txt >>expect &&\n>> +\t\t\t\tprintf \\\"\\n\\\" >>expect\n>> +\t\t\tdone &&\n>> +\t\t\tgit $command_and_args --format=\\\"%${atom}\\\" >actual &&\n>> +\t\t\ttest_cmp expect actual\n>> +\t\t\"\n>> +\tdone\n>> +}\n\nThanks for the review! (and I'll re-read the shell coding guidelines\nbefore sending v4).\n\nCheers,\nPhilippe.\n\n"},{"id":"407505","messageId":"ACDD60BE-34B2-4E3A-881A-E82F2BA39BF9@gmail.com","threadId":"52956","inReplyTo":"xmqqlfgbm6el.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 1/3] t: add lib-crlf-messages.sh for messages containing CRLF","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-10-14T13:22:40Z","receivedAt":"2020-10-14T13:22:47Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Junio,\n\n> Le 12 oct. 2020 à 18:22, Junio C Hamano a écrit :\n> \n> \"Philippe Blain via GitGitGadget\" writes:\n> \n>> From: Philippe Blain \n>> \n>> A following commit will fix a bug in the ref-filter API that causes\n>> commit and tag messages containing CRLF to be incorrectly parsed and\n>> displayed.\n>> \n>> Add a test library (t/lib-crlf-messages.sh) that creates refs with such\n>> commit messages, so that we can easily test that this bug does not\n>> appear in other commands in the future.\n>> ...\n>> The function `test_crlf_subject_body_and_contents` can be used to test\n>> that the `--format` option of `branch`, `tag`, `for-each-ref` and\n>> `log` correctly displays the subject, body and raw content of commits and\n>> tag messages.\n> \n> I am not sure about the wisdom of this arrangement. Surely you do\n> not want to write duplicated set-up for (existing) test scripts for\n> for-each-ref, branch and tag subcommands, assuming that these test\n> scripts are separated for subcommands they test.\n> \n> But you can have a single test script, that is differentiated from\n> all other test scripts by what it tests: having to deal with commits\n> that use CRLF. Then we do not have to add dot-includable test\n> library that lets various tests to create these same funny commits.\n> Instead, we can just do these as normal set-up step(s) for that\n> single test scripts, and then in that test scripts, verify what is\n> shown by various commands that share the underlying ref-filter\n> machinery. No?\n\nYes. I was thinking that it made more sense for the tests\nto be in existing test scripts, but if you feel a separate test\nscript is warranted for these tests, I'll do that instead.\n\nThanks,\n\nPhilippe."},{"id":"407507","messageId":"CAPig+cR3gsqmyM9k1F4waBc8R5Tqj0thw2_7E8Sun-FGV8b1SQ@mail.gmail.com","threadId":"52956","inReplyTo":"CC87B74C-5EC2-4129-82A5-2CD0C81188B9@gmail.com","subject":"Re: [PATCH v3 1/3] t: add lib-crlf-messages.sh for messages containing CRLF","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-10-14T13:45:44Z","receivedAt":"2020-10-14T13:46:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Oct 14, 2020 at 9:20 AM Philippe Blain\n wrote:\n> > Le 12 oct. 2020 à 18:47, Eric Sunshine a écrit :\n> > This is somewhat onerous to digest and compose. Have you considered\n> > making it more automated and easier to read? Perhaps something like\n> > this:\n> >\n> > create_crlf_ref () {\n> > branch=$1\n> > cat >.crlf-message-$branch.txt &&\n> > sed -n \"1,/^$/p\" <.crlf-message-$branch.txt | sed \"/^$/d\" | append_cr >.crlf-subject-$branch.txt &&\n> > sed -n \"/^$/,\\$p\" <.crlf-message-$branch.txt | sed \"1d\" | append_cr >.crlf-body-$branch.txt &&\n> > ...\n> > }\n> >\n> > create_crlf_refs () {\n> > create_crlf_ref crlf <<-\\EOF\n> > Subject first line\n> >\n> > Body first line\n> > Body second line\n> > EOF\n> > ...\n> > }\n>\n> I did not try to do that because I did not think of it.\n> However, I think it's clearer using printf, this way '\\n' and '\\r'\n> appear clearly on all platforms, whatever editor is in use\n> and whatever settings this editor is using to hide or not hide\n> control characters.\n\nSorry, I'm not sure I understand what you are saying about editors and\nhiding or not hiding control characters. There are no hidden control\ncharacters in the example code I posted.\n\nThe code I proposed is very explicit about using CRLF terminators. The\nhere-doc fed to create_crlf_ref() contains only the normal LF, but\nthen create_crlf_ref() explicitly converts those to CRLF by calling\nappend_cr().\n"},{"id":"407508","messageId":"FA717D92-3798-44C0-8671-83596429C388@gmail.com","threadId":"52956","inReplyTo":"CAPig+cR3gsqmyM9k1F4waBc8R5Tqj0thw2_7E8Sun-FGV8b1SQ@mail.gmail.com","subject":"Re: [PATCH v3 1/3] t: add lib-crlf-messages.sh for messages containing CRLF","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-10-14T13:52:37Z","receivedAt":"2020-10-14T13:52:43Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"\n> Le 14 oct. 2020 à 09:45, Eric Sunshine a écrit :\n> \n> On Wed, Oct 14, 2020 at 9:20 AM Philippe Blain\n> wrote:\n>>> Le 12 oct. 2020 à 18:47, Eric Sunshine a écrit :\n>>> This is somewhat onerous to digest and compose. Have you considered\n>>> making it more automated and easier to read? Perhaps something like\n>>> this:\n>>> \n>>> create_crlf_ref () {\n>>> branch=$1\n>>> cat >.crlf-message-$branch.txt &&\n>>> sed -n \"1,/^$/p\" <.crlf-message-$branch.txt | sed \"/^$/d\" | append_cr >.crlf-subject-$branch.txt &&\n>>> sed -n \"/^$/,\\$p\" <.crlf-message-$branch.txt | sed \"1d\" | append_cr >.crlf-body-$branch.txt &&\n>>> ...\n>>> }\n>>> \n>>> create_crlf_refs () {\n>>> create_crlf_ref crlf <<-\\EOF\n>>> Subject first line\n>>> \n>>> Body first line\n>>> Body second line\n>>> EOF\n>>> ...\n>>> }\n>> \n>> I did not try to do that because I did not think of it.\n>> However, I think it's clearer using printf, this way '\\n' and '\\r'\n>> appear clearly on all platforms, whatever editor is in use\n>> and whatever settings this editor is using to hide or not hide\n>> control characters.\n> \n> Sorry, I'm not sure I understand what you are saying about editors and\n> hiding or not hiding control characters. There are no hidden control\n> characters in the example code I posted.\n> \n> The code I proposed is very explicit about using CRLF terminators. The\n> here-doc fed to create_crlf_ref() contains only the normal LF, but\n> then create_crlf_ref() explicitly converts those to CRLF by calling\n> append_cr().\n\nSorry, I missed that. I'll try to see if I can make it simpler using\nthis approach then. \n\n"},{"id":"407557","messageId":"20201014230115.GA14306@flurp.local","threadId":"52956","inReplyTo":"FA717D92-3798-44C0-8671-83596429C388@gmail.com","subject":"Re: [PATCH v3 1/3] t: add lib-crlf-messages.sh for messages containing CRLF","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-10-14T23:01:15Z","receivedAt":"2020-10-15T01:29:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Oct 14, 2020 at 9:52 AM Philippe Blain wrote:\n> > Le 14 oct. 2020 à 09:45, Eric Sunshine a écrit :\n> >>> Le 12 oct. 2020 à 18:47, Eric Sunshine a écrit :\n> >>> create_crlf_ref () {\n> >>> branch=$1\n> >>> cat >.crlf-message-$branch.txt &&\n> >>> sed -n \"1,/^$/p\" <.crlf-message-$branch.txt | sed \"/^$/d\" | append_cr >.crlf-subject-$branch.txt &&\n> >>> sed -n \"/^$/,\\$p\" <.crlf-message-$branch.txt | sed \"1d\" | append_cr >.crlf-body-$branch.txt &&\n> >\n> > The code I proposed is very explicit about using CRLF terminators. The\n> > here-doc fed to create_crlf_ref() contains only the normal LF, but\n> > then create_crlf_ref() explicitly converts those to CRLF by calling\n> > append_cr().\n>\n> Sorry, I missed that. I'll try to see if I can make it simpler using\n> this approach then.\n\nBy the way, if you also need .crlf-message-$branch.txt to have CRLF\nline endings, then you'll probably want to use a temporary file (for\ninstance, .crlf-orig-$branch.txt), perhaps like this:\n\n create_crlf_ref () {\n branch=$1 &&\n cat >.crlf-orig-$branch.txt &&\n append_cr <.crlf-orig-$branch.txt >.crlf-message-$branch.txt &&\n sed -n \"1,/^$/p\" <.crlf-orig-$branch.txt | sed \"/^$/d\" | append_cr >.crlf-subject-$branch.txt &&\n sed -n \"/^$/,\\$p\" <.crlf-orig-$branch.txt | sed \"1d\" | append_cr >.crlf-body-$branch.txt &&\n ...\n }\n"},{"id":"408138","messageId":"pull.576.v4.git.1603335680.gitgitgadget@gmail.com","threadId":"52956","inReplyTo":"pull.576.v3.git.1602526169.gitgitgadget@gmail.com","subject":"[PATCH v4 0/2] ref-filter: handle CRLF at end-of-line more gracefully","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-22T03:01:17Z","receivedAt":"2020-10-22T03:01:25Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Changes since v3 :\n\n * removed lib-crlf-messages.sh and the changes to the existing test\n scripts, and instead added a new test script : t3920-crlf-messages.sh,\n and adjusted the commit message of 1/2 (which was 2/3 in v3) accordingly.\n * tweaked the code and the test script to be in line with the C and shell\n coding guidelines, following comments by Junio and Eric\n * made the generation of the crlf-* refs and helper files in the test\n script more readable and automated, as suggested by Eric.\n\nNotes: \n\n 1. I was really unsure about which number to choose for the new script, so\n I chose one available at the end of section 3 (\"the other basic commands\n \" according to t/README), so I'll change that if we come up with a\n better number.\n 2. the automated range-diff generated by Gitgitgadget (at the end of the\n cover) will be mostly useless because of the refactoring of the test\n library into a test script. So I'm including a few (manually edited)\n blob diffs (git diff v3:file v4:other-file) below to make reviewing\n easier.\n\n\n----------------------------------------------------------------------------\n\n$ git diff pr-576/phil-blain/fix-branch-verbose-crlf-v3:t/lib-crlf-messages.sh HEAD:t/t3920-crlf-messages.sh\ndiff --git a/t/lib-crlf-messages.sh b/t/t3920-crlf-messages.sh\nold mode 100644\nnew mode 100755\nindex 10a2b57280..7a13d6b38a\n--- a/t/lib-crlf-messages.sh\n+++ b/t/t3920-crlf-messages.sh\n@@ -1,16 +1,17 @@\n-# Setup refs with commit and tag messages containing CRLF\n+#!/bin/sh\n+\n+test_description='Test ref-filter and pretty APIs for commit and tag messages using CRLF'\n+. ./test-lib.sh\n\n LIB_CRLF_BRANCHES=\"\"\n\n create_crlf_ref () {\n- message=\"$1\" &&\n- subject=\"$2\" &&\n- body=\"$3\" &&\n- branch=\"$4\" &&\n- printf \"${message}\" >.crlf-message-${branch}.txt &&\n- printf \"${subject}\" >.crlf-subject-${branch}.txt &&\n- printf \"${body}\" >.crlf-body-${branch}.txt &&\n- LIB_CRLF_BRANCHES=\"${LIB_CRLF_BRANCHES} ${branch}\"\n+ branch=\"$1\" &&\n+ cat >.crlf-orig-$branch.txt &&\n+ cat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n+ grep 'Subject' .crlf-orig-$branch.txt | tr '\\n' ' ' | sed 's/[ ]*$//' | tr -d '\\n' >.crlf-subject-$branch.txt &&\n+ grep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true &&\n+ LIB_CRLF_BRANCHES=\"${LIB_CRLF_BRANCHES} ${branch}\" &&\n test_tick &&\n hash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&\n git branch ${branch} ${hash} &&\n@@ -18,73 +19,108 @@ create_crlf_ref () {\n }\n\n create_crlf_refs () {\n- message=\"Subject first line\\r\\n\\r\\nBody first line\\r\\nBody second line\\r\\n\" &&\n- body=\"Body first line\\r\\nBody second line\\r\\n\" &&\n- subject=\"Subject first line\" &&\n- branch=\"crlf\" &&\n- create_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\" &&\n- message=\"Subject first line\\r\\n\\r\\n\\r\\nBody first line\\r\\nBody second line\\r\\n\" &&\n- branch=\"crlf-empty-lines-after-subject\" &&\n- create_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\" &&\n- message=\"Subject first line\\r\\nSubject second line\\r\\n\\r\\nBody first line\\r\\nBody second line\\r\\n\" &&\n- subject=\"Subject first line Subject second line\" &&\n- branch=\"crlf-two-line-subject\" &&\n- create_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\" &&\n- message=\"Subject first line\\r\\nSubject second line\" &&\n- subject=\"Subject first line Subject second line\" &&\n- body=\"\" &&\n- branch=\"crlf-two-line-subject-no-body\" &&\n- create_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\" &&\n- message=\"Subject first line\\r\\nSubject second line\\r\\n\" &&\n- branch=\"crlf-two-line-subject-no-body-trailing-newline\" &&\n- create_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\" &&\n- message=\"Subject first line\\r\\nSubject second line\\r\\n\\r\" &&\n- branch=\"crlf-two-line-subject-no-body-trailing-newline2\" &&\n- create_crlf_ref \"${message}\" \"${subject}\" \"${body}\" \"${branch}\"\n+ create_crlf_ref crlf <<-\\EOF &&\n+ Subject first line\n+\n+ Body first line\n+ Body second line\n+ EOF\n+ create_crlf_ref crlf-empty-lines-after-subject <<-\\EOF &&\n+ Subject first line\n+\n+\n+ Body first line\n+ Body second line\n+ EOF\n+ create_crlf_ref crlf-two-line-subject <<-\\EOF &&\n+ Subject first line\n+ Subject second line\n+\n+ Body first line\n+ Body second line\n+ EOF\n+ create_crlf_ref crlf-two-line-subject-no-body <<-\\EOF &&\n+ Subject first line\n+ Subject second line\n+ EOF\n+ create_crlf_ref crlf-two-line-subject-no-body-trailing-newline <<-\\EOF\n+ Subject first line\n+ Subject second line\n+\n+ EOF\n-test_create_crlf_refs () {\n- test_expect_success 'setup refs with CRLF commit messages' '\n- create_crlf_refs\n- '\n-}\n-\n-cleanup_crlf_refs () {\n- for branch in ${LIB_CRLF_BRANCHES}; do\n- git branch -D ${branch} &&\n- git tag -d tag-${branch} &&\n- rm .crlf-message-${branch}.txt &&\n- rm .crlf-subject-${branch}.txt &&\n- rm .crlf-body-${branch}.txt\n- done\n-}\n\n-test_cleanup_crlf_refs () {\n- test_expect_success 'cleanup refs with CRLF commit messages' '\n- cleanup_crlf_refs\n- '\n }\n\n\n\n test_crlf_subject_body_and_contents() {\n command_and_args=\"$@\" &&\n command=$1 &&\n- if [ ${command} = \"branch\" ] || [ ${command} = \"for-each-ref\" ] || [ ${command} = \"tag\" ]; then\n+ if test ${command} = \"branch\" || test ${command} = \"for-each-ref\" || test ${command} = \"tag\"\n+ then\n atoms=\"(contents:subject) (contents:body) (contents)\"\n- elif [ ${command} = \"log\" ] || [ ${command} = \"show\" ]; then\n+ elif test ${command} = \"log\" || test ${command} = \"show\"\n+ then\n atoms=\"s b B\"\n fi &&\n files=\"subject body message\" &&\n- while [ -n \"${atoms}\" ]; do\n+ while test -n \"${atoms}\"\n+ do\n set ${atoms} && atom=$1 && shift && atoms=\"$*\" &&\n set ${files} && file=$1 && shift && files=\"$*\" &&\n- test_expect_success \"${command}: --format='%${atom}' works with CRLF input\" \"\n+ test_expect_success \"${command}: --format='%${atom}' works with messages using CRLF\" \"\n rm -f expect &&\n- for ref in ${LIB_CRLF_BRANCHES}; do\n+ for ref in ${LIB_CRLF_BRANCHES}\n+ do\n cat .crlf-${file}-\\\"\\${ref}\\\".txt >>expect &&\n printf \\\"\\n\\\" >>expect\n done &&\n git $command_and_args --format=\\\"%${atom}\\\" >actual &&\n test_cmp expect actual\n \"\n done\n }\n+\n+\n+test_expect_success 'Setup refs with commit and tag messages using CRLF' '\n+ test_commit inital &&\n+ create_crlf_refs\n+'\n+\n\ngit diff pr-576/phil-blain/fix-branch-verbose-crlf-v3:t/t3203-branch-output.sh HEAD:t/t3920-crlf-messages.sh \ndiff --git a/t/t3203-branch-output.sh b/t/t3920-crlf-messages.sh\nindex c06eca774f..7a13d6b38a 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3920-crlf-messages.sh\n@@ -1,111 +1,97 @@\n #!/bin/sh\n\n-test_expect_success 'git branch -v works with CRLF input' '\n- cat >expect <<-EOF &&\n- two\n- one\n- Subject first line\n- Subject first line\n- Subject first line Subject second line\n- Subject first line Subject second line\n- Subject first line Subject second line\n- Subject first line Subject second line\n- EOF\n+test_expect_success 'branch: --verbose works with messages using CRLF' '\n+ rm -f expect &&\n+ for branch in $LIB_CRLF_BRANCHES\n+ do\n+ printf \" \" >>expect &&\n+ cat .crlf-subject-${branch}.txt >>expect &&\n+ printf \"\\n\" >>expect\n+ done &&\n git branch -v >tmp &&\n # Remove first two columns, and the line for the currently checked out branch\n current=$(git branch --show-current) &&\n\n$ git diff pr-576/phil-blain/fix-branch-verbose-crlf-v3:t/t4202-log.sh HEAD:t/t3920-crlf-messages.sh\ndiff --git a/t/t4202-log.sh b/t/t3920-crlf-messages.sh\nindex d4942a6f92..7a13d6b38a 100755\n--- a/t/t4202-log.sh\n+++ b/t/t3920-crlf-messages.sh\n@@ -1,1930 +1,126 @@\n-test_expect_success 'oneline with CRLF messages' '\n- for branch in $LIB_CLRF_BRANCHES; do\n+test_expect_success 'log: --oneline works with messages using CRLF' '\n+ for branch in $LIB_CRLF_BRANCHES\n+ do\n cat .crlf-subject-${branch}.txt >expect &&\n+ printf \"\\n\" >>expect &&\n git log --oneline -1 ${branch} >tmp-branch &&\n git log --oneline -1 tag-${branch} >tmp-tag &&\n- awk \"{print \\$NF}\" actual-branch &&\n- awk \"{print \\$NF}\" actual-tag &&\n+ cut -d\" \" -f2- actual-branch &&\n+ cut -d\" \" -f2- actual-tag &&\n test_cmp expect actual-branch &&\n test_cmp expect actual-tag\n done \n '\n\n\n----------------------------------------------------------------------------\n\nThe ref-filter code does not correctly handle commit or tag messages that\nuse CRLF as the line terminator. Such messages can be created with the\n--cleanup=verbatim option of git commit and git tag, or by using git\ncommit-tree directly.\n\nThe function find_subpos in ref-filter.c looks for two consecutive '\\n' to\nfind the end of the subject line, a sequence which is absent in messages\nusing CRLF. This results in the whole message being parsed as the subject\nline (%(contents:subject)), and the body of the message (%(contents:body))\nbeing empty.\n\nMoreover, in copy_subject, '\\n' is replaced by space, but '\\r' is untouched.\n\nThis bug is a regression for git branch --verbose, which bisects down to\n949af06 (branch: use ref-filter printing APIs, 2017-01-10).\n\nThe first patch fixes the bug in ref-filter.c and adds a test script to\ncheck that the ref-filter and pretty APIs deal correctly with CRLF messages.\n\nThe second patch adds tests that check the behaviour of git log andgit show \nin the presence of CRLF in messages, to prevent futur regressions.\n\nPhilippe Blain (2):\n ref-filter: handle CRLF at end-of-line more gracefully\n log, show: add tests for messages containing CRLF\n\n ref-filter.c | 36 ++++++-----\n t/t3920-crlf-messages.sh | 126 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 148 insertions(+), 14 deletions(-)\n create mode 100755 t/t3920-crlf-messages.sh\n\n\nbase-commit: a5fa49ff0a8f3252c6bff49f92b85e7683868f8a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-576%2Fphil-blain%2Ffix-branch-verbose-crlf-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-576/phil-blain/fix-branch-verbose-crlf-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/576\n\nRange-diff vs v3:\n\n 1: f17d182c3b < -: ---------- t: add lib-crlf-messages.sh for messages containing CRLF\n 2: 11d044a4f7 ! 1: 03b2d7d78a ref-filter: handle CRLF at end-of-line more gracefully\n @@ Metadata\n ## Commit message ##\n ref-filter: handle CRLF at end-of-line more gracefully\n \n - The ref-filter code does not correctly handle commit or tag messages that\n - use CRLF as the line terminator. Such messages can be created with the\n - `--verbatim` option of `git commit` and `git tag`, or by using `git\n - commit-tree` directly.\n + The ref-filter code does not correctly handle commit or tag messages\n + that use CRLF as the line terminator. Such messages can be created with\n + the `--cleanup=verbatim` option of `git commit` and `git tag`, or by\n + using `git commit-tree` directly.\n \n The function `find_subpos` in ref-filter.c looks for two consecutive\n LFs to find the end of the subject line, a sequence which is absent in\n messages using CRLF. This results in the whole message being parsed as\n the subject line (`%(contents:subject)`), and the body of the message\n - (`%(contents:body)`) being empty.\n + (`%(contents:body)`) being empty.\n \n Moreover, in `copy_subject`, which wants to return the subject as a\n single line, '\\n' is replaced by space, but '\\r' is\n @@ Commit message\n Fix this bug in ref-filter by hardening the logic in `copy_subject` and\n `find_subpos` to correctly parse messages containing CRLF.\n \n - Add tests for `branch`, `tag` and `for-each-ref` using\n - lib-crlf-messages.sh.\n -\n - The 'make commits' test at the beginning of t3203-branch-output.sh does\n - not use `test_tick` and thus the commit hashes are not reproducible. For\n - simplicity, use `test_commit` to create the commits, as the content and\n - name of the files created in this setup test are irrelevant to the rest\n - of the test script.\n -\n - Use `test_cleanup_crlf_refs` in t3203-branch-output.sh and t7004-tag.sh\n - to avoid having to modify the expected output in later tests.\n + Add a new test script, 't3920-crlf-messages.sh', to test the behaviour\n + of commands using either the ref-filter or the pretty APIs with messages\n + using CRLF line endings. The function `test_crlf_subject_body_and_contents`\n + can be used to test that the `--format` option of `branch`, `tag`,\n + `for-each-ref`, `log` and `show` correctly displays the subject, body\n + and raw content of commit and tag messages using CRLF. Test the\n + output of `branch`, `tag` and `for-each-ref` with such commits.\n \n Helped-by: Junio C Hamano \n + Helped-by: Eric Sunshine \n Signed-off-by: Philippe Blain \n \n ## ref-filter.c ##\n @@ ref-filter.c: static const char *copy_email(const char *buf, struct used_atom *a\n static char *copy_subject(const char *buf, unsigned long len)\n {\n -\tchar *r = xmemdupz(buf, len);\n --\tint i;\n +\tstruct strbuf sb = STRBUF_INIT;\n + \tint i;\n \n -\tfor (i = 0; i < len; i++)\n -\t\tif (r[i] == '\\n')\n -\t\t\tr[i] = ' ';\n -+\tfor (int i = 0; i < len; i++) {\n ++\tfor (i = 0; i < len; i++) {\n +\t\tif (buf[i] == '\\r' && i + 1 < len && buf[i + 1] == '\\n')\n +\t\t\tcontinue; /* ignore CR in CRLF */\n \n @@ ref-filter.c: static void find_subpos(const char *buf,\n \t*sublen = buf - *sub;\n \t/* drop trailing newline, if present */\n -\tif (*sublen && (*sub)[*sublen - 1] == '\\n')\n -+\twhile (*sublen && ((*sub)[*sublen - 1] == '\\n' || (*sub)[*sublen - 1] == '\\r'))\n ++\twhile (*sublen && ((*sub)[*sublen - 1] == '\\n' ||\n ++\t\t\t (*sub)[*sublen - 1] == '\\r'))\n \t\t*sublen -= 1;\n \n \t/* skip any empty lines */\n @@ ref-filter.c: static void find_subpos(const char *buf,\n \t*body = buf;\n \t*bodylen = strlen(buf);\n \n - ## t/t3203-branch-output.sh ##\n + ## t/t3920-crlf-messages.sh (new) ##\n @@\n - test_description='git branch display tests'\n - . ./test-lib.sh\n - . \"$TEST_DIRECTORY\"/lib-terminal.sh\n -+. \"$TEST_DIRECTORY\"/lib-crlf-messages.sh\n - \n - test_expect_success 'make commits' '\n --\techo content >file &&\n --\tgit add file &&\n --\tgit commit -m one &&\n --\techo content >>file &&\n --\tgit commit -a -m two\n -+\ttest_commit one &&\n -+\ttest_commit two\n - '\n - \n - test_expect_success 'make branches' '\n -@@ t/t3203-branch-output.sh: test_expect_success 'git branch --ignore-case --list -v pattern shows branch sum\n - \tawk \"{print \\$NF}\" actual &&\n - \ttest_cmp expect actual\n - '\n -+test_create_crlf_refs\n -+\n -+test_expect_success 'git branch -v works with CRLF input' '\n -+\tcat >expect <<-EOF &&\n -+\t two\n -+\t one\n -+\t Subject first line\n -+\t Subject first line\n -+\t Subject first line Subject second line\n -+\t Subject first line Subject second line\n -+\t Subject first line Subject second line\n -+\t Subject first line Subject second line\n ++#!/bin/sh\n ++\n ++test_description='Test ref-filter and pretty APIs for commit and tag messages using CRLF'\n ++. ./test-lib.sh\n ++\n ++LIB_CRLF_BRANCHES=\"\"\n ++\n ++create_crlf_ref () {\n ++\tbranch=\"$1\" &&\n ++\tcat >.crlf-orig-$branch.txt &&\n ++\tcat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n ++\tgrep 'Subject' .crlf-orig-$branch.txt | tr '\\n' ' ' | sed 's/[ ]*$//' | tr -d '\\n' >.crlf-subject-$branch.txt &&\n ++\tgrep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true &&\n ++\tLIB_CRLF_BRANCHES=\"${LIB_CRLF_BRANCHES} ${branch}\" &&\n ++\ttest_tick &&\n ++\thash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&\n ++\tgit branch ${branch} ${hash} &&\n ++\tgit tag tag-${branch} ${branch} -F .crlf-message-${branch}.txt --cleanup=verbatim\n ++}\n ++\n ++create_crlf_refs () {\n ++\tcreate_crlf_ref crlf <<-\\EOF &&\n ++\tSubject first line\n ++\n ++\tBody first line\n ++\tBody second line\n +\tEOF\n ++\tcreate_crlf_ref crlf-empty-lines-after-subject <<-\\EOF &&\n ++\tSubject first line\n ++\n ++\n ++\tBody first line\n ++\tBody second line\n ++\tEOF\n ++\tcreate_crlf_ref crlf-two-line-subject <<-\\EOF &&\n ++\tSubject first line\n ++\tSubject second line\n ++\n ++\tBody first line\n ++\tBody second line\n ++\tEOF\n ++\tcreate_crlf_ref crlf-two-line-subject-no-body <<-\\EOF &&\n ++\tSubject first line\n ++\tSubject second line\n ++\tEOF\n ++\tcreate_crlf_ref crlf-two-line-subject-no-body-trailing-newline <<-\\EOF\n ++\tSubject first line\n ++\tSubject second line\n ++\n ++\tEOF\n ++}\n ++\n ++test_crlf_subject_body_and_contents() {\n ++\tcommand_and_args=\"$@\" &&\n ++\tcommand=$1 &&\n ++\tif test ${command} = \"branch\" || test ${command} = \"for-each-ref\" || test ${command} = \"tag\"\n ++\tthen\n ++\t\tatoms=\"(contents:subject) (contents:body) (contents)\"\n ++\telif test ${command} = \"log\" || test ${command} = \"show\"\n ++\tthen\n ++\t\tatoms=\"s b B\"\n ++\tfi &&\n ++\tfiles=\"subject body message\" &&\n ++\twhile test -n \"${atoms}\"\n ++\tdo\n ++\t\tset ${atoms} && atom=$1 && shift && atoms=\"$*\" &&\n ++\t\tset ${files} &&\tfile=$1 && shift && files=\"$*\" &&\n ++\t\ttest_expect_success \"${command}: --format='%${atom}' works with messages using CRLF\" \"\n ++\t\t\trm -f expect &&\n ++\t\t\tfor ref in ${LIB_CRLF_BRANCHES}\n ++\t\t\tdo\n ++\t\t\t\tcat .crlf-${file}-\\\"\\${ref}\\\".txt >>expect &&\n ++\t\t\t\tprintf \\\"\\n\\\" >>expect\n ++\t\t\tdone &&\n ++\t\t\tgit $command_and_args --format=\\\"%${atom}\\\" >actual &&\n ++\t\t\ttest_cmp expect actual\n ++\t\t\"\n ++\tdone\n ++}\n ++\n ++\n ++test_expect_success 'Setup refs with commit and tag messages using CRLF' '\n ++\ttest_commit inital &&\n ++\tcreate_crlf_refs\n ++'\n ++\n ++test_expect_success 'branch: --verbose works with messages using CRLF' '\n ++\trm -f expect &&\n ++\tfor branch in $LIB_CRLF_BRANCHES\n ++\tdo\n ++\t\tprintf \" \" >>expect &&\n ++\t\tcat .crlf-subject-${branch}.txt >>expect &&\n ++\t\tprintf \"\\n\" >>expect\n ++\tdone &&\n +\tgit branch -v >tmp &&\n +\t# Remove first two columns, and the line for the currently checked out branch\n +\tcurrent=$(git branch --show-current) &&\n @@ t/t3203-branch-output.sh: test_expect_success 'git branch --ignore-case --list -\n +\n +test_crlf_subject_body_and_contents branch --list crlf*\n +\n -+test_cleanup_crlf_refs\n - \n - test_expect_success 'git branch -v pattern does not show branch summaries' '\n - \ttest_must_fail git branch -v branch*\n -\n - ## t/t6300-for-each-ref.sh ##\n -@@ t/t6300-for-each-ref.sh: test_description='for-each-ref test'\n - . ./test-lib.sh\n - . \"$TEST_DIRECTORY\"/lib-gpg.sh\n - . \"$TEST_DIRECTORY\"/lib-terminal.sh\n -+. \"$TEST_DIRECTORY\"/lib-crlf-messages.sh\n - \n - # Mon Jul 3 23:18:43 2006 +0000\n - datestamp=1151968723\n -@@ t/t6300-for-each-ref.sh: test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' '\n - \ttest_cmp expect actual\n - '\n - \n -+test_create_crlf_refs\n -+\n -+test_crlf_subject_body_and_contents for-each-ref refs/heads/crlf*\n -+\n - test_done\n -\n - ## t/t7004-tag.sh ##\n -@@ t/t7004-tag.sh: Tests for operations with tags.'\n - . ./test-lib.sh\n - . \"$TEST_DIRECTORY\"/lib-gpg.sh\n - . \"$TEST_DIRECTORY\"/lib-terminal.sh\n -+. \"$TEST_DIRECTORY\"/lib-crlf-messages.sh\n - \n - # creating and listing lightweight tags:\n - \n -@@ t/t7004-tag.sh: test_expect_success '--format should list tags as per format given' '\n - \ttest_cmp expect actual\n - '\n - \n -+test_create_crlf_refs\n -+\n +test_crlf_subject_body_and_contents tag --list tag-crlf*\n +\n -+test_cleanup_crlf_refs\n ++test_crlf_subject_body_and_contents for-each-ref refs/heads/crlf*\n +\n - test_expect_success \"set up color tests\" '\n - \techo \"v1.0\" >expect.color &&\n - \techo \"v1.0\" >expect.bare &&\n ++test_done\n 3: 59957d1391 ! 2: 75a87887be log, show: add tests for messages containing CRLF\n @@ Commit message\n such messages, to prevent futur regressions if these commands are\n refactored to use the ref-filter API.\n \n - To prevent having to modify expected output in further tests, use\n - 'test_cleanup_crlf_refs' in t4202 to clean-up after the added tests.\n -\n Signed-off-by: Philippe Blain \n \n - ## t/t4202-log.sh ##\n -@@ t/t4202-log.sh: test_description='git log'\n - . \"$TEST_DIRECTORY/lib-gpg.sh\"\n - . \"$TEST_DIRECTORY/lib-terminal.sh\"\n - . \"$TEST_DIRECTORY/lib-log-graph.sh\"\n -+. \"$TEST_DIRECTORY/lib-crlf-messages.sh\"\n + ## t/t3920-crlf-messages.sh ##\n +@@ t/t3920-crlf-messages.sh: test_crlf_subject_body_and_contents tag --list tag-crlf*\n \n - test_cmp_graph () {\n - \tlib_test_cmp_graph --format=%s \"$@\"\n -@@ t/t4202-log.sh: test_expect_success 'oneline' '\n - \ttest_cmp expect actual\n - '\n + test_crlf_subject_body_and_contents for-each-ref refs/heads/crlf*\n \n -+test_create_crlf_refs\n -+\n -+test_expect_success 'oneline with CRLF messages' '\n -+\tfor branch in $LIB_CLRF_BRANCHES; do\n ++test_expect_success 'log: --oneline works with messages using CRLF' '\n ++\tfor branch in $LIB_CRLF_BRANCHES\n ++\tdo\n +\t\tcat .crlf-subject-${branch}.txt >expect &&\n ++\t\tprintf \"\\n\" >>expect &&\n +\t\tgit log --oneline -1 ${branch} >tmp-branch &&\n +\t\tgit log --oneline -1 tag-${branch} >tmp-tag &&\n -+\t\tawk \"{print \\$NF}\" actual-branch &&\n -+\t\tawk \"{print \\$NF}\" actual-tag &&\n ++\t\tcut -d\" \" -f2- actual-branch &&\n ++\t\tcut -d\" \" -f2- actual-tag &&\n +\t\ttest_cmp expect actual-branch &&\n +\t\ttest_cmp expect actual-tag\n +\tdone\n +'\n -+test_crlf_subject_body_and_contents log --all --reverse --grep Subject\n +\n -+test_cleanup_crlf_refs\n -+\n - test_expect_success 'diff-filter=A' '\n - \n - \tgit log --no-renames --pretty=\"format:%s\" --diff-filter=A HEAD > actual &&\n -\n - ## t/t7007-show.sh ##\n -@@\n - test_description='git show'\n - \n - . ./test-lib.sh\n -+. \"$TEST_DIRECTORY/lib-crlf-messages.sh\"\n - \n - test_expect_success setup '\n - \techo hello world >foo &&\n -@@ t/t7007-show.sh: test_expect_success 'show --graph is forbidden' '\n - test_must_fail git show --graph HEAD\n - '\n - \n -+test_create_crlf_refs\n ++test_crlf_subject_body_and_contents log --all --reverse --grep Subject\n +\n +test_crlf_subject_body_and_contents show $LIB_CRLF_BRANCHES\n +\n\n-- \ngitgitgadget\n"},{"id":"408137","messageId":"75a87887be5d8364b37655f0588ed98a52de92bc.1603335680.git.gitgitgadget@gmail.com","threadId":"52956","inReplyTo":"pull.576.v4.git.1603335680.gitgitgadget@gmail.com","subject":"[PATCH v4 2/2] log, show: add tests for messages containing CRLF","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-22T03:01:19Z","receivedAt":"2020-10-22T03:01:26Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nA previous commit fixed a bug in ref-filter.c causing messages\ncontaining CRLF to be incorrectly parsed and displayed.\n\nAdd tests to also check that `git log` and `git show` correctly handle\nsuch messages, to prevent futur regressions if these commands are\nrefactored to use the ref-filter API.\n\nSigned-off-by: Philippe Blain \n---\n t/t3920-crlf-messages.sh | 18 ++++++++++++++++++\n 1 file changed, 18 insertions(+)\n\ndiff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\nindex 3f0ce02c3f..b6e09be412 100755\n--- a/t/t3920-crlf-messages.sh\n+++ b/t/t3920-crlf-messages.sh\n@@ -105,4 +105,22 @@ test_crlf_subject_body_and_contents tag --list tag-crlf*\n \n test_crlf_subject_body_and_contents for-each-ref refs/heads/crlf*\n \n+test_expect_success 'log: --oneline works with messages using CRLF' '\n+\tfor branch in $LIB_CRLF_BRANCHES\n+\tdo\n+\t\tcat .crlf-subject-${branch}.txt >expect &&\n+\t\tprintf \"\\n\" >>expect &&\n+\t\tgit log --oneline -1 ${branch} >tmp-branch &&\n+\t\tgit log --oneline -1 tag-${branch} >tmp-tag &&\n+\t\tcut -d\" \" -f2- actual-branch &&\n+\t\tcut -d\" \" -f2- actual-tag &&\n+\t\ttest_cmp expect actual-branch &&\n+\t\ttest_cmp expect actual-tag\n+\tdone\n+'\n+\n+test_crlf_subject_body_and_contents log --all --reverse --grep Subject\n+\n+test_crlf_subject_body_and_contents show $LIB_CRLF_BRANCHES\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"408139","messageId":"03b2d7d78a15d15130a68ed1e6092072aa0807cd.1603335680.git.gitgitgadget@gmail.com","threadId":"52956","inReplyTo":"pull.576.v4.git.1603335680.gitgitgadget@gmail.com","subject":"[PATCH v4 1/2] ref-filter: handle CRLF at end-of-line more gracefully","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-22T03:01:18Z","receivedAt":"2020-10-22T03:01:28Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nThe ref-filter code does not correctly handle commit or tag messages\nthat use CRLF as the line terminator. Such messages can be created with\nthe `--cleanup=verbatim` option of `git commit` and `git tag`, or by\nusing `git commit-tree` directly.\n\nThe function `find_subpos` in ref-filter.c looks for two consecutive\nLFs to find the end of the subject line, a sequence which is absent in\nmessages using CRLF. This results in the whole message being parsed as\nthe subject line (`%(contents:subject)`), and the body of the message\n(`%(contents:body)`) being empty.\n\nMoreover, in `copy_subject`, which wants to return the subject as a\nsingle line, '\\n' is replaced by space, but '\\r' is\nuntouched.\n\nThis impacts the output of `git branch`, `git tag` and `git\nfor-each-ref`.\n\nThis bug is a regression for `git branch --verbose`, which\nbisects down to 949af0684c (branch: use ref-filter printing APIs,\n2017-01-10).\n\nFix this bug in ref-filter by hardening the logic in `copy_subject` and\n`find_subpos` to correctly parse messages containing CRLF.\n\nAdd a new test script, 't3920-crlf-messages.sh', to test the behaviour\nof commands using either the ref-filter or the pretty APIs with messages\nusing CRLF line endings. The function `test_crlf_subject_body_and_contents`\ncan be used to test that the `--format` option of `branch`, `tag`,\n`for-each-ref`, `log` and `show` correctly displays the subject, body\nand raw content of commit and tag messages using CRLF. Test the\noutput of `branch`, `tag` and `for-each-ref` with such commits.\n\nHelped-by: Junio C Hamano \nHelped-by: Eric Sunshine \nSigned-off-by: Philippe Blain \n---\n ref-filter.c | 36 ++++++++-----\n t/t3920-crlf-messages.sh | 108 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 130 insertions(+), 14 deletions(-)\n create mode 100755 t/t3920-crlf-messages.sh\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex c62f6b4822..6476686fea 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1097,14 +1097,19 @@ static const char *copy_email(const char *buf, struct used_atom *atom)\n \n static char *copy_subject(const char *buf, unsigned long len)\n {\n-\tchar *r = xmemdupz(buf, len);\n+\tstruct strbuf sb = STRBUF_INIT;\n \tint i;\n \n-\tfor (i = 0; i < len; i++)\n-\t\tif (r[i] == '\\n')\n-\t\t\tr[i] = ' ';\n+\tfor (i = 0; i < len; i++) {\n+\t\tif (buf[i] == '\\r' && i + 1 < len && buf[i + 1] == '\\n')\n+\t\t\tcontinue; /* ignore CR in CRLF */\n \n-\treturn r;\n+\t\tif (buf[i] == '\\n')\n+\t\t\tstrbuf_addch(&sb, ' ');\n+\t\telse\n+\t\t\tstrbuf_addch(&sb, buf[i]);\n+\t}\n+\treturn strbuf_detach(&sb, NULL);\n }\n \n static void grab_date(const char *buf, struct atom_value *v, const char *atomname)\n@@ -1228,20 +1233,23 @@ static void find_subpos(const char *buf,\n \n \t/* subject is first non-empty line */\n \t*sub = buf;\n-\t/* subject goes to first empty line */\n-\twhile (buf < *sig && *buf && *buf != '\\n') {\n-\t\teol = strchrnul(buf, '\\n');\n-\t\tif (*eol)\n-\t\t\teol++;\n-\t\tbuf = eol;\n-\t}\n+\t/* subject goes to first empty line before signature begins */\n+\tif ((eol = strstr(*sub, \"\\n\\n\"))) {\n+\t\teol = eol < *sig ? eol : *sig;\n+\t/* check if message uses CRLF */\n+\t} else if (! (eol = strstr(*sub, \"\\r\\n\\r\\n\"))) {\n+\t\t/* treat whole message as subject */\n+\t\teol = strrchr(*sub, '\\0');\n+\t}\n+\tbuf = eol;\n \t*sublen = buf - *sub;\n \t/* drop trailing newline, if present */\n-\tif (*sublen && (*sub)[*sublen - 1] == '\\n')\n+\twhile (*sublen && ((*sub)[*sublen - 1] == '\\n' ||\n+\t\t\t (*sub)[*sublen - 1] == '\\r'))\n \t\t*sublen -= 1;\n \n \t/* skip any empty lines */\n-\twhile (*buf == '\\n')\n+\twhile (*buf == '\\n' || *buf == '\\r')\n \t\tbuf++;\n \t*body = buf;\n \t*bodylen = strlen(buf);\ndiff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\nnew file mode 100755\nindex 0000000000..3f0ce02c3f\n--- /dev/null\n+++ b/t/t3920-crlf-messages.sh\n@@ -0,0 +1,108 @@\n+#!/bin/sh\n+\n+test_description='Test ref-filter and pretty APIs for commit and tag messages using CRLF'\n+. ./test-lib.sh\n+\n+LIB_CRLF_BRANCHES=\"\"\n+\n+create_crlf_ref () {\n+\tbranch=\"$1\" &&\n+\tcat >.crlf-orig-$branch.txt &&\n+\tcat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n+\tgrep 'Subject' .crlf-orig-$branch.txt | tr '\\n' ' ' | sed 's/[ ]*$//' | tr -d '\\n' >.crlf-subject-$branch.txt &&\n+\tgrep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true &&\n+\tLIB_CRLF_BRANCHES=\"${LIB_CRLF_BRANCHES} ${branch}\" &&\n+\ttest_tick &&\n+\thash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&\n+\tgit branch ${branch} ${hash} &&\n+\tgit tag tag-${branch} ${branch} -F .crlf-message-${branch}.txt --cleanup=verbatim\n+}\n+\n+create_crlf_refs () {\n+\tcreate_crlf_ref crlf <<-\\EOF &&\n+\tSubject first line\n+\n+\tBody first line\n+\tBody second line\n+\tEOF\n+\tcreate_crlf_ref crlf-empty-lines-after-subject <<-\\EOF &&\n+\tSubject first line\n+\n+\n+\tBody first line\n+\tBody second line\n+\tEOF\n+\tcreate_crlf_ref crlf-two-line-subject <<-\\EOF &&\n+\tSubject first line\n+\tSubject second line\n+\n+\tBody first line\n+\tBody second line\n+\tEOF\n+\tcreate_crlf_ref crlf-two-line-subject-no-body <<-\\EOF &&\n+\tSubject first line\n+\tSubject second line\n+\tEOF\n+\tcreate_crlf_ref crlf-two-line-subject-no-body-trailing-newline <<-\\EOF\n+\tSubject first line\n+\tSubject second line\n+\n+\tEOF\n+}\n+\n+test_crlf_subject_body_and_contents() {\n+\tcommand_and_args=\"$@\" &&\n+\tcommand=$1 &&\n+\tif test ${command} = \"branch\" || test ${command} = \"for-each-ref\" || test ${command} = \"tag\"\n+\tthen\n+\t\tatoms=\"(contents:subject) (contents:body) (contents)\"\n+\telif test ${command} = \"log\" || test ${command} = \"show\"\n+\tthen\n+\t\tatoms=\"s b B\"\n+\tfi &&\n+\tfiles=\"subject body message\" &&\n+\twhile test -n \"${atoms}\"\n+\tdo\n+\t\tset ${atoms} && atom=$1 && shift && atoms=\"$*\" &&\n+\t\tset ${files} &&\tfile=$1 && shift && files=\"$*\" &&\n+\t\ttest_expect_success \"${command}: --format='%${atom}' works with messages using CRLF\" \"\n+\t\t\trm -f expect &&\n+\t\t\tfor ref in ${LIB_CRLF_BRANCHES}\n+\t\t\tdo\n+\t\t\t\tcat .crlf-${file}-\\\"\\${ref}\\\".txt >>expect &&\n+\t\t\t\tprintf \\\"\\n\\\" >>expect\n+\t\t\tdone &&\n+\t\t\tgit $command_and_args --format=\\\"%${atom}\\\" >actual &&\n+\t\t\ttest_cmp expect actual\n+\t\t\"\n+\tdone\n+}\n+\n+\n+test_expect_success 'Setup refs with commit and tag messages using CRLF' '\n+\ttest_commit inital &&\n+\tcreate_crlf_refs\n+'\n+\n+test_expect_success 'branch: --verbose works with messages using CRLF' '\n+\trm -f expect &&\n+\tfor branch in $LIB_CRLF_BRANCHES\n+\tdo\n+\t\tprintf \" \" >>expect &&\n+\t\tcat .crlf-subject-${branch}.txt >>expect &&\n+\t\tprintf \"\\n\" >>expect\n+\tdone &&\n+\tgit branch -v >tmp &&\n+\t# Remove first two columns, and the line for the currently checked out branch\n+\tcurrent=$(git branch --show-current) &&\n+\tgrep -v $current actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_crlf_subject_body_and_contents branch --list crlf*\n+\n+test_crlf_subject_body_and_contents tag --list tag-crlf*\n+\n+test_crlf_subject_body_and_contents for-each-ref refs/heads/crlf*\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"408140","messageId":"7E00B1E7-DC57-4708-9D81-4402AD931126@gmail.com","threadId":"52956","inReplyTo":"20201012224706.GA4318@flurp.local","subject":"Re: [PATCH v3 1/3] t: add lib-crlf-messages.sh for messages containing CRLF","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-10-22T03:09:58Z","receivedAt":"2020-10-22T03:10:03Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Eric,\n\n>> +cleanup_crlf_refs () {\n>> +\tfor branch in ${LIB_CRLF_BRANCHES}; do\n> \n> Our style is to place 'do' on its own line:\n> \n> for branch in $LIB_CRLF_BRANCHES\n> do\n> ...\n> \n> This would be a syntax error if LIB_CRLF_BRANCHES is empty for some\n> reason, but I suppose we don't really have to worry about it here(?).\n\nApparently, not in my shell, as I realized I had misspelled LIB_CRLF_BRANCHES as\nLIB_CLRF_BRANCHES (CLRF instead of CRLF) at another place and the test was\npassing correctly (the loop was not being entered at all though):\n\n$ /bin/sh --version\nGNU bash, version 3.2.57(1)-release (x86_64-apple-darwin15)\n$ for a in $b; do echo hello; done; echo $?\n0\n\nI've fixed that in v4 which I just sent. \n\nCheers,\nPhilippe.\n\n"},{"id":"408185","messageId":"ED48F9A2-A8F6-4332-BB5E-C767D0C23AC0@gmail.com","threadId":"52956","inReplyTo":"75a87887be5d8364b37655f0588ed98a52de92bc.1603335680.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 2/2] log, show: add tests for messages containing CRLF","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-10-22T19:24:54Z","receivedAt":"2020-10-22T19:25:06Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"\n> Le 21 oct. 2020 à 23:01, Philippe Blain via GitGitGadget a écrit :\n> \n> From: Philippe Blain \n> \n> A previous commit fixed a bug in ref-filter.c causing messages\n> containing CRLF to be incorrectly parsed and displayed.\n> \n> Add tests to also check that `git log` and `git show` correctly handle\n> such messages, to prevent futur regressions if these commands are\n> refactored to use the ref-filter API.\n> \n> Signed-off-by: Philippe Blain \n> ---\n> t/t3920-crlf-messages.sh | 18 ++++++++++++++++++\n> 1 file changed, 18 insertions(+)\n> \n> diff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\n> index 3f0ce02c3f..b6e09be412 100755\n> --- a/t/t3920-crlf-messages.sh\n> +++ b/t/t3920-crlf-messages.sh\n> @@ -105,4 +105,22 @@ test_crlf_subject_body_and_contents tag --list tag-crlf*\n> \n> test_crlf_subject_body_and_contents for-each-ref refs/heads/crlf*\n> \n> +test_expect_success 'log: --oneline works with messages using CRLF' '\n> +\tfor branch in $LIB_CRLF_BRANCHES\n> +\tdo\n> +\t\tcat .crlf-subject-${branch}.txt >expect &&\n> +\t\tprintf \"\\n\" >>expect &&\n> +\t\tgit log --oneline -1 ${branch} >tmp-branch &&\n> +\t\tgit log --oneline -1 tag-${branch} >tmp-tag &&\n> +\t\tcut -d\" \" -f2- actual-branch &&\n> +\t\tcut -d\" \" -f2- actual-tag &&\n> +\t\ttest_cmp expect actual-branch &&\n> +\t\ttest_cmp expect actual-tag\n> +\tdone\n\nI just realized that I'm missing this in this patch :\n\ndiff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\nindex b6e09be412..70ddce3a2e 100755\n--- a/t/t3920-crlf-messages.sh\n+++ b/t/t3920-crlf-messages.sh\n@@ -115,7 +115,7 @@ test_expect_success 'log: --oneline works with messages using CRLF' '\n \t\tcut -d\" \" -f2- actual-branch &&\n \t\tcut -d\" \" -f2- actual-tag &&\n \t\ttest_cmp expect actual-branch &&\n-\t\ttest_cmp expect actual-tag\n+\t\ttest_cmp expect actual-tag || return 1\n \tdone\n '\n\nI'll wait a few days for any further comments and re-send with this tweak.\n\nPhilippe."},{"id":"408195","messageId":"xmqq8sbxlq62.fsf@gitster.c.googlers.com","threadId":"52956","inReplyTo":"03b2d7d78a15d15130a68ed1e6092072aa0807cd.1603335680.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 1/2] ref-filter: handle CRLF at end-of-line more gracefully","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-23T00:52:21Z","receivedAt":"2020-10-23T00:52:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philippe Blain via GitGitGadget\" writes:\n\n> From: Philippe Blain \n>\n> The ref-filter code does not correctly handle commit or tag messages\n> that use CRLF as the line terminator. Such messages can be created with\n> the `--cleanup=verbatim` option of `git commit` and `git tag`, or by\n> using `git commit-tree` directly.\n>\n> The function `find_subpos` in ref-filter.c looks for two consecutive\n> LFs to find the end of the subject line, a sequence which is absent in\n> messages using CRLF. This results in the whole message being parsed as\n> the subject line (`%(contents:subject)`), and the body of the message\n> (`%(contents:body)`) being empty.\n>\n> Moreover, in `copy_subject`, which wants to return the subject as a\n> single line, '\\n' is replaced by space, but '\\r' is\n> untouched.\n\nHonestly, all of the above signal, at least to me, that these\nobjects are designed to use LF terminated lines and nothing else,\nwhether Windows or DOS existed in the same world or not. There is\nno such thing as commit objects that use CRLF as the line\nterminator. They are commit objects whose payload has CR at the end\nof each and every line. Just like there can be commit objects whose\npayload has trailing SP on each line, or even has binary guck, these\nthings can be created using the \"commit --cleanup=verbatim\" command,\nor the \"hash-objects\" command. It does not mean it is encouraged to\ncreate such objects. It does not mean it is sensible to expect them\nto behave as if these trailing whitespaces (be it SP or CR) are not\nthere.\n\n> This impacts the output of `git branch`, `git tag` and `git\n> for-each-ref`.\n\nThe answer to that problem description is \"then don't\" ;-). If you\ndo not want to have trailing whitespaces, you need to clean them up\nsomehow, and we give an easy way to do so with the default --cleanup\naction. Setting it to verbatim is to decline that easy way offered\nto you, and it makes it your responsibility to do your own clean-up\nif you still want to remove the CR at the end of your lines.\n\nHaving said all that.\n\nHere is how I explained the topic in the \"What's cooking\" report.\n\n A commit and tag object may have CR at the end of each and\n every line (you can create such an object with hash-object or\n using --cleanup=verbatim to decline the default clean-up\n action), but it would make it impossible to have a blank line\n to separate the title from the body of the message. Be lenient\n and accept a line with lone CR on it as a blank line, too.\n\nLet's not call this change a \"bug fix\". The phrase you used in your\ntitle, \"more gracefully\", is a very good one.\n\nIn the meantime, I've squashed your \"oops forgot ||return 1\" change\ninto [PATCH 2/2].\n\nThanks.\n"},{"id":"408197","messageId":"FFAF7079-C759-43F3-96AA-CAF3B73B55B4@gmail.com","threadId":"52956","inReplyTo":"xmqq8sbxlq62.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v4 1/2] ref-filter: handle CRLF at end-of-line more gracefully","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-10-23T01:46:26Z","receivedAt":"2020-10-23T01:46:33Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Junio,\n\n> Le 22 oct. 2020 à 20:52, Junio C Hamano a écrit :\n> \n> \"Philippe Blain via GitGitGadget\" writes:\n> \n>> From: Philippe Blain \n>> \n>> The ref-filter code does not correctly handle commit or tag messages\n>> that use CRLF as the line terminator. Such messages can be created with\n>> the `--cleanup=verbatim` option of `git commit` and `git tag`, or by\n>> using `git commit-tree` directly.\n>> \n>> The function `find_subpos` in ref-filter.c looks for two consecutive\n>> LFs to find the end of the subject line, a sequence which is absent in\n>> messages using CRLF. This results in the whole message being parsed as\n>> the subject line (`%(contents:subject)`), and the body of the message\n>> (`%(contents:body)`) being empty.\n>> \n>> Moreover, in `copy_subject`, which wants to return the subject as a\n>> single line, '\\n' is replaced by space, but '\\r' is\n>> untouched.\n> \n> Honestly, all of the above signal, at least to me, that these\n> objects are designed to use LF terminated lines and nothing else,\n> whether Windows or DOS existed in the same world or not. There is\n> no such thing as commit objects that use CRLF as the line\n> terminator. They are commit objects whose payload has CR at the end\n> of each and every line. Just like there can be commit objects whose\n> payload has trailing SP on each line, or even has binary guck, these\n> things can be created using the \"commit --cleanup=verbatim\" command,\n> or the \"hash-objects\" command. It does not mean it is encouraged to\n> create such objects. It does not mean it is sensible to expect them\n> to behave as if these trailing whitespaces (be it SP or CR) are not\n> there.\n> \n>> This impacts the output of `git branch`, `git tag` and `git\n>> for-each-ref`.\n> \n> The answer to that problem description is \"then don't\" ;-). If you\n> do not want to have trailing whitespaces, you need to clean them up\n> somehow, and we give an easy way to do so with the default --cleanup\n> action. Setting it to verbatim is to decline that easy way offered\n> to you, and it makes it your responsibility to do your own clean-up\n> if you still want to remove the CR at the end of your lines.\n\nI agree with you on that : if you are creating the object yourself,\nyou should let the default cleanup take place.\n\nBut as a lot of projects use GitHub, GitLab or similar services\nto accept contributions, and let these web systems perform the \"merge\" \n(or rebase or whatever) operation to integrate these contributions;\nmaintainers sometime choose to not always have complete control\non all objects that become part of the canonical history of their repository. \nAnd as I wrote in [1], GitLab was creating commits using CRLF up until 9.2... [2].\nSo for these poor projects that are now stuck with these CRLFs in their\nmerge commit messages, I think it's good that Git handles these correctly.\n\n> Having said all that.\n> \n> Here is how I explained the topic in the \"What's cooking\" report.\n> \n> A commit and tag object may have CR at the end of each and\n> every line (you can create such an object with hash-object or\n> using --cleanup=verbatim to decline the default clean-up\n> action), but it would make it impossible to have a blank line\n> to separate the title from the body of the message. Be lenient\n> and accept a line with lone CR on it as a blank line, too.\n\nJust for the sake of searchability, I think it would be good to have \nCRLF spelled out in this topic description (since I gather this is what \nends up in the release notes). But I don't feel that strongly\nabout that.\n\n> Let's not call this change a \"bug fix\". The phrase you used in your\n> title, \"more gracefully\", is a very good one.\n\nIt was your suggestion ;) \n\n> In the meantime, I've squashed your \"oops forgot ||return 1\" change\n> into [PATCH 2/2].\n\nThanks for squashing it in.\n\nCheers,\nPhilippe.\n\n\n[1] https://lore.kernel.org/git/63755050-10A5-4A46-9BB3-8207E055692C@gmail.com/\n[2] https://gitlab.com/gitlab-org/gitlab-foss/-/issues/31671"},{"id":"408612","messageId":"47BFCD45-B3C2-4AF6-BE09-DA5E1BFAF5E6@gmail.com","threadId":"52956","inReplyTo":"xmqq1rhi9jzl.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v4 1/2] ref-filter: handle CRLF at end-of-line more gracefully","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-10-29T01:29:07Z","receivedAt":"2020-10-29T01:32:29Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Junio,\n\n> Le 28 oct. 2020 à 16:24, Junio C Hamano a écrit :\n> \n> Philippe Blain writes:\n> \n>>> Having said all that.\n>>> \n>>> Here is how I explained the topic in the \"What's cooking\" report.\n>>> \n>>> A commit and tag object may have CR at the end of each and\n>>> every line (you can create such an object with hash-object or\n>>> using --cleanup=verbatim to decline the default clean-up\n>>> action), but it would make it impossible to have a blank line\n>>> to separate the title from the body of the message. Be lenient\n>>> and accept a line with lone CR on it as a blank line, too.\n>> \n>> Just for the sake of searchability, I think it would be good to have \n>> CRLF spelled out in this topic description (since I gather this is what \n>> ends up in the release notes). But I don't feel that strongly\n>> about that.\n>> \n>>> Let's not call this change a \"bug fix\". The phrase you used in your\n>>> title, \"more gracefully\", is a very good one.\n>> \n>> It was your suggestion ;) \n>> \n>>> In the meantime, I've squashed your \"oops forgot ||return 1\" change\n>>> into [PATCH 2/2].\n>> \n>> Thanks for squashing it in.\n> \n> Squashing in the \"oops forgot || return 1\" was the only thing I did.\n> I did not rewrite (and will not do so myself) the proposed log\n> message 1/2, which needs to happen before the topic can hit 'next'.\n\nAh! I thought you meant \"Let's not call it a bug fix in the release notes\".\nI'll send a new version to not mention \"bug\" in the log messages.\n\nThanks,\n\nPhilippe."},{"id":"408614","messageId":"xmqq1rhi9jzl.fsf@gitster.c.googlers.com","threadId":"52956","inReplyTo":"FFAF7079-C759-43F3-96AA-CAF3B73B55B4@gmail.com","subject":"Re: [PATCH v4 1/2] ref-filter: handle CRLF at end-of-line more gracefully","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-28T20:24:46Z","receivedAt":"2020-10-29T02:04:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philippe Blain writes:\n\n>> Having said all that.\n>> \n>> Here is how I explained the topic in the \"What's cooking\" report.\n>> \n>> A commit and tag object may have CR at the end of each and\n>> every line (you can create such an object with hash-object or\n>> using --cleanup=verbatim to decline the default clean-up\n>> action), but it would make it impossible to have a blank line\n>> to separate the title from the body of the message. Be lenient\n>> and accept a line with lone CR on it as a blank line, too.\n>\n> Just for the sake of searchability, I think it would be good to have \n> CRLF spelled out in this topic description (since I gather this is what \n> ends up in the release notes). But I don't feel that strongly\n> about that.\n>\n>> Let's not call this change a \"bug fix\". The phrase you used in your\n>> title, \"more gracefully\", is a very good one.\n>\n> It was your suggestion ;) \n>\n>> In the meantime, I've squashed your \"oops forgot ||return 1\" change\n>> into [PATCH 2/2].\n>\n> Thanks for squashing it in.\n\nSquashing in the \"oops forgot || return 1\" was the only thing I did.\nI did not rewrite (and will not do so myself) the proposed log\nmessage 1/2, which needs to happen before the topic can hit 'next'.\n\nThanks.\n"},{"id":"408643","messageId":"06231c315ff6e5a1065b5a77350d7cdc307f760b.1603975709.git.gitgitgadget@gmail.com","threadId":"52956","inReplyTo":"pull.576.v5.git.1603975709.gitgitgadget@gmail.com","subject":"[PATCH v5 1/2] ref-filter: handle CRLF at end-of-line more gracefully","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-29T12:48:28Z","receivedAt":"2020-10-29T12:48:35Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nThe ref-filter code does not correctly handle commit or tag messages\nthat use CRLF as the line terminator. Such messages can be created with\nthe `--cleanup=verbatim` option of `git commit` and `git tag`, or by\nusing `git commit-tree` directly.\n\nThe function `find_subpos` in ref-filter.c looks for two consecutive\nLFs to find the end of the subject line, a sequence which is absent in\nmessages using CRLF. This results in the whole message being parsed as\nthe subject line (`%(contents:subject)`), and the body of the message\n(`%(contents:body)`) being empty.\n\nMoreover, in `copy_subject`, which wants to return the subject as a\nsingle line, '\\n' is replaced by space, but '\\r' is\nuntouched.\n\nThis impacts the output of `git branch`, `git tag` and `git\nfor-each-ref`.\n\nThis behaviour is a regression for `git branch --verbose`, which\nbisects down to 949af0684c (branch: use ref-filter printing APIs,\n2017-01-10).\n\nAdjust the ref-filter code to be more lenient by hardening the logic in\n`copy_subject` and `find_subpos` to correctly parse messages containing\nCRLF.\n\nAdd a new test script, 't3920-crlf-messages.sh', to test the behaviour\nof commands using either the ref-filter or the pretty APIs with messages\nusing CRLF line endings. The function `test_crlf_subject_body_and_contents`\ncan be used to test that the `--format` option of `branch`, `tag`,\n`for-each-ref`, `log` and `show` correctly displays the subject, body\nand raw content of commit and tag messages using CRLF. Test the\noutput of `branch`, `tag` and `for-each-ref` with such commits.\n\nHelped-by: Junio C Hamano \nHelped-by: Eric Sunshine \nSigned-off-by: Philippe Blain \n---\n ref-filter.c | 36 ++++++++-----\n t/t3920-crlf-messages.sh | 108 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 130 insertions(+), 14 deletions(-)\n create mode 100755 t/t3920-crlf-messages.sh\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex c62f6b4822..6476686fea 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1097,14 +1097,19 @@ static const char *copy_email(const char *buf, struct used_atom *atom)\n \n static char *copy_subject(const char *buf, unsigned long len)\n {\n-\tchar *r = xmemdupz(buf, len);\n+\tstruct strbuf sb = STRBUF_INIT;\n \tint i;\n \n-\tfor (i = 0; i < len; i++)\n-\t\tif (r[i] == '\\n')\n-\t\t\tr[i] = ' ';\n+\tfor (i = 0; i < len; i++) {\n+\t\tif (buf[i] == '\\r' && i + 1 < len && buf[i + 1] == '\\n')\n+\t\t\tcontinue; /* ignore CR in CRLF */\n \n-\treturn r;\n+\t\tif (buf[i] == '\\n')\n+\t\t\tstrbuf_addch(&sb, ' ');\n+\t\telse\n+\t\t\tstrbuf_addch(&sb, buf[i]);\n+\t}\n+\treturn strbuf_detach(&sb, NULL);\n }\n \n static void grab_date(const char *buf, struct atom_value *v, const char *atomname)\n@@ -1228,20 +1233,23 @@ static void find_subpos(const char *buf,\n \n \t/* subject is first non-empty line */\n \t*sub = buf;\n-\t/* subject goes to first empty line */\n-\twhile (buf < *sig && *buf && *buf != '\\n') {\n-\t\teol = strchrnul(buf, '\\n');\n-\t\tif (*eol)\n-\t\t\teol++;\n-\t\tbuf = eol;\n-\t}\n+\t/* subject goes to first empty line before signature begins */\n+\tif ((eol = strstr(*sub, \"\\n\\n\"))) {\n+\t\teol = eol < *sig ? eol : *sig;\n+\t/* check if message uses CRLF */\n+\t} else if (! (eol = strstr(*sub, \"\\r\\n\\r\\n\"))) {\n+\t\t/* treat whole message as subject */\n+\t\teol = strrchr(*sub, '\\0');\n+\t}\n+\tbuf = eol;\n \t*sublen = buf - *sub;\n \t/* drop trailing newline, if present */\n-\tif (*sublen && (*sub)[*sublen - 1] == '\\n')\n+\twhile (*sublen && ((*sub)[*sublen - 1] == '\\n' ||\n+\t\t\t (*sub)[*sublen - 1] == '\\r'))\n \t\t*sublen -= 1;\n \n \t/* skip any empty lines */\n-\twhile (*buf == '\\n')\n+\twhile (*buf == '\\n' || *buf == '\\r')\n \t\tbuf++;\n \t*body = buf;\n \t*bodylen = strlen(buf);\ndiff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\nnew file mode 100755\nindex 0000000000..3f0ce02c3f\n--- /dev/null\n+++ b/t/t3920-crlf-messages.sh\n@@ -0,0 +1,108 @@\n+#!/bin/sh\n+\n+test_description='Test ref-filter and pretty APIs for commit and tag messages using CRLF'\n+. ./test-lib.sh\n+\n+LIB_CRLF_BRANCHES=\"\"\n+\n+create_crlf_ref () {\n+\tbranch=\"$1\" &&\n+\tcat >.crlf-orig-$branch.txt &&\n+\tcat .crlf-orig-$branch.txt | append_cr >.crlf-message-$branch.txt &&\n+\tgrep 'Subject' .crlf-orig-$branch.txt | tr '\\n' ' ' | sed 's/[ ]*$//' | tr -d '\\n' >.crlf-subject-$branch.txt &&\n+\tgrep 'Body' .crlf-message-$branch.txt >.crlf-body-$branch.txt || true &&\n+\tLIB_CRLF_BRANCHES=\"${LIB_CRLF_BRANCHES} ${branch}\" &&\n+\ttest_tick &&\n+\thash=$(git commit-tree HEAD^{tree} -p HEAD -F .crlf-message-${branch}.txt) &&\n+\tgit branch ${branch} ${hash} &&\n+\tgit tag tag-${branch} ${branch} -F .crlf-message-${branch}.txt --cleanup=verbatim\n+}\n+\n+create_crlf_refs () {\n+\tcreate_crlf_ref crlf <<-\\EOF &&\n+\tSubject first line\n+\n+\tBody first line\n+\tBody second line\n+\tEOF\n+\tcreate_crlf_ref crlf-empty-lines-after-subject <<-\\EOF &&\n+\tSubject first line\n+\n+\n+\tBody first line\n+\tBody second line\n+\tEOF\n+\tcreate_crlf_ref crlf-two-line-subject <<-\\EOF &&\n+\tSubject first line\n+\tSubject second line\n+\n+\tBody first line\n+\tBody second line\n+\tEOF\n+\tcreate_crlf_ref crlf-two-line-subject-no-body <<-\\EOF &&\n+\tSubject first line\n+\tSubject second line\n+\tEOF\n+\tcreate_crlf_ref crlf-two-line-subject-no-body-trailing-newline <<-\\EOF\n+\tSubject first line\n+\tSubject second line\n+\n+\tEOF\n+}\n+\n+test_crlf_subject_body_and_contents() {\n+\tcommand_and_args=\"$@\" &&\n+\tcommand=$1 &&\n+\tif test ${command} = \"branch\" || test ${command} = \"for-each-ref\" || test ${command} = \"tag\"\n+\tthen\n+\t\tatoms=\"(contents:subject) (contents:body) (contents)\"\n+\telif test ${command} = \"log\" || test ${command} = \"show\"\n+\tthen\n+\t\tatoms=\"s b B\"\n+\tfi &&\n+\tfiles=\"subject body message\" &&\n+\twhile test -n \"${atoms}\"\n+\tdo\n+\t\tset ${atoms} && atom=$1 && shift && atoms=\"$*\" &&\n+\t\tset ${files} &&\tfile=$1 && shift && files=\"$*\" &&\n+\t\ttest_expect_success \"${command}: --format='%${atom}' works with messages using CRLF\" \"\n+\t\t\trm -f expect &&\n+\t\t\tfor ref in ${LIB_CRLF_BRANCHES}\n+\t\t\tdo\n+\t\t\t\tcat .crlf-${file}-\\\"\\${ref}\\\".txt >>expect &&\n+\t\t\t\tprintf \\\"\\n\\\" >>expect\n+\t\t\tdone &&\n+\t\t\tgit $command_and_args --format=\\\"%${atom}\\\" >actual &&\n+\t\t\ttest_cmp expect actual\n+\t\t\"\n+\tdone\n+}\n+\n+\n+test_expect_success 'Setup refs with commit and tag messages using CRLF' '\n+\ttest_commit inital &&\n+\tcreate_crlf_refs\n+'\n+\n+test_expect_success 'branch: --verbose works with messages using CRLF' '\n+\trm -f expect &&\n+\tfor branch in $LIB_CRLF_BRANCHES\n+\tdo\n+\t\tprintf \" \" >>expect &&\n+\t\tcat .crlf-subject-${branch}.txt >>expect &&\n+\t\tprintf \"\\n\" >>expect\n+\tdone &&\n+\tgit branch -v >tmp &&\n+\t# Remove first two columns, and the line for the currently checked out branch\n+\tcurrent=$(git branch --show-current) &&\n+\tgrep -v $current actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_crlf_subject_body_and_contents branch --list crlf*\n+\n+test_crlf_subject_body_and_contents tag --list tag-crlf*\n+\n+test_crlf_subject_body_and_contents for-each-ref refs/heads/crlf*\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"408644","messageId":"f536fee69565402f6a74e9746c7d06c49814e0c5.1603975709.git.gitgitgadget@gmail.com","threadId":"52956","inReplyTo":"pull.576.v5.git.1603975709.gitgitgadget@gmail.com","subject":"[PATCH v5 2/2] log, show: add tests for messages containing CRLF","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-29T12:48:29Z","receivedAt":"2020-10-29T12:48:36Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nA previous commit adjusted the code in ref-filter.c so that messages\ncontaining CRLF are now correctly parsed and displayed.\n\nAdd tests to also check that `git log` and `git show` correctly handle\nsuch messages, to prevent futur regressions if these commands are\nrefactored to use the ref-filter API.\n\nSigned-off-by: Philippe Blain \n---\n t/t3920-crlf-messages.sh | 18 ++++++++++++++++++\n 1 file changed, 18 insertions(+)\n\ndiff --git a/t/t3920-crlf-messages.sh b/t/t3920-crlf-messages.sh\nindex 3f0ce02c3f..70ddce3a2e 100755\n--- a/t/t3920-crlf-messages.sh\n+++ b/t/t3920-crlf-messages.sh\n@@ -105,4 +105,22 @@ test_crlf_subject_body_and_contents tag --list tag-crlf*\n \n test_crlf_subject_body_and_contents for-each-ref refs/heads/crlf*\n \n+test_expect_success 'log: --oneline works with messages using CRLF' '\n+\tfor branch in $LIB_CRLF_BRANCHES\n+\tdo\n+\t\tcat .crlf-subject-${branch}.txt >expect &&\n+\t\tprintf \"\\n\" >>expect &&\n+\t\tgit log --oneline -1 ${branch} >tmp-branch &&\n+\t\tgit log --oneline -1 tag-${branch} >tmp-tag &&\n+\t\tcut -d\" \" -f2- actual-branch &&\n+\t\tcut -d\" \" -f2- actual-tag &&\n+\t\ttest_cmp expect actual-branch &&\n+\t\ttest_cmp expect actual-tag || return 1\n+\tdone\n+'\n+\n+test_crlf_subject_body_and_contents log --all --reverse --grep Subject\n+\n+test_crlf_subject_body_and_contents show $LIB_CRLF_BRANCHES\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"408645","messageId":"pull.576.v5.git.1603975709.gitgitgadget@gmail.com","threadId":"52956","inReplyTo":"pull.576.v4.git.1603335680.gitgitgadget@gmail.com","subject":"[PATCH v5 0/2] ref-filter: handle CRLF at end-of-line more gracefully","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-29T12:48:27Z","receivedAt":"2020-10-29T12:48:37Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Changes since v4 :\n\n * tweaked the wording of both commit messages to not call the behaviour a\n bug\n * added || return 1 in one test - that had already been squashed in by\n Junio\n\n\n----------------------------------------------------------------------------\n\nThe ref-filter code does not correctly handle commit or tag messages that\nuse CRLF as the line terminator. Such messages can be created with the\n--cleanup=verbatim option of git commit and git tag, or by using git\ncommit-tree directly.\n\nThe function find_subpos in ref-filter.c looks for two consecutive '\\n' to\nfind the end of the subject line, a sequence which is absent in messages\nusing CRLF. This results in the whole message being parsed as the subject\nline (%(contents:subject)), and the body of the message (%(contents:body))\nbeing empty.\n\nMoreover, in copy_subject, '\\n' is replaced by space, but '\\r' is untouched.\n\nThis behaviour is a regression for git branch --verbose, which bisects down\nto 949af06 (branch: use ref-filter printing APIs, 2017-01-10).\n\nThe first patch hardens the code in ref-filter.c to handle these messages\nmore gracefully and adds a test script to check that the ref-filter and\npretty APIs deal correctly with CRLF messages.\n\nThe second patch adds tests that check the behaviour of git log andgit show \nin the presence of CRLF in messages, to prevent futur regressions.\n\nCc: Michael J Gruber git@grubix.eu [git@grubix.eu], Matthieu Moy \ngit@matthieu-moy.fr [git@matthieu-moy.fr], John Keeping john@keeping.me.uk\n[john@keeping.me.uk], Karthik Nayak karthik.188@gmail.com\n[karthik.188@gmail.com], Jeff King peff@peff.net [peff@peff.net], Alex\nHenrie alexhenrie24@gmail.com [alexhenrie24@gmail.com]cc: Eric Sunshine \nsunshine@sunshineco.com [sunshine@sunshineco.com]cc: Junio C Hamano \ngitster@pobox.com [gitster@pobox.com]\n\nPhilippe Blain (2):\n ref-filter: handle CRLF at end-of-line more gracefully\n log, show: add tests for messages containing CRLF\n\n ref-filter.c | 36 ++++++-----\n t/t3920-crlf-messages.sh | 126 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 148 insertions(+), 14 deletions(-)\n create mode 100755 t/t3920-crlf-messages.sh\n\n\nbase-commit: a5fa49ff0a8f3252c6bff49f92b85e7683868f8a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-576%2Fphil-blain%2Ffix-branch-verbose-crlf-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-576/phil-blain/fix-branch-verbose-crlf-v5\nPull-Request: https://github.com/gitgitgadget/git/pull/576\n\nRange-diff vs v4:\n\n 1: 03b2d7d78a ! 1: 06231c315f ref-filter: handle CRLF at end-of-line more gracefully\n @@ Commit message\n This impacts the output of `git branch`, `git tag` and `git\n for-each-ref`.\n \n - This bug is a regression for `git branch --verbose`, which\n + This behaviour is a regression for `git branch --verbose`, which\n bisects down to 949af0684c (branch: use ref-filter printing APIs,\n 2017-01-10).\n \n - Fix this bug in ref-filter by hardening the logic in `copy_subject` and\n - `find_subpos` to correctly parse messages containing CRLF.\n + Adjust the ref-filter code to be more lenient by hardening the logic in\n + `copy_subject` and `find_subpos` to correctly parse messages containing\n + CRLF.\n \n Add a new test script, 't3920-crlf-messages.sh', to test the behaviour\n of commands using either the ref-filter or the pretty APIs with messages\n 2: 75a87887be ! 2: f536fee695 log, show: add tests for messages containing CRLF\n @@ Metadata\n ## Commit message ##\n log, show: add tests for messages containing CRLF\n \n - A previous commit fixed a bug in ref-filter.c causing messages\n - containing CRLF to be incorrectly parsed and displayed.\n + A previous commit adjusted the code in ref-filter.c so that messages\n + containing CRLF are now correctly parsed and displayed.\n \n Add tests to also check that `git log` and `git show` correctly handle\n such messages, to prevent futur regressions if these commands are\n @@ t/t3920-crlf-messages.sh: test_crlf_subject_body_and_contents tag --list tag-crl\n +\t\tcut -d\" \" -f2- actual-branch &&\n +\t\tcut -d\" \" -f2- actual-tag &&\n +\t\ttest_cmp expect actual-branch &&\n -+\t\ttest_cmp expect actual-tag\n ++\t\ttest_cmp expect actual-tag || return 1\n +\tdone\n +'\n +\n\n-- \ngitgitgadget\n"}]}