{"thread":{"id":"10353","subject":"[PATCH] git-blame shouldn't crash if run in an unmerged tree","startedAt":"2007-10-18T06:34:07Z","lastAt":"2007-10-18T22:38:23Z","messageCount":4,"participants":["Shawn O. Pearce","Björn Steinbrink","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"56364","messageId":"20071018063407.GA28861@spearce.org","threadId":"10353","inReplyTo":null,"subject":"[PATCH] git-blame shouldn't crash if run in an unmerged tree","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-10-18T06:34:07Z","receivedAt":"2007-10-18T06:34:07Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"I'm applying this patch to my maint tree tonight as it does resolve\nthe issue for now.  What surprised me was the file that we were\ncrashing out on wasn't even the file we wanted to get the blame\ndata for.  :-\\\n\n--8>--\nFrom: Linus Torvalds <torvalds@linux-foundation.org>\nSubject: [PATCH] git-blame shouldn't crash if run in an unmerged tree\n\nIf we are in the middle of resolving a merge conflict there may be\none or more files whose entries in the index represent an unmerged\nstate (index entries in the higher-order stages).\n\nAttempting to run git-blame on any file in such a working directory\nresulted in \"fatal: internal error: ce_mode is 0\" as we use the magic\nmarker for an unmerged entry is 0 (set up by things like diff-lib.c's\ndo_diff_cache() and builtin-read-tree.c's read_tree_unmerged())\nand the ce_match_stat_basic() function gets upset about this.\n\nI'm not entirely sure that the whole \"ce_mode = 0\" case is a good\nidea to begin with, and maybe the right thing to do is to remove\nthat horrid freakish special case, but removing the internal error\nseems to be the simplest fix for now.\n\n                Linus\n\n[sp: Thanks to Björn Steinbrink for the test case]\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n read-cache.c     |    2 +\n t/t8004-blame.sh |   73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 75 insertions(+), 0 deletions(-)\n create mode 100755 t/t8004-blame.sh\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 536f4d0..928e8fa 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -149,6 +149,8 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)\n \t\telse if (ce_compare_gitlink(ce))\n \t\t\tchanged |= DATA_CHANGED;\n \t\treturn changed;\n+\tcase 0: /* Special case: unmerged file in index */\n+\t\treturn MODE_CHANGED | DATA_CHANGED | TYPE_CHANGED;\n \tdefault:\n \t\tdie(\"internal error: ce_mode is %o\", ntohl(ce->ce_mode));\n \t}\ndiff --git a/t/t8004-blame.sh b/t/t8004-blame.sh\nnew file mode 100755\nindex 0000000..ba19ac1\n--- /dev/null\n+++ b/t/t8004-blame.sh\n@@ -0,0 +1,73 @@\n+#!/bin/sh\n+\n+# Based on a test case submitted by Björn Steinbrink.\n+\n+test_description='git blame on conflicted files'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup first case' '\n+\t# Create the old file\n+\techo \"Old line\" > file1 &&\n+\tgit add file1 &&\n+\tgit commit --author \"Old Line <ol@localhost>\" -m file1.a &&\n+\n+\t# Branch\n+\tgit checkout -b foo &&\n+\n+\t# Do an ugly move and change\n+\tgit rm file1 &&\n+\techo \"New line ...\"  > file2 &&\n+\techo \"... and more\" >> file2 &&\n+\tgit add file2 &&\n+\tgit commit --author \"U Gly <ug@localhost>\" -m ugly &&\n+\n+\t# Back to master and change something\n+\tgit checkout master &&\n+\techo \"\n+\n+bla\" >> file1 &&\n+\tgit commit --author \"Old Line <ol@localhost>\" -a -m file1.b &&\n+\n+\t# Back to foo and merge master\n+\tgit checkout foo &&\n+\tif git merge master; then\n+\t\techo needed conflict here\n+\t\texit 1\n+\telse\n+\t\techo merge failed - resolving automatically\n+\tfi &&\n+\techo \"New line ...\n+... and more\n+\n+bla\n+Even more\" > file2 &&\n+\tgit rm file1 &&\n+\tgit commit --author \"M Result <mr@localhost>\" -a -m merged &&\n+\n+\t# Back to master and change file1 again\n+\tgit checkout master &&\n+\tsed s/bla/foo/ <file1 >X &&\n+\trm file1 &&\n+\tmv X file1 &&\n+\tgit commit --author \"No Bla <nb@localhost>\" -a -m replace &&\n+\n+\t# Try to merge into foo again\n+\tgit checkout foo &&\n+\tif git merge master; then\n+\t\techo needed conflict here\n+\t\texit 1\n+\telse\n+\t\techo merge failed - test is setup\n+\tfi\n+'\n+\n+test_expect_success \\\n+\t'blame runs on unconflicted file while other file has conflicts' '\n+\tgit blame file2\n+'\n+\n+test_expect_success 'blame runs on conflicted file in stages 1,3' '\n+\tgit blame file1\n+'\n+\n+test_done\n-- \n1.5.3.4.1231.gac645\n"},{"id":"56373","messageId":"20071018083105.GA21768@atjola.homenet","threadId":"10353","inReplyTo":"20071018063407.GA28861@spearce.org","subject":"Re: [PATCH] git-blame shouldn't crash if run in an unmerged tree","fromName":"Björn Steinbrink","fromEmail":"b.steinbrink@gmx.de","sentAt":"2007-10-18T08:31:05Z","receivedAt":"2007-10-18T08:31:05Z","isPatch":true,"sender":{"key":"b.steinbrink@gmx.de","avatar":"https://avatars.githubusercontent.com/u/230962?v=4"},"body":"On 2007.10.18 02:34:07 -0400, Shawn O. Pearce wrote:\n> I'm applying this patch to my maint tree tonight as it does resolve\n> the issue for now.  What surprised me was the file that we were\n> crashing out on wasn't even the file we wanted to get the blame\n> data for.  :-\\\n\nThe first merge moved some code from file1 (which doesn't exist in the\nbranch anymore) into file2, so I guess the code move detection comes\ninto play here.\n\nActually, in the original case that crashed here, I was curious about\nsome lines in file2 which looked like they had been automatically merged\nfrom file1, so I tried to use git blame with file2 to see if that really\nhappened (I didn't expect git to be even able to follow code moves while\nmerging). Unfortunately, I didn't get such a test case yet, which might\nindicate that I've only imagined that merge, and thinking about it, I\nthink that file2 wasn't marked as modified in \"git status\". Hm, I'll try\nto find that merge conflict again and try that again.\n\nBjörn\n\n> \n> --8>--\n> From: Linus Torvalds <torvalds@linux-foundation.org>\n> Subject: [PATCH] git-blame shouldn't crash if run in an unmerged tree\n> \n> If we are in the middle of resolving a merge conflict there may be\n> one or more files whose entries in the index represent an unmerged\n> state (index entries in the higher-order stages).\n> \n> Attempting to run git-blame on any file in such a working directory\n> resulted in \"fatal: internal error: ce_mode is 0\" as we use the magic\n> marker for an unmerged entry is 0 (set up by things like diff-lib.c's\n> do_diff_cache() and builtin-read-tree.c's read_tree_unmerged())\n> and the ce_match_stat_basic() function gets upset about this.\n> \n> I'm not entirely sure that the whole \"ce_mode = 0\" case is a good\n> idea to begin with, and maybe the right thing to do is to remove\n> that horrid freakish special case, but removing the internal error\n> seems to be the simplest fix for now.\n> \n>                 Linus\n> \n> [sp: Thanks to Björn Steinbrink for the test case]\n> \n> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>\n> ---\n>  read-cache.c     |    2 +\n>  t/t8004-blame.sh |   73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 75 insertions(+), 0 deletions(-)\n>  create mode 100755 t/t8004-blame.sh\n> \n> diff --git a/read-cache.c b/read-cache.c\n> index 536f4d0..928e8fa 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -149,6 +149,8 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)\n>  \t\telse if (ce_compare_gitlink(ce))\n>  \t\t\tchanged |= DATA_CHANGED;\n>  \t\treturn changed;\n> +\tcase 0: /* Special case: unmerged file in index */\n> +\t\treturn MODE_CHANGED | DATA_CHANGED | TYPE_CHANGED;\n>  \tdefault:\n>  \t\tdie(\"internal error: ce_mode is %o\", ntohl(ce->ce_mode));\n>  \t}\n> diff --git a/t/t8004-blame.sh b/t/t8004-blame.sh\n> new file mode 100755\n> index 0000000..ba19ac1\n> --- /dev/null\n> +++ b/t/t8004-blame.sh\n> @@ -0,0 +1,73 @@\n> +#!/bin/sh\n> +\n> +# Based on a test case submitted by Björn Steinbrink.\n> +\n> +test_description='git blame on conflicted files'\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup first case' '\n> +\t# Create the old file\n> +\techo \"Old line\" > file1 &&\n> +\tgit add file1 &&\n> +\tgit commit --author \"Old Line <ol@localhost>\" -m file1.a &&\n> +\n> +\t# Branch\n> +\tgit checkout -b foo &&\n> +\n> +\t# Do an ugly move and change\n> +\tgit rm file1 &&\n> +\techo \"New line ...\"  > file2 &&\n> +\techo \"... and more\" >> file2 &&\n> +\tgit add file2 &&\n> +\tgit commit --author \"U Gly <ug@localhost>\" -m ugly &&\n> +\n> +\t# Back to master and change something\n> +\tgit checkout master &&\n> +\techo \"\n> +\n> +bla\" >> file1 &&\n> +\tgit commit --author \"Old Line <ol@localhost>\" -a -m file1.b &&\n> +\n> +\t# Back to foo and merge master\n> +\tgit checkout foo &&\n> +\tif git merge master; then\n> +\t\techo needed conflict here\n> +\t\texit 1\n> +\telse\n> +\t\techo merge failed - resolving automatically\n> +\tfi &&\n> +\techo \"New line ...\n> +... and more\n> +\n> +bla\n> +Even more\" > file2 &&\n> +\tgit rm file1 &&\n> +\tgit commit --author \"M Result <mr@localhost>\" -a -m merged &&\n> +\n> +\t# Back to master and change file1 again\n> +\tgit checkout master &&\n> +\tsed s/bla/foo/ <file1 >X &&\n> +\trm file1 &&\n> +\tmv X file1 &&\n> +\tgit commit --author \"No Bla <nb@localhost>\" -a -m replace &&\n> +\n> +\t# Try to merge into foo again\n> +\tgit checkout foo &&\n> +\tif git merge master; then\n> +\t\techo needed conflict here\n> +\t\texit 1\n> +\telse\n> +\t\techo merge failed - test is setup\n> +\tfi\n> +'\n> +\n> +test_expect_success \\\n> +\t'blame runs on unconflicted file while other file has conflicts' '\n> +\tgit blame file2\n> +'\n> +\n> +test_expect_success 'blame runs on conflicted file in stages 1,3' '\n> +\tgit blame file1\n> +'\n> +\n> +test_done\n"},{"id":"56378","messageId":"20071018092108.GA24166@atjola.homenet","threadId":"10353","inReplyTo":"20071018083105.GA21768@atjola.homenet","subject":"Re: [PATCH] git-blame shouldn't crash if run in an unmerged tree","fromName":"Björn Steinbrink","fromEmail":"b.steinbrink@gmx.de","sentAt":"2007-10-18T09:21:08Z","receivedAt":"2007-10-18T09:21:08Z","isPatch":true,"sender":{"key":"b.steinbrink@gmx.de","avatar":"https://avatars.githubusercontent.com/u/230962?v=4"},"body":"On 2007.10.18 10:31:05 +0200, Björn Steinbrink wrote:\n> On 2007.10.18 02:34:07 -0400, Shawn O. Pearce wrote:\n> > I'm applying this patch to my maint tree tonight as it does resolve\n> > the issue for now.  What surprised me was the file that we were\n> > crashing out on wasn't even the file we wanted to get the blame\n> > data for.  :-\\\n> \n> The first merge moved some code from file1 (which doesn't exist in the\n> branch anymore) into file2, so I guess the code move detection comes\n> into play here.\n> \n> Actually, in the original case that crashed here, I was curious about\n> some lines in file2 which looked like they had been automatically merged\n> from file1, so I tried to use git blame with file2 to see if that really\n> happened (I didn't expect git to be even able to follow code moves while\n> merging). Unfortunately, I didn't get such a test case yet, which might\n> indicate that I've only imagined that merge, and thinking about it, I\n> think that file2 wasn't marked as modified in \"git status\". Hm, I'll try\n> to find that merge conflict again and try that again.\n\nNope, didn't succeed in reproducing what I probably just pretend to have\nseen.\n\nBjörn\n"},{"id":"56435","messageId":"alpine.LFD.0.999.0710181529490.26902@woody.linux-foundation.org","threadId":"10353","inReplyTo":"20071018063407.GA28861@spearce.org","subject":"Re: [PATCH] git-blame shouldn't crash if run in an unmerged tree","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-10-18T22:38:23Z","receivedAt":"2007-10-18T22:38:23Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 18 Oct 2007, Shawn O. Pearce wrote:\n>\n> I'm applying this patch to my maint tree tonight as it does resolve\n> the issue for now.  What surprised me was the file that we were\n> crashing out on wasn't even the file we wanted to get the blame\n> data for.  :-\\\n\nPlease feel free to add a Signed-off-by: there. I guess I didn't add it in \nthe original email, because I wasn't sure if I'd have the energy to see if \nI could just remove the clearing of \"ce_mode\". I never did.\n\nThat whole \"ce->ce_mode = 0\" thing is really hacky, and we use it for two \ntotally different things (\"git read-tree\" uses it for \"delete this entry\", \nand reading the index uses it when you ask for only merged entries). Bad \nform, and not very logical.\n\n\t\tLinus\n"}]}