* [PATCH 0/2] git-blame bug @ 2009-06-02 19:28 Ben Willard 2009-06-02 19:28 ` [PATCH 1/2] t8006: Add test to show blame failure Ben Willard 2009-06-02 19:29 ` [PATCH 2/2] blame: Fix corner case when a directory becomes a file Ben Willard 0 siblings, 2 replies; 5+ messages in thread From: Ben Willard @ 2009-06-02 19:28 UTC (permalink / raw) To: git; +Cc: gitster Hi, I ran across this git-blame failure the other day: if a directory is deleted and a file is created with the same name in a single commit, then git blame will fail on it with this error: fatal: internal error in blame::find_origin Anyway, patch 1/2 shows this failure with a test, and 2/2 is a possible fix. They should apply on maint. Thanks, -- Ben Ben Willard (2): t8006: Add test to show blame failure blame: Fix corner case when a directory becomes a file builtin-blame.c | 12 ++++++++---- t/t8006-blame.sh | 21 +++++++++++++++++++++ 2 files changed, 29 insertions(+), 4 deletions(-) create mode 100755 t/t8006-blame.sh ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] t8006: Add test to show blame failure 2009-06-02 19:28 [PATCH 0/2] git-blame bug Ben Willard @ 2009-06-02 19:28 ` Ben Willard 2009-06-02 19:29 ` [PATCH 2/2] blame: Fix corner case when a directory becomes a file Ben Willard 1 sibling, 0 replies; 5+ messages in thread From: Ben Willard @ 2009-06-02 19:28 UTC (permalink / raw) To: git; +Cc: gitster 'git blame' fails on a file which was changed from a directory to a file in a single commit. This test shows the failure. Signed-off-by: Ben Willard <benwillard@gmail.com> --- t/t8006-blame.sh | 21 +++++++++++++++++++++ 1 files changed, 21 insertions(+), 0 deletions(-) create mode 100755 t/t8006-blame.sh diff --git a/t/t8006-blame.sh b/t/t8006-blame.sh new file mode 100755 index 0000000..7c271f0 --- /dev/null +++ b/t/t8006-blame.sh @@ -0,0 +1,21 @@ +#!/bin/sh + +test_description='git blame on file which changed from a directory to a file in the same commit' +. ./test-lib.sh + +test_expect_success 'setup' ' + mkdir dir && + touch dir/file && + git add dir/file && + git commit -m "Add dir/file" && + git rm dir/file && + echo "File contents" > dir && + git add dir && + git commit -m "Add dir as a file" +' + +test_expect_failure 'blame runs on dir' ' + git blame dir +' + +test_done -- 1.6.3.1.279.gd4bf4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] blame: Fix corner case when a directory becomes a file 2009-06-02 19:28 [PATCH 0/2] git-blame bug Ben Willard 2009-06-02 19:28 ` [PATCH 1/2] t8006: Add test to show blame failure Ben Willard @ 2009-06-02 19:29 ` Ben Willard 2009-06-03 7:43 ` Junio C Hamano 1 sibling, 1 reply; 5+ messages in thread From: Ben Willard @ 2009-06-02 19:29 UTC (permalink / raw) To: git; +Cc: gitster find_origin() assumes that there will be only one listing in diff_queued_diff, but this is not the case when a directory becomes a file in a single commit. So, don't fail in this case. Signed-off-by: Ben Willard <benwillard@gmail.com> --- builtin-blame.c | 12 ++++++++---- t/t8006-blame.sh | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/builtin-blame.c b/builtin-blame.c index cf74a92..3e217af 100644 --- a/builtin-blame.c +++ b/builtin-blame.c @@ -363,16 +363,14 @@ static struct origin *find_origin(struct scoreboard *sb, diffcore_std(&diff_opts); /* It is either one entry that says "modified", or "created", - * or nothing. + * or nothing, or two entries (a "deleted" and an "add"). */ if (!diff_queued_diff.nr) { /* The path is the same as parent */ porigin = get_origin(sb, parent, origin->path); hashcpy(porigin->blob_sha1, origin->blob_sha1); } - else if (diff_queued_diff.nr != 1) - die("internal error in blame::find_origin"); - else { + else if (diff_queued_diff.nr == 1) { struct diff_filepair *p = diff_queued_diff.queue[0]; switch (p->status) { default: @@ -388,6 +386,12 @@ static struct origin *find_origin(struct scoreboard *sb, break; } } + else if (diff_queued_diff.nr == 2) { + /* Both added and removed, (ie. directory became a file) */ + } + else { + die("internal error in blame::find_origin"); + } diff_flush(&diff_opts); diff_tree_release_paths(&diff_opts); if (porigin) { diff --git a/t/t8006-blame.sh b/t/t8006-blame.sh index 7c271f0..17f38ff 100755 --- a/t/t8006-blame.sh +++ b/t/t8006-blame.sh @@ -14,7 +14,7 @@ test_expect_success 'setup' ' git commit -m "Add dir as a file" ' -test_expect_failure 'blame runs on dir' ' +test_expect_success 'blame runs on dir' ' git blame dir ' -- 1.6.3.1.279.gd4bf4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] blame: Fix corner case when a directory becomes a file 2009-06-02 19:29 ` [PATCH 2/2] blame: Fix corner case when a directory becomes a file Ben Willard @ 2009-06-03 7:43 ` Junio C Hamano 2009-06-03 12:40 ` Ben Willard 0 siblings, 1 reply; 5+ messages in thread From: Junio C Hamano @ 2009-06-03 7:43 UTC (permalink / raw) To: Ben Willard; +Cc: git Ben Willard <benwillard@gmail.com> writes: > find_origin() assumes that there will be only one listing in > diff_queued_diff, but this is not the case when a directory becomes a > file in a single commit. So, don't fail in this case. Thanks. Your problem analysis is almost correct but the solution is wrong. By the way, I'd rather not see people waste a whole _new_ test script when there are existing test scripts availble for the command. -- >8 -- Subject: [PATCH] blame: correctly handle a path that used to be a directory When trying to see if the same path exists in the parent, we ran "diff-tree" with pathspec set to the path we are interested in with the parent, and expect either to have exactly one resulting filepair (either "changed from the parent", "created when there was none") or nothing (when there is no change from the parent). If the path used to be a directory, however, we will also see unbounded number of entries that talk about the files that used to exist underneath the directory in question. Correctly pick only the entry that describes the path we are interested in in such a case (namely, the creation of the path as a regular file). Noticed by Ben Willard. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin-blame.c | 26 ++++++++++++++++++-------- t/t8003-blame.sh | 15 +++++++++++++++ 2 files changed, 33 insertions(+), 8 deletions(-) diff --git a/builtin-blame.c b/builtin-blame.c index cf74a92..0afdb16 100644 --- a/builtin-blame.c +++ b/builtin-blame.c @@ -362,18 +362,28 @@ static struct origin *find_origin(struct scoreboard *sb, "", &diff_opts); diffcore_std(&diff_opts); - /* It is either one entry that says "modified", or "created", - * or nothing. - */ if (!diff_queued_diff.nr) { /* The path is the same as parent */ porigin = get_origin(sb, parent, origin->path); hashcpy(porigin->blob_sha1, origin->blob_sha1); - } - else if (diff_queued_diff.nr != 1) - die("internal error in blame::find_origin"); - else { - struct diff_filepair *p = diff_queued_diff.queue[0]; + } else { + /* + * Since origin->path is a pathspec, if the parent + * commit had it as a directory, we will see a whole + * bunch of deletion of files in the directory that we + * do not care about. + */ + int i; + struct diff_filepair *p = NULL; + for (i = 0; i < diff_queued_diff.nr; i++) { + const char *name; + p = diff_queued_diff.queue[i]; + name = p->one->path ? p->one->path : p->two->path; + if (!strcmp(name, origin->path)) + break; + } + if (!p) + die("internal error in blame::find_origin"); switch (p->status) { default: die("internal error in blame::find_origin (%c)", diff --git a/t/t8003-blame.sh b/t/t8003-blame.sh index 966bb0a..13c25f1 100755 --- a/t/t8003-blame.sh +++ b/t/t8003-blame.sh @@ -129,4 +129,19 @@ test_expect_success 'blame wholesale copy and more' ' ' +test_expect_success 'blame path that used to be a directory' ' + mkdir path && + echo A A A A A >path/file && + echo B B B B B >path/elif && + git add path && + test_tick && + git commit -m "path was a directory" && + rm -fr path && + echo A A A A A >path && + git add path && + test_tick && + git commit -m "path is a regular file" && + git blame HEAD^.. -- path +' + test_done -- 1.6.3.1.263.g85347 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] blame: Fix corner case when a directory becomes a file 2009-06-03 7:43 ` Junio C Hamano @ 2009-06-03 12:40 ` Ben Willard 0 siblings, 0 replies; 5+ messages in thread From: Ben Willard @ 2009-06-03 12:40 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Wed, Jun 03, 2009 at 12:43:22AM -0700, Junio C Hamano wrote: > Thanks. > > Your problem analysis is almost correct but the solution is wrong. Ah, I guess it was more complicated than I thought. > By the way, I'd rather not see people waste a whole _new_ test script when > there are existing test scripts availble for the command. I'll remember that for next time. > -- >8 -- > Subject: [PATCH] blame: correctly handle a path that used to be a directory > ... I tested this patch and it works. Thanks Junio, -- Ben ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-06-03 12:40 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-06-02 19:28 [PATCH 0/2] git-blame bug Ben Willard 2009-06-02 19:28 ` [PATCH 1/2] t8006: Add test to show blame failure Ben Willard 2009-06-02 19:29 ` [PATCH 2/2] blame: Fix corner case when a directory becomes a file Ben Willard 2009-06-03 7:43 ` Junio C Hamano 2009-06-03 12:40 ` Ben Willard
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).