{"thread":{"id":"19640","subject":"[PATCH 0/2] git-blame bug","startedAt":"2009-06-02T19:28:39Z","lastAt":"2009-06-03T12:40:45Z","messageCount":5,"participants":["Ben Willard","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"115268","messageId":"cover.1243969358.git.benwillard@gmail.com","threadId":"19640","inReplyTo":null,"subject":"[PATCH 0/2] git-blame bug","fromName":"Ben Willard","fromEmail":"benwillard@gmail.com","sentAt":"2009-06-02T19:28:39Z","receivedAt":"2009-06-02T19:28:39Z","isPatch":true,"sender":{"key":"benwillard@gmail.com","avatar":null},"body":"Hi,\n\nI ran across this git-blame failure the other day: if a directory is deleted\nand a file is created with the same name in a single commit, then git blame\nwill fail on it with this error:\n\tfatal: internal error in blame::find_origin\n\t\nAnyway, patch 1/2 shows this failure with a test, and 2/2 is a possible fix.\nThey should apply on maint. \n\nThanks,\n--\nBen\n\nBen Willard (2):\n  t8006: Add test to show blame failure\n  blame: Fix corner case when a directory becomes a file\n\n builtin-blame.c  |   12 ++++++++----\n t/t8006-blame.sh |   21 +++++++++++++++++++++\n 2 files changed, 29 insertions(+), 4 deletions(-)\n create mode 100755 t/t8006-blame.sh\n"},{"id":"115269","messageId":"4a257d7b.1358560a.504d.47c3@mx.google.com","threadId":"19640","inReplyTo":"cover.1243969358.git.benwillard@gmail.com","subject":"[PATCH 1/2] t8006: Add test to show blame failure","fromName":"Ben Willard","fromEmail":"benwillard@gmail.com","sentAt":"2009-06-02T19:28:55Z","receivedAt":"2009-06-02T19:28:55Z","isPatch":true,"sender":{"key":"benwillard@gmail.com","avatar":null},"body":"'git blame' fails on a file which was changed from a directory to a file\nin a single commit.  This test shows the failure.\n\nSigned-off-by: Ben Willard <benwillard@gmail.com>\n---\n t/t8006-blame.sh |   21 +++++++++++++++++++++\n 1 files changed, 21 insertions(+), 0 deletions(-)\n create mode 100755 t/t8006-blame.sh\n\ndiff --git a/t/t8006-blame.sh b/t/t8006-blame.sh\nnew file mode 100755\nindex 0000000..7c271f0\n--- /dev/null\n+++ b/t/t8006-blame.sh\n@@ -0,0 +1,21 @@\n+#!/bin/sh\n+\n+test_description='git blame on file which changed from a directory to a file in the same commit'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tmkdir dir &&\n+\ttouch dir/file &&\n+\tgit add dir/file &&\n+\tgit commit -m \"Add dir/file\" &&\n+\tgit rm dir/file &&\n+\techo \"File contents\" > dir &&\n+\tgit add dir &&\n+\tgit commit -m \"Add dir as a file\"\n+'\n+\n+test_expect_failure 'blame runs on dir' '\n+\tgit blame dir\n+'\n+\n+test_done\n-- \n1.6.3.1.279.gd4bf4\n"},{"id":"115270","messageId":"4a257d91.0407560a.04d2.ffffe775@mx.google.com","threadId":"19640","inReplyTo":"cover.1243969358.git.benwillard@gmail.com","subject":"[PATCH 2/2] blame: Fix corner case when a directory becomes a file","fromName":"Ben Willard","fromEmail":"benwillard@gmail.com","sentAt":"2009-06-02T19:29:17Z","receivedAt":"2009-06-02T19:29:17Z","isPatch":true,"sender":{"key":"benwillard@gmail.com","avatar":null},"body":"find_origin() assumes that there will be only one listing in\ndiff_queued_diff, but this is not the case when a directory becomes a\nfile in a single commit.  So, don't fail in this case.\n\nSigned-off-by: Ben Willard <benwillard@gmail.com>\n---\n builtin-blame.c  |   12 ++++++++----\n t/t8006-blame.sh |    2 +-\n 2 files changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex cf74a92..3e217af 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -363,16 +363,14 @@ static struct origin *find_origin(struct scoreboard *sb,\n \tdiffcore_std(&diff_opts);\n \n \t/* It is either one entry that says \"modified\", or \"created\",\n-\t * or nothing.\n+\t * or nothing, or two entries (a \"deleted\" and an \"add\").\n \t */\n \tif (!diff_queued_diff.nr) {\n \t\t/* The path is the same as parent */\n \t\tporigin = get_origin(sb, parent, origin->path);\n \t\thashcpy(porigin->blob_sha1, origin->blob_sha1);\n \t}\n-\telse if (diff_queued_diff.nr != 1)\n-\t\tdie(\"internal error in blame::find_origin\");\n-\telse {\n+\telse if (diff_queued_diff.nr == 1) {\n \t\tstruct diff_filepair *p = diff_queued_diff.queue[0];\n \t\tswitch (p->status) {\n \t\tdefault:\n@@ -388,6 +386,12 @@ static struct origin *find_origin(struct scoreboard *sb,\n \t\t\tbreak;\n \t\t}\n \t}\n+\telse if (diff_queued_diff.nr == 2) {\n+\t\t/* Both added and removed, (ie. directory became a file) */\n+\t}\n+\telse {\n+\t\tdie(\"internal error in blame::find_origin\");\n+\t}\n \tdiff_flush(&diff_opts);\n \tdiff_tree_release_paths(&diff_opts);\n \tif (porigin) {\ndiff --git a/t/t8006-blame.sh b/t/t8006-blame.sh\nindex 7c271f0..17f38ff 100755\n--- a/t/t8006-blame.sh\n+++ b/t/t8006-blame.sh\n@@ -14,7 +14,7 @@ test_expect_success 'setup' '\n \tgit commit -m \"Add dir as a file\"\n '\n \n-test_expect_failure 'blame runs on dir' '\n+test_expect_success 'blame runs on dir' '\n \tgit blame dir\n '\n \n-- \n1.6.3.1.279.gd4bf4\n"},{"id":"115323","messageId":"7vr5y1bw4l.fsf@alter.siamese.dyndns.org","threadId":"19640","inReplyTo":"4a257d91.0407560a.04d2.ffffe775@mx.google.com","subject":"Re: [PATCH 2/2] blame: Fix corner case when a directory becomes a file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-03T07:43:22Z","receivedAt":"2009-06-03T07:43:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Willard <benwillard@gmail.com> writes:\n\n> find_origin() assumes that there will be only one listing in\n> diff_queued_diff, but this is not the case when a directory becomes a\n> file in a single commit.  So, don't fail in this case.\n\nThanks.\n\nYour problem analysis is almost correct but the solution is wrong.\n\nBy the way, I'd rather not see people waste a whole _new_ test script when\nthere are existing test scripts availble for the command.\n\n-- >8 --\nSubject: [PATCH] blame: correctly handle a path that used to be a directory\n\nWhen trying to see if the same path exists in the parent, we ran\n\"diff-tree\" with pathspec set to the path we are interested in with the\nparent, and expect either to have exactly one resulting filepair (either\n\"changed from the parent\", \"created when there was none\") or nothing (when\nthere is no change from the parent).\n\nIf the path used to be a directory, however, we will also see unbounded\nnumber of entries that talk about the files that used to exist underneath\nthe directory in question.  Correctly pick only the entry that describes\nthe path we are interested in in such a case (namely, the creation of the\npath as a regular file).\n\nNoticed by Ben Willard.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin-blame.c  |   26 ++++++++++++++++++--------\n t/t8003-blame.sh |   15 +++++++++++++++\n 2 files changed, 33 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex cf74a92..0afdb16 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -362,18 +362,28 @@ static struct origin *find_origin(struct scoreboard *sb,\n \t\t\t       \"\", &diff_opts);\n \tdiffcore_std(&diff_opts);\n \n-\t/* It is either one entry that says \"modified\", or \"created\",\n-\t * or nothing.\n-\t */\n \tif (!diff_queued_diff.nr) {\n \t\t/* The path is the same as parent */\n \t\tporigin = get_origin(sb, parent, origin->path);\n \t\thashcpy(porigin->blob_sha1, origin->blob_sha1);\n-\t}\n-\telse if (diff_queued_diff.nr != 1)\n-\t\tdie(\"internal error in blame::find_origin\");\n-\telse {\n-\t\tstruct diff_filepair *p = diff_queued_diff.queue[0];\n+\t} else {\n+\t\t/*\n+\t\t * Since origin->path is a pathspec, if the parent\n+\t\t * commit had it as a directory, we will see a whole\n+\t\t * bunch of deletion of files in the directory that we\n+\t\t * do not care about.\n+\t\t */\n+\t\tint i;\n+\t\tstruct diff_filepair *p = NULL;\n+\t\tfor (i = 0; i < diff_queued_diff.nr; i++) {\n+\t\t\tconst char *name;\n+\t\t\tp = diff_queued_diff.queue[i];\n+\t\t\tname = p->one->path ? p->one->path : p->two->path;\n+\t\t\tif (!strcmp(name, origin->path))\n+\t\t\t\tbreak;\n+\t\t}\n+\t\tif (!p)\n+\t\t\tdie(\"internal error in blame::find_origin\");\n \t\tswitch (p->status) {\n \t\tdefault:\n \t\t\tdie(\"internal error in blame::find_origin (%c)\",\ndiff --git a/t/t8003-blame.sh b/t/t8003-blame.sh\nindex 966bb0a..13c25f1 100755\n--- a/t/t8003-blame.sh\n+++ b/t/t8003-blame.sh\n@@ -129,4 +129,19 @@ test_expect_success 'blame wholesale copy and more' '\n \n '\n \n+test_expect_success 'blame path that used to be a directory' '\n+\tmkdir path &&\n+\techo A A A A A >path/file &&\n+\techo B B B B B >path/elif &&\n+\tgit add path &&\n+\ttest_tick &&\n+\tgit commit -m \"path was a directory\" &&\n+\trm -fr path &&\n+\techo A A A A A >path &&\n+\tgit add path &&\n+\ttest_tick &&\n+\tgit commit -m \"path is a regular file\" &&\n+\tgit blame HEAD^.. -- path\n+'\n+\n test_done\n-- \n1.6.3.1.263.g85347\n"},{"id":"115340","messageId":"20090603124045.GB3151@ben-laptop","threadId":"19640","inReplyTo":"7vr5y1bw4l.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] blame: Fix corner case when a directory becomes a file","fromName":"Ben Willard","fromEmail":"benwillard@gmail.com","sentAt":"2009-06-03T12:40:45Z","receivedAt":"2009-06-03T12:40:45Z","isPatch":true,"sender":{"key":"benwillard@gmail.com","avatar":null},"body":"On Wed, Jun 03, 2009 at 12:43:22AM -0700, Junio C Hamano wrote:\n> Thanks.\n> \n> Your problem analysis is almost correct but the solution is wrong.\n\nAh, I guess it was more complicated than I thought.\n\n> By the way, I'd rather not see people waste a whole _new_ test script when\n> there are existing test scripts availble for the command.\n\nI'll remember that for next time.\n\n> -- >8 --\n> Subject: [PATCH] blame: correctly handle a path that used to be a directory\n> ...\n\nI tested this patch and it works.\n\nThanks Junio,\n--\nBen\n"}]}