{"thread":{"id":"43140","subject":"Re: Bug in \"git blame -C\"","startedAt":"2006-11-29T04:27:24Z","lastAt":"2006-11-29T06:32:00Z","messageCount":4,"participants":["Junio C Hamano","Linus Torvalds"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"295031","messageId":"Pine.LNX.4.64.0611282013430.3395@woody.osdl.org","threadId":"43140","inReplyTo":null,"subject":"Bug in \"git blame -C\"","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-11-29T04:27:24Z","receivedAt":"2006-11-29T04:27:24Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nOk, this is interesting.\n\nTry this sequence (which is a good sequence for showign how something like \n\"git blame -C\" _should_ work, but only ends up showing that it doesn't at \nall, because of some bug ;^):\n\n\t#\n\t# create 'testing' repository\n\t#\n\tmkdir testing\n\tcd testing/\n\tgit init-db\n\n\t#\n\t# copy git.c and sha1_file.c there and commit initial\n\t# (Just to get _some_ initial state)\n\t#\n\tcp ~/git/git.c .\n\tcp ~/git/sha1_file.c .\n\tgit add git.c sha1_file.c\n\tgit commit -m Initial\n\n\t#\n\t# move the prepend_to_path() function from git.c to\n\t# sha1_file.c (I did it to just after the\n\t#\t#ifndef O_NOATIME\n\t# block of preprocessor stuff\n\t#\n\tem git.c sha1_file.c\n\tgit commit -a -m Movement\n\tgit log -p\n\nand the result of that \"git log -p\" should show something like\n\n\tcommit a583b5aee68b89b7d554b8f900a95057e8ed61d9\n\tAuthor: Linus Torvalds <torvalds@woody.osdl.org>\n\tDate:   Tue Nov 28 20:13:00 2006 -0800\n\t\n\t    Movement\n\t\n\tdiff --git a/git.c b/git.c\n\tindex 357330e..43c01fd 100644\n\t--- a/git.c\n\t+++ b/git.c\n\t@@ -18,28 +18,6 @@\n\t const char git_usage_string[] =\n\t        \"git [--version] [--exec-path[=GIT_EXEC_PATH]] [-p|--paginate] [--bare] [--git-dir=GIT_DIR] [--help] COMMAND [AR\n\t\n\t-static void prepend_to_path(const char *dir, int len)\n\t-{\n\t-       const char *old_path = getenv(\"PATH\");\n\t-       char *path;\n\t-       int path_len = len;\n\t...\n\tdiff --git a/sha1_file.c b/sha1_file.c\n\tindex 63f416b..20168aa 100644\n\t--- a/sha1_file.c\n\t+++ b/sha1_file.c\n\t@@ -22,6 +22,28 @@\n\t #endif\n\t #endif\n\t\n\t+static void prepend_to_path(const char *dir, int len)\n\t+{\n\t+       const char *old_path = getenv(\"PATH\");\n\t+       char *path;\n\t+       int path_len = len;\n\t+\n\t...\n\nto show how that top commit moved the function. Ok, everything looks fine \nso far.\n\nFor the surreal behaviour, now do\n\n\tgit blame -C sha1_file.c\n\nand watch the result make no sense what-so-ever. It doesn't show the \nmovement at all. It shows that everything in that file came from the \noriginal commit, even though the file obviously did change since.\n\nWhich is kind of \"true\", but it's still _wrong_. Yes, all the data comes \nfrom the same (initial) commit, but it doesn't come from the same _files_ \nin the same commit, so the fact that we don't see the filenames and \noriginal lines in those filenames is _broken_. The commit information is \nright, but it's decided not to show all the _other_ information that is \ncrucial..\n\nSo this shows two problems:\n\n - the line numbers that \"git blame -C\" shows are the current line numbers \n   only, not the line numbers it came from in the version it shows. That \n   makes them useless. We _know_ the current linenumbers. What we want to \n   know is what they were in the commit that they came from.\n\n   So right now, the line number information that \"git blame -C\" shows is \n   just the same thing we could have gotten by doing a \"cat -n file\".\n\n - \"git blame -C\" has apparently decided that it doesn't need to show \n   filenames that things came from, because they all came from the same \n   commit, but that's not a logical thing to compare. \"same commit\" does \n   not mean \"same filename\", so not showing the filename makes no sense.\n\nI tried to bisect this a bit, but I don't think pickaxe has ever gotten \nthis right, so I couldn't find a place where it was correct to start \nbisecting at ;)\n\n"},{"id":"296460","messageId":"7vu00iu7lb.fsf@assigned-by-dhcp.cox.net","threadId":"43140","inReplyTo":"Pine.LNX.4.64.0611282013430.3395@woody.osdl.org","subject":"Re: Bug in \"git blame -C\"","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-29T05:48:00Z","receivedAt":"2006-11-29T05:48:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> writes:\n\n> So this shows two problems:\n>\n>  - the line numbers that \"git blame -C\" shows are the current line numbers \n>    only, not the line numbers it came from in the version it shows. That \n>    makes them useless. We _know_ the current linenumbers. What we want to \n>    know is what they were in the commit that they came from.\n>\n>    So right now, the line number information that \"git blame -C\" shows is \n>    just the same thing we could have gotten by doing a \"cat -n file\".\n\nPlease use -f -n, which gives the origin filename and origin linenumber.\n\nf9c03ba7 sha1_file.c   22 (Junio  2006-11-28 21:10:50 -0800   22) #endif\nf9c03ba7 sha1_file.c   23 (Junio  2006-11-28 21:10:50 -0800   23) #endif\nf9c03ba7 sha1_file.c   24 (Junio  2006-11-28 21:10:50 -0800   24) \nf9c03ba7 git.c         21 (Junio  2006-11-28 21:10:50 -0800   25) static v..\nf9c03ba7 git.c         22 (Junio  2006-11-28 21:10:50 -0800   26) {\nf9c03ba7 git.c         23 (Junio  2006-11-28 21:10:50 -0800   27) \tco..\nf9c03ba7 git.c         24 (Junio  2006-11-28 21:10:50 -0800   28) \tch..\nf9c03ba7 git.c         25 (Junio  2006-11-28 21:10:50 -0800   29) \tin..\n\n>  - \"git blame -C\" has apparently decided that it doesn't need to show \n>    filenames that things came from, because they all came from the same \n>    commit, but that's not a logical thing to compare. \"same commit\" does \n>    not mean \"same filename\", so not showing the filename makes no sense.\n\nThis is definitely a bug, and I am sorry to have sent you to a\nwild goose chase (a root commit corner case that did not exist).\n\nI am not sure how often you would want the origin line-number,\nand I think it is debatable if we should turn show-number on in\nthis case, but we should definitely turn show-name on when two\npaths are involved; that's what -C is about.\n\nFor that matter, I do not think the line number in the final\nrevision is interesting at all; they are there for hysterical\nraisins only (I know pickaxe imitated blame which inherited it\nfrom annotate).\n\nSomething like this?\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex 066dee7..eee0b90 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -1344,29 +1344,30 @@ static void output(struct scoreboard *sb, int option)\n {\n \tstruct blame_entry *ent;\n \n-\tif (option & OUTPUT_PORCELAIN) {\n-\t\tfor (ent = sb->ent; ent; ent = ent->next) {\n-\t\t\tstruct blame_entry *oth;\n-\t\t\tstruct origin *suspect = ent->suspect;\n-\t\t\tstruct commit *commit = suspect->commit;\n-\t\t\tif (commit->object.flags & MORE_THAN_ONE_PATH)\n+\tfor (ent = sb->ent; ent; ent = ent->next) {\n+\t\tstruct blame_entry *oth;\n+\t\tstruct origin *suspect = ent->suspect;\n+\t\tstruct commit *commit = suspect->commit;\n+\n+\t\tif (strcmp(suspect->path, sb->path) &&\n+\t\t    !(option & OUTPUT_PORCELAIN))\n+\t\t\toption |= OUTPUT_SHOW_NAME | OUTPUT_SHOW_NUMBER;\n+\t\tif (commit->object.flags & MORE_THAN_ONE_PATH)\n+\t\t\tcontinue;\n+\t\tfor (oth = ent->next; oth; oth = oth->next) {\n+\t\t\tif ((oth->suspect->commit != commit) ||\n+\t\t\t    !strcmp(oth->suspect->path, suspect->path))\n \t\t\t\tcontinue;\n-\t\t\tfor (oth = ent->next; oth; oth = oth->next) {\n-\t\t\t\tif ((oth->suspect->commit != commit) ||\n-\t\t\t\t    !strcmp(oth->suspect->path, suspect->path))\n-\t\t\t\t\tcontinue;\n-\t\t\t\tcommit->object.flags |= MORE_THAN_ONE_PATH;\n-\t\t\t\tbreak;\n-\t\t\t}\n+\t\t\tcommit->object.flags |= MORE_THAN_ONE_PATH;\n+\t\t\tbreak;\n \t\t}\n \t}\n \n \tfor (ent = sb->ent; ent; ent = ent->next) {\n \t\tif (option & OUTPUT_PORCELAIN)\n \t\t\temit_porcelain(sb, ent);\n-\t\telse {\n+\t\telse\n \t\t\temit_other(sb, ent, option);\n-\t\t}\n \t}\n }\n \n@@ -1438,8 +1439,6 @@ static void find_alignment(struct scoreboard *sb, int *option)\n \t\tif (!(suspect->commit->object.flags & METAINFO_SHOWN)) {\n \t\t\tsuspect->commit->object.flags |= METAINFO_SHOWN;\n \t\t\tget_commit_info(suspect->commit, &ci, 1);\n-\t\t\tif (strcmp(suspect->path, sb->path))\n-\t\t\t\t*option |= OUTPUT_SHOW_NAME;\n \t\t\tnum = strlen(suspect->path);\n \t\t\tif (longest_file < num)\n \t\t\t\tlongest_file = num;\n"},{"id":"293907","messageId":"7vmz6au626.fsf@assigned-by-dhcp.cox.net","threadId":"43140","inReplyTo":"7vu00iu7lb.fsf@assigned-by-dhcp.cox.net","subject":"Re: Bug in \"git blame -C\"","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-29T06:21:05Z","receivedAt":"2006-11-29T06:21:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <junkio@cox.net> writes:\n\n> Linus Torvalds <torvalds@osdl.org> writes:\n>\n>>  - \"git blame -C\" has apparently decided that it doesn't need to show \n>>    filenames that things came from, because they all came from the same \n>>    commit, but that's not a logical thing to compare. \"same commit\" does \n>>    not mean \"same filename\", so not showing the filename makes no sense.\n>\n> This is definitely a bug,\n\nA replacement patch that should be much cleaner and to the\npoint.\n\n---\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex 066dee7..4090a80 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -1435,11 +1435,12 @@ static void find_alignment(struct scoreboard *sb, int *option)\n \t\tstruct commit_info ci;\n \t\tint num;\n \n+\t\tif (strcmp(suspect->path, sb->path))\n+\t\t\t*option |= OUTPUT_SHOW_NAME | OUTPUT_SHOW_NUMBER;\n+\n \t\tif (!(suspect->commit->object.flags & METAINFO_SHOWN)) {\n \t\t\tsuspect->commit->object.flags |= METAINFO_SHOWN;\n \t\t\tget_commit_info(suspect->commit, &ci, 1);\n-\t\t\tif (strcmp(suspect->path, sb->path))\n-\t\t\t\t*option |= OUTPUT_SHOW_NAME;\n \t\t\tnum = strlen(suspect->path);\n \t\t\tif (longest_file < num)\n \t\t\t\tlongest_file = num;\n"},{"id":"295495","messageId":"7vhcwiu5jz.fsf_-_@assigned-by-dhcp.cox.net","threadId":"43140","inReplyTo":"7vmz6au626.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH (take 3)] git blame -C: fix output format tweaks when crossing file boundary.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-29T06:32:00Z","receivedAt":"2006-11-29T06:32:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We used to get the case that more than two paths came from the\nsame commit wrong when computing the output width and deciding\nto turn on --show-name option automatically.  When we find that\nlines that came from a path that is different from what we\nstarted digging from, we should always turn --show-name on, and\nwe should count the name length for all files involved.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n\n * BLUSH.  Both of the previous ones were botched.\n   Third time lucky, hopefully.\n\n builtin-blame.c |   10 +++++-----\n 1 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex 066dee7..53fed45 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -1435,14 +1435,14 @@ static void find_alignment(struct scoreboard *sb, int *option)\n \t\tstruct commit_info ci;\n \t\tint num;\n \n+\t\tif (strcmp(suspect->path, sb->path))\n+\t\t\t*option |= OUTPUT_SHOW_NAME;\n+\t\tnum = strlen(suspect->path);\n+\t\tif (longest_file < num)\n+\t\t\tlongest_file = num;\n \t\tif (!(suspect->commit->object.flags & METAINFO_SHOWN)) {\n \t\t\tsuspect->commit->object.flags |= METAINFO_SHOWN;\n \t\t\tget_commit_info(suspect->commit, &ci, 1);\n-\t\t\tif (strcmp(suspect->path, sb->path))\n-\t\t\t\t*option |= OUTPUT_SHOW_NAME;\n-\t\t\tnum = strlen(suspect->path);\n-\t\t\tif (longest_file < num)\n-\t\t\t\tlongest_file = num;\n \t\t\tnum = strlen(ci.author);\n \t\t\tif (longest_author < num)\n \t\t\t\tlongest_author = num;\n-- \n1.4.4.1.gad0c3-dirty\n\n"}]}