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

[PATCH v2 2/2] File commited with CRLF should roundtrip diff and apply

From
tboegi@web.de <tboegi@web.de>
Date
Aug 17, 2017, 06:06 UTC
Message-ID
<20170817060633.28491-1-tboegi@web.de>
In-Reply-To
<xmqqa832vymw.fsf@gitster.mtv.corp.google.com>
From: Torsten Bögershausen <tboegi@web.de>

When a file had been commited with CRLF but now .gitattributes say "* text=auto" (or core.autocrlf is true), the following does not roundtrip, `git apply` fails:

printf "Added line\r\n" >>file && git diff >patch && git checkout -- . && git apply patch

Before applying the patch, the file from working tree is converted into the index format (clean filter, CRLF conversion, ...) Here, when commited with CRLF, the line endings should not be converted.

Note that `git apply --index` or `git apply --cache` doesn't call convert_to_git() because the source material is already in index format.

Analyze the patch if there is a) any context line with CRLF, or b) if any line with CRLF is to be removed. In this case the patch file `patch` has mixed line endings, for a) it looks like this (ignore the $ at the begin of the line):

$ diff --git a/one b/one $ index 533790e..c30dea8 100644 $ --- a/one $ +++ b/one $ @@ -1 +1,2 @@ $ a\r $ +b\r

And for b) it looks like this:

$ diff --git a/one b/one $ index 533790e..485540d 100644 $ --- a/one $ +++ b/one $ @@ -1 +1 @@ $ -a\r $ +b\r

If `git apply` detects that the patch itself has CRLF, (look at the line
" a\r" or "-a\r" above), the new flag has_crlf is set in "struct patch"
and two things will happen:
- read_old_data() will not convert CRLF into LF by calling
  convert_to_git(..., SAFE_CRLF_KEEP_CRLF);
- The WS_CR_AT_EOL bit is set in the "white space rule",
  CRLF are no longer treated as white space.
Thanks to Junio C Hamano, his input became the base for the changes in t4124.
One test case is split up into 3:
- Detect the " a\r" line in the patch
- Detect the "-a\r" line in the patch
- Use LF in repo and CLRF in the worktree. (*)
* This one proves that convert_to_git(&the_index,...) still needs to pass
the &index, otherwise Git will crash.
Reported-by: Anthony Sottile <asottile@umich.edu>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 apply.c                  | 28 +++++++++++++++++++++++-----
 t/t4124-apply-ws-rule.sh | 33 +++++++++++++++++++++++++++------
 2 files changed, 50 insertions(+), 11 deletions(-)
diff --git a/apply.c b/apply.c
index f2d599141d..bebb176099 100644
--- a/apply.c
+++ b/apply.c
@@ -220,6 +220,7 @@ struct patch {
 	unsigned int recount:1;
 	unsigned int conflicted_threeway:1;
 	unsigned int direct_to_threeway:1;
+	unsigned int has_crlf:1;
 	struct fragment *fragments;
 	char *result;
 	size_t resultsize;
@@ -1662,6 +1663,17 @@ static void check_whitespace(struct apply_state *state,
 	record_ws_error(state, result, line + 1, len - 2, state->linenr);
 }
 
+/* Check if the patch has context lines with CRLF or
+   the patch wants to remove lines with CRLF */
+static void check_old_for_crlf(struct patch *patch, const char *line, int len)
+{
+	if (len >= 2 && line[len-1] == '\n' && line[len-2] == '\r') {
+		patch->ws_rule |= WS_CR_AT_EOL;
+		patch->has_crlf = 1;
+	}
+}
+
+
 /*
  * Parse a unified diff. Note that this really needs to parse each
  * fragment separately, since the only way to know the difference
@@ -1712,11 +1724,13 @@ static int parse_fragment(struct apply_state *state,
 			if (!deleted && !added)
 				leading++;
 			trailing++;
+			check_old_for_crlf(patch, line, len);
 			if (!state->apply_in_reverse &&
 			    state->ws_error_action == correct_ws_error)
 				check_whitespace(state, line, len, patch->ws_rule);
 			break;
 		case '-':
+			check_old_for_crlf(patch, line, len);
 			if (state->apply_in_reverse &&
 			    state->ws_error_action != nowarn_ws_error)
 				check_whitespace(state, line, len, patch->ws_rule);
@@ -2268,8 +2282,11 @@ static void show_stats(struct apply_state *state, struct patch *patch)
 		add, pluses, del, minuses);
 }
 
-static int read_old_data(struct stat *st, const char *path, struct strbuf *buf)
+static int read_old_data(struct stat *st, struct patch *patch,
+			 const char *path, struct strbuf *buf)
 {
+	enum safe_crlf safe_crlf = patch->has_crlf ?
+		SAFE_CRLF_KEEP_CRLF : SAFE_CRLF_FALSE;
 	switch (st->st_mode & S_IFMT) {
 	case S_IFLNK:
 		if (strbuf_readlink(buf, path, st->st_size) < 0)
@@ -2278,7 +2295,7 @@ static int read_old_data(struct stat *st, const char *path, struct strbuf *buf)
 	case S_IFREG:
 		if (strbuf_read_file(buf, path, st->st_size) != st->st_size)
 			return error(_("unable to open or read %s"), path);
-		convert_to_git(&the_index, path, buf->buf, buf->len, buf, 0);
+		convert_to_git(&the_index, path, buf->buf, buf->len, buf, safe_crlf);
 		return 0;
 	default:
 		return -1;
@@ -3384,6 +3401,7 @@ static int load_patch_target(struct apply_state *state,
 			     struct strbuf *buf,
 			     const struct cache_entry *ce,
 			     struct stat *st,
+			     struct patch *patch,
 			     const char *name,
 			     unsigned expected_mode)
 {
@@ -3399,7 +3417,7 @@ static int load_patch_target(struct apply_state *state,
 		} else if (has_symlink_leading_path(name, strlen(name))) {
 			return error(_("reading from '%s' beyond a symbolic link"), name);
 		} else {
-			if (read_old_data(st, name, buf))
+			if (read_old_data(st, patch, name, buf))
 				return error(_("failed to read %s"), name);
 		}
 	}
@@ -3432,7 +3450,7 @@ static int load_preimage(struct apply_state *state,
 		/* We have a patched copy in memory; use that. */
 		strbuf_add(&buf, previous->result, previous->resultsize);
 	} else {
-		status = load_patch_target(state, &buf, ce, st,
+		status = load_patch_target(state, &buf, ce, st, patch,
 					   patch->old_name, patch->old_mode);
 		if (status < 0)
 			return status;
@@ -3520,7 +3538,7 @@ static int load_current(struct apply_state *state,
 	if (verify_index_match(ce, &st))
 		return error(_("%s: does not match index"), name);
 
-	status = load_patch_target(state, &buf, ce, &st, name, mode);
+	status = load_patch_target(state, &buf, ce, &st, patch, name, mode);
 	if (status < 0)
 		return status;
 	else if (status)
diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
index d350065f25..4fc27c51f7 100755
--- a/t/t4124-apply-ws-rule.sh
+++ b/t/t4124-apply-ws-rule.sh
@@ -467,21 +467,42 @@ test_expect_success 'same, but with CR-LF line endings && cr-at-eol set' '
 	test_cmp one expect
 '
 
-test_expect_success 'same, but with CR-LF line endings && cr-at-eol unset' '
+test_expect_success 'CR-LF line endings && add line && text=auto' '
 	git config --unset core.whitespace &&
 	printf "a\r\n" >one &&
+	cp one save-one &&
+	git add one &&
 	printf "b\r\n" >>one &&
-	printf "c\r\n" >>one &&
+	cp one expect &&
+	git diff -- one >patch &&
+	mv save-one one &&
+	echo "one text=auto" >.gitattributes &&
+	git apply patch &&
+	test_cmp one expect
+'
+
+test_expect_success 'CR-LF line endings && change line && text=auto' '
+	printf "a\r\n" >one &&
 	cp one save-one &&
-	printf "                 \r\n" >>one &&
 	git add one &&
+	printf "b\r\n" >one &&
 	cp one expect &&
-	printf "d\r\n" >>one &&
 	git diff -- one >patch &&
 	mv save-one one &&
-	echo d >>expect &&
+	echo "one text=auto" >.gitattributes &&
+	git apply patch &&
+	test_cmp one expect
+'
 
-	git apply --ignore-space-change --whitespace=fix patch &&
+test_expect_success 'LF in repo, CRLF in worktree && change line && text=auto' '
+	printf "a\n" >one &&
+	git add one &&
+	printf "b\r\n" >one &&
+	git diff -- one >patch &&
+	printf "a\r\n" >one &&
+	echo "one text=auto" >.gitattributes &&
+	git -c core.eol=CRLF apply patch &&
+	printf "b\r\n" >expect &&
 	test_cmp one expect
 '
 

-- 
2.14.1.145.gb3622a4ee9
Previous: tboegi@web.deNext: Junio C Hamano
Message 17 of 36 in “core.autocrlf=true causes `git apply` to fail on patch generated with `git diff-index HEAD --patch`”
  1. Anthony SottileAug 1, 2017
  2. Torsten BögershausenAug 1, 2017
  3. Anthony SottileAug 1, 2017
  4. Torsten BögershausenAug 2, 2017
  5. 1/1 correct apply for files commited with CRLFtboegi@web.de, Aug 2, 2017
  6. Junio C HamanoAug 2, 2017
  7. Torsten BögershausenAug 4, 2017
  8. Junio C HamanoAug 4, 2017
  9. Junio C HamanoAug 4, 2017
  10. Junio C HamanoAug 2, 2017
  11. Torsten BögershausenAug 12, 2017
  12. Torsten BögershausenAug 12, 2017
  13. convert: Add SAFE_CRLF_KEEP_CRLFtboegi@web.de, Aug 12, 2017
  14. File commited with CRLF should roundtrip diff and applytboegi@web.de, Aug 12, 2017
  15. Junio C HamanoAug 14, 2017
  16. 1/2 convert: Add SAFE_CRLF_KEEP_CRLFtboegi@web.de, Aug 17, 2017
  17. 2/2 File commited with CRLF should roundtrip diff and applytboegi@web.de, Aug 17, 2017
  18. Junio C HamanoAug 17, 2017
  19. 1/2 convert: Add SAFE_CRLF_KEEP_CRLFtboegi@web.de, Aug 17, 2017
  20. 2/2 File commited with CRLF should roundtrip diff and applytboegi@web.de, Aug 17, 2017
  21. Junio C HamanoAug 17, 2017
  22. Junio C HamanoAug 17, 2017
  23. 1/2 convert: Add SAFE_CRLF_KEEP_CRLFtboegi@web.de, Aug 19, 2017
  24. 2/2 File commited with CRLF should roundtrip diff and applytboegi@web.de, Aug 19, 2017
  25. 1/2 convert: Add SAFE_CRLF_KEEP_CRLFtboegi@web.de, Aug 13, 2017
  26. 2/2 File commited with CRLF should roundtrip diff and applytboegi@web.de, Aug 13, 2017
  27. 3/2 apply: remove unused member apply_state.flagsJunio C Hamano, Aug 16, 2017
  28. Junio C HamanoAug 16, 2017
  29. 4/2 apply: only pay attention to CRLF in the preimageJunio C Hamano, Aug 16, 2017
  30. 5/2 apply: localize the CRLF business to read_old_data()Junio C Hamano, Aug 16, 2017
  31. 6/2 apply: clarify read_old_data() is about no-index caseJunio C Hamano, Aug 16, 2017
  32. Torsten BögershausenAug 17, 2017
  33. Junio C HamanoAug 17, 2017
  34. Junio C HamanoAug 17, 2017
  35. Torsten BögershausenAug 17, 2017
  36. Junio C HamanoAug 17, 2017

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.