{"thread":{"id":"9624","subject":"[TESTCASE] Failing 'git am' when core.autocrlf=true","startedAt":"2007-08-23T14:07:39Z","lastAt":"2007-08-23T20:11:21Z","messageCount":8,"participants":["Marius Storm-Olsen","Linus Torvalds","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"51371","messageId":"46CD94AB.7070709@trolltech.com","threadId":"9624","inReplyTo":null,"subject":"[TESTCASE] Failing 'git am' when core.autocrlf=true","fromName":"Marius Storm-Olsen","fromEmail":"marius@trolltech.com","sentAt":"2007-08-23T14:07:39Z","receivedAt":"2007-08-23T14:07:39Z","isPatch":false,"sender":{"key":"marius@trolltech.com","avatar":"https://gravatar.com/avatar/a40071d8f651862c6ab10bd7996f0ad84d94f06c0399de9e3fa4f06beb390a71?d=mp&s=160"},"body":"Hi,\n\nI have an issue with git-rebase failing on a repository using\n     core.autocrlf=true\n\nI've tracked it down to git-am failing with core.autocrlf=true and \npassing with core.autocrlf=false. I've tried digging deeper into the \ncode, but for some reason ce_match_stat_basic() (read-cache.c:~187) \nreports the size of the file in the index to be 0 (when \ncore.autocrlf=true), which is why git-am bails out on the patch. \n(ce->ce_size == 0, while st->st_size == the correct size on disk)\n\nIf I force the bailout in check_patch() (builtin-apply.c:~2101) to \n_not_ happen, the patch applies without problems.\n\nCan anyone please enlighten me on why this may happen?\n\nI've attached a SH script which will reproduce the problem. Simply run\n     ./git_am_crlf_testcase.sh\nto reproduce the problem, and run\n     ./git_am_crlf_testcase.sh --no-crlf\nto show it working when core.autocrlf=false.\n\nThanks for your help!\n\n--\n.marius\n\n\n\n#!/bin/sh\n#\n# Testcase for failing 'git am' on a repository with core.autocrlf = true\n#     ./git_am_crlf_testcase.sh              fails for Git 1.5.3.rc4\n#     ./git_am_crlf_testcase.sh --no-crlf    passes\n#\n# --------------------------------------------------------------------------------------------------\n\nTESTCASE_REPO=git_am_crlf_testcase\nGIT_AUTOCRLF=true\n\n# parse the arguments\nwhile [ \"$#\" -gt 0 ]; do\n    case \"$1\" in\n    --no-crlf) # Turn off autocrlf for the repository\n        GIT_AUTOCRLF=false\n        ;;\n    --crlf) # Turn on autocrlf for the repository\n        GIT_AUTOCRLF=true\n        ;;\n    esac\n    shift\ndone\n\n\n# Functions ----------------------------------------------------------------------------------------\n\ndie() {\n    echo >&2 \"$@\"\n    exit 1\n}\n\nprint_patch() {\ncat << __END_OF_PATCH__\nFrom 38be10072e45dd6b08ce40851e3fca60a31a340b Mon Sep 17 00:00:00 2001\nFrom: Marius Storm-Olsen <x@y.com>\nDate: Thu, 23 Aug 2007 13:00:00 +0200\nSubject: test1\n\n---\n foo |    1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n create mode 100644 foo\n\ndiff --git a/foo b/foo\nnew file mode 100644\nindex 0000000000000000000000000000000000000000..5716ca5987cbf97d6bb54920bea6adde242d87e6\n--- /dev/null\n+++ b/foo\n@@ -0,0 +1 @@\n+bar\n-- \n1.5.3.rc4.mingw.2.3.g3318a-dirty\n\n\nFrom 3bb3855cf028fc90010e2f51aceb6d3a90039c5c Mon Sep 17 00:00:00 2001\nFrom: Marius Storm-Olsen <x@y.com>\nDate: Thu, 23 Aug 2007 13:00:01 +0200\nSubject: test2\n\n---\n foo |    1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/foo b/foo\nindex 5716ca5987cbf97d6bb54920bea6adde242d87e6..e2994c5ae2f7c41a306d9b41b64af4d6b7242793 100644\n--- a/foo\n+++ b/foo\n@@ -1 +1,2 @@\n bar\n+baz\n-- \n1.5.3.rc4.mingw.2.3.g3318a-dirty\n\n\nFrom 38212fd002da735b3626c6f1d801c8ca37ce04e6 Mon Sep 17 00:00:00 2001\nFrom: Marius Storm-Olsen <x@y.com>\nDate: Thu, 23 Aug 2007 13:00:02 +0200\nSubject: test3\n\n---\n foo |    1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/foo b/foo\nindex e2994c5ae2f7c41a306d9b41b64af4d6b7242793..ad94f9e431a24bbe67e9e5935e2a8cf20a73e723 100644\n--- a/foo\n+++ b/foo\n@@ -1,2 +1,3 @@\n bar\n baz\n+blah\n-- \n1.5.3.rc4.mingw.2.3.g3318a-dirty\n__END_OF_PATCH__\n}\n\n\n# 'main' -------------------------------------------------------------------------------------------\n\n# Kill old testcase repository\ntest -d \"$TESTCASE_REPO\" && rm -rf $TESTCASE_REPO\n\n# Create new testcase repository\nmkdir $TESTCASE_REPO &&\ncd $TESTCASE_REPO &&\ngit init || die \"Couldn\\'t create testcase repository \\'$TESTCASE_REPO\\'!\"\n\n# Set autocrlf explicitly\ngit config core.autocrlf $GIT_AUTOCRLF || die \"Couldn\\'t set core.autocrlf to \\'$GIT_AUTOCRLF\\'!\"\n\n# Create initial commit\n(echo foo > bar &&\ngit add bar &&\ngit commit -m \"initial commit\") || die \"Couldn\\'t create initial file!\"\n\n# Apply failing patch set\nprint_patch | git am --binary -3 || die \"Applying patch set to autocrlf=true repo failed!\"\n"},{"id":"51379","messageId":"alpine.LFD.0.999.0708230956190.30176@woody.linux-foundation.org","threadId":"9624","inReplyTo":"46CD94AB.7070709@trolltech.com","subject":"Re: [TESTCASE] Failing 'git am' when core.autocrlf=true","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-08-23T17:18:54Z","receivedAt":"2007-08-23T17:18:54Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 23 Aug 2007, Marius Storm-Olsen wrote:\n> \n> I have an issue with git-rebase failing on a repository using\n>     core.autocrlf=true\n> \n> I've tracked it down to git-am failing with core.autocrlf=true and passing\n> with core.autocrlf=false. I've tried digging deeper into the code, but for\n> some reason ce_match_stat_basic() (read-cache.c:~187) reports the size of the\n> file in the index to be 0 (when core.autocrlf=true), which is why git-am bails\n> out on the patch. (ce->ce_size == 0, while st->st_size == the correct size on\n> disk)\n\nVery interesting.\n\nAdding some instrumentation to \"git-am.sh\" (namely a lot of\n\n\tgit diff --quiet || exit\n\nlines to figure out exactly *where* the index gets out of sync with the \nworking tree), I get this trace:\n\n\ttrace: built-in: git 'mailsplit' '-d4' '-o.dotest' '-b' '--'\n\ttrace: built-in: git 'diff-index' '--cached' '--name-only' 'HEAD'\n\ttrace: built-in: git 'diff' '--quiet'\n\ttrace: built-in: git 'mailinfo' '-u' '.dotest/msg' '.dotest/patch'\n\ttrace: built-in: git 'stripspace'\n\ttrace: built-in: git 'diff' '--quiet'\n\ttrace: built-in: git 'apply' '--allow-binary-replacement' '--index' '.dotest/patch'\n\ttrace: built-in: git 'diff' '--quiet'\n\ttrace: built-in: git 'diff' '--quiet'\n\ttrace: built-in: git 'write-tree'\n\ttrace: built-in: git 'diff' '--quiet'\n\nie everything was fine after the \"apply\" phase, but the index and the \nworking tree went out-of-kilter after \"git write-tree\".\n\nThe reason? \"git write-tree\" doesn't read the config file, so it never \neven reads the \"core.autocrlf=true\" variable. As a result, it seems to \nscrew up the index matching when it does the cache_tree_fully_valid() \n(which will fail due to \"git apply --index\" having invalidated the tree \nSHA1's) followed by cache_tree_update().\n\n> Can anyone please enlighten me on why this may happen?\n\nThis patch should fix it. \n\nJunio - it fixes the test for me, but quite frankly, I don't see why \nwrite-tree would *ever* change any non-tree index entries. But it does. I \nthink there's another bug somewhere, or I'm missing something.\n\n\t\tLinus\n\n---\n builtin-write-tree.c |    1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-write-tree.c b/builtin-write-tree.c\nindex 88f34ba..b89d02e 100644\n--- a/builtin-write-tree.c\n+++ b/builtin-write-tree.c\n@@ -72,6 +72,7 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)\n \tconst char *prefix = NULL;\n \tunsigned char sha1[20];\n \n+\tgit_config(git_default_config);\n \twhile (1 < argc) {\n \t\tconst char *arg = argv[1];\n \t\tif (!strcmp(arg, \"--missing-ok\"))\n"},{"id":"51380","messageId":"46CDC7B1.7070908@trolltech.com","threadId":"9624","inReplyTo":"alpine.LFD.0.999.0708230956190.30176@woody.linux-foundation.org","subject":"Re: [TESTCASE] Failing 'git am' when core.autocrlf=true","fromName":"Marius Storm-Olsen","fromEmail":"marius@trolltech.com","sentAt":"2007-08-23T17:45:21Z","receivedAt":"2007-08-23T17:45:21Z","isPatch":false,"sender":{"key":"marius@trolltech.com","avatar":"https://gravatar.com/avatar/a40071d8f651862c6ab10bd7996f0ad84d94f06c0399de9e3fa4f06beb390a71?d=mp&s=160"},"body":"Linus Torvalds said the following on 23.08.2007 19:18:\n> On Thu, 23 Aug 2007, Marius Storm-Olsen wrote:\n>> I have an issue with git-rebase failing on a repository using\n>>     core.autocrlf=true\n> \n> Very interesting.\n> \n> Adding some instrumentation to \"git-am.sh\" (namely a lot of\n> \n> \tgit diff --quiet || exit\n\nAah! Very neat way to narrow it down. I'll keep that in mind for later \ndebugging. Thanks!\n\n\n> This patch should fix it. \n\nYep, it did. I just rebased 80 commits with core.autocrlf=true. Thanks \nagain, Linus, for the quick reply!\n\n-- \n.marius\n\n"},{"id":"51384","messageId":"7vhcmqt7oh.fsf@gitster.siamese.dyndns.org","threadId":"9624","inReplyTo":"alpine.LFD.0.999.0708230956190.30176@woody.linux-foundation.org","subject":"Re: [TESTCASE] Failing 'git am' when core.autocrlf=true","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-23T18:41:34Z","receivedAt":"2007-08-23T18:41:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> On Thu, 23 Aug 2007, Marius Storm-Olsen wrote:\n>> \n>> I have an issue with git-rebase failing on a repository using\n>>     core.autocrlf=true\n>> \n>> I've tracked it down to git-am failing with core.autocrlf=true and passing\n>> with core.autocrlf=false. I've tried digging deeper into the code, but for\n>> some reason ce_match_stat_basic() (read-cache.c:~187) reports the size of the\n>> file in the index to be 0 (when core.autocrlf=true), which is why git-am bails\n>> out on the patch. (ce->ce_size == 0, while st->st_size == the correct size on\n>> disk)\n>\n> Very interesting.\n> ...\n> \ttrace: built-in: git 'apply' '--allow-binary-replacement' '--index' '.dotest/patch'\n> \ttrace: built-in: git 'diff' '--quiet'\n> \ttrace: built-in: git 'diff' '--quiet'\n> \ttrace: built-in: git 'write-tree'\n> \ttrace: built-in: git 'diff' '--quiet'\n>\n> ie everything was fine after the \"apply\" phase, but the index and the \n> working tree went out-of-kilter after \"git write-tree\".\n>\n> The reason? \"git write-tree\" doesn't read the config file, so it never \n> even reads the \"core.autocrlf=true\" variable. As a result, it seems to \n> screw up the index matching when it does the cache_tree_fully_valid() \n> (which will fail due to \"git apply --index\" having invalidated the tree \n> SHA1's) followed by cache_tree_update().\n>\n>> Can anyone please enlighten me on why this may happen?\n>\n> This patch should fix it. \n>\n> Junio - it fixes the test for me, but quite frankly, I don't see why \n> write-tree would *ever* change any non-tree index entries. But it does. I \n> think there's another bug somewhere, or I'm missing something.\n\nAs you said, there is something else going on.  write-tree is\nabout reading the index entries and writing them out as a set of\ntrees, and at that point it should not even matter if you have\ngarbage in the work tree or if you do not even have a work tree.\nAll the crlf conversions have been done when the object hit the\nindex, so its reading or not reading core.autocrlf should not\nchange its behaviour.\n"},{"id":"51388","messageId":"alpine.LFD.0.999.0708231153150.30176@woody.linux-foundation.org","threadId":"9624","inReplyTo":"alpine.LFD.0.999.0708230956190.30176@woody.linux-foundation.org","subject":"Re: [TESTCASE] Failing 'git am' when core.autocrlf=true","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-08-23T19:23:39Z","receivedAt":"2007-08-23T19:23:39Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 23 Aug 2007, Linus Torvalds wrote:\n> \n> Junio - it fixes the test for me, but quite frankly, I don't see why \n> write-tree would *ever* change any non-tree index entries. But it does. I \n> think there's another bug somewhere, or I'm missing something.\n\nLooking more carefully at the index file before and after, the _only_ \ndifference really is the ce_size field for \"foo\".\n\nIn the fixed one, ce_size remains at 5 (which is correct: the file \ncontents are \"bar\\n\\r\"), in the broken case it gets zeroed by \ngit-write-tree for some reason.\n\nAnd the reason is really interesting: it only happens if the index file \nhas the same date as the entry in the index, in which case we end up doign \nthe \"careful\" check in ce_smudge_racily_clean_entry() and there the \n\"ce_modified_check_fs()\" will end up re-reading the file, and if we don't \nhave the right CRLF behaviour, we will now return DATA_CHANGED.\n\nSo the call-chain for this is:\n\n\tcmd_write_tree ->\n\t  write_tree ->\n\t    write_index ->\n\t      ce_smudge_racily_clean_entry -> \n\t\tce_modified_check_fs ->\n\t\t  ce_compare_data ->\n\t\t    index_fd ->\n\t\t      convert_to_git ->\n\t\t\t** wrong answer unless auto_crlf is set **\n\n\t\tand now \"ce_smudge_racily_clean_entry()\" will do\n\t\t\tce->ce_size = htonl(0);\n\nand the one-liner fix I sent out is actually the right fix.\n\nThis was harder to find than it should have been, because it actually \ndepends on the datestamp of the index file matching the datestamp of the \nfile in question!\n\n\t\tLinus\n"},{"id":"51390","messageId":"alpine.LFD.0.999.0708231225180.30176@woody.linux-foundation.org","threadId":"9624","inReplyTo":"7vhcmqt7oh.fsf@gitster.siamese.dyndns.org","subject":"Re: [TESTCASE] Failing 'git am' when core.autocrlf=true","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-08-23T19:25:48Z","receivedAt":"2007-08-23T19:25:48Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 23 Aug 2007, Junio C Hamano wrote:\n> \n> As you said, there is something else going on.  write-tree is\n> about reading the index entries and writing them out as a set of\n> trees, and at that point it should not even matter if you have\n> garbage in the work tree or if you do not even have a work tree.\n> All the crlf conversions have been done when the object hit the\n> index, so its reading or not reading core.autocrlf should not\n> change its behaviour.\n\nOk, I just sent out the analysis of this, maybe you should add that as the \nchangelog for the one-liner, and sign-off from me.\n\n\t\tLinus\n"},{"id":"51391","messageId":"7vabsit4re.fsf@gitster.siamese.dyndns.org","threadId":"9624","inReplyTo":"alpine.LFD.0.999.0708231153150.30176@woody.linux-foundation.org","subject":"Re: [TESTCASE] Failing 'git am' when core.autocrlf=true","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-23T19:44:37Z","receivedAt":"2007-08-23T19:44:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> And the reason is really interesting: it only happens if the index file \n> has the same date as the entry in the index, in which case we end up doign \n> the \"careful\" check in ce_smudge_racily_clean_entry() and there the \n> \"ce_modified_check_fs()\" will end up re-reading the file, and if we don't \n> have the right CRLF behaviour, we will now return DATA_CHANGED.\n>\n> So the call-chain for this is:\n>\n> \tcmd_write_tree ->\n> \t  write_tree ->\n> \t    write_index ->\n> \t      ce_smudge_racily_clean_entry -> \n> \t\tce_modified_check_fs ->\n> \t\t  ce_compare_data ->\n> \t\t    index_fd ->\n> \t\t      convert_to_git ->\n> \t\t\t** wrong answer unless auto_crlf is set **\n>\n> \t\tand now \"ce_smudge_racily_clean_entry()\" will do\n> \t\t\tce->ce_size = htonl(0);\n>\n> and the one-liner fix I sent out is actually the right fix.\n>\n> This was harder to find than it should have been, because it actually \n> depends on the datestamp of the index file matching the datestamp of the \n> file in question!\n\nAhhhh, I forgot that we made write-tree to write back into index\nbecause at that point we have a fully populated cache-tree.\n\nYour analysis makes sense to me.  Thanks.\n"},{"id":"51394","messageId":"46CDE9E9.8070702@trolltech.com","threadId":"9624","inReplyTo":"alpine.LFD.0.999.0708231153150.30176@woody.linux-foundation.org","subject":"Re: [TESTCASE] Failing 'git am' when core.autocrlf=true","fromName":"Marius Storm-Olsen","fromEmail":"marius@trolltech.com","sentAt":"2007-08-23T20:11:21Z","receivedAt":"2007-08-23T20:11:21Z","isPatch":false,"sender":{"key":"marius@trolltech.com","avatar":"https://gravatar.com/avatar/a40071d8f651862c6ab10bd7996f0ad84d94f06c0399de9e3fa4f06beb390a71?d=mp&s=160"},"body":"Linus Torvalds wrote:\n> So the call-chain for this is:\n> \n> \tcmd_write_tree ->\n> \t  write_tree ->\n> \t    write_index ->\n> \t      ce_smudge_racily_clean_entry -> \n> \t\tce_modified_check_fs ->\n> \t\t  ce_compare_data ->\n> \t\t    index_fd ->\n> \t\t      convert_to_git ->\n> \t\t\t** wrong answer unless auto_crlf is set **\n> \n> \t\tand now \"ce_smudge_racily_clean_entry()\" will do\n> \t\t\tce->ce_size = htonl(0);\n> \n> and the one-liner fix I sent out is actually the right fix.\n> \n> This was harder to find than it should have been, because it\n> actually depends on the datestamp of the index file matching the\n> datestamp of the file in question!\n\nThanks for the detailed analysis of the chain of events. It\nreally helps understand the inner-workings of Git.\n\nI also tried to track this down after your initial patch, but\ncouldn't find it. The fact that the index is smudged at the time of\nthe second commit, so the third failed, made me look in the wrong\nplace.\n\nHow about adding a trace for the smudged case, so it's easier to\nfind similar issues in the future?\n\n--\n.marius\n\n\nFrom b858610ff4cd42f57a05c815a3e3b43428d67f99 Mon Sep 17 00:00:00 2001\nFrom: Marius Storm-Olsen <mstormo_git@storm-olsen.com>\nDate: Thu, 23 Aug 2007 22:03:35 +0200\nSubject: [PATCH] Add a trace to more easily show that the index has been smudged.\n\nSigned-off-by: Marius Storm-Olsen <mstormo_git@storm-olsen.com>\n---\n read-cache.c |    1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 849e8d6..000451c 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1088,6 +1088,7 @@ static void ce_smudge_racily_clean_entry(struct cache_entry *ce)\n \t\t * for \"frotz\" stays 6 which does not match the filesystem.\n \t\t */\n \t\tce->ce_size = htonl(0);\n+\t\ttrace_printf(\"trace: index: Index object for '%s' smudged due to being racily clean\\n\", ce->name);\n \t}\n }\n \n-- \n1.5.3.rc4.mingw.2.3.g3318a-dirty\n\n\n"}]}