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

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

[Marius Storm-Olsen]

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 <marius@trolltech.com>
---
 (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

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

[Marius Storm-Olsen]

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 <marius@trolltech.com>
---
 (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

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

[Johannes Sixt]

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

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

[Marius Storm-Olsen]

Johannes Sixt said the following on 09.06.2008 15:37:
> 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? ;-)

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.

>>  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_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 '!' ?

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

Thanks

--
.marius

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

[Johannes Sixt]

Marius Storm-Olsen schrieb:
> Johannes Sixt said the following on 09.06.2008 15:37:
>> 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? ;-)
> 
> 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? ;)

>>>  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'.

>>> +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

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

[Marius Storm-Olsen]

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

[PATCH 1/2] Add testcase for merging in a CRLF repo

[Johannes Schindelin]

From: Marius Storm-Olsen <marius@trolltech.com>

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 <marius@trolltech.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 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

[PATCH 2/2] merge-recursive: respect core.autocrlf

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 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

Re: [PATCH 2/2] merge-recursive: respect core.autocrlf

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  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;

Fairly straightforward fix, except that I suspect this needs to be done
only for regular files and not symlinks.

I think entry.c:write_entry() shows how this should be done.

[PATCH v2] merge-recursive: respect core.autocrlf

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Mon, 9 Jun 2008, Junio C Hamano wrote:

	> Fairly straightforward fix, except that I suspect this needs to 
	> be done only for regular files and not symlinks.
	> 
	> I think entry.c:write_entry() shows how this should be done.

	Right.  And the relevant clause is actually already there.  D'oh.

 builtin-merge-recursive.c |   12 +++++++++++-
 t/t6033-merge-crlf.sh     |    2 +-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c
index 7643f17..1fbff3a 100644
--- a/builtin-merge-recursive.c
+++ b/builtin-merge-recursive.c
@@ -535,13 +535,22 @@ 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);
-
 		if (make_room_for_path(path) < 0) {
 			update_wd = 0;
 			goto update_index;
 		}
 		if (S_ISREG(mode) || (!has_symlinks && S_ISLNK(mode))) {
 			int fd;
+			struct strbuf strbuf;
+
+			strbuf_init(&strbuf, 0);
+			if (convert_to_working_tree(path, buf, size, &strbuf)) {
+				size_t newsize = 0;
+				free(buf);
+				buf = strbuf_detach(&strbuf, &newsize);
+				size = newsize;
+			}
+
 			if (mode & 0100)
 				mode = 0777;
 			else
@@ -560,6 +569,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

Re: [PATCH v2] merge-recursive: respect core.autocrlf

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>
> 	On Mon, 9 Jun 2008, Junio C Hamano wrote:
>
> 	> Fairly straightforward fix, except that I suspect this needs to 
> 	> be done only for regular files and not symlinks.
> 	> 
> 	> I think entry.c:write_entry() shows how this should be done.
>
> 	Right.  And the relevant clause is actually already there.  D'oh.

Well, you actually have "double d'oh".  "This ought to be a symlink but
the filesystem is lacking, so we instead write out what the readlink from
such a symlink would return" codepath should not convert_to_worktree().

I'll fix it up, no need to resend.  Thanks for the fix.

Re: [PATCH v2] merge-recursive: respect core.autocrlf

[Johannes Schindelin]

Hi,

On Mon, 9 Jun 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >
> > 	On Mon, 9 Jun 2008, Junio C Hamano wrote:
> >
> > 	> Fairly straightforward fix, except that I suspect this needs to 
> > 	> be done only for regular files and not symlinks.
> > 	> 
> > 	> I think entry.c:write_entry() shows how this should be done.
> >
> > 	Right.  And the relevant clause is actually already there.  D'oh.
> 
> Well, you actually have "double d'oh".  "This ought to be a symlink but 
> the filesystem is lacking, so we instead write out what the readlink 
> from such a symlink would return" codepath should not 
> convert_to_worktree().

I actually thought about that a bit, and just assumed that the rest of the 
Git code respects autocrlf for "fake" symlinks.

IMO it makes no sense at all to write the textual symlink files without 
CR/LF when the user clearly asked for it with autocrlf = true.  After all, 
it _is_ a text file then.

But yes, I tried to save some time and did not check.

Ciao,
Dscho

[PATCH 1/2] Add testcases for verifying that staged files in a conflict are CRLF, when core.autocrlf = true

[Marius Storm-Olsen]

When you 'git show :2:<file>' in a conflict, the file should have CRLF EOLs,
if the repo is configured with core.autocrlf = true.

Signed-off-by: Marius Storm-Olsen <marius@trolltech.com>
---
 t/t6033-merge-crlf.sh |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/t/t6033-merge-crlf.sh b/t/t6033-merge-crlf.sh
index 75d9602..f161b40 100755
--- a/t/t6033-merge-crlf.sh
+++ b/t/t6033-merge-crlf.sh
@@ -49,4 +49,22 @@ test_expect_success 'Check that conflict file is CRLF' '
 	test_cmp file file.temp
 '
 
+test_expect_failure 'Check that staged file :1: is CRLF' '
+	git show :1:file >staged.temp1 &&
+	git show :1:file | remove_cr | append_cr >staged.temp2 &&
+	test_cmp staged.temp1 staged.temp2
+'
+
+test_expect_failure 'Check that staged file :2: is CRLF' '
+	git show :2:file >staged.temp1 &&
+	git show :2:file | remove_cr | append_cr >staged.temp2 &&
+	test_cmp staged.temp1 staged.temp2
+'
+
+test_expect_failure 'Check that staged file :3: is CRLF' '
+	git show :3:file >staged.temp1 &&
+	git show :3:file | remove_cr | append_cr >staged.temp2 &&
+	test_cmp staged.temp1 staged.temp2
+'
+
 test_done
-- 
1.5.6.rc2.158.g3478

[PATCH 2/2] Ensure that objects shown in a core.autocrlf = true repo have CRLF EOLs

[Marius Storm-Olsen]

When you show an object, it should be shown with the EOLs which the repo
is configured for, and not how it's stored internally in the object store.

Signed-off-by: Marius Storm-Olsen <marius@trolltech.com>
---
 builtin-log.c         |   19 ++++++++++++++-----
 t/t6033-merge-crlf.sh |    6 +++---
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 9817d6f..94367f6 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -287,8 +287,8 @@ static void show_tagger(char *buf, int len, struct rev_info *rev)
 	       show_date(date, tz, rev->date_mode));
 }
 
-static int show_object(const unsigned char *sha1, int show_tag_object,
-	struct rev_info *rev)
+static int show_object(const unsigned char *sha1, const char *name,
+	int show_tag_object, struct rev_info *rev)
 {
 	unsigned long size;
 	enum object_type type;
@@ -309,8 +309,17 @@ static int show_object(const unsigned char *sha1, int show_tag_object,
 			offset = new_offset;
 		}
 
-	if (offset < size)
+	if (offset < size) {
+		struct strbuf strbuf;
+		strbuf_init(&strbuf, 0);
+		if (convert_to_working_tree(name, buf + offset, size - offset, &strbuf)) {
+			free(buf);
+			offset = 0;
+			size = strbuf.len;
+			buf = strbuf_detach(&strbuf, NULL);
+		}
 		fwrite(buf + offset, size - offset, 1, stdout);
+	}
 	free(buf);
 	return 0;
 }
@@ -350,7 +359,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 		const char *name = objects[i].name;
 		switch (o->type) {
 		case OBJ_BLOB:
-			ret = show_object(o->sha1, 0, NULL);
+			ret = show_object(o->sha1, name, 0, NULL);
 			break;
 		case OBJ_TAG: {
 			struct tag *t = (struct tag *)o;
@@ -359,7 +368,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 					diff_get_color_opt(&rev.diffopt, DIFF_COMMIT),
 					t->tag,
 					diff_get_color_opt(&rev.diffopt, DIFF_RESET));
-			ret = show_object(o->sha1, 1, &rev);
+			ret = show_object(o->sha1, name, 1, &rev);
 			objects[i].item = (struct object *)t->tagged;
 			i--;
 			break;
diff --git a/t/t6033-merge-crlf.sh b/t/t6033-merge-crlf.sh
index f161b40..d1d1dcb 100755
--- a/t/t6033-merge-crlf.sh
+++ b/t/t6033-merge-crlf.sh
@@ -49,19 +49,19 @@ test_expect_success 'Check that conflict file is CRLF' '
 	test_cmp file file.temp
 '
 
-test_expect_failure 'Check that staged file :1: is CRLF' '
+test_expect_success 'Check that staged file :1: is CRLF' '
 	git show :1:file >staged.temp1 &&
 	git show :1:file | remove_cr | append_cr >staged.temp2 &&
 	test_cmp staged.temp1 staged.temp2
 '
 
-test_expect_failure 'Check that staged file :2: is CRLF' '
+test_expect_success 'Check that staged file :2: is CRLF' '
 	git show :2:file >staged.temp1 &&
 	git show :2:file | remove_cr | append_cr >staged.temp2 &&
 	test_cmp staged.temp1 staged.temp2
 '
 
-test_expect_failure 'Check that staged file :3: is CRLF' '
+test_expect_success 'Check that staged file :3: is CRLF' '
 	git show :3:file >staged.temp1 &&
 	git show :3:file | remove_cr | append_cr >staged.temp2 &&
 	test_cmp staged.temp1 staged.temp2
-- 
1.5.6.rc2.158.g3478

[PATCH 0/2] Respecting core.autocrlf when showing objects

[Marius Storm-Olsen]

When you use 'git show <rev>:<file>' or 'git show :<stage>:<file>',
the objects are shows as they are in the object store, ignoring the
core.autocrlf configuration.

This series adds testcases which checks the stage files in a merge
conflict, and a fix for the problem.

Running all testcases before and after the fix reveals no regressions:

Before patch series:
    ./aggregate-results.sh test-results/t*-*
    fixed   1
    success 3374
    failed  0
    broken  2
    total   3377
    
After patch series:    
    ./aggregate-results.sh test-results/t*-*
    fixed   1
    success 3377
    failed  0
    broken  2
    total   3380
    rm -f -r 'trash directory' test-results

Marius Storm-Olsen (2):
  Add testcases for verifying that staged files in a conflict are CRLF,
    when core.autocrlf = true
  Ensure that objects shown in a core.autocrlf = true repo have CRLF
    EOLs

 builtin-log.c         |   19 ++++++++++++++-----
 t/t6033-merge-crlf.sh |   18 ++++++++++++++++++
 2 files changed, 32 insertions(+), 5 deletions(-)

Re: [PATCH 0/2] Respecting core.autocrlf when showing objects

[Johannes Schindelin]

Hi,

On Tue, 10 Jun 2008, Marius Storm-Olsen wrote:

> When you use 'git show <rev>:<file>' or 'git show :<stage>:<file>', the 
> objects are shows as they are in the object store, ignoring the 
> core.autocrlf configuration.

I think this is the correct behaviour: inside the object repository, the 
files are supposed to be LF clean.

Likewise, things in the unmerged stages are in the index, which again is 
not the working directory, so they should be LF clean.

_Only_ when writing a file to the working directory, it should get 
clobbered.

Ciao,
Dscho

Re: [PATCH 0/2] Respecting core.autocrlf when showing objects

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Tue, 10 Jun 2008, Marius Storm-Olsen wrote:
>
>> When you use 'git show <rev>:<file>' or 'git show :<stage>:<file>', the 
>> objects are shows as they are in the object store, ignoring the 
>> core.autocrlf configuration.
>
> I think this is the correct behaviour: inside the object repository, the 
> files are supposed to be LF clean.
>
> Likewise, things in the unmerged stages are in the index, which again is 
> not the working directory, so they should be LF clean.
>
> _Only_ when writing a file to the working directory, it should get 
> clobbered.

I'd agree with your argument on general principle, but it might make sense
to give an option to let you say "here is a blob contents, and use the
attribute for this path to munge it out to the filesystem."  I am not sure
if that belongs to "git show" Porcelain, though.  It _could_ be more like:

        git checkout-blob $blob_sha1 $path

that (1) reads the blob object specified by its object name, (2)
grabs attribute for the $path, and (3) applies convert_to_worktree()
filtering given that attribute and deposits the results to $path.

Alternatively, the interface could be:

        git cat-file blob $blob_sha1 |
        git filter-blob --use-attr-for=$path >$path.old

so that you can then do:

	git diff --no-index $path.old $path

I dunno.

Re: [PATCH 0/2] Respecting core.autocrlf when showing objects

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 2466 bytes --]

Junio C Hamano said the following on 11.06.2008 00:25:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> On Tue, 10 Jun 2008, Marius Storm-Olsen wrote:
>>> When you use 'git show <rev>:<file>' or 'git show :<stage>:<file>', the 
>>> objects are shows as they are in the object store, ignoring the 
>>> core.autocrlf configuration.
>> I think this is the correct behaviour: inside the object repository, the 
>> files are supposed to be LF clean.
>>
>> Likewise, things in the unmerged stages are in the index, which again is 
>> not the working directory, so they should be LF clean.
>>
>> _Only_ when writing a file to the working directory, it should get 
>> clobbered.
> 
> I'd agree with your argument on general principle, but it might make sense
> to give an option to let you say "here is a blob contents, and use the
> attribute for this path to munge it out to the filesystem."  I am not sure
> if that belongs to "git show" Porcelain, though.  It _could_ be more like:
> 
>         git checkout-blob $blob_sha1 $path
> 
> that (1) reads the blob object specified by its object name, (2)
> grabs attribute for the $path, and (3) applies convert_to_worktree()
> filtering given that attribute and deposits the results to $path.
> 
> Alternatively, the interface could be:
> 
>         git cat-file blob $blob_sha1 |
>         git filter-blob --use-attr-for=$path >$path.old
> 
> so that you can then do:
> 
> 	git diff --no-index $path.old $path
> 
> I dunno.

Well, consider this:
Say you are merging two branches, and know that you want to just use 
the parts which conflict from the branch being merged in. Then you 
simply do:
	git merge side
	git show :3:file.txt > file.txt
	git add file.txt
	git commit ...blah blah...

Now, with the current behavior, this workflow breaks for 
core.autocrlf=true repos. Given that 'git show' *is* porcelain, I'd 
expect it to work 'naturally' in my workflow, and not dump raw object 
store content.
However, I also see that it can be useful at times. Almost makes me 
consider a --raw option to 'git show' for those seldom cases. IMO, 
'git show' *should* care about autocrlf. Not doing so is just 
confusing to the end-user.

The fact that the stage files are in the index doesn't matter. I'd 
want CRLF files from 'git show v1.5.6-rc0:builtin-log.c' as well.

-- 
.marius [@trolltech.com]
'if you know what you're doing, it's not research'


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH 0/2] Respecting core.autocrlf when showing objects

[Jakub Narebski]

Marius Storm-Olsen <marius@trolltech.com> writes:

> However, I also see that it can be useful at times. Almost makes me
> consider a --raw option to 'git show' for those seldom cases. IMO,
> 'git show' *should* care about autocrlf. Not doing so is just
> confusing to the end-user.

There is always "git cat-file -p" which is porcelain, and should not
care about attributes (perhaps with the exception of explicitely told
so with some command option).

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH 0/2] Respecting core.autocrlf when showing objects

[Johannes Schindelin]

Hi,

On Wed, 11 Jun 2008, Marius Storm-Olsen wrote:

> Well, consider this:
>
> Say you are merging two branches, and know that you want to just use the 
> parts which conflict from the branch being merged in. Then you simply 
> do:
>
> 	git merge side
> 	git show :3:file.txt > file.txt

This is not really how I would do things.  I would do

	git checkout side file.txt here.

The _point_ is: "git show" is supposed to show you the contents _in the 
repository_.  For example, no smudge/clean filters will be heeded, and 
neither other attributes.

Further, "git show" will work without any problems in any bare repository.

In other words: "git show" is _not_ an operation on a working directory.

"git checkout" is.  So use that instead.

> Given that 'git show' *is* porcelain, I'd expect it to work 'naturally' 
> in my workflow, and not dump raw object store content.

Do not confuse porcelain with "works on the working directory".

> The fact that the stage files are in the index doesn't matter. I'd want 
> CRLF files from 'git show v1.5.6-rc0:builtin-log.c' as well.

But it _does_ matter!

The index works on raw objects, not on smudged files.  Period.

Ciao,
Dscho

Re: [PATCH 0/2] Respecting core.autocrlf when showing objects

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 3135 bytes --]

Johannes Schindelin said the following on 11.06.2008 21:06:
> On Wed, 11 Jun 2008, Marius Storm-Olsen wrote:
>> Well, consider this:
>>
>> Say you are merging two branches, and know that you want to just use the 
>> parts which conflict from the branch being merged in. Then you simply 
>> do:
>>
>> 	git merge side
>> 	git show :3:file.txt > file.txt
> 
> This is not really how I would do things.  I would do
> 
> 	git checkout side file.txt here.

Uhm, 'git checkout side file.txt' is not the same file content 
(ignoring EOLs please) as 'git show :3:file.txt'.
Ref: user-manual.html#conflict-resolution

> The _point_ is: "git show" is supposed to show you the contents _in the 
> repository_.  For example, no smudge/clean filters will be heeded, and 
> neither other attributes.

You are describing "git cat-file".
IMO, "git show" should have more consideration towards the repo 
settings. I doubt anyone, excluding yourself and a few more 
old-timers, think the content they get out from "git show <file>" is 
*not* the content they'll get when they decide to "git checkout 
<file>". For most people the commands a mostly the same, except that 
"show" just stdout-dumps the content, while "checkout" writes it to 
disk. The subtle difference there is simply just confusing, and is 
what we need to fix so people won't find Git so hard to use. It's all 
about usability. Let "git cat-file" do raw dumps, and "git show" what 
most people would expect.

Seen another way: If you "git show" any object, they are formated in a 
nice way for the user to see the output; not raw dumps. There's no 
reason why the user should even consider that when they show a plain 
blob, *then* it's raw (in the sense that EOLs are not handled properly).

The "show" command is too nice and convenient for it to have such a 
disrespect for the user.

> Further, "git show" will work without any problems in any bare repository.

Sure, it writes to stdout, and not to file. People understand that.

> In other words: "git show" is _not_ an operation on a working directory.

See above. Nobody expect it to touch files. However, any repo (even 
bare) still has a config file though, and "git show" should respect 
its settings.

> "git checkout" is.  So use that instead.

"git checkout" doesn't munge :<stage>:, which is what the 
documentation is referring to when it comes to conflict resolution.

>> Given that 'git show' *is* porcelain, I'd expect it to work 'naturally' 
>> in my workflow, and not dump raw object store content.
> 
> Do not confuse porcelain with "works on the working directory".

I don't. But I'm trying to see the workflow from a non-git-master POV, 
you're obviously not.

>> The fact that the stage files are in the index doesn't matter. I'd want 
>> CRLF files from 'git show v1.5.6-rc0:builtin-log.c' as well.
> 
> But it _does_ matter!
> The index works on raw objects, not on smudged files.  Period.

You misunderstood me. I don't smudged files in the index.

-- 
.marius [@trolltech.com]
'if you know what you're doing, it's not research'


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH 0/2] Respecting core.autocrlf when showing objects

[Junio C Hamano]

Marius Storm-Olsen <marius@trolltech.com> writes:

> Johannes Schindelin said the following on 11.06.2008 21:06:
>> On Wed, 11 Jun 2008, Marius Storm-Olsen wrote:
>>> Well, consider this:
>>>
>>> Say you are merging two branches, and know that you want to just
>>> use the parts which conflict from the branch being merged in. Then
>>> you simply do:
>>>
>>> 	git merge side
>>> 	git show :3:file.txt > file.txt
>>
>> This is not really how I would do things.  I would do
>>
>> 	git checkout side file.txt here.
>
> Uhm, 'git checkout side file.txt' is not the same file content
> (ignoring EOLs please) as 'git show :3:file.txt'.
> Ref: user-manual.html#conflict-resolution

Bruce, I think the comment in this quoted section is wrong.

True, the combined diff can show only the interesting differences between
the three, but that is not because we munge stage #2 and #3.  They contain
verbatim copies from the HEAD and the MERGE_HEAD respectively, and the
combining logic runs three-way diffs between the three stages to discard
the hunks that the result comes solely from either stage #2 or stage #3.

So perhaps something like this is in order...

---

 Documentation/user-manual.txt |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
index bfde507..64a820b 100644
--- a/Documentation/user-manual.txt
+++ b/Documentation/user-manual.txt
@@ -1254,16 +1254,15 @@ these three "file stages" represents a different version of the file:
 
 -------------------------------------------------
 $ git show :1:file.txt	# the file in a common ancestor of both branches
-$ git show :2:file.txt	# the version from HEAD, but including any
-			# nonconflicting changes from MERGE_HEAD
-$ git show :3:file.txt	# the version from MERGE_HEAD, but including any
-			# nonconflicting changes from HEAD.
+$ git show :2:file.txt	# the version from HEAD.
+$ git show :3:file.txt	# the version from MERGE_HEAD.
 -------------------------------------------------
 
-Since the stage 2 and stage 3 versions have already been updated with
-nonconflicting changes, the only remaining differences between them are
-the important ones; thus linkgit:git-diff[1] can use the information in
-the index to show only those conflicts.
+When you ask linkgit:git-diff[1] to show the conflicts, it runs a
+three-way diff between the conflicted merge results in the work tree with
+stages 2 and 3 to show only hunks whose contents come from both sides,
+mixed (in other words, when a hunk's merge results come only from stage 2,
+that part is not conflicting and is not shown.  Same for stage 3).
 
 The diff above shows the differences between the working-tree version of
 file.txt and the stage 2 and stage 3 versions.  So instead of preceding

Re: [PATCH 0/2] Respecting core.autocrlf when showing objects

[J. Bruce Fields]

On Thu, Jun 12, 2008 at 12:33:01PM -0700, Junio C Hamano wrote:
> Marius Storm-Olsen <marius@trolltech.com> writes:
> 
> > Johannes Schindelin said the following on 11.06.2008 21:06:
> >> On Wed, 11 Jun 2008, Marius Storm-Olsen wrote:
> >>> Well, consider this:
> >>>
> >>> Say you are merging two branches, and know that you want to just
> >>> use the parts which conflict from the branch being merged in. Then
> >>> you simply do:
> >>>
> >>> 	git merge side
> >>> 	git show :3:file.txt > file.txt
> >>
> >> This is not really how I would do things.  I would do
> >>
> >> 	git checkout side file.txt here.
> >
> > Uhm, 'git checkout side file.txt' is not the same file content
> > (ignoring EOLs please) as 'git show :3:file.txt'.
> > Ref: user-manual.html#conflict-resolution
> 
> Bruce, I think the comment in this quoted section is wrong.
> 
> True, the combined diff can show only the interesting differences between
> the three, but that is not because we munge stage #2 and #3.  They contain
> verbatim copies from the HEAD and the MERGE_HEAD respectively, and the
> combining logic runs three-way diffs between the three stages to discard
> the hunks that the result comes solely from either stage #2 or stage #3.

Oops, thanks for catching that!  I don't know how I got the idea it
worked that way.

(Is there any advantage, then, to the :n:filename syntax to a user?
Is it useful in any cases when they couldn't use HEAD or MERGE_HEAD
instead?  If not I might be tempted to cut this bit entirely (or
postpone it till later.)

--b.

> 
> So perhaps something like this is in order...
> 
> ---
> 
>  Documentation/user-manual.txt |   15 +++++++--------
>  1 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
> index bfde507..64a820b 100644
> --- a/Documentation/user-manual.txt
> +++ b/Documentation/user-manual.txt
> @@ -1254,16 +1254,15 @@ these three "file stages" represents a different version of the file:
>  
>  -------------------------------------------------
>  $ git show :1:file.txt	# the file in a common ancestor of both branches
> -$ git show :2:file.txt	# the version from HEAD, but including any
> -			# nonconflicting changes from MERGE_HEAD
> -$ git show :3:file.txt	# the version from MERGE_HEAD, but including any
> -			# nonconflicting changes from HEAD.
> +$ git show :2:file.txt	# the version from HEAD.
> +$ git show :3:file.txt	# the version from MERGE_HEAD.
>  -------------------------------------------------
>  
> -Since the stage 2 and stage 3 versions have already been updated with
> -nonconflicting changes, the only remaining differences between them are
> -the important ones; thus linkgit:git-diff[1] can use the information in
> -the index to show only those conflicts.
> +When you ask linkgit:git-diff[1] to show the conflicts, it runs a
> +three-way diff between the conflicted merge results in the work tree with
> +stages 2 and 3 to show only hunks whose contents come from both sides,
> +mixed (in other words, when a hunk's merge results come only from stage 2,
> +that part is not conflicting and is not shown.  Same for stage 3).
>  
>  The diff above shows the differences between the working-tree version of
>  file.txt and the stage 2 and stage 3 versions.  So instead of preceding

Re: [PATCH 0/2] Respecting core.autocrlf when showing objects

[Marius Storm-Olsen]

Junio C Hamano wrote:
> Marius Storm-Olsen <marius@trolltech.com> writes:
>> Uhm, 'git checkout side file.txt' is not the same file content 
>> (ignoring EOLs please) as 'git show :3:file.txt'. Ref:
>> user-manual.html#conflict-resolution
> 
> Bruce, I think the comment in this quoted section is wrong.
> 
> True, the combined diff can show only the interesting differences
> between the three, but that is not because we munge stage #2 and #3.
> They contain verbatim copies from the HEAD and the MERGE_HEAD
> respectively, and the combining logic runs three-way diffs between
> the three stages to discard the hunks that the result comes solely
> from either stage #2 or stage #3.
...
> -Since the stage 2 and stage 3 versions have already been updated
> with -nonconflicting changes, the only remaining differences between
> them are -the important ones; thus linkgit:git-diff[1] can use the
> information in -the index to show only those conflicts. +When you ask
> linkgit:git-diff[1] to show the conflicts, it runs a +three-way diff
> between the conflicted merge results in the work tree with +stages 2
> and 3 to show only hunks whose contents come from both sides, +mixed
> (in other words, when a hunk's merge results come only from stage 2, 
> +that part is not conflicting and is not shown.  Same for stage 3).

Aah, that certainly clears things up a bit. A good patch I'd say.

However, it doesn't change the fact that IMO "git show" should respect 
core.autocrlf, while "git cat-file" shouldn't.

I think many would consider
     git show MERGE_HEAD:file.txt > file.txt
before
     git checkout MERGE_HEAD file.txt
if only because they'd be scared to do a "checkout" in the middle of a 
merge conflict.

Personally I think the latter is nice, short and sweet, but that doesn't 
mean that it's less scary for people staring out on git. The fact that 
the two commands above are *not* identical in result, are the kind of 
things that we need to iron out, to make git more accessible to the 
general public.

--
.marius

Re: [PATCH 0/2] Respecting core.autocrlf when showing objects

[Jakub Narebski]

"J. Bruce Fields" <bfields@fieldses.org> writes:

> (Is there any advantage, then, to the :n:filename syntax to a user?
> Is it useful in any cases when they couldn't use HEAD or MERGE_HEAD
> instead?  If not I might be tempted to cut this bit entirely (or
> postpone it till later.)

I'm not sure, but I think that while HEAD and MERGE_HEAD vs :n:
differ in the tree represented (in the index trivial / tree conflicts
are resolved) they have the same file contents for conflicting files.

I think that :n: syntax is just shorter, especially for the ancestor
(c.f. $(git merge-base HEAD MERGE_HEAD)).

And of course there is octopus merge to be considered...
-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH 0/2] Respecting core.autocrlf when showing objects

[Junio C Hamano]

"J. Bruce Fields" <bfields@fieldses.org> writes:

> (Is there any advantage, then, to the :n:filename syntax to a user?
> Is it useful in any cases when they couldn't use HEAD or MERGE_HEAD
> instead?  If not I might be tempted to cut this bit entirely (or
> postpone it till later.)

I am somewhat torn between the two.

This section is only about merge conflicts, so using "checkout HEAD path"
would be a good substitute.  The text flows better that way, because the
previous paragraph talks about HEAD and MERGE_HEAD.

When people run "am -3", however, they may wish that they learned how the
notation to name blob objects in the index (e.g. :2:path) can be used to
examine and resolve the conflict, as there is no HEAD/MERGE_HEAD in that
usage context.

Re: [PATCH 0/2] Respecting core.autocrlf when showing objects

[Jon Loeliger]

On Thu, 2008-06-12 at 13:45 -0700, Junio C Hamano wrote:
> "J. Bruce Fields" <bfields@fieldses.org> writes:
> 
> > (Is there any advantage, then, to the :n:filename syntax to a user?
> > Is it useful in any cases when they couldn't use HEAD or MERGE_HEAD
> > instead?  If not I might be tempted to cut this bit entirely (or
> > postpone it till later.)
> 
> I am somewhat torn between the two.
> 
> This section is only about merge conflicts, so using "checkout HEAD path"
> would be a good substitute.  The text flows better that way, because the
> previous paragraph talks about HEAD and MERGE_HEAD.
> 
> When people run "am -3", however, they may wish that they learned how the
> notation to name blob objects in the index (e.g. :2:path) can be used to
> examine and resolve the conflict, as there is no HEAD/MERGE_HEAD in that
> usage context.


Hi Junio,

I was planning on specifically pointing out the :n: forms as well.
So I'm watching this one a bit carefully and would appreciate a
bit of long-term guidance on the issue here.

Thanks,
jdl