Failing 'git am' when core.autocrlf=true

Hi,

I have an issue with git-rebase failing on a repository using
core.autocrlf=true

I've tracked it down to git-am failing with core.autocrlf=true and passing with core.autocrlf=false. I've tried digging deeper into the code, but for some reason ce_match_stat_basic() (read-cache.c:~187) reports the size of the file in the index to be 0 (when core.autocrlf=true), which is why git-am bails out on the patch. (ce->ce_size == 0, while st->st_size == the correct size on disk)

If I force the bailout in check_patch() (builtin-apply.c:~2101) to _not_ happen, the patch applies without problems.

Can anyone please enlighten me on why this may happen?

I've attached a SH script which will reproduce the problem. Simply run
./git_am_crlf_testcase.sh
to reproduce the problem, and run

    ./git_am_crlf_testcase.sh --no-crlf

to show it working when core.autocrlf=false.

Thanks for your help!

--
.marius

#!/bin/sh
#
# Testcase for failing 'git am' on a repository with core.autocrlf = true

#     ./git_am_crlf_testcase.sh              fails for Git 1.5.3.rc4
#     ./git_am_crlf_testcase.sh --no-crlf    passes

#
# --------------------------------------------------------------------------------------------------

TESTCASE_REPO=git_am_crlf_testcase
GIT_AUTOCRLF=true

# parse the arguments
while [ "$#" -gt 0 ]; do
    case "$1" in
    --no-crlf) # Turn off autocrlf for the repository
        GIT_AUTOCRLF=false
        ;;
    --crlf) # Turn on autocrlf for the repository
        GIT_AUTOCRLF=true
        ;;
    esac
    shift
done

# Functions ----------------------------------------------------------------------------------------

die() {
    echo >&2 "$@"
    exit 1
}

print_patch() {
cat << __END_OF_PATCH__
From 38be10072e45dd6b08ce40851e3fca60a31a340b Mon Sep 17 00:00:00 2001
From: Marius Storm-Olsen <[email protected]>
Date: Thu, 23 Aug 2007 13:00:00 +0200
Subject: test1

---
 foo |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 foo

new file mode 100644
index 0000000000000000000000000000000000000000..5716ca5987cbf97d6bb54920bea6adde242d87e6
@@ -0,0 +1 @@
+bar

1.5.3.rc4.mingw.2.3.g3318a-dirty


From 3bb3855cf028fc90010e2f51aceb6d3a90039c5c Mon Sep 17 00:00:00 2001
From: Marius Storm-Olsen <[email protected]>
Date: Thu, 23 Aug 2007 13:00:01 +0200
Subject: test2

---
 foo |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

index 5716ca5987cbf97d6bb54920bea6adde242d87e6..e2994c5ae2f7c41a306d9b41b64af4d6b7242793 100644
@@ -1 +1,2 @@
 bar
+baz

1.5.3.rc4.mingw.2.3.g3318a-dirty


From 38212fd002da735b3626c6f1d801c8ca37ce04e6 Mon Sep 17 00:00:00 2001
From: Marius Storm-Olsen <[email protected]>
Date: Thu, 23 Aug 2007 13:00:02 +0200
Subject: test3

---
 foo |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

index e2994c5ae2f7c41a306d9b41b64af4d6b7242793..ad94f9e431a24bbe67e9e5935e2a8cf20a73e723 100644
@@ -1,2 +1,3 @@
 bar
 baz
+blah

1.5.3.rc4.mingw.2.3.g3318a-dirty
__END_OF_PATCH__
}


# 'main' -------------------------------------------------------------------------------------------

# Kill old testcase repository
test -d "$TESTCASE_REPO" && rm -rf $TESTCASE_REPO

# Create new testcase repository
mkdir $TESTCASE_REPO &&
cd $TESTCASE_REPO &&
git init || die "Couldn\'t create testcase repository \'$TESTCASE_REPO\'!"

# Set autocrlf explicitly
git config core.autocrlf $GIT_AUTOCRLF || die "Couldn\'t set core.autocrlf to \'$GIT_AUTOCRLF\'!"

# Create initial commit
(echo foo > bar &&
git add bar &&
git commit -m "initial commit") || die "Couldn\'t create initial file!"

# Apply failing patch set
print_patch | git am --binary -3 || die "Applying patch set to autocrlf=true repo failed!"

Very interesting.

Adding some instrumentation to "git-am.sh" (namely a lot of

	git diff --quiet || exit

lines to figure out exactly *where* the index gets out of sync with the working tree), I get this trace:

trace: built-in: git 'mailsplit' '-d4' '-o.dotest' '-b' '--'
trace: built-in: git 'diff-index' '--cached' '--name-only' 'HEAD'
trace: built-in: git 'diff' '--quiet'
trace: built-in: git 'mailinfo' '-u' '.dotest/msg' '.dotest/patch'
trace: built-in: git 'stripspace'
trace: built-in: git 'diff' '--quiet'
trace: built-in: git 'apply' '--allow-binary-replacement' '--index' '.dotest/patch'
trace: built-in: git 'diff' '--quiet'
trace: built-in: git 'diff' '--quiet'
trace: built-in: git 'write-tree'
trace: built-in: git 'diff' '--quiet'

ie everything was fine after the "apply" phase, but the index and the working tree went out-of-kilter after "git write-tree".

The reason? "git write-tree" doesn't read the config file, so it never even reads the "core.autocrlf=true" variable. As a result, it seems to screw up the index matching when it does the cache_tree_fully_valid() (which will fail due to "git apply --index" having invalidated the tree SHA1's) followed by cache_tree_update().

This patch should fix it.

Junio - it fixes the test for me, but quite frankly, I don't see why write-tree would *ever* change any non-tree index entries. But it does. I think there's another bug somewhere, or I'm missing something.

Linus

---
 builtin-write-tree.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

index 88f34ba..b89d02e 100644
@@ -72,6 +72,7 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
 	const char *prefix = NULL;
 	unsigned char sha1[20];
 
+	git_config(git_default_config);
 	while (1 < argc) {
 		const char *arg = argv[1];
 		if (!strcmp(arg, "--missing-ok"))

Linus Torvalds said the following on 23.08.2007 19:18:

Aah! Very neat way to narrow it down. I'll keep that in mind for later debugging. Thanks!

Yep, it did. I just rebased 80 commits with core.autocrlf=true. Thanks again, Linus, for the quick reply!

.marius

As you said, there is something else going on. write-tree is about reading the index entries and writing them out as a set of trees, and at that point it should not even matter if you have garbage in the work tree or if you do not even have a work tree. All the crlf conversions have been done when the object hit the index, so its reading or not reading core.autocrlf should not change its behaviour.

Ok, I just sent out the analysis of this, maybe you should add that as the changelog for the one-liner, and sign-off from me.

Linus

Looking more carefully at the index file before and after, the _only_ difference really is the ce_size field for "foo".

In the fixed one, ce_size remains at 5 (which is correct: the file contents are "bar\n\r"), in the broken case it gets zeroed by git-write-tree for some reason.

And the reason is really interesting: it only happens if the index file has the same date as the entry in the index, in which case we end up doign the "careful" check in ce_smudge_racily_clean_entry() and there the "ce_modified_check_fs()" will end up re-reading the file, and if we don't have the right CRLF behaviour, we will now return DATA_CHANGED.

So the call-chain for this is:

	cmd_write_tree ->
	  write_tree ->
	    write_index ->
	      ce_smudge_racily_clean_entry -> 
		ce_modified_check_fs ->
		  ce_compare_data ->
		    index_fd ->
		      convert_to_git ->
			** wrong answer unless auto_crlf is set **

		and now "ce_smudge_racily_clean_entry()" will do
			ce->ce_size = htonl(0);

and the one-liner fix I sent out is actually the right fix.

This was harder to find than it should have been, because it actually depends on the datestamp of the index file matching the datestamp of the file in question!

Linus

Thanks for the detailed analysis of the chain of events. It really helps understand the inner-workings of Git.

I also tried to track this down after your initial patch, but couldn't find it. The fact that the index is smudged at the time of the second commit, so the third failed, made me look in the wrong place.

How about adding a trace for the smudged case, so it's easier to find similar issues in the future?

--
.marius

From b858610ff4cd42f57a05c815a3e3b43428d67f99 Mon Sep 17 00:00:00 2001
From: Marius Storm-Olsen <[email protected]>
Date: Thu, 23 Aug 2007 22:03:35 +0200
Subject: [PATCH] Add a trace to more easily show that the index has been smudged.

---
read-cache.c | 1 +

1 files changed, 1 insertions(+), 0 deletions(-)

index 849e8d6..000451c 100644
@@ -1088,6 +1088,7 @@ static void ce_smudge_racily_clean_entry(struct cache_entry *ce)

		 * for "frotz" stays 6 which does not match the filesystem.
		 */
		ce->ce_size = htonl(0);
+		trace_printf("trace: index: Index object for '%s' smudged due to being racily clean\n", ce->name);
	}
}

1.5.3.rc4.mingw.2.3.g3318a-dirty

Ahhhh, I forgot that we made write-tree to write back into index because at that point we have a fully populated cache-tree.

Your analysis makes sense to me. Thanks.