[PATCH] Add testcase for merging in a CRLF repo, showing that conflict file is in LF only

An LF only conflict file results in the resolved file being in LF,
the commit is in LF and a warning saying that LF will be replaced
by CRLF, and the working dir ends up with a mix of CRLF and LF files.

Signed-off-by: Marius Storm-Olsen <redacted>
---
 (Resend due to "git reset --hard initial" instead of "git reset
 --hard a", in the first testcase)
 
 Sorry, no patch to actually *fix* the problem.
 Someone who knows the code in question will probably find the solution in a
 fraction of the time that I would.
 Also note that :1:file, :2:file and :3:file all are also in LF format, and not
 CRLF, which you would want if core.autocrlf == true.

 t/t6033-merge-crlf.sh |   52 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 52 insertions(+), 0 deletions(-)
 create mode 100755 t/t6033-merge-crlf.sh

diff --git a/t/t6033-merge-crlf.sh b/t/t6033-merge-crlf.sh
new file mode 100755
index 0000000..8bff2f4
--- /dev/null
+++ b/t/t6033-merge-crlf.sh

@@ -0,0 +1,52 @@
+#!/bin/sh
+
+append_cr () {
+	sed -e 's/$/Q/' | tr Q '\015'
+}
+
+remove_cr () {
+	tr '\015' Q | sed -e 's/Q$//'
+}
+
+test_description='merge conflict in crlf repo
+
+		b---M
+	       /   /
+	initial---a
+
+'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	git config core.autocrlf true &&
+	echo foo | append_cr >file &&
+	git add file &&
+	git commit -m "Initial" &&
+	git tag initial &&
+	git branch side &&
+	echo line from a | append_cr >file &&
+	git commit -m "add line from a" file &&
+	git tag a &&
+	git checkout side &&
+	echo line from b | append_cr >file &&
+	git commit -m "add line from b" file &&
+	git tag b &&
+	git checkout master
+'
+
+test_expect_success 'Check "ours" is CRLF' '
+	git reset --hard a &&
+	git merge side -s ours &&
+	cat file | remove_cr | append_cr >file.temp &&
+	test_cmp file file.temp
+'
+
+test_expect_success 'Check that conflict file is CRLF' '
+	git reset --hard a &&
+	! git merge side &&
+	cat file | remove_cr | append_cr >file.temp &&
+	test_cmp file file.temp
+'
+
+test_done

-- 
1.5.6.rc0.162.gaeac2.dirty

Marius Storm-Olsen schrieb:

An LF only conflict file results in the resolved file being in LF,
the commit is in LF and a warning saying that LF will be replaced
by CRLF, and the working dir ends up with a mix of CRLF and LF files.

After reading these 3 lines I've no idea what you are talking about. Can
you translate this to English, please? ;-)

 Sorry, no patch to actually *fix* the problem.

Then you should use test_expect_failure instead of test_expect_success.
And maybe also mention it in the commit message.

+test_expect_success 'Check that conflict file is CRLF' '
+	git reset --hard a &&
+	! git merge side &&

	test_must_fail git merge side &&

+	cat file | remove_cr | append_cr >file.temp &&
+	test_cmp file file.temp
+'

-- Hannes

Johannes Sixt said the following on 09.06.2008 15:37:

Marius Storm-Olsen schrieb:

quoted

An LF only conflict file results in the resolved file being in LF,
the commit is in LF and a warning saying that LF will be replaced
by CRLF, and the working dir ends up with a mix of CRLF and LF files.

After reading these 3 lines I've no idea what you are talking about. Can
you translate this to English, please? ;-)

Certainly :-)
It means that if you work on a repo with core.autocrlf == true, you'd 
expect every text file to have CRLF EOLs. However, if you by some 
operation, get a conflict, then the conflicted file has LF EOLs.
Now, of course you'd go about resolving the files conflict, and then 
'git add <file>'. When you do that, you'll get the warning saying that 
LF will be replaced by CRLF. Then you commit. The end result is that 
you have a workingdir with a mix of LF and CRLF files, which after 
some more operations may trigger a "whole file changed" diff, due to 
the workingdir file now having LF EOLs.

quoted

 Sorry, no patch to actually *fix* the problem.

Then you should use test_expect_failure instead of test_expect_success.
And maybe also mention it in the commit message.

Well, the test case is written in a way that it *should* pass (iow, it 
_expects_ a success), but it currently doesn't. So, the goal is that 
someone, who is more intimate with the code, can just run the testcase 
until it passes (fixing in between each run, of course ;-)

quoted

+test_expect_success 'Check that conflict file is CRLF' '
+	git reset --hard a &&
+	! git merge side &&

	test_must_fail git merge side &&

Ah, I checked a few other testcases, where I saw the ! construct. I 
don't mind changing it, if it's important. Does it add 'feature' to 
the testcase by using test_must_fail, instead of '!' ?

quoted

+	cat file | remove_cr | append_cr >file.temp &&
+	test_cmp file file.temp
+'

-- Hannes

Thanks

--
.marius

Marius Storm-Olsen schrieb:

Johannes Sixt said the following on 09.06.2008 15:37:

quoted

Marius Storm-Olsen schrieb:

quoted

An LF only conflict file results in the resolved file being in LF,
the commit is in LF and a warning saying that LF will be replaced
by CRLF, and the working dir ends up with a mix of CRLF and LF files.

After reading these 3 lines I've no idea what you are talking about. Can
you translate this to English, please? ;-)

Certainly :-)
It means that if you work on a repo with core.autocrlf == true, you'd
expect every text file to have CRLF EOLs. However, if you by some
operation, get a conflict, then the conflicted file has LF EOLs.
Now, of course you'd go about resolving the files conflict, and then
'git add <file>'. When you do that, you'll get the warning saying that
LF will be replaced by CRLF. Then you commit. The end result is that you
have a workingdir with a mix of LF and CRLF files, which after some more
operations may trigger a "whole file changed" diff, due to the
workingdir file now having LF EOLs.

Aha! Care to write it this way in the commit message in the next round? ;)

quoted

quoted

 Sorry, no patch to actually *fix* the problem.

Then you should use test_expect_failure instead of test_expect_success.
And maybe also mention it in the commit message.

Well, the test case is written in a way that it *should* pass (iow, it
_expects_ a success), but it currently doesn't. So, the goal is that
someone, who is more intimate with the code, can just run the testcase
until it passes (fixing in between each run, of course ;-)

test_expect_failure has changed its meaning. It's now used to say precisly
what you describe here.

It means: "We should expect this command sequence to complete
successfully, but we know that there is a bug in a git command, and hence
we must expect failure until it is fixed."

Such a test is marked as "still broken", and the test run is not
interrupted. If the bug is fixed, the test is marked as "FIXED" until the
'test_expect_failure' is turned into 'test_expect_success'.

quoted

quoted

+test_expect_success 'Check that conflict file is CRLF' '
+    git reset --hard a &&
+    ! git merge side &&

    test_must_fail git merge side &&

Ah, I checked a few other testcases, where I saw the ! construct. I
don't mind changing it, if it's important. Does it add 'feature' to the
testcase by using test_must_fail, instead of '!' ?

'! git cmd' says that any unusual exit is ok, even a segfault and
incorrect usage. 'test_must_fail git cmd' says that only deliberate error
exits are ok.

-- Hannes

Marius Storm-Olsen said the following on 09.06.2008 16:46:

... Then you commit. The end result is that 
you have a workingdir with a mix of LF and CRLF files, which after 
some more operations may trigger a "whole file changed" diff, due to 
the workingdir file now having LF EOLs.

..actually, it's more like applying patches and cherry-picking then 
breaks when the touch the same file, if I recall correctly.

Thanks for all the pointers, I'll send an updated patch tomorrow.

--
.marius

From: Marius Storm-Olsen <redacted>

If you work on a repo with core.autocrlf == true, you would expect
every text file to have CRLF EOLs. However, if you by some operation,
get a conflict, then the conflicted file has LF EOLs.

Now, of course you'd go about resolving the files conflict, and then 'git
add <file>'. When you do that, you'll get the warning saying that LF will
be replaced by CRLF. Then you commit. The end result is that you have a
workingdir with a mix of LF and CRLF files, which after some more
operations may trigger a "whole file changed" diff, due to the workingdir
file now having LF EOLs.

An LF only conflict file results in the resolved file being in LF,
the commit is in LF and a warning saying that LF will be replaced
by CRLF, and the working dir ends up with a mix of CRLF and LF files.

Signed-off-by: Marius Storm-Olsen <redacted>
Signed-off-by: Johannes Schindelin <redacted>
---
 t/t6033-merge-crlf.sh |   52 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 52 insertions(+), 0 deletions(-)
 create mode 100755 t/t6033-merge-crlf.sh

diff --git a/t/t6033-merge-crlf.sh b/t/t6033-merge-crlf.sh
new file mode 100755
index 0000000..ea22837
--- /dev/null
+++ b/t/t6033-merge-crlf.sh

@@ -0,0 +1,52 @@
+#!/bin/sh
+
+append_cr () {
+	sed -e 's/$/Q/' | tr Q '\015'
+}
+
+remove_cr () {
+	tr '\015' Q | sed -e 's/Q$//'
+}
+
+test_description='merge conflict in crlf repo
+
+		b---M
+	       /   /
+	initial---a
+
+'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	git config core.autocrlf true &&
+	echo foo | append_cr >file &&
+	git add file &&
+	git commit -m "Initial" &&
+	git tag initial &&
+	git branch side &&
+	echo line from a | append_cr >file &&
+	git commit -m "add line from a" file &&
+	git tag a &&
+	git checkout side &&
+	echo line from b | append_cr >file &&
+	git commit -m "add line from b" file &&
+	git tag b &&
+	git checkout master
+'
+
+test_expect_success 'Check "ours" is CRLF' '
+	git reset --hard initial &&
+	git merge side -s ours &&
+	cat file | remove_cr | append_cr >file.temp &&
+	test_cmp file file.temp
+'
+
+test_expect_failure 'Check that conflict file is CRLF' '
+	git reset --hard a &&
+	test_must_fail git merge side &&
+	cat file | remove_cr | append_cr >file.temp &&
+	test_cmp file file.temp
+'
+
+test_done

-- 
1.5.6.rc1.181.gb439d

Signed-off-by: Johannes Schindelin <redacted>
---
 builtin-merge-recursive.c |    8 ++++++++
 t/t6033-merge-crlf.sh     |    2 +-
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c
index 7643f17..edd023f 100644
--- a/builtin-merge-recursive.c
+++ b/builtin-merge-recursive.c

@@ -525,6 +525,7 @@ static void update_file_flags(const unsigned char *sha,
 		enum object_type type;
 		void *buf;
 		unsigned long size;
+		struct strbuf strbuf;
 
 		if (S_ISGITLINK(mode))
 			die("cannot read object %s '%s': It is a submodule!",

@@ -535,6 +536,12 @@ static void update_file_flags(const unsigned char *sha,
 			die("cannot read object %s '%s'", sha1_to_hex(sha), path);
 		if (type != OBJ_BLOB)
 			die("blob expected for %s '%s'", sha1_to_hex(sha), path);
+		strbuf_init(&strbuf, 0);
+		if (convert_to_working_tree(path, buf, size, &strbuf)) {
+			free(buf);
+			size = strbuf.len;
+			buf = strbuf_detach(&strbuf, NULL);
+		}
 
 		if (make_room_for_path(path) < 0) {
 			update_wd = 0;

@@ -560,6 +567,7 @@ static void update_file_flags(const unsigned char *sha,
 		} else
 			die("do not know what to do with %06o %s '%s'",
 			    mode, sha1_to_hex(sha), path);
+		free(buf);
 	}
  update_index:
 	if (update_cache)

diff --git a/t/t6033-merge-crlf.sh b/t/t6033-merge-crlf.sh
index ea22837..75d9602 100755
--- a/t/t6033-merge-crlf.sh
+++ b/t/t6033-merge-crlf.sh

@@ -42,7 +42,7 @@ test_expect_success 'Check "ours" is CRLF' '
 	test_cmp file file.temp
 '
 
-test_expect_failure 'Check that conflict file is CRLF' '
+test_expect_success 'Check that conflict file is CRLF' '
 	git reset --hard a &&
 	test_must_fail git merge side &&
 	cat file | remove_cr | append_cr >file.temp &&

-- 
1.5.6.rc1.181.gb439d