git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v2 2/3] ref-filter: fix the API to correctly handle CRLF

From
Philippe Blain via GitGitGadget <gitgitgadget@gmail.com>
Date
Mar 10, 2020, 02:24 UTC
Message-ID
<aab1f45ba976d088a8c68573a21ed2458915d6a6.1583807093.git.gitgitgadget@gmail.com>
In-Reply-To
<pull.576.v2.git.1583807093.gitgitgadget@gmail.com>
From: Philippe Blain <levraiphilippeblain@gmail.com>

The ref-filter API does not correctly handle commit or tag messages that use CRLF as the line terminator. Such messages can be created with the `--verbatim` option of `git commit` and `git tag`, or by using `git commit-tree` directly.

This impacts the output `git branch`, `git tag` and `git for-each-ref` when used with a `--format` argument containing the atoms `%(contents:subject)` or `%(contents:body)`, as well as the output of `git branch --verbose`, which uses `%(contents:subject)` internally.

The function find_subpos in ref-filter.c looks for two consecutive '\n' to find the end of the subject line, a sequence which is absent in messages using CRLF. This results in the whole message being parsed as the subject line (`%(contents:subject)`), and the body of the message (`%(contents:body)`) being empty.

Moreover, in copy_subject, '\n' is replaced by space, but '\r' is untouched, resulting in the escape sequence '^M' being output verbatim in most terminal emulators:

    $ git branch --verbose
    * crlf    2113b0e Subject first line^M ^M Body first line^M Body second line

This bug is a regression for `git branch --verbose`, which bisects down to 949af0684c (branch: use ref-filter printing APIs, 2017-01-10).

Fix this bug in ref-filter by hardening the logic in `copy_subject` and `find_subpos` to correctly parse messages containing CRFL.

Add tests for `branch`, `tag` and `for-each-ref` using lib-crlf-messages.sh.

The 'make commits' test at the beginning of t3203-branch-output.sh needs to be modified since it did not use `test_tick` and thus the commit hashes were not reproducible. For simplicity, use `test_commit` as the content and name of the files created in this setup test are irrelevant to the rest of the test script.

`test_cleanup_crlf_refs` is used in t3203-branch-output.sh and t7004-tag.sh to avoid having to modify the expected output in later tests.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 ref-filter.c             | 19 +++++++++++++++++--
 t/t3203-branch-output.sh | 26 +++++++++++++++++++++-----
 t/t6300-for-each-ref.sh  |  5 +++++
 t/t7004-tag.sh           |  7 +++++++
 4 files changed, 50 insertions(+), 7 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 79bb5206783..537cc4de42c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1050,10 +1050,18 @@ static char *copy_subject(const char *buf, unsigned long len)
 {
 	char *r = xmemdupz(buf, len);
 	int i;
+	struct strbuf sb = STRBUF_INIT;
 
-	for (i = 0; i < len; i++)
+	strbuf_attach(&sb, r, len, len + 1);
+	for (i = 0; i < sb.len; i++) {
 		if (r[i] == '\n')
 			r[i] = ' ';
+		if (r[i] == '\r') {
+			strbuf_remove(&sb, i, 1);
+			i -= 1;
+		}
+	}
+	strbuf_detach(&sb, NULL);
 
 	return r;
 }
@@ -1184,15 +1192,22 @@ static void find_subpos(const char *buf,
 		eol = strchrnul(buf, '\n');
 		if (*eol)
 			eol++;
+		/*  protect against messages that might contain \r\n */
+		if (*eol == '\r')
+			eol++;
 		buf = eol;
 	}
 	*sublen = buf - *sub;
 	/* drop trailing newline, if present */
 	if (*sublen && (*sub)[*sublen - 1] == '\n')
 		*sublen -= 1;
+	/*  protect against commit messages that might contain \r\n */
+	else if (*sublen && (*sub)[*sublen - 1] == '\r')
+		*sublen -= 3; /* drop '\r\n\r' */
 
 	/* skip any empty lines */
-	while (*buf == '\n')
+	/* and protect against commit messages that might contain \r\n */
+	while (*buf == '\n' || *buf == '\r')
 		buf++;
 	*body = buf;
 	*bodylen = strlen(buf);
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index 71818b90f00..1235089619c 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -3,13 +3,11 @@
 test_description='git branch display tests'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
+. "$TEST_DIRECTORY"/lib-crlf-messages.sh
 
 test_expect_success 'make commits' '
-	echo content >file &&
-	git add file &&
-	git commit -m one &&
-	echo content >>file &&
-	git commit -a -m two
+	test_commit one &&
+	test_commit two
 '
 
 test_expect_success 'make branches' '
@@ -95,6 +93,24 @@ test_expect_success 'git branch --ignore-case --list -v pattern shows branch sum
 	awk "{print \$NF}" <tmp >actual &&
 	test_cmp expect actual
 '
+test_create_crlf_refs
+
+test_expect_success 'git branch -v works with CRLF input' '
+	cat >expect <<-EOF &&
+	  branch-one                     139b20d two
+	  branch-two                     d79ce16 one
+	  crlf                           2113b0e Subject first line
+	  crlf-empty-lines-after-subject 0a9530d Subject first line
+	  crlf-two-line-subject          f9ded1f Subject first line Subject second line
+	* master                         139b20d two
+	EOF
+	git branch -v >actual &&
+	test_cmp expect actual
+'
+
+test_crlf_subject_body_and_contents branch --list crlf*
+
+test_cleanup_crlf_refs
 
 test_expect_success 'git branch -v pattern does not show branch summaries' '
 	test_must_fail git branch -v branch*
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 9c910ce7467..467924de3df 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -8,6 +8,7 @@ test_description='for-each-ref test'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-gpg.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
+. "$TEST_DIRECTORY"/lib-crlf-messages.sh
 
 # Mon Jul 3 23:18:43 2006 +0000
 datestamp=1151968723
@@ -888,4 +889,8 @@ test_expect_success 'for-each-ref --ignore-case ignores case' '
 	test_cmp expect actual
 '
 
+test_create_crlf_refs
+
+test_crlf_subject_body_and_contents for-each-ref refs/heads/crlf*
+
 test_done
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 6db92bd3ba6..25f465f34ea 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -10,6 +10,7 @@ Tests for operations with tags.'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-gpg.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
+. "$TEST_DIRECTORY"/lib-crlf-messages.sh
 
 # creating and listing lightweight tags:
 
@@ -1969,6 +1970,12 @@ test_expect_success '--format should list tags as per format given' '
 	test_cmp expect actual
 '
 
+test_create_crlf_refs
+
+test_crlf_subject_body_and_contents tag --list tag-crlf*
+
+test_cleanup_crlf_refs
+
 test_expect_success "set up color tests" '
 	echo "<RED>v1.0<RESET>" >expect.color &&
 	echo "v1.0" >expect.bare &&

-- 
gitgitgadget
Previous: Philippe Blain via GitGitGadgetNext: Junio C Hamano
Message 9 of 38 in “Teach ref-filter API to correctly handle CRLF in messages”
  1. 0/3 Teach ref-filter API to correctly handle CRLF in messagesPhilippe Blain via GitGitGadget, Mar 8, 2020
  2. 3/3 log: add tests for messages containing CRLF to t4202Philippe Blain via GitGitGadget, Mar 8, 2020
  3. 1/3 t: add lib-crlf-messages.sh for messages containing CRLFPhilippe Blain via GitGitGadget, Mar 8, 2020
  4. 2/3 ref-filter: teach the API to correctly handle CRLFPhilippe Blain via GitGitGadget, Mar 8, 2020
  5. Junio C HamanoMar 9, 2020
  6. Philippe BlainMar 10, 2020
  7. 0/3 Teach ref-filter API to correctly handle CRLF in messagesPhilippe Blain via GitGitGadget, Mar 10, 2020
  8. 1/3 t: add lib-crlf-messages.sh for messages containing CRLFPhilippe Blain via GitGitGadget, Mar 10, 2020
  9. 2/3 ref-filter: fix the API to correctly handle CRLFPhilippe Blain via GitGitGadget, Mar 10, 2020
  10. Junio C HamanoMar 10, 2020
  11. 3/3 log: add tests for messages containing CRLF to t4202Philippe Blain via GitGitGadget, Mar 10, 2020
  12. Junio C HamanoMar 10, 2020
  13. Junio C HamanoMar 10, 2020
  14. 0/3 ref-filter: handle CRLF at end-of-line more gracefullyPhilippe Blain via GitGitGadget, Oct 12, 2020
  15. 2/3 ref-filter: handle CRLF at end-of-line more gracefullyPhilippe Blain via GitGitGadget, Oct 12, 2020
  16. Junio C HamanoOct 12, 2020
  17. Philippe BlainOct 14, 2020
  18. 1/3 t: add lib-crlf-messages.sh for messages containing CRLFPhilippe Blain via GitGitGadget, Oct 12, 2020
  19. Junio C HamanoOct 12, 2020
  20. Philippe BlainOct 14, 2020
  21. Eric SunshineOct 12, 2020
  22. Philippe BlainOct 14, 2020
  23. Eric SunshineOct 14, 2020
  24. Philippe BlainOct 14, 2020
  25. Eric SunshineOct 14, 2020
  26. Philippe BlainOct 22, 2020
  27. 3/3 log, show: add tests for messages containing CRLFPhilippe Blain via GitGitGadget, Oct 12, 2020
  28. 0/2 ref-filter: handle CRLF at end-of-line more gracefullyPhilippe Blain via GitGitGadget, Oct 22, 2020
  29. 2/2 log, show: add tests for messages containing CRLFPhilippe Blain via GitGitGadget, Oct 22, 2020
  30. Philippe BlainOct 22, 2020
  31. 1/2 ref-filter: handle CRLF at end-of-line more gracefullyPhilippe Blain via GitGitGadget, Oct 22, 2020
  32. Junio C HamanoOct 23, 2020
  33. Philippe BlainOct 23, 2020
  34. Junio C HamanoOct 28, 2020
  35. Philippe BlainOct 29, 2020
  36. 0/2 ref-filter: handle CRLF at end-of-line more gracefullyPhilippe Blain via GitGitGadget, Oct 29, 2020
  37. 1/2 ref-filter: handle CRLF at end-of-line more gracefullyPhilippe Blain via GitGitGadget, Oct 29, 2020
  38. 2/2 log, show: add tests for messages containing CRLFPhilippe Blain via GitGitGadget, Oct 29, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.