Bug in "git blame -C"

Bug in "git blame -C"

[Linus Torvalds]

Ok, this is interesting.

Try this sequence (which is a good sequence for showign how something like 
"git blame -C" _should_ work, but only ends up showing that it doesn't at 
all, because of some bug ;^):

	#
	# create 'testing' repository
	#
	mkdir testing
	cd testing/
	git init-db

	#
	# copy git.c and sha1_file.c there and commit initial
	# (Just to get _some_ initial state)
	#
	cp ~/git/git.c .
	cp ~/git/sha1_file.c .
	git add git.c sha1_file.c
	git commit -m Initial

	#
	# move the prepend_to_path() function from git.c to
	# sha1_file.c (I did it to just after the
	#	#ifndef O_NOATIME
	# block of preprocessor stuff
	#
	em git.c sha1_file.c
	git commit -a -m Movement
	git log -p

and the result of that "git log -p" should show something like

	commit a583b5aee68b89b7d554b8f900a95057e8ed61d9
	Author: Linus Torvalds <torvalds@woody.osdl.org>
	Date:   Tue Nov 28 20:13:00 2006 -0800
	
	    Movement
	
	diff --git a/git.c b/git.c
	index 357330e..43c01fd 100644
	--- a/git.c
	+++ b/git.c
	@@ -18,28 +18,6 @@
	 const char git_usage_string[] =
	        "git [--version] [--exec-path[=GIT_EXEC_PATH]] [-p|--paginate] [--bare] [--git-dir=GIT_DIR] [--help] COMMAND [AR
	
	-static void prepend_to_path(const char *dir, int len)
	-{
	-       const char *old_path = getenv("PATH");
	-       char *path;
	-       int path_len = len;
	...
	diff --git a/sha1_file.c b/sha1_file.c
	index 63f416b..20168aa 100644
	--- a/sha1_file.c
	+++ b/sha1_file.c
	@@ -22,6 +22,28 @@
	 #endif
	 #endif
	
	+static void prepend_to_path(const char *dir, int len)
	+{
	+       const char *old_path = getenv("PATH");
	+       char *path;
	+       int path_len = len;
	+
	...

to show how that top commit moved the function. Ok, everything looks fine 
so far.

For the surreal behaviour, now do

	git blame -C sha1_file.c

and watch the result make no sense what-so-ever. It doesn't show the 
movement at all. It shows that everything in that file came from the 
original commit, even though the file obviously did change since.

Which is kind of "true", but it's still _wrong_. Yes, all the data comes 
from the same (initial) commit, but it doesn't come from the same _files_ 
in the same commit, so the fact that we don't see the filenames and 
original lines in those filenames is _broken_. The commit information is 
right, but it's decided not to show all the _other_ information that is 
crucial..

So this shows two problems:

 - the line numbers that "git blame -C" shows are the current line numbers 
   only, not the line numbers it came from in the version it shows. That 
   makes them useless. We _know_ the current linenumbers. What we want to 
   know is what they were in the commit that they came from.

   So right now, the line number information that "git blame -C" shows is 
   just the same thing we could have gotten by doing a "cat -n file".

 - "git blame -C" has apparently decided that it doesn't need to show 
   filenames that things came from, because they all came from the same 
   commit, but that's not a logical thing to compare. "same commit" does 
   not mean "same filename", so not showing the filename makes no sense.

I tried to bisect this a bit, but I don't think pickaxe has ever gotten 
this right, so I couldn't find a place where it was correct to start 
bisecting at ;)

Re: Bug in "git blame -C"

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> So this shows two problems:
>
>  - the line numbers that "git blame -C" shows are the current line numbers 
>    only, not the line numbers it came from in the version it shows. That 
>    makes them useless. We _know_ the current linenumbers. What we want to 
>    know is what they were in the commit that they came from.
>
>    So right now, the line number information that "git blame -C" shows is 
>    just the same thing we could have gotten by doing a "cat -n file".

Please use -f -n, which gives the origin filename and origin linenumber.

f9c03ba7 sha1_file.c   22 (Junio  2006-11-28 21:10:50 -0800   22) #endif
f9c03ba7 sha1_file.c   23 (Junio  2006-11-28 21:10:50 -0800   23) #endif
f9c03ba7 sha1_file.c   24 (Junio  2006-11-28 21:10:50 -0800   24) 
f9c03ba7 git.c         21 (Junio  2006-11-28 21:10:50 -0800   25) static v..
f9c03ba7 git.c         22 (Junio  2006-11-28 21:10:50 -0800   26) {
f9c03ba7 git.c         23 (Junio  2006-11-28 21:10:50 -0800   27) 	co..
f9c03ba7 git.c         24 (Junio  2006-11-28 21:10:50 -0800   28) 	ch..
f9c03ba7 git.c         25 (Junio  2006-11-28 21:10:50 -0800   29) 	in..

>  - "git blame -C" has apparently decided that it doesn't need to show 
>    filenames that things came from, because they all came from the same 
>    commit, but that's not a logical thing to compare. "same commit" does 
>    not mean "same filename", so not showing the filename makes no sense.

This is definitely a bug, and I am sorry to have sent you to a
wild goose chase (a root commit corner case that did not exist).

I am not sure how often you would want the origin line-number,
and I think it is debatable if we should turn show-number on in
this case, but we should definitely turn show-name on when two
paths are involved; that's what -C is about.

For that matter, I do not think the line number in the final
revision is interesting at all; they are there for hysterical
raisins only (I know pickaxe imitated blame which inherited it
from annotate).

Something like this?

diff --git a/builtin-blame.c b/builtin-blame.c
index 066dee7..eee0b90 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -1344,29 +1344,30 @@ static void output(struct scoreboard *sb, int option)
 {
 	struct blame_entry *ent;
 
-	if (option & OUTPUT_PORCELAIN) {
-		for (ent = sb->ent; ent; ent = ent->next) {
-			struct blame_entry *oth;
-			struct origin *suspect = ent->suspect;
-			struct commit *commit = suspect->commit;
-			if (commit->object.flags & MORE_THAN_ONE_PATH)
+	for (ent = sb->ent; ent; ent = ent->next) {
+		struct blame_entry *oth;
+		struct origin *suspect = ent->suspect;
+		struct commit *commit = suspect->commit;
+
+		if (strcmp(suspect->path, sb->path) &&
+		    !(option & OUTPUT_PORCELAIN))
+			option |= OUTPUT_SHOW_NAME | OUTPUT_SHOW_NUMBER;
+		if (commit->object.flags & MORE_THAN_ONE_PATH)
+			continue;
+		for (oth = ent->next; oth; oth = oth->next) {
+			if ((oth->suspect->commit != commit) ||
+			    !strcmp(oth->suspect->path, suspect->path))
 				continue;
-			for (oth = ent->next; oth; oth = oth->next) {
-				if ((oth->suspect->commit != commit) ||
-				    !strcmp(oth->suspect->path, suspect->path))
-					continue;
-				commit->object.flags |= MORE_THAN_ONE_PATH;
-				break;
-			}
+			commit->object.flags |= MORE_THAN_ONE_PATH;
+			break;
 		}
 	}
 
 	for (ent = sb->ent; ent; ent = ent->next) {
 		if (option & OUTPUT_PORCELAIN)
 			emit_porcelain(sb, ent);
-		else {
+		else
 			emit_other(sb, ent, option);
-		}
 	}
 }
 
@@ -1438,8 +1439,6 @@ static void find_alignment(struct scoreboard *sb, int *option)
 		if (!(suspect->commit->object.flags & METAINFO_SHOWN)) {
 			suspect->commit->object.flags |= METAINFO_SHOWN;
 			get_commit_info(suspect->commit, &ci, 1);
-			if (strcmp(suspect->path, sb->path))
-				*option |= OUTPUT_SHOW_NAME;
 			num = strlen(suspect->path);
 			if (longest_file < num)
 				longest_file = num;

Re: Bug in "git blame -C"

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> Linus Torvalds <torvalds@osdl.org> writes:
>
>>  - "git blame -C" has apparently decided that it doesn't need to show 
>>    filenames that things came from, because they all came from the same 
>>    commit, but that's not a logical thing to compare. "same commit" does 
>>    not mean "same filename", so not showing the filename makes no sense.
>
> This is definitely a bug,

A replacement patch that should be much cleaner and to the
point.

---

diff --git a/builtin-blame.c b/builtin-blame.c
index 066dee7..4090a80 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -1435,11 +1435,12 @@ static void find_alignment(struct scoreboard *sb, int *option)
 		struct commit_info ci;
 		int num;
 
+		if (strcmp(suspect->path, sb->path))
+			*option |= OUTPUT_SHOW_NAME | OUTPUT_SHOW_NUMBER;
+
 		if (!(suspect->commit->object.flags & METAINFO_SHOWN)) {
 			suspect->commit->object.flags |= METAINFO_SHOWN;
 			get_commit_info(suspect->commit, &ci, 1);
-			if (strcmp(suspect->path, sb->path))
-				*option |= OUTPUT_SHOW_NAME;
 			num = strlen(suspect->path);
 			if (longest_file < num)
 				longest_file = num;

[PATCH (take 3)] git blame -C: fix output format tweaks when crossing file boundary.

[Junio C Hamano]

We used to get the case that more than two paths came from the
same commit wrong when computing the output width and deciding
to turn on --show-name option automatically.  When we find that
lines that came from a path that is different from what we
started digging from, we should always turn --show-name on, and
we should count the name length for all files involved.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 * BLUSH.  Both of the previous ones were botched.
   Third time lucky, hopefully.

 builtin-blame.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin-blame.c b/builtin-blame.c
index 066dee7..53fed45 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -1435,14 +1435,14 @@ static void find_alignment(struct scoreboard *sb, int *option)
 		struct commit_info ci;
 		int num;
 
+		if (strcmp(suspect->path, sb->path))
+			*option |= OUTPUT_SHOW_NAME;
+		num = strlen(suspect->path);
+		if (longest_file < num)
+			longest_file = num;
 		if (!(suspect->commit->object.flags & METAINFO_SHOWN)) {
 			suspect->commit->object.flags |= METAINFO_SHOWN;
 			get_commit_info(suspect->commit, &ci, 1);
-			if (strcmp(suspect->path, sb->path))
-				*option |= OUTPUT_SHOW_NAME;
-			num = strlen(suspect->path);
-			if (longest_file < num)
-				longest_file = num;
 			num = strlen(ci.author);
 			if (longest_author < num)
 				longest_author = num;
-- 
1.4.4.1.gad0c3-dirty