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

Johannes Schindelin [off-list ref] writes:

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.

Signed-off-by: Johannes Schindelin <redacted>
---

	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

Johannes Schindelin [off-list ref] writes:

Signed-off-by: Johannes Schindelin <redacted>
---

	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.

Hi,

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

Johannes Schindelin [off-list ref] writes:

quoted

Signed-off-by: Johannes Schindelin <redacted>
---

	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

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(-)

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

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

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